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-2011 Jasper Spaans