[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <1231942990.8542.7.camel@gaara.bos.redhat.com>
Date: Wed, 14 Jan 2009 09:23:10 -0500
From: Kristian Høgsberg <krh@...hat.com>
To: Andrew Morton <akpm@...ux-foundation.org>
Cc: Manfred Spraul <manfred@...orfullife.com>,
stefanr@...6.in-berlin.de, dcm@....org, Nadia.Derbey@...l.net,
linux1394-devel@...ts.sourceforge.net,
linux-kernel@...r.kernel.org, paulmck@...ibm.com
Subject: Re: [PATCH] lib/idr.c: Zero memory properly in idr_remove_all
On Tue, 2009-01-13 at 14:48 -0800, Andrew Morton wrote:
> On Mon, 12 Jan 2009 21:50:36 +0100
> Manfred Spraul <manfred@...orfullife.com> wrote:
>
> > Kristian H__gsberg wrote:
> > > On Mon, 2009-01-12 at 20:53 +0100, Manfred Spraul wrote:
> > >
> > >> Kristian H__gsberg wrote:
> > >>
> > >>> The problem
> > >>> isn't about returning un-zeroed-out objects to the kmem cache, the
> > >>> problem is returning them to the idr free list.
> > >>>
> > >>>
> > >> I think this is wrong:
> > >> The slab allocator assumes that the objects that are given to
> > >> kmem_cache_free() are properly constructed.
> > >> I.e.: No additional constructor is called prior to returning the object
> > >> from the next kmem_cache_alloc() call.
> > >>
> > >
> > > That's fine, the ctor associated with the kmem cache is called, and in
> > > the case of idr, it does a memset().
> > >
> > No.
> > As I said, the construtor is not called.
> > An object that is given to kmem_cache_free() must be properly constructed.
> > kmem_cache_free() just adds the obj pointer to a list, the next
> > kmem_cache_alloc returns the pointer.
> >
> > This is also documented in mm/slab.c:
> > * The memory is organized in caches, one cache for each object type.
> > * (e.g. inode_cache, dentry_cache, buffer_head, vm_area_struct)
> > * Each cache consists out of many slabs (they are small (usually one
> > * page long) and always contiguous), and each slab contains multiple
> > * initialized objects.
> > *
> > * This means, that your constructor is used only for newly allocated
> > * slabs and you must pass objects with the same initializations to
> > * kmem_cache_free.
> > *
> >
> > If the idr code passes uninitialized objects to kmem_cache_free(), then
> > the next kmem_cache_alloc will return a bad object.
> >
>
> None of this got us much closer to fixing the bug ;)
>
> What do we think of just removing the constructor and using
> kmem_cache_zalloc()?
We still need to zero out the idr_layer before returning it to the idr's
internal free list. I had a memset(p, 0, sizeof *p) just before the
free_layer() call in idr_remove_all() in the original version. I don't
know why that was taken out (some rcu thing?) but if you're looking for
a minimal change to just fix the bug, just put that back in. Or maybe
it now needs to be in idr_layer_rcu_free(), I never really figured out
how rcu works.
> --- a/lib/idr.c~a
> +++ a/lib/idr.c
> @@ -121,7 +121,7 @@ int idr_pre_get(struct idr *idp, gfp_t g
> {
> while (idp->id_free_cnt < IDR_FREE_MAX) {
> struct idr_layer *new;
> - new = kmem_cache_alloc(idr_layer_cache, gfp_mask);
> + new = kmem_cache_zalloc(idr_layer_cache, gfp_mask);
> if (new == NULL)
> return (0);
> move_to_free_list(idp, new);
> @@ -623,16 +623,10 @@ void *idr_replace(struct idr *idp, void
> }
> EXPORT_SYMBOL(idr_replace);
>
> -static void idr_cache_ctor(void *idr_layer)
> -{
> - memset(idr_layer, 0, sizeof(struct idr_layer));
> -}
> -
> void __init idr_init_cache(void)
> {
> idr_layer_cache = kmem_cache_create("idr_layer_cache",
> - sizeof(struct idr_layer), 0, SLAB_PANIC,
> - idr_cache_ctor);
> + sizeof(struct idr_layer), 0, SLAB_PANIC, NULL);
> }
>
> /**
> _
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists