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

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

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/