diff mbox

net: reference the ipv4 sysctl table header

Message ID 20120326222359.GB28123@dztty
State Rejected, archived
Delegated to: David Miller
Headers show

Commit Message

Djalal Harouni March 26, 2012, 10:23 p.m. UTC
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(-)

Comments

David Miller March 26, 2012, 10:24 p.m. UTC | #1
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.
--
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
Djalal Harouni March 26, 2012, 10:42 p.m. UTC | #2
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
Eric W. Biederman March 26, 2012, 10:50 p.m. UTC | #3
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
--
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
Djalal Harouni March 26, 2012, 11:21 p.m. UTC | #4
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.
Eric W. Biederman March 28, 2012, 2:35 a.m. UTC | #5
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
--
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
Steven Rostedt March 28, 2012, 4:32 p.m. UTC | #6
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

--
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
David Miller March 28, 2012, 8:51 p.m. UTC | #7
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.
--
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
Djalal Harouni March 31, 2012, 2:25 p.m. UTC | #8
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.
Djalal Harouni March 31, 2012, 2:29 p.m. UTC | #9
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
diff mbox

Patch

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