[PATCH v2] Compiler Attributes: don't pollute userspace with macro definitions

[PATCH v2] Compiler Attributes: don't pollute userspace with macro definitions

[Xiaozhou Liu]

Macros 'inline' and '__gnu_inline' used to be defined within __KERNEL__.
Commit 815f0ddb346c ("include/linux/compiler*.h: make compiler-*.h mutually
exclusive") had them exposed to userspace (unintentionally).

Then commit a3f8a30f3f00 ("Compiler Attributes: use feature checks instead
of version checks") moved __gnu_inline back into __KERNEL__ and inline
was left behind. Since inline depends on __gnu_inline, compiling error
showing "unknown type name ‘__gnu_inline’" will pop up, if userspace
somehow includes <linux/compiler.h>.

Other macros like __must_check, notrace, etc. used to be defined within
__KERNEL__ too. So just move these macros back into __KERNEL__.

v2: update commit message.

Signed-off-by: Xiaozhou Liu <liuxiaozhou@bytedance.com>
---
 include/linux/compiler_types.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index 4a3f9c09c92d..9e23ec015221 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -161,6 +161,8 @@ struct ftrace_likely_data {
 #define __diag_error(compiler, version, option, comment) \
 	__diag_ ## compiler(version, error, option)
 
+#ifdef __KERNEL__
+
 #ifdef CONFIG_ENABLE_MUST_CHECK
 #define __must_check		__attribute__((__warn_unused_result__))
 #else
@@ -215,4 +217,6 @@ struct ftrace_likely_data {
  */
 #define noinline_for_stack noinline
 
+#endif /* __KERNEL */
+
 #endif /* __LINUX_COMPILER_TYPES_H */
-- 
2.11.0

Re: [PATCH v2] Compiler Attributes: don't pollute userspace with macro definitions

[Miguel Ojeda]

Hi Xiaozhou,

Couple of comments.

On Sun, Dec 9, 2018 at 4:27 AM Xiaozhou Liu <liuxiaozhou@bytedance.com> wrote:
>
> v2: update commit message.

This line should go below the "---", since it shouldn't be part of the
commit message.

> +#ifdef __KERNEL__
> +
>  #ifdef CONFIG_ENABLE_MUST_CHECK
>  #define __must_check           __attribute__((__warn_unused_result__))
>  #else
> @@ -215,4 +217,6 @@ struct ftrace_likely_data {
>   */
>  #define noinline_for_stack noinline
>
> +#endif /* __KERNEL */

I wonder if we can/should simply move them into the __KERNEL__ &&
!__ASSEMBLY__ block that is above, instead. It would be simpler to
read, and there aren't apparently dependencies to those outside it
that go after the block.

I took a look at where the macros were at each "step", and, on one
hand, compiler-gcc.h was (and is) included entirely inside it, which
is from where most of the macros come originally from. On the other
hand, not all do: __must_check (the generic version, not the one in
-gcc.h) and noinline_for_stack were defined in __KERNEL__ (only)
before commit 815f0ddb346c ("include/linux/compiler*.h: make
compiler-*.h mutually exclusive"). But anyway using those two in
assembly does not make sense, right?

What do you think?

Greg/Linus, are you going to pick this (or a v3) directly? If not, I
can pick it up in compiler-attributes tree linux-next.

PS: In addition (related to this but not for this patch), we should
review whether other macros that are currently outside should be there
or simply pushed back into __KERNEL__ (and possibly __ASSEMBLY__). For
instance, __latent_entropy (the generic one) is outside, but it is
only defined in compiler-gcc.h, so the generic one should be inside
the __KERNEL__ && !__ASSEMBLY__ block, no?

Cheers,
Miguel

Re: [PATCH v2] Compiler Attributes: don't pollute userspace with macro definitions

[Miguel Ojeda]

On Thu, Dec 13, 2018 at 10:59 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> What do you think?

[Cc'ing Nick et. al. as well.]

Cheers,
Miguel

Re: [PATCH v2] Compiler Attributes: don't pollute userspace with macro definitions

[Nick Desaulniers]

>> compiling error
>> showing "unknown type name ‘__gnu_inline’" will pop up, if userspace
>> somehow includes <linux/compiler.h>.

Oops.

> If not, I can pick it up in compiler-attributes tree linux-next.

That's probably the best, unless we'd like this fix in mainline ASAP?

Moving the __KERNEL__ guard should not affect the kernel, only what
userspace sees.  __gnu_inline only affects which
implementation/definition you get, so even if userspace doesn't know
what the kernel's inline is redefined to, it should not matter as
userspace should only ever care about the function signature, which
does not change between our definitions of inline.

Acked-by: Nick Desaulniers <ndesaulniers@google.com>

-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v2] Compiler Attributes: don't pollute userspace with macro definitions

[Miguel Ojeda]

On Thu, Dec 13, 2018 at 11:25 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> Moving the __KERNEL__ guard should not affect the kernel, only what
> userspace sees.  __gnu_inline only affects which
> implementation/definition you get, so even if userspace doesn't know
> what the kernel's inline is redefined to, it should not matter as
> userspace should only ever care about the function signature, which
> does not change between our definitions of inline.

Hm... I am unsure what you mean by this. Were you replying to me or to
the original patch?

Cheers,
Miguel

Re: [PATCH v2] Compiler Attributes: don't pollute userspace with macro definitions

[刘晓舟]

Hi Miguel,

On Thu, Dec 13, 2018 at 10:59:10PM +0100, Miguel Ojeda wrote:

> I wonder if we can/should simply move them into the __KERNEL__ &&
> !__ASSEMBLY__ block that is above, instead. It would be simpler to
> read, and there aren't apparently dependencies to those outside it
> that go after the block.

Yes, this is also more accurate. I will send a v3.

> I took a look at where the macros were at each "step", and, on one
> hand, compiler-gcc.h was (and is) included entirely inside it, which
> is from where most of the macros come originally from. On the other
> hand, not all do: __must_check (the generic version, not the one in
> -gcc.h) and noinline_for_stack were defined in __KERNEL__ (only)
> before commit 815f0ddb346c ("include/linux/compiler*.h: make
> compiler-*.h mutually exclusive"). But anyway using those two in
> assembly does not make sense, right?
>
> What do you think?

A simple grep shows no assembly are using those macros. Pretty sure
it is safe to do it this way.


Thanks,
Xiaozhou

Re: [PATCH v2] Compiler Attributes: don't pollute userspace with macro definitions

[Nick Desaulniers]

On Fri, Dec 14, 2018 at 2:02 AM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Thu, Dec 13, 2018 at 11:25 PM Nick Desaulniers
> <ndesaulniers@google.com> wrote:
> >
> > Moving the __KERNEL__ guard should not affect the kernel, only what
> > userspace sees.  __gnu_inline only affects which
> > implementation/definition you get, so even if userspace doesn't know
> > what the kernel's inline is redefined to, it should not matter as
> > userspace should only ever care about the function signature, which
> > does not change between our definitions of inline.
>
> Hm... I am unsure what you mean by this. Were you replying to me or to
> the original patch?

Justifying my thoughts for ack'ing the original patch.

-- 
Thanks,
~Nick Desaulniers