[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@xxxxxxxxx>
---

 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 {



-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/