Messages in this thread Patch in this message | | | Date | Tue, 13 Jan 2009 14:48:02 -0800 | From | Andrew Morton <> | Subject | Re: [PATCH] lib/idr.c: Zero memory properly in idr_remove_all |
| |
On Mon, 12 Jan 2009 21:50:36 +0100 Manfred Spraul <manfred@colorfullife.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()?
--- 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); } /** _
| |