* [PATCH] net: reference the ipv4 sysctl table header @ 2012-03-26 22:23 Djalal Harouni 2012-03-26 22:24 ` David Miller 2012-03-26 22:50 ` Eric W. Biederman 0 siblings, 2 replies; 10+ messages in thread From: Djalal Harouni @ 2012-03-26 22:23 UTC (permalink / raw) To: David S. Miller, Eric W. Biederman, Al Viro; +Cc: netdev I've been analysing some kmemleak reports of an internal module, and found that there are false positive reports of unreferenced objects. The following patch is just a clean up for one of those false positives, this is for the /proc/sys/net/ipv4 sysctl table. As I've said there are other reports but don't know if it is worth to write patches for them. --- From: Djalal Harouni <tixxdz@opendz.org> Subject: [PATCH] net: reference the ipv4 sysctl table header Reference the ipv4 sysctl table header allocated and returned by register_sysctl_paths(). Signed-off-by: Djalal Harouni <tixxdz@opendz.org> --- include/net/ip.h | 2 +- net/ipv4/af_inet.c | 12 ++++++++++-- net/ipv4/route.c | 10 ++++++++-- 3 files changed, 19 insertions(+), 5 deletions(-) diff --git a/include/net/ip.h b/include/net/ip.h index b53d65f..6a12687 100644 --- a/include/net/ip.h +++ b/include/net/ip.h @@ -235,7 +235,7 @@ extern int sysctl_ip_dynaddr; extern void ipfrag_init(void); -extern void ip_static_sysctl_init(void); +extern int ip_static_sysctl_init(void); static inline bool ip_is_fragment(const struct iphdr *iph) { diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c index fdf49fd..340d298 100644 --- a/net/ipv4/af_inet.c +++ b/net/ipv4/af_inet.c @@ -1666,10 +1666,14 @@ static int __init inet_init(void) * Tell SOCKET that we are alive... */ - (void)sock_register(&inet_family_ops); + rc = sock_register(&inet_family_ops); + if (rc) + goto out_unregister_ping_prot; #ifdef CONFIG_SYSCTL - ip_static_sysctl_init(); + rc = ip_static_sysctl_init(); + if (rc) + goto out_unregister_sock; #endif tcp_prot.sysctl_mem = init_net.ipv4.sysctl_tcp_mem; @@ -1751,6 +1755,10 @@ static int __init inet_init(void) rc = 0; out: return rc; +out_unregister_sock: + sock_unregister(PF_INET); +out_unregister_ping_prot: + proto_unregister(&ping_prot); out_unregister_raw_proto: proto_unregister(&raw_prot); out_unregister_udp_proto: diff --git a/net/ipv4/route.c b/net/ipv4/route.c index 12ccf88..bc899f2 100644 --- a/net/ipv4/route.c +++ b/net/ipv4/route.c @@ -3500,12 +3500,18 @@ int __init ip_rt_init(void) } #ifdef CONFIG_SYSCTL +static struct ctl_table_header *ip4_base; + /* * We really need to sanitize the damn ipv4 init order, then all * this nonsense will go away. */ -void __init ip_static_sysctl_init(void) +int __init ip_static_sysctl_init(void) { - register_sysctl_paths(ipv4_path, ipv4_skeleton); + ip4_base = register_sysctl_paths(ipv4_path, ipv4_skeleton); + if (!ip4_base) + return -ENOMEM; + + return 0; } #endif -- 1.7.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] net: reference the ipv4 sysctl table header 2012-03-26 22:23 [PATCH] net: reference the ipv4 sysctl table header Djalal Harouni @ 2012-03-26 22:24 ` David Miller 2012-03-26 22:42 ` Djalal Harouni 2012-03-28 16:32 ` Steven Rostedt 2012-03-26 22:50 ` Eric W. Biederman 1 sibling, 2 replies; 10+ messages in thread From: David Miller @ 2012-03-26 22:24 UTC (permalink / raw) To: tixxdz; +Cc: ebiederm, viro, netdev From: Djalal Harouni <tixxdz@opendz.org> Date: Mon, 26 Mar 2012 23:23:59 +0100 > +static struct ctl_table_header *ip4_base; > + > /* > * We really need to sanitize the damn ipv4 init order, then all > * this nonsense will go away. > */ > -void __init ip_static_sysctl_init(void) > +int __init ip_static_sysctl_init(void) > { > - register_sysctl_paths(ipv4_path, ipv4_skeleton); > + ip4_base = register_sysctl_paths(ipv4_path, ipv4_skeleton); This is so incredibly stupid, just panic() or similar if this returns NULL. And find another way to annotate this for memleak so we don't need to waste an entire pointer, which is never used, in the data section. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] net: reference the ipv4 sysctl table header 2012-03-26 22:24 ` David Miller @ 2012-03-26 22:42 ` Djalal Harouni 2012-03-28 16:32 ` Steven Rostedt 1 sibling, 0 replies; 10+ messages in thread From: Djalal Harouni @ 2012-03-26 22:42 UTC (permalink / raw) To: David Miller; +Cc: ebiederm, viro, netdev On Mon, Mar 26, 2012 at 06:24:11PM -0400, David Miller wrote: > From: Djalal Harouni <tixxdz@opendz.org> > Date: Mon, 26 Mar 2012 23:23:59 +0100 > > > +static struct ctl_table_header *ip4_base; > > + > > /* > > * We really need to sanitize the damn ipv4 init order, then all > > * this nonsense will go away. > > */ > > -void __init ip_static_sysctl_init(void) > > +int __init ip_static_sysctl_init(void) > > { > > - register_sysctl_paths(ipv4_path, ipv4_skeleton); > > + ip4_base = register_sysctl_paths(ipv4_path, ipv4_skeleton); > > This is so incredibly stupid, just panic() or similar if this > returns NULL. > > And find another way to annotate this for memleak so we don't need to > waste an entire pointer, which is never used, in the data section. Ok, thank you. > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- tixxdz http://opendz.org ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] net: reference the ipv4 sysctl table header 2012-03-26 22:24 ` David Miller 2012-03-26 22:42 ` Djalal Harouni @ 2012-03-28 16:32 ` Steven Rostedt 2012-03-28 20:51 ` David Miller 1 sibling, 1 reply; 10+ messages in thread From: Steven Rostedt @ 2012-03-28 16:32 UTC (permalink / raw) To: David Miller; +Cc: tixxdz, ebiederm, viro, netdev On Mon, Mar 26, 2012 at 06:24:11PM -0400, David Miller wrote: > From: Djalal Harouni <tixxdz@opendz.org> > Date: Mon, 26 Mar 2012 23:23:59 +0100 > > > +static struct ctl_table_header *ip4_base; > > + > > /* > > * We really need to sanitize the damn ipv4 init order, then all > > * this nonsense will go away. > > */ > > -void __init ip_static_sysctl_init(void) > > +int __init ip_static_sysctl_init(void) > > { > > - register_sysctl_paths(ipv4_path, ipv4_skeleton); > > + ip4_base = register_sysctl_paths(ipv4_path, ipv4_skeleton); > > This is so incredibly stupid, just panic() or similar if this > returns NULL. > > And find another way to annotate this for memleak so we don't need to > waste an entire pointer, which is never used, in the data section. I just started using kmemleak and notice that it reports false positives for several __init functions that call register_sysctl_paths(). The fix you want is: { sturct ctl_table_header *head; head = register_sysctl_paths(ipv4_path, ipv4_skeleton); BUG_ON(!head); kmemleak_ignore(head); No need to waste a pointer just to keep the reference around for kmemleak. -- Steve ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] net: reference the ipv4 sysctl table header 2012-03-28 16:32 ` Steven Rostedt @ 2012-03-28 20:51 ` David Miller 2012-03-31 14:29 ` Djalal Harouni 0 siblings, 1 reply; 10+ messages in thread From: David Miller @ 2012-03-28 20:51 UTC (permalink / raw) To: rostedt; +Cc: tixxdz, ebiederm, viro, netdev From: Steven Rostedt <rostedt@goodmis.org> Date: Wed, 28 Mar 2012 12:32:20 -0400 > I just started using kmemleak and notice that it reports false positives > for several __init functions that call register_sysctl_paths(). The fix > you want is: > > { > sturct ctl_table_header *head; > > head = register_sysctl_paths(ipv4_path, ipv4_skeleton); > BUG_ON(!head); > kmemleak_ignore(head); > > > No need to waste a pointer just to keep the reference around for > kmemleak. That looks a lot saner than the other suggestions, indeed. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] net: reference the ipv4 sysctl table header 2012-03-28 20:51 ` David Miller @ 2012-03-31 14:29 ` Djalal Harouni 0 siblings, 0 replies; 10+ messages in thread From: Djalal Harouni @ 2012-03-31 14:29 UTC (permalink / raw) To: David Miller; +Cc: rostedt, ebiederm, viro, netdev On Wed, Mar 28, 2012 at 04:51:32PM -0400, David Miller wrote: > From: Steven Rostedt <rostedt@goodmis.org> > Date: Wed, 28 Mar 2012 12:32:20 -0400 > > > I just started using kmemleak and notice that it reports false positives > > for several __init functions that call register_sysctl_paths(). The fix > > you want is: > > > > { > > sturct ctl_table_header *head; > > > > head = register_sysctl_paths(ipv4_path, ipv4_skeleton); > > BUG_ON(!head); > > kmemleak_ignore(head); > > > > > > No need to waste a pointer just to keep the reference around for > > kmemleak. Thanks Steven, yes it's a nice solution for false positive ones. > That looks a lot saner than the other suggestions, indeed. Yes, however now I'm not sure that these are real false positive ones, please check my other email, and someone who knows the code should take a closer look. Thank. > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- tixxdz http://opendz.org ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] net: reference the ipv4 sysctl table header 2012-03-26 22:23 [PATCH] net: reference the ipv4 sysctl table header Djalal Harouni 2012-03-26 22:24 ` David Miller @ 2012-03-26 22:50 ` Eric W. Biederman 2012-03-26 23:21 ` Djalal Harouni 1 sibling, 1 reply; 10+ messages in thread From: Eric W. Biederman @ 2012-03-26 22:50 UTC (permalink / raw) To: Djalal Harouni; +Cc: David S. Miller, Al Viro, netdev Djalal Harouni <tixxdz@opendz.org> writes: > I've been analysing some kmemleak reports of an internal module, and > found that there are false positive reports of unreferenced objects. > > The following patch is just a clean up for one of those false positives, > this is for the /proc/sys/net/ipv4 sysctl table. > As I've said there are other reports but don't know if it is worth to > write patches for them. So the problem here is that you register a sysctl and don't keep a pointer to the returned sysctl_header? So kmemleak complains? I would expect the other sysctl data structures to have such a pointer, so I don't know why kmemleak would complain. Does my recent sysctl rewrite affect when this kmemleak is reported? Scratching my head to understand what the complain is. > --- > From: Djalal Harouni <tixxdz@opendz.org> > Subject: [PATCH] net: reference the ipv4 sysctl table header > > Reference the ipv4 sysctl table header allocated and returned by > register_sysctl_paths(). > > Signed-off-by: Djalal Harouni <tixxdz@opendz.org> > --- > include/net/ip.h | 2 +- > net/ipv4/af_inet.c | 12 ++++++++++-- > net/ipv4/route.c | 10 ++++++++-- > 3 files changed, 19 insertions(+), 5 deletions(-) > > diff --git a/include/net/ip.h b/include/net/ip.h > index b53d65f..6a12687 100644 > --- a/include/net/ip.h > +++ b/include/net/ip.h > @@ -235,7 +235,7 @@ extern int sysctl_ip_dynaddr; > > extern void ipfrag_init(void); > > -extern void ip_static_sysctl_init(void); > +extern int ip_static_sysctl_init(void); > > static inline bool ip_is_fragment(const struct iphdr *iph) > { > diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c > index fdf49fd..340d298 100644 > --- a/net/ipv4/af_inet.c > +++ b/net/ipv4/af_inet.c > @@ -1666,10 +1666,14 @@ static int __init inet_init(void) > * Tell SOCKET that we are alive... > */ > > - (void)sock_register(&inet_family_ops); > + rc = sock_register(&inet_family_ops); > + if (rc) > + goto out_unregister_ping_prot; > > #ifdef CONFIG_SYSCTL > - ip_static_sysctl_init(); > + rc = ip_static_sysctl_init(); > + if (rc) > + goto out_unregister_sock; > #endif > > tcp_prot.sysctl_mem = init_net.ipv4.sysctl_tcp_mem; > @@ -1751,6 +1755,10 @@ static int __init inet_init(void) > rc = 0; > out: > return rc; > +out_unregister_sock: > + sock_unregister(PF_INET); > +out_unregister_ping_prot: > + proto_unregister(&ping_prot); > out_unregister_raw_proto: > proto_unregister(&raw_prot); > out_unregister_udp_proto: > diff --git a/net/ipv4/route.c b/net/ipv4/route.c > index 12ccf88..bc899f2 100644 > --- a/net/ipv4/route.c > +++ b/net/ipv4/route.c > @@ -3500,12 +3500,18 @@ int __init ip_rt_init(void) > } > > #ifdef CONFIG_SYSCTL > +static struct ctl_table_header *ip4_base; > + > /* > * We really need to sanitize the damn ipv4 init order, then all > * this nonsense will go away. > */ > -void __init ip_static_sysctl_init(void) > +int __init ip_static_sysctl_init(void) > { > - register_sysctl_paths(ipv4_path, ipv4_skeleton); > + ip4_base = register_sysctl_paths(ipv4_path, ipv4_skeleton); > + if (!ip4_base) > + return -ENOMEM; > + > + return 0; > } > #endif ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] net: reference the ipv4 sysctl table header 2012-03-26 22:50 ` Eric W. Biederman @ 2012-03-26 23:21 ` Djalal Harouni 2012-03-28 2:35 ` Eric W. Biederman 0 siblings, 1 reply; 10+ messages in thread From: Djalal Harouni @ 2012-03-26 23:21 UTC (permalink / raw) To: Eric W. Biederman; +Cc: David S. Miller, Al Viro, netdev On Mon, Mar 26, 2012 at 03:50:30PM -0700, Eric W. Biederman wrote: > Djalal Harouni <tixxdz@opendz.org> writes: > > > I've been analysing some kmemleak reports of an internal module, and > > found that there are false positive reports of unreferenced objects. > > > > The following patch is just a clean up for one of those false positives, > > this is for the /proc/sys/net/ipv4 sysctl table. > > As I've said there are other reports but don't know if it is worth to > > write patches for them. > > So the problem here is that you register a sysctl and don't keep a > pointer to the returned sysctl_header? So kmemleak complains? Right. > I would expect the other sysctl data structures to have such a pointer, > so I don't know why kmemleak would complain. > > Does my recent sysctl rewrite affect when this kmemleak is reported? Actually yes, after a recent pull (which includes your recent sysctl work), some of these false positive reports started to appear. Anyway they seem false positive ones, since keeping a reference to sysctl_header as in my previous (ugly) patch will quiet the last two ones. unreferenced object 0xffff88003dc17c70 (size 192): comm "swapper/0", pid 0, jiffies 4294667341 (age 86.781s) hex dump (first 32 bytes): 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ backtrace: [<ffffffff819b5d05>] kmemleak_alloc+0x25/0x50 [<ffffffff81154203>] __kmalloc+0x133/0x240 [<ffffffff811dc9b7>] __register_sysctl_paths+0x127/0x1d0 [<ffffffff811dca76>] register_sysctl_paths+0x16/0x20 [<ffffffff811dca93>] register_sysctl_table+0x13/0x20 [<ffffffff820cbe55>] sysctl_init+0x10/0x14 [<ffffffff820d6633>] proc_sys_init+0x2f/0x31 [<ffffffff820d6410>] proc_root_init+0xa5/0xa7 [<ffffffff820b5c39>] start_kernel+0x35c/0x38a [<ffffffff820b5321>] x86_64_start_reservations+0x131/0x136 [<ffffffff820b5413>] x86_64_start_kernel+0xed/0xf4 [<ffffffffffffffff>] 0xffffffffffffffff unreferenced object 0xffff88003d65c000 (size 96): comm "swapper/0", pid 0, jiffies 4294667341 (age 86.782s) hex dump (first 32 bytes): 60 86 1c 82 ff ff ff ff 00 00 00 00 01 00 00 00 `............... 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ backtrace: [<ffffffff819b5d05>] kmemleak_alloc+0x25/0x50 [<ffffffff81154203>] __kmalloc+0x133/0x240 [<ffffffff811dc1f2>] __register_sysctl_table+0x52/0x520 [<ffffffff811dc7ac>] register_leaf_sysctl_tables+0xec/0x1b0 [<ffffffff811dc77c>] register_leaf_sysctl_tables+0xbc/0x1b0 [<ffffffff811dc77c>] register_leaf_sysctl_tables+0xbc/0x1b0 [<ffffffff811dc9e9>] __register_sysctl_paths+0x159/0x1d0 [<ffffffff811dca76>] register_sysctl_paths+0x16/0x20 [<ffffffff811dca93>] register_sysctl_table+0x13/0x20 [<ffffffff820cbe55>] sysctl_init+0x10/0x14 [<ffffffff820d6633>] proc_sys_init+0x2f/0x31 [<ffffffff820d6410>] proc_root_init+0xa5/0xa7 [<ffffffff820b5c39>] start_kernel+0x35c/0x38a [<ffffffff820b5321>] x86_64_start_reservations+0x131/0x136 [<ffffffff820b5413>] x86_64_start_kernel+0xed/0xf4 [<ffffffffffffffff>] 0xffffffffffffffff unreferenced object 0xffff88003d65ddd0 (size 96): comm "swapper/0", pid 0, jiffies 4294667341 (age 86.782s) hex dump (first 32 bytes): 00 86 1c 82 ff ff ff ff 00 00 00 00 01 00 00 00 ................ 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ backtrace: [<ffffffff819b5d05>] kmemleak_alloc+0x25/0x50 [<ffffffff81154203>] __kmalloc+0x133/0x240 [<ffffffff811dc1f2>] __register_sysctl_table+0x52/0x520 [<ffffffff811dc7ac>] register_leaf_sysctl_tables+0xec/0x1b0 [<ffffffff811dc77c>] register_leaf_sysctl_tables+0xbc/0x1b0 [<ffffffff811dc9e9>] __register_sysctl_paths+0x159/0x1d0 [<ffffffff811dca76>] register_sysctl_paths+0x16/0x20 [<ffffffff811dca93>] register_sysctl_table+0x13/0x20 [<ffffffff820cbe55>] sysctl_init+0x10/0x14 [<ffffffff820d6633>] proc_sys_init+0x2f/0x31 [<ffffffff820d6410>] proc_root_init+0xa5/0xa7 [<ffffffff820b5c39>] start_kernel+0x35c/0x38a [<ffffffff820b5321>] x86_64_start_reservations+0x131/0x136 [<ffffffff820b5413>] x86_64_start_kernel+0xed/0xf4 [<ffffffffffffffff>] 0xffffffffffffffff unreferenced object 0xffff88003bc41090 (size 96): comm "swapper/0", pid 1, jiffies 4294669841 (age 84.299s) hex dump (first 32 bytes): c0 be ae 82 ff ff ff ff 00 00 00 00 01 00 00 00 ................ 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ backtrace: [<ffffffff819b5d05>] kmemleak_alloc+0x25/0x50 [<ffffffff81154203>] __kmalloc+0x133/0x240 [<ffffffff811dc1f2>] __register_sysctl_table+0x52/0x520 [<ffffffff811dca3d>] __register_sysctl_paths+0x1ad/0x1d0 [<ffffffff811dca76>] register_sysctl_paths+0x16/0x20 [<ffffffff820ef4d6>] sysctl_core_init+0x17/0x38 [<ffffffff810001cd>] do_one_initcall+0x3d/0x170 [<ffffffff820b563c>] kernel_init+0xd9/0x15f [<ffffffff819e1194>] kernel_thread_helper+0x4/0x10 [<ffffffffffffffff>] 0xffffffffffffffff unreferenced object 0xffff88003bc40ee8 (size 96): comm "swapper/0", pid 1, jiffies 4294669854 (age 84.294s) hex dump (first 32 bytes): 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ backtrace: [<ffffffff819b5d05>] kmemleak_alloc+0x25/0x50 [<ffffffff81154203>] __kmalloc+0x133/0x240 [<ffffffff811dc9b7>] __register_sysctl_paths+0x127/0x1d0 [<ffffffff811dca76>] register_sysctl_paths+0x16/0x20 [<ffffffff820f0c82>] ip_static_sysctl_init+0x17/0x1b [<ffffffff820f187f>] inet_init+0xbe/0x2dd [<ffffffff810001cd>] do_one_initcall+0x3d/0x170 [<ffffffff820b563c>] kernel_init+0xd9/0x15f [<ffffffff819e1194>] kernel_thread_helper+0x4/0x10 [<ffffffffffffffff>] 0xffffffffffffffff unreferenced object 0xffff88003cd27090 (size 96): comm "swapper/0", pid 1, jiffies 4294669854 (age 84.294s) hex dump (first 32 bytes): c0 dc ae 82 ff ff ff ff 00 00 00 00 01 00 00 00 ................ 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ backtrace: [<ffffffff819b5d05>] kmemleak_alloc+0x25/0x50 [<ffffffff81154203>] __kmalloc+0x133/0x240 [<ffffffff811dc1f2>] __register_sysctl_table+0x52/0x520 [<ffffffff811dc7ac>] register_leaf_sysctl_tables+0xec/0x1b0 [<ffffffff811dc77c>] register_leaf_sysctl_tables+0xbc/0x1b0 [<ffffffff811dc9e9>] __register_sysctl_paths+0x159/0x1d0 [<ffffffff811dca76>] register_sysctl_paths+0x16/0x20 [<ffffffff820f0c82>] ip_static_sysctl_init+0x17/0x1b [<ffffffff820f187f>] inet_init+0xbe/0x2dd [<ffffffff810001cd>] do_one_initcall+0x3d/0x170 [<ffffffff820b563c>] kernel_init+0xd9/0x15f [<ffffffff819e1194>] kernel_thread_helper+0x4/0x10 [<ffffffffffffffff>] 0xffffffffffffffff Thanks. -- tixxdz http://opendz.org ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] net: reference the ipv4 sysctl table header 2012-03-26 23:21 ` Djalal Harouni @ 2012-03-28 2:35 ` Eric W. Biederman 2012-03-31 14:25 ` Djalal Harouni 0 siblings, 1 reply; 10+ messages in thread From: Eric W. Biederman @ 2012-03-28 2:35 UTC (permalink / raw) To: Djalal Harouni; +Cc: David S. Miller, Al Viro, netdev Djalal Harouni <tixxdz@opendz.org> writes: > On Mon, Mar 26, 2012 at 03:50:30PM -0700, Eric W. Biederman wrote: >> Djalal Harouni <tixxdz@opendz.org> writes: >> >> > I've been analysing some kmemleak reports of an internal module, and >> > found that there are false positive reports of unreferenced objects. >> > >> > The following patch is just a clean up for one of those false positives, >> > this is for the /proc/sys/net/ipv4 sysctl table. >> > As I've said there are other reports but don't know if it is worth to >> > write patches for them. >> >> So the problem here is that you register a sysctl and don't keep a >> pointer to the returned sysctl_header? So kmemleak complains? > Right. > >> I would expect the other sysctl data structures to have such a pointer, >> so I don't know why kmemleak would complain. >> >> Does my recent sysctl rewrite affect when this kmemleak is reported? > Actually yes, after a recent pull (which includes your recent sysctl work), > some of these false positive reports started to appear. > > > Anyway they seem false positive ones, since keeping a reference to > sysctl_header as in my previous (ugly) patch will quiet the last two > ones. Ok thanks. If that is what it is. Then clean way to quite this will ultimately be converting these table to be compatible with my brand new register_sysctl() and using that to register them. In fact I am pretty certain we can just do: register_sysctl("net/ipv4/route", ipv4_route_table); register_sysctl("net/ipv4/neigh", empty); instead of: register_sysctl_paths(ipv4_path, ipv4_skeleton); And kill ipv4_skeleton and ipv4_path as they are now unused. There was a tremendous cleanup and speed up that came with not allowing sysctl tables to support .child entries in the core, and the older registration routines break apart the tables and return a compatilibty sysctl_table_header if we do that, and I believe we are just leaking that compatibility sysctl_table_header. Eric ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] net: reference the ipv4 sysctl table header 2012-03-28 2:35 ` Eric W. Biederman @ 2012-03-31 14:25 ` Djalal Harouni 0 siblings, 0 replies; 10+ messages in thread From: Djalal Harouni @ 2012-03-31 14:25 UTC (permalink / raw) To: Eric W. Biederman; +Cc: David S. Miller, Al Viro, netdev, Steven Rostedt On Tue, Mar 27, 2012 at 07:35:33PM -0700, Eric W. Biederman wrote: > Djalal Harouni <tixxdz@opendz.org> writes: > > > On Mon, Mar 26, 2012 at 03:50:30PM -0700, Eric W. Biederman wrote: [...] > > Anyway they seem false positive ones, since keeping a reference to > > sysctl_header as in my previous (ugly) patch will quiet the last two > > ones. > > Ok thanks. If that is what it is. Then clean way to quite this will > ultimately be converting these table to be compatible with my brand > new register_sysctl() and using that to register them. > > In fact I am pretty certain we can just do: > register_sysctl("net/ipv4/route", ipv4_route_table); > register_sysctl("net/ipv4/neigh", empty); Thanks, I've tested this and it works, however it seems that there are still memleaks, please see below. > instead of: > register_sysctl_paths(ipv4_path, ipv4_skeleton); > > And kill ipv4_skeleton and ipv4_path as they are now unused. Right, and use the path directly from the .data section. > There was a tremendous cleanup and speed up that came with not allowing > sysctl tables to support .child entries in the core, and the older > registration routines break apart the tables and return a compatilibty > sysctl_table_header if we do that, and I believe we are just > leaking that compatibility sysctl_table_header. Thanks for the explanation, however I don't understand all of this since I'm not familiar with the old/new code, but I want to report some notes (I'm not sure if they are correct). 1) memleaks: After testing your proposed change: register_sysctl("net/ipv4/route", ipv4_route_table); for ipv4_route_table it is ok, no memleak is reported since the returned ctl_table_header is referenced by the ctl_node nodes. This will silence the memleak report. for: register_sysctl("net/ipv4/neigh", empty); Here we'll still have the memleak, I don't know if this is the "leaking that compatibility sysctl_table_header" you are referring to. But here the returned ctl_table_header will not be referenced by the ctl_node nodes since we have an empty ctl_table. see init_header() So is there real memleaks that got hidden by the false positive ones ? or perhaps these are real memleaks that were never spotted ? 2) From the __register_sysctl_table(), if the ctl_table is empty then nr_entries == 0. later we have: header = kzalloc(sizeof(struct ctl_table_header) + sizeof(struct ctl_node)*nr_entries, GFP_KERNEL); ... node = (struct ctl_node *)(header + 1); init_header(header, root, set, node, table); ... node points to somewhere, but its use in this situations seems restricted. 3) Why we always alloc ctl_table_header in __register_sysctl_table() at the beginning ? if I'm right, the current design is: "each directory should have its ctl_table_header", so if some ctl_tables share the same directory then they should also share the same ctl_table_header ? In this case the code can be improved, check if it's a new dir, then alloc a new ctl_table_header, otherwise just return the old one and increment the counter and insert ctl_table entries there. I say this since currently doing: register_sysctl("net/ipv4/neigh", empty); register_sysctl("net/ipv4/neigh", empty); register_sysctl("net/ipv4/neigh", empty); Will allocate three ctl_table_header however at the second call the code successfully detects that this is _not_ a new dir, but it will return the ctl_table_header that was allocated at the beginning of __register_sysctl_table(). Is this also related to the "leaking that compatibility sysctl_table_header" ? 4) Is the order of sysctl locking inside insert_links() correct ? If I'm missing something, please just point-me to an http-link and I'll try to take a closer look when time permits, otherwise someone who knows this should just take a look at it. Sorry for the delay (just got some free time). Thanks. -- tixxdz http://opendz.org ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2012-03-31 14:26 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-03-26 22:23 [PATCH] net: reference the ipv4 sysctl table header Djalal Harouni 2012-03-26 22:24 ` David Miller 2012-03-26 22:42 ` Djalal Harouni 2012-03-28 16:32 ` Steven Rostedt 2012-03-28 20:51 ` David Miller 2012-03-31 14:29 ` Djalal Harouni 2012-03-26 22:50 ` Eric W. Biederman 2012-03-26 23:21 ` Djalal Harouni 2012-03-28 2:35 ` Eric W. Biederman 2012-03-31 14:25 ` Djalal Harouni
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).