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

[Krishna Kumar <kumarkr@xxxxxxxxxx>]

> 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@xxxxxxx>

maximilian attems <max@xxxxxxx> Sent by: netdev-bounce@xxxxxxxxxxx 06/21/2004 01:04 PM

To: "YOSHIFUJI Hideaki / ?$B5HF#1QL@" <yoshfuji@xxxxxxxxxxxxxx> cc: romieu@xxxxxxxxxxxxx, janitor@xxxxxxxxxxxxxx, netdev@xxxxxxxxxxx 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@xxxxxxxxxxxxxxx> (at Mon, 21 Jun 2004 19:18:32 +0200), maximilian attems <janitor@xxxxxxxxxxxxxx> says:
>
> > From: Francois Romieu <romieu@xxxxxxxxxxxxx>
> >
> > kmem_cache_create leak.
> >
> > Note: fib_hash_init() can be called many times.
> >
> > Signed-off-by: Maximilian Attems <janitor@xxxxxxxxxxxxxx>
>
> 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