[patch-kj] net/ipv4/fib_hash.c: check kmem_cache_create()

[patch-kj] net/ipv4/fib_hash.c: check kmem_cache_create()

[maximilian attems]

From: Francois Romieu <romieu@fr.zoreil.com>

kmem_cache_create leak.

Note: fib_hash_init() can be called many times.

Signed-off-by: Maximilian Attems <janitor@sternwelten.at>



---

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

diff -puN net/ipv4/fib_hash.c~ipv4_fib_hash_check net/ipv4/fib_hash.c
--- linux-2.6.7/net/ipv4/fib_hash.c~ipv4_fib_hash_check	2004-06-18 09:10:27.000000000 +0200
+++ linux-2.6.7-max/net/ipv4/fib_hash.c	2004-06-18 09:10:27.000000000 +0200
@@ -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;
 }
 
 /* ------------------------------------------------------------------------ */

_

Re: [patch-kj] net/ipv4/fib_hash.c: check kmem_cache_create()

[YOSHIFUJI Hideaki / 吉藤英明]

In article <20040621171832.GE1545@sputnik.stro.at> (at Mon, 21 Jun 2004 19:18:32 +0200), maximilian attems <janitor@sternwelten.at> says:

> From: Francois Romieu <romieu@fr.zoreil.com>
> 
> kmem_cache_create leak.
> 
> Note: fib_hash_init() can be called many times.
> 
> Signed-off-by: Maximilian Attems <janitor@sternwelten.at>

Please tell us what kind of leakage do you see?
Is it just enough to return NULL if kmem_cache_create() fails
like this?

