[Kernel-janitors] Re: 2.6.1: net/ipv4/fib_hash.c: check kmem_cache

[Kernel-janitors] Re: 2.6.1: net/ipv4/fib_hash.c: check kmem_cache

[Arnaldo Carvalho de Melo]

Em Tue, Feb 17, 2004 at 10:32:20AM +0100, Walter Harms escreveu:
> 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
> 
> 
> --- linux-2.6.1/net/ipv4/fib_hash.c.org 2004-02-17 09:19:21.000000000 +0100
> +++ linux-2.6.1/net/ipv4/fib_hash.c     2004-02-17 10:20:08.000000000 +0100
> @@ -870,16 +870,23 @@
>  #endif
>  {
>         struct fib_table *tb;
> -
> -       if (fn_hash_kmem = NULL)
> +       int must_free=0;
> +       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)
> +               return NULL;
> +               must_free=1;

Funny code 8) Please let me know when must_free=1; will ever be reached...

In this case we can say "!Never Say Never" 8)

- Arnaldo
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [Kernel-janitors] Re: 2.6.1: net/ipv4/fib_hash.c: check kme

[Arnaldo Carvalho de Melo]

Em Tue, Feb 17, 2004 at 11:09:12AM +0100, Walter Harms escreveu:
> 
> 
> hi acme,
> must free will be reached.
>                if (!fn_hash_kmem)
>                            return NULL;
>                must_free=1;
> 
> lost tab in space :)

Errm... whistles... 8)

- Arnaldo
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [Kernel-janitors] Re: 2.6.1: net/ipv4/fib_hash.c: check kme

[Randy.Dunlap]

On Tue, 17 Feb 2004 07:54:37 -0300 Arnaldo Carvalho de Melo <acme@conectiva.com.br> wrote:

| Em Tue, Feb 17, 2004 at 11:09:12AM +0100, Walter Harms escreveu:
| > 
| > 
| > hi acme,
| > must free will be reached.
| >                if (!fn_hash_kmem)
| >                            return NULL;
| >                must_free=1;
| > 
| > lost tab in space :)
| 
| Errm... whistles... 8)
| 
| - Arnaldo
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

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

--
~Randy
kernel-janitors project:  http://janitor.kernelnewbies.org/
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [Kernel-janitors] Re: 2.6.1: net/ipv4/fib_hash.c: check kme

[Francois Romieu]

Randy.Dunlap <rddunlap@osdl.org> :
[...]
> 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
--- linux-2.6.3-rc1-mm1/net/ipv4/fib_hash.c~kj-fib_hash_init-bad-cache-free	2004-02-18 22:26:40.000000000 +0100
+++ linux-2.6.3-rc1-mm1-fr/net/ipv4/fib_hash.c	2004-02-18 22:37:10.000000000 +0100
@@ -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;
@@ -890,6 +893,11 @@ struct fib_table * __init fib_hash_init(
 	tb->tb_dump = fn_hash_dump;
 	memset(tb->tb_data, 0, sizeof(struct fn_hash));
 	return tb;
+
+err_free:
+	kfree(tb);
+err_out:
+	return NULL;
 }
 
 /* ------------------------------------------------------------------------ */

_
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [Kernel-janitors] Re: 2.6.1: net/ipv4/fib_hash.c: check kme

[Arnaldo Carvalho de Melo]

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

Em Wed, Feb 18, 2004 at 10:48:34PM +0100, Francois Romieu escreveu:
> Randy.Dunlap <rddunlap@osdl.org> :
> [...]
> > 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
> --- linux-2.6.3-rc1-mm1/net/ipv4/fib_hash.c~kj-fib_hash_init-bad-cache-free	2004-02-18 22:26:40.000000000 +0100
> +++ linux-2.6.3-rc1-mm1-fr/net/ipv4/fib_hash.c	2004-02-18 22:37:10.000000000 +0100
> @@ -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;
> @@ -890,6 +893,11 @@ struct fib_table * __init fib_hash_init(
>  	tb->tb_dump = fn_hash_dump;
>  	memset(tb->tb_data, 0, sizeof(struct fn_hash));

Here is the place I'd put err_out

err_out:
>  	return tb;
> +
> +err_free:
> +	kfree(tb);

	tb = NULL;
	goto err_out;

> +err_out:
> +	return NULL;
>  }

