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

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|
|         |           [off-list ref]    |
|         |           Sent by:         |
|         |           netdev-bounce@oss|
|         |           .sgi.com         |
|         |                            |
|         |                            |
|         |           06/21/2004 01:04 |
|         |           PM               |
|         |                            |
|---------+---------------------------->
  >---------------------------------------------------------------------------------------------------------------|
  |                                                                                                               |
  |       To:       "YOSHIFUJI Hideaki / ?$B5HF#1QL@" [off-list ref]                                   |
  |       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 [ref] (at Mon, 21 Jun 2004

19:18:32 +0200), maximilian attems [off-list ref] says:

quoted hunk

quoted

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

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

quoted hunk

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