Possible memory leaks in proc_sysctl.c

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.

From 349f2cf150ba9aabdcc24c174f7dab09d3da4508 Mon Sep 17 00:00:00 2001

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.

Quoted 5 lines · Catalin Marinas

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).

Quoted 7 lines · Eric W. Biederman

Quoted 5 lines · Catalin Marinas

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.

Quoted 3 lines

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.

Quoted 3 lines

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.

Quoted 8 lines · Catalin Marinas

Quoted 7 lines · Eric W. Biederman

Quoted 5 lines · Catalin Marinas

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.

Quoted 8 lines

Quoted 3 lines

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.

Quoted 26 lines · Eric W. Biederman

Quoted 8 lines · Catalin Marinas

Quoted 7 lines · Eric W. Biederman

Quoted 5 lines · Catalin Marinas

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.

Quoted 8 lines

Quoted 3 lines

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.

Quoted 6 lines

- 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).

Quoted 5 lines · Catalin Marinas

Quoted 3 lines · Eric W. Biederman

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.

Quoted 7 lines

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.