But as I said, this would be the way I'd do it, to be consistent with
my style of having as few exit points as possible 8)

- Arnaldo
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [Kernel-janitors] Re: 2.6.1: net/ipv4/fib_hash.c: check kme

[Francois Romieu]

Arnaldo Carvalho de Melo <acme@conectiva.com.br> :
> 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
--- linux-2.6.3/net/ipv4/fib_hash.c~janitor-fib_hash_init-bad-cache-free	2004-02-18 23:40:02.000000000 +0100
+++ linux-2.6.3-fr/net/ipv4/fib_hash.c	2004-02-19 01:17:01.000000000 +0100
@@ -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
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [Kernel-janitors] Re: 2.6.1: net/ipv4/fib_hash.c: check kme

[Arnaldo Carvalho de Melo]

Em Thu, Feb 19, 2004 at 01:33:25AM +0100, Francois Romieu escreveu:
> Arnaldo Carvalho de Melo <acme@conectiva.com.br> :
> > Seems ok. But I'm picky... see below
> 
> Funny: I thought about it as well first :o)

:-) Done deal with me! Thank you guys
 
> 
> 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
> --- linux-2.6.3/net/ipv4/fib_hash.c~janitor-fib_hash_init-bad-cache-free	2004-02-18 23:40:02.000000000 +0100
> +++ linux-2.6.3-fr/net/ipv4/fib_hash.c	2004-02-19 01:17:01.000000000 +0100
> @@ -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
> Kernel-janitors@lists.osdl.org
> http://lists.osdl.org/mailman/listinfo/kernel-janitors
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: Re: [Kernel-janitors] Re: 2.6.1: net/ipv4/fib_hash.c: check

[Francois Romieu]

Walter Harms <WHarms@bfs.de> :
[...]
> 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 :)

It is called twice to have a single return point in the function.

We do not want to cross the streams, do we ?

--
Ueimor
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [Kernel-janitors] Re: 2.6.1: net/ipv4/fib_hash.c: check

[Randy.Dunlap]

On Fri, 20 Feb 2004 00:02:13 +0100 Francois Romieu wrote:

| Walter Harms <WHarms@bfs.de> :
| [...]
| > 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 :)
| 
| It is called twice to have a single return point in the function.
| 
| We do not want to cross the streams, do we ?

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...

--
~Randy


Walter's first patch for ipv4/fib_hash.c:

--- net/ipv4/fib_hash.c.org     2004-01-24 12:42:41.662419720 +0100
+++ net/ipv4/fib_hash.c 2004-01-24 12:44:19.357567800 +0100
@@ -876,6 +876,8 @@
                                                 sizeof(struct fib_node),
                                                 0, SLAB_HWCACHE_ALIGN,
                                                 NULL, NULL);
+       if (!fn_hash_kmem)
+               return NULL;
 
        tb = kmalloc(sizeof(struct fib_table) + sizeof(struct fn_hash), GFP_KERNEL);
        if (tb = NULL)
_______________________________________________
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [Kernel-janitors] Re: 2.6.1: net/ipv4/fib_hash.c: check

[Francois Romieu]

Randy.Dunlap <rddunlap@osdl.org> :
[...]
> 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...

If the second allocation (kmalloc) in the function fails, the first one
(slab) is not freed. Even if it's right, it is not completely kosher/easy to
check (it depends on the behavior of some code in a different function).
It can be done with a "slab_free_needed" variable but it is not exactly
pretty.

OTOH, if the conditional slab allocation is done after the kmalloc, one
can always free the previously/unconditionally kmalloced area. It is
self-contained and shows no dependancy on external code.

The virtues of gotos and single return point in a function are a different
story.

--
Ueimor
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors