[PATCH] Compiler Attributes: move kernel-only attributes into __KERNEL__

[PATCH] Compiler Attributes: move kernel-only attributes into __KERNEL__

[Xiaozhou Liu]

Attributes such as `__gnu_inline' are meant to be used within the
kernel. When userspace somehow includes <linux/compiler.h>
(eg. tools/bpf), compilation errors would be shown:

        "error: unknown type name ‘__gnu_inline’"

So just move these things into __KERNEL__ and the behavior is kept
as before.

Fixes: a3f8a30f3f00 ("Compiler Attributes: use feature checks instead of version checks")
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..2fb2c311e5d6 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] Compiler Attributes: move kernel-only attributes into __KERNEL__

[Miguel Ojeda]

Hi Xiaozhou,

On Wed, Nov 28, 2018 at 3:09 PM Xiaozhou Liu <liuxiaozhou@bytedance.com> wrote:
>
> Attributes such as `__gnu_inline' are meant to be used within the
> kernel. When userspace somehow includes <linux/compiler.h>
> (eg. tools/bpf), compilation errors would be shown:
>
>         "error: unknown type name ‘__gnu_inline’"
>
> So just move these things into __KERNEL__ and the behavior is kept
> as before.

That is not exactly correct -- a3f8a30f3f00 moved some attributes to
another file, moving them into __KERNEL__ (in particular,__gnu_inline
is).

The problem is, instead, that __gnu_inline is not anymore defined
outside __KERNEL__, but something else that uses it is (the inline
macro definition, if I had to guess).

If your problem is fixed by putting __gnu_inline into __KERNEL__
again, it means we can simply move the inline definition inside
__KERNEL__ too. That way, we don't pollute userspace users with macro
definitions.

Having said that, does someone know whether userspace should have
access to those attributes (or rather, other code that uses in turn
those attributes)?

Cheers,
Miguel

Re: [PATCH] Compiler Attributes: move kernel-only attributes into __KERNEL__

[Miguel Ojeda]

On Wed, Nov 28, 2018 at 6:35 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> If your problem is fixed by putting __gnu_inline into __KERNEL__

s/__gnu_inline/inline/

Cheers,
Miguel

Re: [PATCH] Compiler Attributes: move kernel-only attributes into __KERNEL__

[Xiaozhou Liu]

Hi Miguel,

On Wed, Nov 28, 2018 at 06:35:18PM +0100, Miguel Ojeda wrote:
> Hi Xiaozhou,
> 
> On Wed, Nov 28, 2018 at 3:09 PM Xiaozhou Liu <liuxiaozhou@bytedance.com> wrote:
> >
> > Attributes such as `__gnu_inline' are meant to be used within the
> > kernel. When userspace somehow includes <linux/compiler.h>
> > (eg. tools/bpf), compilation errors would be shown:
> >
> >         "error: unknown type name ‘__gnu_inline’"
> >
> > So just move these things into __KERNEL__ and the behavior is kept
> > as before.

By `these' I mean inline and the like, to be clear.

> That is not exactly correct -- a3f8a30f3f00 moved some attributes to
> another file, moving them into __KERNEL__ (in particular,__gnu_inline
> is).

Yes, that is what a3f8a30f3f00 did. Sorry.
Turns out the commits in question are 815f0ddb346c and a3f8a30f3f00.

> The problem is, instead, that __gnu_inline is not anymore defined
> outside __KERNEL__, but something else that uses it is (the inline
> macro definition, if I had to guess).

Yes and no. Let's recall the whole story.

Before 815f0ddb346c("include/linux/compiler*.h: make compiler-*.h mutually exclusive"),
__gnu_inline and inline were both *in* __KERNEL__ as the were in
<linux/compiler-gcc.h>, which was entirely put to __KERNEL__ in
<linux/compiler_types.h>. Everything was fine.

Then 815f0ddb346c moved inline and __gnu_inline *out* of __KERNEL__
and put them in <linux/compiler_types.h> so userspace could see them
both. Not sure if it's intended behavior, but everything looked fine.

Then a3f8a30f3f00 moved __gnu_inline back into __KERNEL__ and left
inline behind. Since inline depends on __gnu_inline, error showing
"unknown type name ‘__gnu_inline’" pops up.

