Re: [PATCH] fix memory leak in mm/slab.c::alloc_kmemlist() (try #2)

Pekka Enberg
2006-03-19T18:40:36Z

On 3/18/06, Jesper Juhl <jesper.juhl@gmail.com> wrote:
> Currently the only caller of alloc_kmemlist() will BUG() if alloc_kmemlist()
> fails, but that doesn't mean we shouldn't clean up properly IMHO. Also, the
> caller (do_tune_cpucache()) could maybe be changed in the future to do
> something more clever than just BUG() and in that case we really shouldn't
> be leaking memory when we return -ENOMEM.

Yeah, and BUG() can be no-op for embedded.

On 3/18/06, Jesper Juhl <jesper.juhl@gmail.com> wrote:
> The patch has been compile and boot tested on x86, but since I'm not very
> intimate with the slab code I'd appreciate it if someone would take a close
> look on the changes before merging them.

You probably didn't hit the error path on your x86 box. The patch
looks good to me for -mm although there's few comments below.

> +/*
> + If one or more allocations fail we need to undo all allocations done up to
> + this point.
> + Unfortunately this means yet another loop, but since this only happens on
> + failure and frees up memory in a memory-tight situation, it's not too bad.
> + */

The formatting of this comment looks strange.

> + for_each_online_node(node) {
> + if (count <= 0)
> + break;
> + if (cachep->nodelists[node]) {

Would probably make sense to extract the above expression into local
variable to reduce kernel text size.

> + kfree(cachep->nodelists[node]->shared);
> + free_alien_cache(cachep->nodelists[node]->alien);
> + kfree(cachep->nodelists[node]);
> + cachep->nodelists[node] = NULL;
> + }
> + count--;
> + }
> + return -ENOMEM;