2.6.1: net/ipv4/fib_hash.c: check kmem_cache

hi list,
this patch is the same as before and fixes the problem acme found.
must_free is only set if kmem_cache_create is realy used and only then freed.

background:
fib_hash_init() can be called many times. if fn_hash_kmem
is already set and kmalloc fails the wrong fn_hash_kmem is freed. I think this can only happen if CONFIG_IP_MULTIPLE_TABLES is set.

regards,
walter

net/ipv4/fib_hash.c 2004-02-17 10:20:08.000000000 +0100 renamed from net/ipv4/fib_hash.c.org 2004-02-17 09:19:21.000000000 +0100 +0 -1

@@ -870,16 +870,23 @@
 #endif
 {
        struct fib_table *tb;
-

hi acme,
must free will be reached.

               if (!fn_hash_kmem)
                           return NULL;
               must_free=1;

lost tab in space :)

So, Arnaldo, does this mean that you like this patch?

Randy.Dunlap <[email protected]> :
[...]

Quoted 1 line

So, Arnaldo, does this mean that you like this patch?

Walter, do you see any logic error in the following version of your patch ?

kmem_cache_create leak.

Note: fib_hash_init() can be called many times.

 net/ipv4/fib_hash.c |   18 +++++++++++++-----
 1 files changed, 13 insertions(+), 5 deletions(-)

diff -puN net/ipv4/fib_hash.c~kj-fib_hash_init-bad-cache-free net/ipv4/fib_hash.c

net/ipv4/fib_hash.c renamed from net/ipv4/fib_hash.c~kj-fib_hash_init-bad-cache-free +8 -5

@@ -871,15 +871,18 @@ struct fib_table * __init fib_hash_init(
 {
 	struct fib_table *tb;
 
-	if (fn_hash_kmem = NULL)
+	tb = kmalloc(sizeof(struct fib_table) + sizeof(struct fn_hash), GFP_KERNEL);
+	if (!tb)
+		goto err_out;
+
+	if (!fn_hash_kmem) {
 		fn_hash_kmem = kmem_cache_create("ip_fib_hash",
 						 sizeof(struct fib_node),
 						 0, SLAB_HWCACHE_ALIGN,
 						 NULL, NULL);
-
-	tb = kmalloc(sizeof(struct fib_table) + sizeof(struct fn_hash), GFP_KERNEL);
-	if (tb = NULL)
-		return NULL;
+		if (!fn_hash_kmem)
+			goto err_free;
+	}
 
 	tb->tb_id = id;
 	tb->tb_lookup = fn_hash_lookup;

Seems ok. But I'm picky... see below

Arnaldo Carvalho de Melo <[email protected]> :

Quoted 1 line

Seems ok. But I'm picky... see below

Funny: I thought about it as well first :o)

kmem_cache_create leak.

Note: fib_hash_init() can be called many times.

 net/ipv4/fib_hash.c |   19 ++++++++++++++-----
 1 files changed, 14 insertions(+), 5 deletions(-)

diff -puN net/ipv4/fib_hash.c~janitor-fib_hash_init-bad-cache-free net/ipv4/fib_hash.c

net/ipv4/fib_hash.c renamed from net/ipv4/fib_hash.c~janitor-fib_hash_init-bad-cache-free +14 -5

@@ -871,15 +871,18 @@ struct fib_table * __init fib_hash_init(
 {
 	struct fib_table *tb;
 
-	if (fn_hash_kmem = NULL)
+	tb = kmalloc(sizeof(struct fib_table) + sizeof(struct fn_hash), GFP_KERNEL);
+	if (!tb)
+		goto err_out;
+
+	if (!fn_hash_kmem) {
 		fn_hash_kmem = kmem_cache_create("ip_fib_hash",
 						 sizeof(struct fib_node),
 						 0, SLAB_HWCACHE_ALIGN,
 						 NULL, NULL);
-
-	tb = kmalloc(sizeof(struct fib_table) + sizeof(struct fn_hash), GFP_KERNEL);
-	if (tb = NULL)
-		return NULL;
+		if (!fn_hash_kmem)
+			goto err_free;
+	}
 
 	tb->tb_id = id;
 	tb->tb_lookup = fn_hash_lookup;

@@ -889,7 +892,13 @@ struct fib_table * __init fib_hash_init(
 	tb->tb_select_default = fn_hash_select_default;
 	tb->tb_dump = fn_hash_dump;
 	memset(tb->tb_data, 0, sizeof(struct fn_hash));
+err_out:
 	return tb;
+
+err_free:
+	kfree(tb);
+	tb = NULL;
+	goto err_out;
 }
 
 /* ------------------------------------------------------------------------ */

_

_______________________________________________
Kernel-janitors mailing list
[email protected]
http://lists.osdl.org/mailman/listinfo/kernel-janitors

nice idea to turn the problem on its head. I would prefer a
if (!tb)
return NULL;
instead a goto (as opposed to acme) i dont like goto's if called only once :)

Hm, well, ok .... I've just reviewed 15 emails about ipv4/fib_hash.c, and (sadly ?) I've come to the conclusion that Walter's very first patch (at least first that I see now) is the best one I've seen for this. However, I'm probably missing something, so tell me what/why...