Re: [PATCH] lib/idr.c: Zero memory properly in idr_remove_all

[Stefan Richter <stefanr@s5r6.in-berlin.de>]

Andrew Morton wrote:
> On Wed, 14 Jan 2009 09:59:07 +0100 Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
> 
>> > --- a/lib/idr.c~lib-idrc-use-kmem_cache_zalloc-for-the-idr_layer-cache
>> > +++ 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);
>> ...
>> 
>> I wonder if it would be more robust --- or even necessary --- to instead
>> add proper initialization code to get_from_free_list().
>> 
>> As far as David and I tested the new idr using code in firewire, we
>> called idr_remove_all() *and* idr_destroy() before any subsequent
>> idr_get_new().  But in practice, idr_get_new() may of course also happen
>> between idr_remove_all() and idr_destroy().
>> 
>> And then this fix won't be sufficient, would it?
> 
> Maybe I'm having a thick day, but I'm not following you at all here.
> 
> What do you think the remaining problem is?  get_from_free_list()
> starts out with a not-fully-zeroed object?  Something else?

AFAICS:

Before the faulty commit, all code sites which moved something into the
per-idr free list cleared the respective idr_layer.

After the faulty commit, idr_remove_all() forgot to do so.

After your patch, idr_remove_all() still doesn't clear anything.  But
there will typically be idr_destroy() called right after an
idr_remove_all().  idr_destroy() moves all idr_layers out of this idr's
free list back into the kernel-global idr_layer_cache.

Your fix only clears idr_layers which an idr_get_new*() pulls out of the
idr_layer_cache, but not any one which it pulls out of the idr's own
free list.

Hmm, OK.  Seems to be harmless as long as idr users don't start to add
new entries to an idr after they did idr_remove_all().

I presume there is none such user, is there?  Such a usage would be
unusual but not illegal, I suppose.

(Furthermore, I may by thoroughly mistaken about how lib/idr.c works.)
-- 
Stefan Richter
-=====-==--= ---= -===-
http://arcgraph.de/sr/