userns: suppress kmemleak message

PATCH NAK Dmitry Torokhov 6 participants 7 messages Nov 3, 2016 – Dec 16, 2016

Share & tools

Dmitry Torokhov

author

2016-11-03 05:39 UTC #0 lore raw

PATCH 1/1 userns: suppress kmemleak message

We do not ever intend to unregister "user" sysctl table, unfortunately it leads kmemleak to believe that we are leaking memory:

unreferenced object 0xffff8807383bfd48 (size 96):
comm "swapper/0", pid 1, jiffies 4294894636 (age 278.320s) hex dump (first 32 bytes):

    a0 b4 b0 ba 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:
    [<ffffffffb7de59e8>] kmemleak_alloc+0x28/0x50
    [<ffffffffb676e2f6>] __kmalloc+0x206/0x5a0
    [<ffffffffb69be2d3>] __register_sysctl_table+0xb3/0x1130
    [<ffffffffb69bf36b>] register_sysctl+0x1b/0x20
    [<ffffffffba840de1>] user_namespace_sysctl_init+0x17/0x4c
    [<ffffffffb60022b7>] do_one_initcall+0xb7/0x2a0
    [<ffffffffba7eb102>] kernel_init_freeable+0x597/0x636
    [<ffffffffb7de0433>] kernel_init+0x13/0x140
    [<ffffffffb7dfb36a>] ret_from_fork+0x2a/0x40t show
    [<ffffffffffffffff>] 0xffffffffffffffff

Let's annotate the pointer as kmemleak_not_leak() to suppress the kmemleak false positive.

Reported-by Jakub Kicinski <[email protected]>

Signed-off-by Dmitry Torokhov <[email protected]>

---

This was only compiled; Jakub, could you give it a spin?

 kernel/ucount.c | 2 ++
 1 file changed, 2 insertions(+)

kernel/ucount.c +2 -0

index 9d20d5d..07d69b2 100644
@@ -5,6 +5,7 @@
  *  License.
  */
 
+#include <linux/kmemleak.h>
 #include <linux/stat.h>
 #include <linux/sysctl.h>
 #include <linux/slab.h>

@@ -226,6 +227,7 @@ static __init int user_namespace_sysctl_init(void)
 	 */
 	user_header = register_sysctl("user", empty);
 	BUG_ON(!user_header);
+	kmemleak_not_leak(user_header);
 	BUG_ON(!setup_userns_sysctls(&init_user_ns));
 #endif
 	return 0;

Signature · 5 lines

2.8.0.rc3.226.g39d4020


-- 
Dmitry

Jakub Kicinski

Tested-by on v1 1/1

2016-11-03 14:04 UTC #1 lore raw

Quoted 20 lines · Dmitry Torokhov

We do not ever intend to unregister "user" sysctl table, unfortunately it leads kmemleak to believe that we are leaking memory:

unreferenced object 0xffff8807383bfd48 (size 96):
comm "swapper/0", pid 1, jiffies 4294894636 (age 278.320s) hex dump (first 32 bytes):

    a0 b4 b0 ba 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:
    [<ffffffffb7de59e8>] kmemleak_alloc+0x28/0x50
    [<ffffffffb676e2f6>] __kmalloc+0x206/0x5a0
    [<ffffffffb69be2d3>] __register_sysctl_table+0xb3/0x1130
    [<ffffffffb69bf36b>] register_sysctl+0x1b/0x20
    [<ffffffffba840de1>] user_namespace_sysctl_init+0x17/0x4c
    [<ffffffffb60022b7>] do_one_initcall+0xb7/0x2a0
    [<ffffffffba7eb102>] kernel_init_freeable+0x597/0x636
    [<ffffffffb7de0433>] kernel_init+0x13/0x140
    [<ffffffffb7dfb36a>] ret_from_fork+0x2a/0x40t show
    [<ffffffffffffffff>] 0xffffffffffffffff

Quoted 8 lines

Let's annotate the pointer as kmemleak_not_leak() to suppress the kmemleak false positive.
Reported-by Jakub Kicinski <[email protected]>
Signed-off-by Dmitry Torokhov <[email protected]>
---
This was only compiled; Jakub, could you give it a spin?

Tested-by Jakub Kicinski <[email protected]>

Thanks!

Eric W. Biederman

Nacked-by on v1 1/1 NAK

2016-11-03 14:57 UTC #2 lore raw

In reply to Dmitry Torokhov

Quoted 2 lines · Dmitry Torokhov

We do not ever intend to unregister "user" sysctl table, unfortunately it leads kmemleak to believe that we are leaking memory:

Sounds like an issue with kmemleak because we do retain references.

So no we don't intend to unregister the table.

As for the patch.

Nacked-by "Eric W. Biederman" <[email protected]>

I can't see the using kmemleak_not_leak is possibly good form. I would much rather have suggestions about constructs that won't confuse kmemleak and won't need ugly annotations that serve no purpose but to appease a tool. Perhaps the user_header variable needs to be moved out of user_namespace_sysctl_init.

Eric

Quoted 51 lines

