Messages in this thread | | | Date | Wed, 14 Jan 2009 10:48:52 +0100 | From | Stefan Richter <> | Subject | Re: [PATCH] lib/idr.c: Zero memory properly in idr_remove_all |
| |
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/
| |