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

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

[Jesper Juhl]

The Coverity checker found that we may leak memory in
mm/slab.c::alloc_kmemlist()
This should fix the leak and coverity bug #589

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.

The patch introduces one more loop to the function in the failure path :-(
But, since we are very unlikely to ever hit the failure path this shouldn't
be too painfull.

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.
IMO this patch should not go into 2.6.16, but instead spend some time in -mm
with the intention to merge it for 2.6.17 - although it does fix a real leak 
it's not a regression compared to 2.6.15.


Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
---

 mm/slab.c |   35 +++++++++++++++++++++++++++++------
 1 files changed, 29 insertions(+), 6 deletions(-)

--- linux-2.6.16-rc6-mm2-orig/mm/slab.c	2006-03-18 16:55:55.000000000 +0100
+++ linux-2.6.16-rc6-mm2/mm/slab.c	2006-03-18 21:10:56.000000000 +0100
@@ -3399,12 +3399,17 @@ EXPORT_SYMBOL_GPL(kmem_cache_name);
 static int alloc_kmemlist(struct kmem_cache *cachep)
 {
 	int node;
+	int count = -1;
 	struct kmem_list3 *l3;
-	int err = 0;
+	struct array_cache *new;
+	struct array_cache **new_alien;
 
 	for_each_online_node(node) {
-		struct array_cache *nc = NULL, *new;
-		struct array_cache **new_alien = NULL;
+		struct array_cache *nc = NULL;
+
+		new = NULL;
+		new_alien = NULL;
+		count++;
 #ifdef CONFIG_NUMA
 		new_alien = alloc_alien_cache(node, cachep->limit);
 		if (!new_alien)
@@ -3447,10 +3452,28 @@ static int alloc_kmemlist(struct kmem_ca
 					cachep->batchcount + cachep->num;
 		cachep->nodelists[node] = l3;
 	}
-	return err;
+	return 0;
+/*
+   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.
+ */
 fail:
-	err = -ENOMEM;
-	return err;
+	kfree(new);
+	free_alien_cache(new_alien);
+	for_each_online_node(node) {
+		if (count <= 0)
+			break;
+		if (cachep->nodelists[node]) {
+			kfree(cachep->nodelists[node]->shared);
+			free_alien_cache(cachep->nodelists[node]->alien);
+			kfree(cachep->nodelists[node]);
+			cachep->nodelists[node] = NULL;
+		}
+		count--;
+	}
+	return -ENOMEM;
 }
 
 struct ccupdate_struct {

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

[Pekka Enberg]

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;

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

[Jesper Juhl]

Hi Pekka,

On 3/19/06, Pekka Enberg <penberg@cs.helsinki.fi> wrote:
> 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;
>

Thank you very much for your feedback.

I'll create an updated patch with the changes you suggest. They make
perfect sense.


--
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

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

[Jesper Juhl]

Hi Pekka, Andrew, Christoph & everyone else,


On Monday 20 March 2006 09:55, Jesper Juhl wrote:
> Hi Pekka,
> 
> On 3/19/06, Pekka Enberg <penberg@cs.helsinki.fi> wrote:
> > 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;
> >
> 
> Thank you very much for your feedback.
> 
> I'll create an updated patch with the changes you suggest. They make
> perfect sense.
> 

Here's the latest version of my patch to fix the mem leak in alloc_kmemlist().
It should address Pekkas's comments.

Andrew: Do you think this could go into -mm and get some field testing, so 
perhaps it has a chance of making 2.6.17?


Fix memory leak in mm/slab.c::alloc_kmemlist().
If one allocation fails we have to roll-back all allocations made up to the 
point of failure.


Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
---

 mm/slab.c |   36 ++++++++++++++++++++++++++++++------
 1 files changed, 30 insertions(+), 6 deletions(-)

--- linux-2.6.16-rc6-mm2-orig/mm/slab.c	2006-03-18 16:55:55.000000000 +0100
+++ linux-2.6.16-rc6-mm2/mm/slab.c	2006-03-21 22:33:45.000000000 +0100
@@ -3399,12 +3399,17 @@ EXPORT_SYMBOL_GPL(kmem_cache_name);
 static int alloc_kmemlist(struct kmem_cache *cachep)
 {
 	int node;
+	int count = -1;
 	struct kmem_list3 *l3;
-	int err = 0;
+	struct array_cache *new;
+	struct array_cache **new_alien;
 
 	for_each_online_node(node) {
-		struct array_cache *nc = NULL, *new;
-		struct array_cache **new_alien = NULL;
+		struct array_cache *nc = NULL;
+
+		new = NULL;
+		new_alien = NULL;
+		count++;
 #ifdef CONFIG_NUMA
 		new_alien = alloc_alien_cache(node, cachep->limit);
 		if (!new_alien)
@@ -3447,10 +3452,29 @@ static int alloc_kmemlist(struct kmem_ca
 					cachep->batchcount + cachep->num;
 		cachep->nodelists[node] = l3;
 	}
-	return err;
+	return 0;
+/**
+ * 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.
+ */
 fail:
-	err = -ENOMEM;
-	return err;
+	kfree(new);
+	free_alien_cache(new_alien);
+	for_each_online_node(node) {
+		if (count <= 0)
+			break;
+		l3 = cachep->nodelists[node];
+		if (l3) {
+			kfree(l3->shared);
+			free_alien_cache(l3->alien);
+			kfree(l3);
+			cachep->nodelists[node] = NULL;
+		}
+		count--;
+	}
+	return -ENOMEM;
 }
 
 struct ccupdate_struct {

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

[Christoph Lameter]

On Wed, 22 Mar 2006, Jesper Juhl wrote:

> --- linux-2.6.16-rc6-mm2-orig/mm/slab.c	2006-03-18 16:55:55.000000000 +0100
> +++ linux-2.6.16-rc6-mm2/mm/slab.c	2006-03-21 22:33:45.000000000 +0100
> @@ -3399,12 +3399,17 @@ EXPORT_SYMBOL_GPL(kmem_cache_name);
>  static int alloc_kmemlist(struct kmem_cache *cachep)
>  {
>  	int node;
> +	int count = -1;

Count? One could simply go backwards on the node and undo all 
allocations for present nodes. That may be much simpler.

while (node >= 0 && node_isset(node, node_online_map) {

....

	node--;
}

>  	struct kmem_list3 *l3;
> -	int err = 0;

Ok. err is removed sinc we never set it to a nonzero value.

> +	struct array_cache *new;
> +	struct array_cache **new_alien;

Ok. Also not checked.

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

[Christoph Lameter]

On Wed, 22 Mar 2006, Jesper Juhl wrote:

> Fix memory leak in mm/slab.c::alloc_kmemlist().
> If one allocation fails we have to roll-back all allocations made up to the 
> point of failure.

Sorry but you cannot roll back. alloc_kmemlist() could have been used for
tuning the cpucache while accesses to the slab continue. "Rolling back" 
would partially destroy the slab for some nodes and likely cause the 
system to crash. We can only roll back if this is actually an initial 
allocation and we are assured that the whole thing is not yet in use.

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

[Andrew Morton]

Christoph Lameter <clameter@sgi.com> wrote:
>
> On Wed, 22 Mar 2006, Jesper Juhl wrote:
> 
> > Fix memory leak in mm/slab.c::alloc_kmemlist().
> > If one allocation fails we have to roll-back all allocations made up to the 
> > point of failure.
> 
> Sorry but you cannot roll back. alloc_kmemlist() could have been used for
> tuning the cpucache while accesses to the slab continue. "Rolling back" 
> would partially destroy the slab for some nodes and likely cause the 
> system to crash. We can only roll back if this is actually an initial 
> allocation and we are assured that the whole thing is not yet in use.

Well that's a big pickle.  How about allocating everything first, saving it
locally then, if that all worked out, install it?

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

[Christoph Lameter]

On Tue, 21 Mar 2006, Andrew Morton wrote:

> Well that's a big pickle.  How about allocating everything first, saving it
> locally then, if that all worked out, install it?

That wont work for the tuning case. We need to know somehow if we are 
updating or creating a new kmemlist.

But there is so much messy stuff in there. Maybe we should start with a 
cleanup patch to increase our comprehension of the situation?




alloc_kmemlist: Some cleanup in preparation for a real memory leak fix

Inspired by Jesper Juhl's patch from today

1. Get rid of err
	We do not set it to anything else but zero.

2. Drop the CONFIG_NUMA stuff.
	There are definitions for alloc_alien_cache and free_alien_cache()
	that do the right thing for the non NUMA case.

3. Better naming of variables.

4. Remove redundant cachep->nodelists[node] expressions.

Signed-off-by: Christoph Lameter <clameter@sgi.com>
Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>

Index: linux-2.6.16-rc6-mm2/mm/slab.c
===================================================================
--- linux-2.6.16-rc6-mm2.orig/mm/slab.c	2006-03-21 17:45:02.000000000 -0800
+++ linux-2.6.16-rc6-mm2/mm/slab.c	2006-03-21 17:49:16.000000000 -0800
@@ -3421,37 +3421,38 @@ static int alloc_kmemlist(struct kmem_ca
 {
 	int node;
 	struct kmem_list3 *l3;
-	int err = 0;
+	struct array_cache *new_shared;
+	struct array_cache **new_alien;
 
 	for_each_online_node(node) {
-		struct array_cache *nc = NULL, *new;
-		struct array_cache **new_alien = NULL;
-#ifdef CONFIG_NUMA
+
 		new_alien = alloc_alien_cache(node, cachep->limit);
 		if (!new_alien)
 			goto fail;
-#endif
-		new = alloc_arraycache(node, cachep->shared*cachep->batchcount,
+
+		new_shared = alloc_arraycache(node, cachep->shared*cachep->batchcount,
 					0xbaadf00d);
-		if (!new)
+		if (!new_shared)
 			goto fail;
+
 		l3 = cachep->nodelists[node];
 		if (l3) {
+			struct array_cache *shared = l3->shared;
+
 			spin_lock_irq(&l3->list_lock);
 
-			nc = cachep->nodelists[node]->shared;
-			if (nc)
-				free_block(cachep, nc->entry, nc->avail, node);
+			if (shared)
+				free_block(cachep, shared->entry, shared->avail, node);
 
-			l3->shared = new;
-			if (!cachep->nodelists[node]->alien) {
+			l3->shared = new_shared;
+			if (!l3->alien) {
 				l3->alien = new_alien;
 				new_alien = NULL;
 			}
 			l3->free_limit = (1 + nr_cpus_node(node)) *
 					cachep->batchcount + cachep->num;
 			spin_unlock_irq(&l3->list_lock);
-			kfree(nc);
+			kfree(shared);
 			free_alien_cache(new_alien);
 			continue;
 		}
@@ -3462,16 +3463,15 @@ static int alloc_kmemlist(struct kmem_ca
 		kmem_list3_init(l3);
 		l3->next_reap = jiffies + REAPTIMEOUT_LIST3 +
 				((unsigned long)cachep) % REAPTIMEOUT_LIST3;
-		l3->shared = new;
+		l3->shared = new_shared;
 		l3->alien = new_alien;
 		l3->free_limit = (1 + nr_cpus_node(node)) *
 					cachep->batchcount + cachep->num;
 		cachep->nodelists[node] = l3;
 	}
-	return err;
+	return 0;
 fail:
-	err = -ENOMEM;
-	return err;
+	return -ENOMEM;
 }
 
 struct ccupdate_struct {

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

[Christoph Lameter]

Maybe it will work this way? (Testing may be difficult without proper 
failure scenarios, not tested yet).


Fix memory leak in alloc_kmemlist

We have had this memory leak for awhile now. The situation is complicated by
the use of alloc_kmemlist() as a function to resize various caches by
do_tune_cpucache().

What we do here is first of all make sure that we deallocate properly
in the loop over all the nodes.

If we are just resizing caches then we can simply return with -ENOMEM
if an allocation fails.

If the cache is new then we need to rollback and remove all earlier
allocations.

We detect that a cache is new by checking if the link to the global
cache chain has been setup. This is a bit hackish ....

(also fix up too overlong lines that I added in the last patch...)

Signed-off-by: Christoph Lameter <clameter@sgi.com>

Index: linux-2.6.16-rc6-mm2/mm/slab.c
===================================================================
--- linux-2.6.16-rc6-mm2.orig/mm/slab.c	2006-03-21 17:49:16.000000000 -0800
+++ linux-2.6.16-rc6-mm2/mm/slab.c	2006-03-21 18:27:28.000000000 -0800
@@ -3415,7 +3415,7 @@ const char *kmem_cache_name(struct kmem_
 EXPORT_SYMBOL_GPL(kmem_cache_name);
 
 /*
- * This initializes kmem_list3 for all nodes.
+ * This initializes kmem_list3 or resizes varioius caches for all nodes.
  */
 static int alloc_kmemlist(struct kmem_cache *cachep)
 {
@@ -3430,10 +3430,13 @@ static int alloc_kmemlist(struct kmem_ca
 		if (!new_alien)
 			goto fail;
 
-		new_shared = alloc_arraycache(node, cachep->shared*cachep->batchcount,
+		new_shared = alloc_arraycache(node,
+				cachep->shared*cachep->batchcount,
 					0xbaadf00d);
-		if (!new_shared)
+		if (!new_shared) {
+			free_alien_cache(new_alien);
 			goto fail;
+		}
 
 		l3 = cachep->nodelists[node];
 		if (l3) {
@@ -3442,7 +3445,8 @@ static int alloc_kmemlist(struct kmem_ca
 			spin_lock_irq(&l3->list_lock);
 
 			if (shared)
-				free_block(cachep, shared->entry, shared->avail, node);
+				free_block(cachep, shared->entry,
+						shared->avail, node);
 
 			l3->shared = new_shared;
 			if (!l3->alien) {
@@ -3457,8 +3461,11 @@ static int alloc_kmemlist(struct kmem_ca
 			continue;
 		}
 		l3 = kmalloc_node(sizeof(struct kmem_list3), GFP_KERNEL, node);
-		if (!l3)
+		if (!l3) {
+			free_alien_cache(new_alien);
+			kfree(new_shared);
 			goto fail;
+		}
 
 		kmem_list3_init(l3);
 		l3->next_reap = jiffies + REAPTIMEOUT_LIST3 +
@@ -3470,7 +3477,21 @@ static int alloc_kmemlist(struct kmem_ca
 		cachep->nodelists[node] = l3;
 	}
 	return 0;
+
 fail:
+	if (!cachep->next.next) {
+		/* Cache is not active yet. Roll back what we did */
+		node--;
+		while (node >= 0 && cachep->nodelists[node]) {
+			l3 = cachep->nodelists[node];
+
+			kfree(l3->shared);
+			free_alien_cache(l3->alien);
+			kfree(l3);
+			cachep->nodelists[node] = NULL;
+			node--;
+		}
+	}
 	return -ENOMEM;
 }