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

Previous Message by Date: click to view message preview

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

Seems ok. But I'm picky... see below Em Wed, Feb 18, 2004 at 10:48:34PM +0100, Francois Romieu escreveu: > Randy.Dunlap <rddunlap@xxxxxxxx> : > [...] > > 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

Next Message by Date: click to view message preview

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

Em Thu, Feb 19, 2004 at 01:33:25AM +0100, Francois Romieu escreveu: > Arnaldo Carvalho de Melo <acme@xxxxxxxxxxxxxxxx> : > > 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@xxxxxxxxxxxxxx > http://lists.osdl.org/mailman/listinfo/kernel-janitors

Previous Message by Thread: click to view message preview

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

Seems ok. But I'm picky... see below Em Wed, Feb 18, 2004 at 10:48:34PM +0100, Francois Romieu escreveu: > Randy.Dunlap <rddunlap@xxxxxxxx> : > [...] > > 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

Next Message by Thread: click to view message preview

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

Em Thu, Feb 19, 2004 at 01:33:25AM +0100, Francois Romieu escreveu: > Arnaldo Carvalho de Melo <acme@xxxxxxxxxxxxxxxx> : > > 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@xxxxxxxxxxxxxx > http://lists.osdl.org/mailman/listinfo/kernel-janitors