Possible memory leaks in proc_sysctl.c

Possible memory leaks in proc_sysctl.c

[Catalin Marinas]

Hi Eric,

Following your commit f728019bb (sysctl: register only tables of sysctl
files), I get several kmemleak reports. They all seem to be header
allocations with kzalloc() in __register_sysctl_table() and
__register_sysctl_paths(). The patch isn't simple to quickly figure out
what may be wrong.

Tested on an ARM platform with 3.4-rc3.

unreferenced object 0xbf86c380 (size 128):
  comm "swapper/0", pid 0, jiffies 4294937306 (age 69938.200s)
  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 2c eb 4c 80 00 00 00 00 00 00 00 00  ....,.L.........
  backtrace:
    [<800be064>] create_object+0xe8/0x224
    [<800bb120>] __kmalloc+0x118/0x19c
    [<801116b8>] __register_sysctl_paths+0x128/0x1d0
    [<8048a248>] sysctl_init+0xc/0x18
    [<80483810>] start_kernel+0x310/0x32c
    [<60008044>] 0x60008044
    [<ffffffff>] 0xffffffff
unreferenced object 0xbf862380 (size 64):
  comm "swapper/0", pid 0, jiffies 4294937306 (age 69938.200s)
  hex dump (first 32 bytes):
    84 bf 4e 80 00 00 00 00 01 00 00 00 01 00 00 00  ..N.............
    00 00 00 00 00 00 00 00 28 31 4d 80 28 31 4d 80  ........(1M.(1M.
  backtrace:
    [<800be064>] create_object+0xe8/0x224
    [<800bb120>] __kmalloc+0x118/0x19c
    [<80110f5c>] __register_sysctl_table+0x4c/0x468
    [<801114a4>] register_leaf_sysctl_tables+0x12c/0x200
    [<8011151c>] register_leaf_sysctl_tables+0x1a4/0x200
    [<801116e4>] __register_sysctl_paths+0x154/0x1d0
    [<8048a248>] sysctl_init+0xc/0x18
    [<80483810>] start_kernel+0x310/0x32c
    [<60008044>] 0x60008044
    [<ffffffff>] 0xffffffff
unreferenced object 0xbf8623c0 (size 64):
  comm "swapper/0", pid 0, jiffies 4294937306 (age 69938.200s)
  hex dump (first 32 bytes):
    a8 bf 4e 80 00 00 00 00 01 00 00 00 01 00 00 00  ..N.............
    00 00 00 00 00 00 00 00 28 31 4d 80 28 31 4d 80  ........(1M.(1M.
  backtrace:
    [<800be064>] create_object+0xe8/0x224
    [<800bb120>] __kmalloc+0x118/0x19c
    [<80110f5c>] __register_sysctl_table+0x4c/0x468
    [<801114a4>] register_leaf_sysctl_tables+0x12c/0x200
    [<8011151c>] register_leaf_sysctl_tables+0x1a4/0x200
    [<801116e4>] __register_sysctl_paths+0x154/0x1d0
    [<8048a248>] sysctl_init+0xc/0x18
    [<80483810>] start_kernel+0x310/0x32c
    [<60008044>] 0x60008044
    [<ffffffff>] 0xffffffff
unreferenced object 0xbf973380 (size 64):
  comm "swapper/0", pid 1, jiffies 4294937392 (age 69937.350s)
  hex dump (first 32 bytes):
    c0 06 50 80 00 00 00 00 01 00 00 00 01 00 00 00  ..P.............
    00 00 00 00 c0 06 50 80 28 31 4d 80 28 31 4d 80  ......P.(1M.(1M.
  backtrace:
    [<800be064>] create_object+0xe8/0x224
    [<800bb120>] __kmalloc+0x118/0x19c
    [<80110f5c>] __register_sysctl_table+0x4c/0x468
    [<80111754>] __register_sysctl_paths+0x1c4/0x1d0
    [<8049aa54>] sysctl_core_init+0x1c/0x38
    [<800086dc>] do_one_initcall+0x34/0x17c
    [<804839a0>] kernel_init+0x174/0x21c
    [<8000f978>] kernel_thread_exit+0x0/0x8
    [<ffffffff>] 0xffffffff
unreferenced object 0xbfa27280 (size 64):
  comm "swapper/0", pid 1, jiffies 4294937393 (age 69937.340s)
  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 80 33 4e 80 00 00 00 00 00 00 00 00  .....3N.........
  backtrace:
    [<800be064>] create_object+0xe8/0x224
    [<800bb120>] __kmalloc+0x118/0x19c
    [<801116b8>] __register_sysctl_paths+0x128/0x1d0
    [<8049c810>] inet_init+0xa8/0x28c
    [<800086dc>] do_one_initcall+0x34/0x17c
    [<804839a0>] kernel_init+0x174/0x21c
    [<8000f978>] kernel_thread_exit+0x0/0x8
    [<ffffffff>] 0xffffffff
unreferenced object 0xbfa27100 (size 64):
  comm "swapper/0", pid 1, jiffies 4294937393 (age 69937.340s)
  hex dump (first 32 bytes):
    18 0d 50 80 00 00 00 00 01 00 00 00 01 00 00 00  ..P.............
    00 00 00 00 00 00 00 00 28 31 4d 80 28 31 4d 80  ........(1M.(1M.
  backtrace:
    [<800be064>] create_object+0xe8/0x224
    [<800bb120>] __kmalloc+0x118/0x19c
    [<80110f5c>] __register_sysctl_table+0x4c/0x468
    [<801114a4>] register_leaf_sysctl_tables+0x12c/0x200
    [<8011151c>] register_leaf_sysctl_tables+0x1a4/0x200
    [<801116e4>] __register_sysctl_paths+0x154/0x1d0
    [<8049c810>] inet_init+0xa8/0x28c
    [<800086dc>] do_one_initcall+0x34/0x17c
    [<804839a0>] kernel_init+0x174/0x21c
    [<8000f978>] kernel_thread_exit+0x0/0x8
    [<ffffffff>] 0xffffffff


Thanks.

-- 
Catalin

Re: Possible memory leaks in proc_sysctl.c

[Eric W. Biederman]

Catalin Marinas <catalin.marinas@arm.com> writes:

> Hi Eric,
>
> Following your commit f728019bb (sysctl: register only tables of sysctl
> files), I get several kmemleak reports. They all seem to be header
> allocations with kzalloc() in __register_sysctl_table() and
> __register_sysctl_paths(). The patch isn't simple to quickly figure out
> what may be wrong.

Due to a change in the data structure places where we register the
sysctl permanently and ignore the result from the register_sysctl_...
family of functions now report this leak.

majianpeng has done a good of getting kmemleak_not_leak annotations into
the net tree, and I have one of his patches pending to put into my
sysctl tree (see below).

I don't know if the issue is serious enough to warrant putting the
changes into 3.4 but the patches are trivial.

The pending patch for kernel/sysctl.c

Eric


>From 349f2cf150ba9aabdcc24c174f7dab09d3da4508 Mon Sep 17 00:00:00 2001
From: majianpeng <majianpeng@gmail.com>
Date: Tue, 17 Apr 2012 13:45:52 +0800
Subject: [PATCH] sysctl:Remove memleak reports by kmemeleak_not_leak


Signed-off-by: majianpeng <majianpeng@gmail.com>
---
 kernel/sysctl.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 4ab1187..136ac3b 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -1551,7 +1551,7 @@ static struct ctl_table dev_table[] = {
 
 int __init sysctl_init(void)
 {
-	register_sysctl_table(sysctl_base_table);
+	kmemleak_not_leak(register_sysctl_table(sysctl_base_table));
 	return 0;
 }



> Tested on an ARM platform with 3.4-rc3.
>
> unreferenced object 0xbf86c380 (size 128):
>   comm "swapper/0", pid 0, jiffies 4294937306 (age 69938.200s)
>   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 2c eb 4c 80 00 00 00 00 00 00 00 00  ....,.L.........
>   backtrace:
>     [<800be064>] create_object+0xe8/0x224
>     [<800bb120>] __kmalloc+0x118/0x19c
>     [<801116b8>] __register_sysctl_paths+0x128/0x1d0
>     [<8048a248>] sysctl_init+0xc/0x18
>     [<80483810>] start_kernel+0x310/0x32c
>     [<60008044>] 0x60008044
>     [<ffffffff>] 0xffffffff
> unreferenced object 0xbf862380 (size 64):
>   comm "swapper/0", pid 0, jiffies 4294937306 (age 69938.200s)
>   hex dump (first 32 bytes):
>     84 bf 4e 80 00 00 00 00 01 00 00 00 01 00 00 00  ..N.............
>     00 00 00 00 00 00 00 00 28 31 4d 80 28 31 4d 80  ........(1M.(1M.
>   backtrace:
>     [<800be064>] create_object+0xe8/0x224
>     [<800bb120>] __kmalloc+0x118/0x19c
>     [<80110f5c>] __register_sysctl_table+0x4c/0x468
>     [<801114a4>] register_leaf_sysctl_tables+0x12c/0x200
>     [<8011151c>] register_leaf_sysctl_tables+0x1a4/0x200
>     [<801116e4>] __register_sysctl_paths+0x154/0x1d0
>     [<8048a248>] sysctl_init+0xc/0x18
>     [<80483810>] start_kernel+0x310/0x32c
>     [<60008044>] 0x60008044
>     [<ffffffff>] 0xffffffff
> unreferenced object 0xbf8623c0 (size 64):
>   comm "swapper/0", pid 0, jiffies 4294937306 (age 69938.200s)
>   hex dump (first 32 bytes):
>     a8 bf 4e 80 00 00 00 00 01 00 00 00 01 00 00 00  ..N.............
>     00 00 00 00 00 00 00 00 28 31 4d 80 28 31 4d 80  ........(1M.(1M.
>   backtrace:
>     [<800be064>] create_object+0xe8/0x224
>     [<800bb120>] __kmalloc+0x118/0x19c
>     [<80110f5c>] __register_sysctl_table+0x4c/0x468
>     [<801114a4>] register_leaf_sysctl_tables+0x12c/0x200
>     [<8011151c>] register_leaf_sysctl_tables+0x1a4/0x200
>     [<801116e4>] __register_sysctl_paths+0x154/0x1d0
>     [<8048a248>] sysctl_init+0xc/0x18
>     [<80483810>] start_kernel+0x310/0x32c
>     [<60008044>] 0x60008044
>     [<ffffffff>] 0xffffffff
> unreferenced object 0xbf973380 (size 64):
>   comm "swapper/0", pid 1, jiffies 4294937392 (age 69937.350s)
>   hex dump (first 32 bytes):
>     c0 06 50 80 00 00 00 00 01 00 00 00 01 00 00 00  ..P.............
>     00 00 00 00 c0 06 50 80 28 31 4d 80 28 31 4d 80  ......P.(1M.(1M.
>   backtrace:
>     [<800be064>] create_object+0xe8/0x224
>     [<800bb120>] __kmalloc+0x118/0x19c
>     [<80110f5c>] __register_sysctl_table+0x4c/0x468
>     [<80111754>] __register_sysctl_paths+0x1c4/0x1d0
>     [<8049aa54>] sysctl_core_init+0x1c/0x38
>     [<800086dc>] do_one_initcall+0x34/0x17c
>     [<804839a0>] kernel_init+0x174/0x21c
>     [<8000f978>] kernel_thread_exit+0x0/0x8
>     [<ffffffff>] 0xffffffff
> unreferenced object 0xbfa27280 (size 64):
>   comm "swapper/0", pid 1, jiffies 4294937393 (age 69937.340s)
>   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 80 33 4e 80 00 00 00 00 00 00 00 00  .....3N.........
>   backtrace:
>     [<800be064>] create_object+0xe8/0x224
>     [<800bb120>] __kmalloc+0x118/0x19c
>     [<801116b8>] __register_sysctl_paths+0x128/0x1d0
>     [<8049c810>] inet_init+0xa8/0x28c
>     [<800086dc>] do_one_initcall+0x34/0x17c
>     [<804839a0>] kernel_init+0x174/0x21c
>     [<8000f978>] kernel_thread_exit+0x0/0x8
>     [<ffffffff>] 0xffffffff
> unreferenced object 0xbfa27100 (size 64):
>   comm "swapper/0", pid 1, jiffies 4294937393 (age 69937.340s)
>   hex dump (first 32 bytes):
>     18 0d 50 80 00 00 00 00 01 00 00 00 01 00 00 00  ..P.............
>     00 00 00 00 00 00 00 00 28 31 4d 80 28 31 4d 80  ........(1M.(1M.
>   backtrace:
>     [<800be064>] create_object+0xe8/0x224
>     [<800bb120>] __kmalloc+0x118/0x19c
>     [<80110f5c>] __register_sysctl_table+0x4c/0x468
>     [<801114a4>] register_leaf_sysctl_tables+0x12c/0x200
>     [<8011151c>] register_leaf_sysctl_tables+0x1a4/0x200
>     [<801116e4>] __register_sysctl_paths+0x154/0x1d0
>     [<8049c810>] inet_init+0xa8/0x28c
>     [<800086dc>] do_one_initcall+0x34/0x17c
>     [<804839a0>] kernel_init+0x174/0x21c
>     [<8000f978>] kernel_thread_exit+0x0/0x8
>     [<ffffffff>] 0xffffffff
>
>
> Thanks.

Re: Possible memory leaks in proc_sysctl.c

[Catalin Marinas]

On Wed, Apr 18, 2012 at 02:22:09PM +0100, Eric W. Biederman wrote:
> Catalin Marinas <catalin.marinas@arm.com> writes:
> > Following your commit f728019bb (sysctl: register only tables of sysctl
> > files), I get several kmemleak reports. They all seem to be header
> > allocations with kzalloc() in __register_sysctl_table() and
> > __register_sysctl_paths(). The patch isn't simple to quickly figure out
> > what may be wrong.
> 
> Due to a change in the data structure places where we register the
> sysctl permanently and ignore the result from the register_sysctl_...
> family of functions now report this leak.

But is the header (or subheader, basically any pointer inside the
kmalloc'ed object) never referenced from anywhere? I'm just trying to
understand why kmemleak reports it as it seems that the header object is
inserted in a ctl_dir.

> majianpeng has done a good of getting kmemleak_not_leak annotations into
> the net tree, and I have one of his patches pending to put into my
> sysctl tree (see below).

If the header is referenced from somewhere, we can tell kmemleak where
it is referenced from and avoid the not_leak annotations. But I'm not
familiar with this code to be sure.

Thanks.

-- 
Catalin

Re: Possible memory leaks in proc_sysctl.c

[Eric W. Biederman]

Catalin Marinas <catalin.marinas@arm.com> writes:

> On Wed, Apr 18, 2012 at 02:22:09PM +0100, Eric W. Biederman wrote:
>> Catalin Marinas <catalin.marinas@arm.com> writes:
>> > Following your commit f728019bb (sysctl: register only tables of sysctl
>> > files), I get several kmemleak reports. They all seem to be header
>> > allocations with kzalloc() in __register_sysctl_table() and
>> > __register_sysctl_paths(). The patch isn't simple to quickly figure out
>> > what may be wrong.
>> 
>> Due to a change in the data structure places where we register the
>> sysctl permanently and ignore the result from the register_sysctl_...
>> family of functions now report this leak.
>
> But is the header (or subheader, basically any pointer inside the
> kmalloc'ed object) never referenced from anywhere? I'm just trying to
> understand why kmemleak reports it as it seems that the header object is
> inserted in a ctl_dir.

It is never reference from anywhere because we never free the structure.
The job of the header is to be the structure that tells us how to free
things.

I see a couple of things going on.
- For compatibility the header that is returned is a dummy that just
  points to the real headers.

- Even without the compatibility we can get the same symptom if
  we register an empty directory.

So simply saying kmemleak shut up this is deliberate in these few cases
where we don't intend to unregister the structure and have a deliberate
leak seems the clean and maintainable way to go.

>> majianpeng has done a good of getting kmemleak_not_leak annotations into
>> the net tree, and I have one of his patches pending to put into my
>> sysctl tree (see below).
>
> If the header is referenced from somewhere, we can tell kmemleak where
> it is referenced from and avoid the not_leak annotations. But I'm not
> familiar with this code to be sure.

Nope.  There honestly are no references.  We reference lower parts of
the structure be we don't have a back pointer in all cases.

If we were good citizens and kept a reference to the returned
sysctl_header so we could unregister sysctls when our module unloads
(as the api is designed to do) we wouldn't have these warnings.  As
it is we have just been getting lucky in the past.   So I think just
saying kmemleak shut up I know I am being bad is reasonable.

I can change how we are registering things and get rid of the code that
where we there are no references today.  But then someone might refactor
the code tomorrow and problems might show up again.  Shrug.  So saying
I mean to leak this things don't worry about it seems clean.

Eric

Re: Possible memory leaks in proc_sysctl.c

[Catalin Marinas]

On Wed, Apr 18, 2012 at 03:52:58PM +0100, Eric W. Biederman wrote:
> Catalin Marinas <catalin.marinas@arm.com> writes:
> > On Wed, Apr 18, 2012 at 02:22:09PM +0100, Eric W. Biederman wrote:
> >> Catalin Marinas <catalin.marinas@arm.com> writes:
> >> > Following your commit f728019bb (sysctl: register only tables of sysctl
> >> > files), I get several kmemleak reports. They all seem to be header
> >> > allocations with kzalloc() in __register_sysctl_table() and
> >> > __register_sysctl_paths(). The patch isn't simple to quickly figure out
> >> > what may be wrong.
> >> 
> >> Due to a change in the data structure places where we register the
> >> sysctl permanently and ignore the result from the register_sysctl_...
> >> family of functions now report this leak.
> >
> > But is the header (or subheader, basically any pointer inside the
> > kmalloc'ed object) never referenced from anywhere? I'm just trying to
> > understand why kmemleak reports it as it seems that the header object is
> > inserted in a ctl_dir.
> 
> It is never reference from anywhere because we never free the structure.
> The job of the header is to be the structure that tells us how to free
> things.
> 
> I see a couple of things going on.
> - For compatibility the header that is returned is a dummy that just
>   points to the real headers.
> 
> - Even without the compatibility we can get the same symptom if
>   we register an empty directory.
> 
> So simply saying kmemleak shut up this is deliberate in these few cases
> where we don't intend to unregister the structure and have a deliberate
> leak seems the clean and maintainable way to go.

OK, I got it now, sounds fair. But please add a comment to the
kmemleak_not_leak() annotations so that others know it's a deliberate
leak (rather than a false positive).

Also the patch should include the linux/kmemleak.h file for the
kmemleak_not_leak() prototype as header changes in the future may cause
problems (I think the one you posted did not include it).

Thanks.

-- 
Catalin

Re: Possible memory leaks in proc_sysctl.c

[Eric W. Biederman]

Catalin Marinas <catalin.marinas@arm.com> writes:

> On Wed, Apr 18, 2012 at 03:52:58PM +0100, Eric W. Biederman wrote:
>> Catalin Marinas <catalin.marinas@arm.com> writes:
>> > On Wed, Apr 18, 2012 at 02:22:09PM +0100, Eric W. Biederman wrote:
>> >> Catalin Marinas <catalin.marinas@arm.com> writes:
>> >> > Following your commit f728019bb (sysctl: register only tables of sysctl
>> >> > files), I get several kmemleak reports. They all seem to be header
>> >> > allocations with kzalloc() in __register_sysctl_table() and
>> >> > __register_sysctl_paths(). The patch isn't simple to quickly figure out
>> >> > what may be wrong.
>> >> 
>> >> Due to a change in the data structure places where we register the
>> >> sysctl permanently and ignore the result from the register_sysctl_...
>> >> family of functions now report this leak.
>> >
>> > But is the header (or subheader, basically any pointer inside the
>> > kmalloc'ed object) never referenced from anywhere? I'm just trying to
>> > understand why kmemleak reports it as it seems that the header object is
>> > inserted in a ctl_dir.
>> 
>> It is never reference from anywhere because we never free the structure.
>> The job of the header is to be the structure that tells us how to free
>> things.
>> 
>> I see a couple of things going on.
>> - For compatibility the header that is returned is a dummy that just
>>   points to the real headers.
>> 
>> - Even without the compatibility we can get the same symptom if
>>   we register an empty directory.
>> 
>> So simply saying kmemleak shut up this is deliberate in these few cases
>> where we don't intend to unregister the structure and have a deliberate
>> leak seems the clean and maintainable way to go.
>
> OK, I got it now, sounds fair. But please add a comment to the
> kmemleak_not_leak() annotations so that others know it's a deliberate
> leak (rather than a false positive).
>
> Also the patch should include the linux/kmemleak.h file for the
> kmemleak_not_leak() prototype as header changes in the future may cause
> problems (I think the one you posted did not include it).

I will take a look when I merge the patch.

Would something like kmemleak_ignore() be better?  What I want is
kmemleak_this_is_a_deliberate_leak_so_shut_up(), but the API doesn't
seem to exactly include that function.  I'm not certain what the proper
name is as I haven't worked much with kmemleak.

Eric

Re: Possible memory leaks in proc_sysctl.c

[Catalin Marinas]

On Wed, Apr 18, 2012 at 04:44:27PM +0100, Eric W. Biederman wrote:
> Catalin Marinas <catalin.marinas@arm.com> writes:
> > On Wed, Apr 18, 2012 at 03:52:58PM +0100, Eric W. Biederman wrote:
> >> So simply saying kmemleak shut up this is deliberate in these few cases
> >> where we don't intend to unregister the structure and have a deliberate
> >> leak seems the clean and maintainable way to go.
> >
> > OK, I got it now, sounds fair. But please add a comment to the
> > kmemleak_not_leak() annotations so that others know it's a deliberate
> > leak (rather than a false positive).
> >
> > Also the patch should include the linux/kmemleak.h file for the
> > kmemleak_not_leak() prototype as header changes in the future may cause
> > problems (I think the one you posted did not include it).
> 
> I will take a look when I merge the patch.
> 
> Would something like kmemleak_ignore() be better?  What I want is
> kmemleak_this_is_a_deliberate_leak_so_shut_up(), but the API doesn't
> seem to exactly include that function.  I'm not certain what the proper
> name is as I haven't worked much with kmemleak.

kmemleak_ignore() can be used as long as the object does not reference
other objects since it won't even be scanned. In this case, it only
hides the returned header object but there seem to be two more objects
allocated during the register_sysctl_table() call and referenced from
the returned header.

kmemleak_not_leak() would do the right thing here, though it was meant
for annotating false positives rather than real leaks. I think a comment
on the calling site would be enough.

-- 
Catalin