lib/idr.c: initialize struct idr_layer

lib/idr.c: initialize struct idr_layer

[Vegard Nossum]

Hi,

I am testing my kmemcheck patches, and it has come up with a couple of
uses of uninitialized memory in lib/idr.c. These are (the line numbers
may differ slightly):

line 135 (sub_alloc): bm = ~p->bitmap;
p->bitmap is uninitialized

line 171 (sub_alloc): if (!p->ary[m]) {
p->ary is uninitialized

line 249 (idr_get_new_above_int): pa[0]->count++;
pa[0]->count is uninitialized

I cannot guarantee that these are truly errors, but I would be
grateful if you could help me confirm/deny the validity of the
reports. Personally, I can get rid of the errors using this patch:

diff --git a/lib/idr.c b/lib/idr.c
index afbb0b1..dd28ee5 100644
--- a/lib/idr.c
+++ b/lib/idr.c
@@ -39,12 +39,16 @@ static struct idr_layer *alloc_layer(struct idr *idp)
 {
        struct idr_layer *p;
        unsigned long flags;
+       int i;

        spin_lock_irqsave(&idp->lock, flags);
        if ((p = idp->id_free)) {
                idp->id_free = p->ary[0];
                idp->id_free_cnt--;
-               p->ary[0] = NULL;
+               p->bitmap = 0;
+               for(i = 0; i < ARRAY_SIZE(p->ary); ++i)
+                       p->ary[i] = NULL;
+               p->count = 0;
        }
        spin_unlock_irqrestore(&idp->lock, flags);
        return(p);


Thanks a lot.

Vegard

Re: lib/idr.c: initialize struct idr_layer

[Pekka Enberg]

Hi Vegard,

On Jan 27, 2008 10:07 PM, Vegard Nossum <vegard.nossum@gmail.com> wrote:
> I am testing my kmemcheck patches, and it has come up with a couple of
> uses of uninitialized memory in lib/idr.c. These are (the line numbers
> may differ slightly):

[snip]

> @@ -39,12 +39,16 @@ static struct idr_layer *alloc_layer(struct idr *idp)
>  {
>         struct idr_layer *p;
>         unsigned long flags;
> +       int i;
>
>         spin_lock_irqsave(&idp->lock, flags);
>         if ((p = idp->id_free)) {
>                 idp->id_free = p->ary[0];
>                 idp->id_free_cnt--;
> -               p->ary[0] = NULL;
> +               p->bitmap = 0;
> +               for(i = 0; i < ARRAY_SIZE(p->ary); ++i)
> +                       p->ary[i] = NULL;
> +               p->count = 0;

But aren't these zeroed by idr_cache_ctor() already?

>         }
>         spin_unlock_irqrestore(&idp->lock, flags);
>         return(p);

                        Pekka

Re: lib/idr.c: initialize struct idr_layer

[Vegard Nossum]

On Jan 27, 2008 10:00 PM, Pekka Enberg <penberg@cs.helsinki.fi> wrote:
> Hi Vegard,
>
> On Jan 27, 2008 10:07 PM, Vegard Nossum <vegard.nossum@gmail.com> wrote:
> > I am testing my kmemcheck patches, and it has come up with a couple of
> > uses of uninitialized memory in lib/idr.c. These are (the line numbers
> > may differ slightly):
>
> [snip]
>
> > @@ -39,12 +39,16 @@ static struct idr_layer *alloc_layer(struct idr *idp)
> >  {
> >         struct idr_layer *p;
> >         unsigned long flags;
> > +       int i;
> >
> >         spin_lock_irqsave(&idp->lock, flags);
> >         if ((p = idp->id_free)) {
> >                 idp->id_free = p->ary[0];
> >                 idp->id_free_cnt--;
> > -               p->ary[0] = NULL;
> > +               p->bitmap = 0;
> > +               for(i = 0; i < ARRAY_SIZE(p->ary); ++i)
> > +                       p->ary[i] = NULL;
> > +               p->count = 0;
>
> But aren't these zeroed by idr_cache_ctor() already?

That would make sense. However...

idr_layer_cache is only used for allocations from idr_pre_get().
idr_pre_get() is only called from ida_pre_get().

If this analysis is correct, can this mean that the user has failed to
call ida_pre_get() before idr_get_new() was called?

Though in this case, idr_pre_get() actually *is* called first. Hmm...
I think there's a pretty big chance that kmemcheck is at fault :-(

Vegard

Re: lib/idr.c: initialize struct idr_layer

[Pekka J Enberg]

Hi Vegard,

On Sun, 27 Jan 2008, Vegard Nossum wrote:
> Though in this case, idr_pre_get() actually *is* called first. Hmm...
> I think there's a pretty big chance that kmemcheck is at fault :-(

Depends on how you track object initialization. An object returned by 
kmem_cache_alloc() is always initialized if the cache it comes from has a 
constructor.

			Pekka

Re: lib/idr.c: initialize struct idr_layer

[Vegard Nossum]

On Jan 27, 2008 10:21 PM, Pekka J Enberg <penberg@cs.helsinki.fi> wrote:
> On Sun, 27 Jan 2008, Vegard Nossum wrote:
> > Though in this case, idr_pre_get() actually *is* called first. Hmm...
> > I think there's a pretty big chance that kmemcheck is at fault :-(
>
> Depends on how you track object initialization. An object returned by
> kmem_cache_alloc() is always initialized if the cache it comes from has a
> constructor.

I think there's a pretty big chance I'm wrong (or misunderstanding
something) here, so I'll just ask:
setup_object() from mm/slub.c is what calls the ctor. Shouldn't this
be called from slab_alloc() as well? (I'm marking the data
"uninitialized" there before returning the object.) Otherwise you
might get back an object that is initialized with the previous owner's
data. Or is this intentional?

Thanks.

Vegard

Re: lib/idr.c: initialize struct idr_layer

[Pekka Enberg]

Hi Vegard,

On Jan 27, 2008 11:30 PM, Vegard Nossum <vegard.nossum@gmail.com> wrote:
> > Depends on how you track object initialization. An object returned by
> > kmem_cache_alloc() is always initialized if the cache it comes from has a
> > constructor.
>
> I think there's a pretty big chance I'm wrong (or misunderstanding
> something) here, so I'll just ask:
> setup_object() from mm/slub.c is what calls the ctor. Shouldn't this
> be called from slab_alloc() as well? (I'm marking the data
> "uninitialized" there before returning the object.) Otherwise you
> might get back an object that is initialized with the previous owner's
> data. Or is this intentional?

It's intentional. The caller of kmem_cache_free() is expected to put
the object in such a state that it can be recycled immediately when
kmem_cache_alloc() for that cache is called. You can find the design
rationale for that in Bonwick's original paper on slab:
http://citeseer.ist.psu.edu/bonwick94slab.html

                        Pekka

Re: lib/idr.c: initialize struct idr_layer

[Vegard Nossum]

On Jan 27, 2008 10:35 PM, Pekka Enberg <penberg@cs.helsinki.fi> wrote:
> > I think there's a pretty big chance I'm wrong (or misunderstanding
> > something) here, so I'll just ask:
> > setup_object() from mm/slub.c is what calls the ctor. Shouldn't this
> > be called from slab_alloc() as well? (I'm marking the data
> > "uninitialized" there before returning the object.) Otherwise you
> > might get back an object that is initialized with the previous owner's
> > data. Or is this intentional?
>
> It's intentional. The caller of kmem_cache_free() is expected to put
> the object in such a state that it can be recycled immediately when
> kmem_cache_alloc() for that cache is called. You can find the design
> rationale for that in Bonwick's original paper on slab:
> http://citeseer.ist.psu.edu/bonwick94slab.html

Ow. I guess this is the end of the thread, then :-) My fault. Thanks!

Vegard