Messages in this thread | | | Date | Wed, 14 Jan 2009 09:59:07 +0100 | From | Stefan Richter <> | Subject | Re: [PATCH] lib/idr.c: Zero memory properly in idr_remove_all |
| |
Andrew Morton wrote: > This problem was introduced by > > commit cf481c20c476ad2c0febdace9ce23f5a4db19582 ... > which was first released in 2.6.27. > > There are no known codesites which trigger this bug in 2.6.27 or 2.6.28. > The post-2.6.28 firewire changes are the only known triggerer.
(They became post-2.6.29 changes since I missed my chance by two days or so.)
> There might of course be not-yet-discovered triggerers in 2.6.27 and > 2.6.28, and there might be out-of-tree triggerers which are added to those > kernel versions. I'll let the -stable guys decide whether they want to > backport this fix.
I vote for the cf481c20c breakage be fixed in 2.6.27.y and 2.6.28.y. Either by your patch or by something equivalent.
*Every* call to idr_remove_all() will add unclean idr_layers to the pool. There may be more actual occurrences of this than what was found so far because - the results may be kernel panics without traces, or with traces that don't point to an idr problem so clearly, - one or another kernel debugging build option prevents this bug from happening. A kernel developer who does most of his tests with debug options enabled won't notice.
... > diff -puN lib/idr.c~lib-idrc-use-kmem_cache_zalloc-for-the-idr_layer-cache lib/idr.c > --- 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? -- Stefan Richter -=====-==--= ---= -===- http://arcgraph.de/sr/
| |