> If your problem is fixed by putting __gnu_inline into __KERNEL__
> again, it means we can simply move the inline definition inside
> __KERNEL__ too. That way, we don't pollute userspace users with macro
> definitions.

It'd be good. That's exactly what my patch does.

> Having said that, does someone know whether userspace should have
> access to those attributes (or rather, other code that uses in turn
> those attributes)?

It'd be better to keep those attributes out of userspace, as is the
case before 815f0ddb346c.

BTW, the userspace code failed to compile in my case is under
directory tools/bpf/.


Thanks,
Xiaozhou

Re: [PATCH] Compiler Attributes: move kernel-only attributes into __KERNEL__

[Miguel Ojeda]

On Thu, Nov 29, 2018 at 3:16 AM Xiaozhou Liu <lxz1983@gmail.com> wrote:
>
> On Wed, Nov 28, 2018 at 06:35:18PM +0100, Miguel Ojeda wrote:
>
> By `these' I mean inline and the like, to be clear.

Ah, that makes more sense! Sorry.

> > That is not exactly correct -- a3f8a30f3f00 moved some attributes to
> > another file, moving them into __KERNEL__ (in particular,__gnu_inline
> > is).
>
> Yes, that is what a3f8a30f3f00 did. Sorry.
> Turns out the commits in question are 815f0ddb346c and a3f8a30f3f00.

No problem! It was a bit confusing indeed.

> Yes and no. Let's recall the whole story.
>
> Before 815f0ddb346c("include/linux/compiler*.h: make compiler-*.h mutually exclusive"),
> __gnu_inline and inline were both *in* __KERNEL__ as the were in
> <linux/compiler-gcc.h>, which was entirely put to __KERNEL__ in
> <linux/compiler_types.h>. Everything was fine.
>
> Then 815f0ddb346c moved inline and __gnu_inline *out* of __KERNEL__
> and put them in <linux/compiler_types.h> so userspace could see them
> both. Not sure if it's intended behavior, but everything looked fine.
>
> Then a3f8a30f3f00 moved __gnu_inline back into __KERNEL__ and left
> inline behind. Since inline depends on __gnu_inline, error showing
> "unknown type name ‘__gnu_inline’" pops up.

Exactly, thanks a lot for clarifying it up (we should put this in the
commit message, I would say). That also answers my question: it is
clear everything should be back into __KERNEL__. The only worry is
that the v4.19 release contained 815f0ddb346c, so it has them exposed,
so someone could have started relying on them. Or, more likely, the
exposed macros could break some source code out there. Hm... Should a
"fix" be backported?

Cheers,
Miguel

Re: [PATCH] Compiler Attributes: move kernel-only attributes into __KERNEL__

[Nick Desaulniers]

On Wed, Nov 28, 2018 at 9:35 AM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> Hi Xiaozhou,
>
> On Wed, Nov 28, 2018 at 3:09 PM Xiaozhou Liu <liuxiaozhou@bytedance.com> wrote:
> >
> > Attributes such as `__gnu_inline' are meant to be used within the
> > kernel. When userspace somehow includes <linux/compiler.h>
> > (eg. tools/bpf), compilation errors would be shown:
> >
> >         "error: unknown type name ‘__gnu_inline’"
> >
> > So just move these things into __KERNEL__ and the behavior is kept
> > as before.
>
> That is not exactly correct -- a3f8a30f3f00 moved some attributes to
> another file, moving them into __KERNEL__ (in particular,__gnu_inline
> is).
>
> The problem is, instead, that __gnu_inline is not anymore defined
> outside __KERNEL__, but something else that uses it is (the inline
> macro definition, if I had to guess).
>
> If your problem is fixed by putting __gnu_inline into __KERNEL__
> again, it means we can simply move the inline definition inside
> __KERNEL__ too. That way, we don't pollute userspace users with macro
> definitions.
>
> Having said that, does someone know whether userspace should have
> access to those attributes (or rather, other code that uses in turn
> those attributes)?

This is tricky territory; the kernel is redefining `inline` in headers
in order to get __attribute__((gnu_inline)) semantics.  If non kernel
users include those headers, the kernel may be redefining the
semantics of `inline` for those programs (unexpectedly).  Admittedly,
gnu_inline kind of an edge case so most users might not notice, but
that may be an argument for `-f gnu_inline` rather than redefining
`inline` in a header.

