lkml.org 
[lkml]   [2009]   [Jan]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [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);
    }

    /**
    _


    \
     
     \ /
      Last update: 2009-01-13 23:51    [from the cache]
    ©2003-2014 Jasper Spaans. hosted at Digital Ocean