--- a/net/ipv4/fib_hash.c	10 Nov 2003 23:40:57 -0000	1.1.1.13
+++ b/net/ipv4/fib_hash.c	21 Jun 2004 18:19:16 -0000
@@ -871,12 +871,14 @@
 {
 	struct fib_table *tb;
 
-	if (fn_hash_kmem == NULL)
+	if (fn_hash_kmem == NULL) {
 		fn_hash_kmem = kmem_cache_create("ip_fib_hash",
 						 sizeof(struct fib_node),
 						 0, SLAB_HWCACHE_ALIGN,
 						 NULL, NULL);
-
+		if (fn_hash_kmem == NULL)
+			return NULL;
+	}
 	tb = kmalloc(sizeof(struct fib_table) + sizeof(struct fn_hash), GFP_KERNEL);
 	if (tb == NULL)
 		return NULL;

-- 
Hideaki YOSHIFUJI @ USAGI Project <yoshfuji@linux-ipv6.org>
GPG FP: 9022 65EB 1ECF 3AD1 0BDF  80D8 4807 F894 E062 0EEA

Re: [patch-kj] net/ipv4/fib_hash.c: check kmem_cache_create()

[maximilian attems]

On Tue, 22 Jun 2004, YOSHIFUJI Hideaki / ?$B5HF#1QL@ wrote:

> In article <20040621171832.GE1545@sputnik.stro.at> (at Mon, 21 Jun 2004 19:18:32 +0200), maximilian attems <janitor@sternwelten.at> says:
> 
> > From: Francois Romieu <romieu@fr.zoreil.com>
> > 
> > kmem_cache_create leak.
> > 
> > Note: fib_hash_init() can be called many times.
> > 
> > Signed-off-by: Maximilian Attems <janitor@sternwelten.at>
> 
> Please tell us what kind of leakage do you see?
> Is it just enough to return NULL if kmem_cache_create() fails
> like this?
> 
> --- a/net/ipv4/fib_hash.c	10 Nov 2003 23:40:57 -0000	1.1.1.13
> +++ b/net/ipv4/fib_hash.c	21 Jun 2004 18:19:16 -0000
> @@ -871,12 +871,14 @@
>  {
>  	struct fib_table *tb;
>  
> -	if (fn_hash_kmem == NULL)
> +	if (fn_hash_kmem == NULL) {
>  		fn_hash_kmem = kmem_cache_create("ip_fib_hash",
>  						 sizeof(struct fib_node),
>  						 0, SLAB_HWCACHE_ALIGN,
>  						 NULL, NULL);
> -
> +		if (fn_hash_kmem == NULL)
> +			return NULL;
> +	}
>  	tb = kmalloc(sizeof(struct fib_table) + sizeof(struct fn_hash), GFP_KERNEL);
>  	if (tb == NULL)
>  		return NULL;

doesn't seem to be enough,
because if (tb == NULL) fn_hash_kmem won't be freed, 
or am i overseeing something?

a++ maks

Re: [patch-kj] net/ipv4/fib_hash.c: check kmem_cache_create()

[YOSHIFUJI Hideaki / 吉藤英明]

In article <20040621200420.GI1545@sputnik.stro.at> (at Mon, 21 Jun 2004 22:04:20 +0200), maximilian attems <max@stro.at> says:

> doesn't seem to be enough,
> because if (tb == NULL) fn_hash_kmem won't be freed, 
> or am i overseeing something?

I believe fn_hash_kmem is not required to (and should not) be freed even 
if kmalloc() fails for the new table.
Users in existing table should be able to continue using fn_hash_kmem.

--yoshfuji

Re: [patch-kj] net/ipv4/fib_hash.c: check kmem_cache_create()

[Krishna Kumar]

[-- Attachment #1.1: Type: text/plain, Size: 3515 bytes --]





> doesn't seem to be enough,
> because if (tb == NULL) fn_hash_kmem won't be freed,
> or am i overseeing something?

Your patch doesn't seem to fix the real problem (and fn_hash_kmem
is not getting leaked, the 'leak' seems intentional to keep using
it even if other allocations in the same routine failed.

The question is should we free all allocated memory and fail the
load since these pointers are used without checking in some ipv4
code (eg fib_lookup) ? Eg. ip_rt_init() return -ENOMEM on memory
failure, but ip_init() doesn't check the value and neither does
inet_init() which calls ip_init(). Should the entire startup
sequence be cleaned up to check return values ? Why continue ipv6
load if some 160 odd bytes are not available on the system ?

(BTW, this code is buggy only if IP_MULTIPLE_TABLES is not defined.)

Thanks,

- KK



|---------+---------------------------->
|         |           maximilian attems|
|         |           <max@stro.at>    |
|         |           Sent by:         |
|         |           netdev-bounce@oss|
|         |           .sgi.com         |
|         |                            |
|         |                            |
|         |           06/21/2004 01:04 |
|         |           PM               |
|         |                            |
|---------+---------------------------->
  >---------------------------------------------------------------------------------------------------------------|
  |                                                                                                               |
  |       To:       "YOSHIFUJI Hideaki / ?$B5HF#1QL@" <yoshfuji@linux-ipv6.org>                                   |
  |       cc:       romieu@fr.zoreil.com, janitor@sternwelten.at, netdev@oss.sgi.com                              |
  |       Subject:  Re: [patch-kj] net/ipv4/fib_hash.c: check kmem_cache_create()                                 |
  |                                                                                                               |
  >---------------------------------------------------------------------------------------------------------------|




On Tue, 22 Jun 2004, YOSHIFUJI Hideaki / ?$B5HF#1QL@ wrote:

> In article <20040621171832.GE1545@sputnik.stro.at> (at Mon, 21 Jun 2004
19:18:32 +0200), maximilian attems <janitor@sternwelten.at> says:
>
> > From: Francois Romieu <romieu@fr.zoreil.com>
> >
> > kmem_cache_create leak.
> >
> > Note: fib_hash_init() can be called many times.
> >
> > Signed-off-by: Maximilian Attems <janitor@sternwelten.at>
>
> Please tell us what kind of leakage do you see?
> Is it just enough to return NULL if kmem_cache_create() fails
> like this?
>
> --- a/net/ipv4/fib_hash.c          10 Nov 2003 23:40:57 -0000
1.1.1.13
> +++ b/net/ipv4/fib_hash.c          21 Jun 2004 18:19:16 -0000
> @@ -871,12 +871,14 @@
>  {
>            struct fib_table *tb;
>
> -          if (fn_hash_kmem == NULL)
> +          if (fn_hash_kmem == NULL) {
>                        fn_hash_kmem = kmem_cache_create("ip_fib_hash",
>
sizeof(struct fib_node),
>
0, SLAB_HWCACHE_ALIGN,
>
NULL, NULL);
> -
> +                      if (fn_hash_kmem == NULL)
> +                                  return NULL;
> +          }
>            tb = kmalloc(sizeof(struct fib_table) + sizeof(struct
fn_hash), GFP_KERNEL);
>            if (tb == NULL)
>                        return NULL;

doesn't seem to be enough,
because if (tb == NULL) fn_hash_kmem won't be freed,
or am i overseeing something?

a++ maks




[-- Attachment #1.2: Type: text/html, Size: 4522 bytes --]

[-- Attachment #2: graycol.gif --]
[-- Type: image/gif, Size: 105 bytes --]

[-- Attachment #3: ecblank.gif --]
[-- Type: image/gif, Size: 45 bytes --]

[-- Attachment #4: pic05285.gif --]
[-- Type: image/gif, Size: 1255 bytes --]