* [patch-kj] net/ipv4/fib_hash.c: check kmem_cache_create()
@ 2004-06-21 17:18 maximilian attems
2004-06-21 18:21 ` YOSHIFUJI Hideaki / 吉藤英明
0 siblings, 1 reply; 5+ messages in thread
From: maximilian attems @ 2004-06-21 17:18 UTC (permalink / raw)
To: netdev
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;
}
/* ------------------------------------------------------------------------ */
_
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [patch-kj] net/ipv4/fib_hash.c: check kmem_cache_create()
2004-06-21 17:18 [patch-kj] net/ipv4/fib_hash.c: check kmem_cache_create() maximilian attems
@ 2004-06-21 18:21 ` YOSHIFUJI Hideaki / 吉藤英明
2004-06-21 20:04 ` maximilian attems
0 siblings, 1 reply; 5+ messages in thread
From: YOSHIFUJI Hideaki / 吉藤英明 @ 2004-06-21 18:21 UTC (permalink / raw)
To: romieu, janitor; +Cc: netdev
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [patch-kj] net/ipv4/fib_hash.c: check kmem_cache_create()
2004-06-21 18:21 ` YOSHIFUJI Hideaki / 吉藤英明
@ 2004-06-21 20:04 ` maximilian attems
2004-06-21 23:51 ` YOSHIFUJI Hideaki / 吉藤英明
0 siblings, 1 reply; 5+ messages in thread
From: maximilian attems @ 2004-06-21 20:04 UTC (permalink / raw)
To: YOSHIFUJI Hideaki / ?$B5HF#1QL@; +Cc: romieu, janitor, netdev
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [patch-kj] net/ipv4/fib_hash.c: check kmem_cache_create()
2004-06-21 20:04 ` maximilian attems
@ 2004-06-21 23:51 ` YOSHIFUJI Hideaki / 吉藤英明
0 siblings, 0 replies; 5+ messages in thread
From: YOSHIFUJI Hideaki / 吉藤英明 @ 2004-06-21 23:51 UTC (permalink / raw)
To: max; +Cc: romieu, janitor, netdev, yoshfuji
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [patch-kj] net/ipv4/fib_hash.c: check kmem_cache_create()
@ 2004-06-21 23:52 Krishna Kumar
0 siblings, 0 replies; 5+ messages in thread
From: Krishna Kumar @ 2004-06-21 23:52 UTC (permalink / raw)
To: maximilian attems
Cc: janitor, netdev, romieu, YOSHIFUJI Hideaki / ?$B5HF#1QL@
[-- 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 --]
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2004-06-21 23:52 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2004-06-21 17:18 [patch-kj] net/ipv4/fib_hash.c: check kmem_cache_create() maximilian attems
2004-06-21 18:21 ` YOSHIFUJI Hideaki / 吉藤英明
2004-06-21 20:04 ` maximilian attems
2004-06-21 23:51 ` YOSHIFUJI Hideaki / 吉藤英明
-- strict thread matches above, loose matches on Subject: below --
2004-06-21 23:52 Krishna Kumar
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).