>
> Cheers,
> Miguel



-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] Compiler Attributes: move kernel-only attributes into __KERNEL__

[Xiaozhou Liu]

On Thu, Nov 29, 2018 at 11:42:11AM +0100, Miguel Ojeda wrote:

> Exactly, thanks a lot for clarifying it up (we should put this in the
> commit message, I would say). That also answers my question: it is
> clear everything should be back into __KERNEL__. The only worry is
> that the v4.19 release contained 815f0ddb346c, so it has them exposed,
> so someone could have started relying on them. Or, more likely, the
> exposed macros could break some source code out there. Hm... Should a
> "fix" be backported?

What about letting v4.19 maintainers make the decision?

Should I send a v2 patch updating the comments and Acked-by (you)?


Thanks,
Xiaozhou

Re: [PATCH] Compiler Attributes: move kernel-only attributes into __KERNEL__

[Greg KH]

On Thu, Dec 06, 2018 at 08:39:26PM +0800, Xiaozhou Liu wrote:
> On Thu, Nov 29, 2018 at 11:42:11AM +0100, Miguel Ojeda wrote:
> 
> > Exactly, thanks a lot for clarifying it up (we should put this in the
> > commit message, I would say). That also answers my question: it is
> > clear everything should be back into __KERNEL__. The only worry is
> > that the v4.19 release contained 815f0ddb346c, so it has them exposed,
> > so someone could have started relying on them. Or, more likely, the
> > exposed macros could break some source code out there. Hm... Should a
> > "fix" be backported?
> 
> What about letting v4.19 maintainers make the decision?

If something is fixed in Linus's tree for this, I want to take it into
the 4.19-stable tree as well.

thanks,

greg k-h

Re: [PATCH] Compiler Attributes: move kernel-only attributes into __KERNEL__

[Miguel Ojeda]

Hi Greg, Nick, Xiaozhou,

On Thu, Dec 6, 2018 at 1:50 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> If something is fixed in Linus's tree for this, I want to take it into
> the 4.19-stable tree as well.

This ended up in Linus' tree before the holidays, i.e. 4.20 has it,
see commit 71391bdd2e9a ("include/linux/compiler_types.h: don't
pollute userspace with macro definitions").

In case you want to still backport this to 4.19: you can't cherry-pick
it without conflicts because some stuff was moved around due to the
Compiler Attributes patch series (which also went in with 4.20 too),
but you can move the macros like this commit does. There are 2
conflicts:

  * The big block of macros for attributes:
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/include/linux/compiler_types.h?h=v4.19.16#n189
  * The __always_inline macro:
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/include/linux/compiler_types.h?h=v4.19.16#n275

All those are nowadays inside __KERNEL__ && !__ASSEMBLY__, so it
should be fine to move those too along the rest that this patch moves.

Cheers,
Miguel

Re: [PATCH] Compiler Attributes: move kernel-only attributes into __KERNEL__

[Greg KH]

On Sat, Jan 19, 2019 at 07:22:50PM +0100, Miguel Ojeda wrote:
> Hi Greg, Nick, Xiaozhou,
> 
> On Thu, Dec 6, 2018 at 1:50 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > If something is fixed in Linus's tree for this, I want to take it into
> > the 4.19-stable tree as well.
> 
> This ended up in Linus' tree before the holidays, i.e. 4.20 has it,
> see commit 71391bdd2e9a ("include/linux/compiler_types.h: don't
> pollute userspace with macro definitions").
> 
> In case you want to still backport this to 4.19: you can't cherry-pick
> it without conflicts because some stuff was moved around due to the
> Compiler Attributes patch series (which also went in with 4.20 too),
> but you can move the macros like this commit does. There are 2
> conflicts:
> 
>   * The big block of macros for attributes:
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/include/linux/compiler_types.h?h=v4.19.16#n189
>   * The __always_inline macro:
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/include/linux/compiler_types.h?h=v4.19.16#n275
> 
> All those are nowadays inside __KERNEL__ && !__ASSEMBLY__, so it
> should be fine to move those too along the rest that this patch moves.

I have no idea why I would want to backport this, sorry :(

If this resolves a problem, great, but someone has to do the backport
for me to be able to take it.

thanks,

greg k-h

> 
> Cheers,
> Miguel