Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1542686 > unrolled thread

Re: [PATCH] userns: suppress kmemleak message

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] userns: suppress kmemleak message Fubo Chen <fubo.chen@gmail.com> - 2016-12-15 13:20 +0100
    Re: [PATCH] userns: suppress kmemleak message Johannes Berg <johannes@sipsolutions.net> - 2016-12-16 10:50 +0100
      Re: [PATCH] userns: suppress kmemleak message Fubo Chen <fubo.chen@gmail.com> - 2016-12-16 15:00 +0100

#1542686 — Re: [PATCH] userns: suppress kmemleak message

On Thu, Nov 3, 2016 at 3:54 PM, Eric W. Biederman <ebiederm@xmission.com> wrote:
> Dmitry Torokhov <dmitry.torokhov@gmail.com> writes:
>
>> 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" <ebiederm@xmission.com>
>
> 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.

[toc] | [next] | [standalone]

#1543320

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

> 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

[toc] | [prev] | [next] | [standalone]

#1543471

On Fri, Dec 16, 2016 at 10:39 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
>
>> > 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.
>
>> 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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web