unreferenced object 0xffff8807383bfd48 (size 96):
comm "swapper/0", pid 1, jiffies 4294894636 (age 278.320s) hex dump (first 32 bytes):

    a0 b4 b0 ba 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:
    [<ffffffffb7de59e8>] kmemleak_alloc+0x28/0x50
    [<ffffffffb676e2f6>] __kmalloc+0x206/0x5a0
    [<ffffffffb69be2d3>] __register_sysctl_table+0xb3/0x1130
    [<ffffffffb69bf36b>] register_sysctl+0x1b/0x20
    [<ffffffffba840de1>] user_namespace_sysctl_init+0x17/0x4c
    [<ffffffffb60022b7>] do_one_initcall+0xb7/0x2a0
    [<ffffffffba7eb102>] kernel_init_freeable+0x597/0x636
    [<ffffffffb7de0433>] kernel_init+0x13/0x140
    [<ffffffffb7dfb36a>] ret_from_fork+0x2a/0x40t show
    [<ffffffffffffffff>] 0xffffffffffffffff

Let's annotate the pointer as kmemleak_not_leak() to suppress the kmemleak false positive.

Reported-by Jakub Kicinski <[email protected]>

Signed-off-by Dmitry Torokhov <[email protected]>

---

This was only compiled; Jakub, could you give it a spin?

 kernel/ucount.c | 2 ++
 1 file changed, 2 insertions(+)

kernel/ucount.c +2 -0

index 9d20d5d..07d69b2 100644
@@ -5,6 +5,7 @@
  *  License.
  */
 
+#include <linux/kmemleak.h>
 #include <linux/stat.h>
 #include <linux/sysctl.h>
 #include <linux/slab.h>

@@ -226,6 +227,7 @@ static __init int user_namespace_sysctl_init(void)
 	 */
 	user_header = register_sysctl("user", empty);
 	BUG_ON(!user_header);
+	kmemleak_not_leak(user_header);
 	BUG_ON(!setup_userns_sysctls(&init_user_ns));
 #endif
 	return 0;

Signature · 1 line

2.8.0.rc3.226.g39d4020

Jakub Kicinski

2016-11-03 15:06 UTC #3 lore raw

Quoted 11 lines · Eric W. Biederman

Quoted 2 lines · Dmitry Torokhov
We do not ever intend to unregister "user" sysctl table, unfortunately it leads kmemleak to believe that we are leaking memory:
Sounds like an issue with kmemleak because we do retain references.
So no we don't intend to unregister the table.
As for the patch.

Quoted 7 lines

Nacked-by "Eric W. Biederman" <[email protected]>
I can't see the using kmemleak_not_leak is possibly good form. I would much rather have suggestions about constructs that won't confuse kmemleak and won't need ugly annotations that serve no purpose but to appease a tool. Perhaps the user_header variable needs to be moved out of user_namespace_sysctl_init.

FWIW the problem now is that the compiler is clever enough to never write the pointer to memory so kmemleak can't find it. user_header is just held in a register for as long as it's needed even though the variable is static.

Fubo Chen

2016-12-15 12:16 UTC #4 lore raw

In reply to Eric W. Biederman

Quoted 11 lines · Eric W. Biederman

Quoted 2 lines · Dmitry Torokhov
We do not ever intend to unregister "user" sysctl table, unfortunately it leads kmemleak to believe that we are leaking memory:
Sounds like an issue with kmemleak because we do retain references.
So no we don't intend to unregister the table.
As for the patch.

Quoted 7 lines

Nacked-by "Eric W. Biederman" <[email protected]>
I can't see the using kmemleak_not_leak is possibly good form. I would much rather have suggestions about constructs that won't confuse kmemleak and won't need ugly annotations that serve no purpose but to appease a tool. Perhaps the user_header variable needs to be moved out of user_namespace_sysctl_init.

The only alternative I see is to use WRITE_ONCE() instead of "=" to set "user_header" such that the compiler cannot optimize that variable away. Which of these two approaches do you prefer?

Fubo.

Johannes Berg

2016-12-16 09:40 UTC #5 lore raw

Quoted 6 lines

Quoted 6 lines
I can't see the using kmemleak_not_leak is possibly good form. I would much rather have suggestions about constructs that won't confuse kmemleak and won't need ugly annotations that serve no purpose but to appease a tool. Perhaps the user_header variable needs to be moved out of user_namespace_sysctl_init.

The user_header variable is probably (rightfully so) optimised away by the compiler since it can't ever be read. Therefore, it simply doesn't exist in the resulting binary (and it really shouldn't either) and the kmemleak_not_leak() really is the only way to resolve that, I'd say.

Quoted 3 lines

The only alternative I see is to use WRITE_ONCE() instead of "=" to set "user_header" such that the compiler cannot optimize that variable away. Which of these two approaches do you prefer?

That seems really wrong - forcing the linker/compiler to retain a variable in the image that can never possibly be read (by anything other than kmemleak) is just a complete waste of space.

johannes

Fubo Chen

2016-12-16 13:53 UTC #6 lore raw

Quoted 12 lines · Johannes Berg

Quoted 6 lines
Quoted 6 lines
I can't see the using kmemleak_not_leak is possibly good form. I would much rather have suggestions about constructs that won't confuse kmemleak and won't need ugly annotations that serve no purpose but to appease a tool. Perhaps the user_header variable needs to be moved out of user_namespace_sysctl_init.
The user_header variable is probably (rightfully so) optimised away by the compiler since it can't ever be read. Therefore, it simply doesn't exist in the resulting binary (and it really shouldn't either) and the kmemleak_not_leak() really is the only way to resolve that, I'd say.

Quoted 7 lines

Quoted 3 lines
The only alternative I see is to use WRITE_ONCE() instead of "=" to set "user_header" such that the compiler cannot optimize that variable away. Which of these two approaches do you prefer?
That seems really wrong - forcing the linker/compiler to retain a variable in the image that can never possibly be read (by anything other than kmemleak) is just a complete waste of space.

Does this reply count as a Reviewed-by for the original patch?

Fubo.