linux-kernel - Re: [PATCH] lib/idr.c: Zero memory properly in idr_remove

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <1231792712.6365.31.camel@gaara.bos.redhat.com>
Date:	Mon, 12 Jan 2009 15:38:32 -0500
From:	Kristian Høgsberg <krh@...hat.com>
To:	Manfred Spraul <manfred@...orfullife.com>
Cc:	Stefan Richter <stefanr@...6.in-berlin.de>,
	Andrew Morton <akpm@...ux-foundation.org>, dcm@....org,
	Nadia Derbey <Nadia.Derbey@...l.net>,
	linux1394-devel <linux1394-devel@...ts.sourceforge.net>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	"Paul E. McKenney" <paulmck@...ibm.com>
Subject: Re: [PATCH] lib/idr.c: Zero memory properly in idr_remove_all

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().

> > Every idr use I've seen could just do the whole thing
> > under a mutex and not worry about the awkward retry idea.
> Unfortunately there are some users that do idr_get_new() within a spinlock.
> e.g. from drivers/gpu/drm/drm_gem.c:
> >         if (idr_pre_get(&file_priv->object_idr, GFP_KERNEL) == 0)
> >                 return -ENOMEM;
> >
> >         /* do the allocation under our spinlock */
> >         spin_lock(&file_priv->table_lock);
> >         ret = idr_get_new_above(&file_priv->object_idr, obj, 1, handlep);
> >         spin_unlock(&file_priv->table_lock);
> :-(

That's valid use of the idr interface, idr_get_new_above() won't block
or allocate, just return -EAGAIN if the idr_layer struct allocated by
idr_pre_get() got snatched up in between the two calls.  My complaint
was that in many cases you don't need to access the idr from interrupt
context and you can then just put the idr_pre_get() and
idr_get_new_above() under one big mutex.  Even so, many people just
copy-n-paste the boiler plate code that does the
idr_pre_get()/idr_get_new_above()/if(EAGAIN) goto retry sequence.

cheers,
Kristian

--
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/