[GIT PULL] Compiler Attributes for v4.20-rc1

[GIT PULL] Compiler Attributes for v4.20-rc1

[Miguel Ojeda]

Hi Linus,

Here it is the Compiler Attributes series/tree, which tries to disentangle
the include/linux/compiler*.h headers and bring them up to date.

The patches have been in linux-next for a while, *except* the last two
from Nick which came a bit later. Since AFAIU there will be no linux-next
this week, I included them here; but let me know if you prefer
to take them out.

You may see merge conflicts from a few other trees, from what we have
seen in linux-next.

I am not sure if you followed this, so please let me know if there is
anything you don't agree with.

And welcome back!

Cheers,
Miguel

The following changes since commit 17b57b1883c1285f3d0dc2266e8f79286a7bef38:

  Linux 4.19-rc6 (2018-09-30 07:15:35 -0700)

are available in the Git repository at:

  https://github.com/ojeda/linux.git tags/compiler-attributes-for-linus-4.20-rc1

for you to fetch changes up to 1ff2fea5e30ca15752777441ecb64a169fe22e9e:

  compiler-gcc: remove comment about gcc 4.5 from unreachable() (2018-10-19 08:47:30 +0200)

----------------------------------------------------------------
The Compiler Attributes series

This is an effort to disentangle the include/linux/compiler*.h headers
and bring them up to date.

The main idea behind the series is to use feature checking macros
(i.e. __has_attribute) instead of compiler version checks (e.g. GCC_VERSION),
which are compiler-agnostic (so they can be shared, reducing the size
of compiler-specific headers) and version-agnostic.

Other related improvements have been performed in the headers as well,
which on top of the use of __has_attribute it has amounted to a significant
simplification of these headers (e.g. GCC_VERSION is now only guarding
a few non-attribute macros).

This series should also help the efforts to support compiling the kernel
with clang and icc. A fair amount of documentation and comments have also
been added, clarified or removed; and the headers are now more readable,
which should help kernel developers in general.

The series was triggered due to the move to gcc >= 4.6. In turn, this series
has also triggered Sparse to gain the ability to recognize __has_attribute
on its own.

Finally, the __nonstring variable attribute series has been also applied
on top; plus two related patches from Nick Desaulniers for unreachable()
that came a bit afterwards.

----------------------------------------------------------------
Miguel Ojeda (15):
      Compiler Attributes: remove unused attributes
      Compiler Attributes: always use the extra-underscores syntax
      Compiler Attributes: remove unneeded tests
      Compiler Attributes: homogenize __must_be_array
      Compiler Attributes: remove unneeded sparse (__CHECKER__) tests
      Compiler Attributes: add missing SPDX ID in compiler_types.h
      Compiler Attributes: use feature checks instead of version checks
      Compiler Attributes: KENTRY used twice the "used" attribute
      Compiler Attributes: remove uses of __attribute__ from compiler.h
      Compiler Attributes: add Doc/process/programming-language.rst
      Compiler Attributes: add MAINTAINERS entry
      Compiler Attributes: add support for __nonstring (gcc >= 8)
      Compiler Attributes: enable -Wstringop-truncation on W=1 (gcc >= 8)
      Compiler Attributes: auxdisplay: panel: use __nonstring
      Compiler Attributes: ext4: remove local __nonstring definition

ndesaulniers@google.com (2):
      compiler.h: update definition of unreachable()
      compiler-gcc: remove comment about gcc 4.5 from unreachable()

 Documentation/process/index.rst                |   1 +
 Documentation/process/programming-language.rst |  45 +++++
 MAINTAINERS                                    |   5 +
 drivers/auxdisplay/panel.c                     |   7 +-
 fs/ext4/ext4.h                                 |   9 -
 include/linux/compiler-clang.h                 |   5 -
 include/linux/compiler-gcc.h                   |  74 +------
 include/linux/compiler-intel.h                 |   9 -
 include/linux/compiler.h                       |  24 +--
 include/linux/compiler_attributes.h            | 258 +++++++++++++++++++++++++
 include/linux/compiler_types.h                 | 101 ++--------
 scripts/Makefile.extrawarn                     |   1 +
 12 files changed, 345 insertions(+), 194 deletions(-)
 create mode 100644 Documentation/process/programming-language.rst
 create mode 100644 include/linux/compiler_attributes.h

[GIT PULL] Compiler Attributes for v4.20-rc1

[Miguel Ojeda]

Hi Linus,

Here it is the Compiler Attributes series/tree, which tries to disentangle
the include/linux/compiler*.h headers and bring them up to date.

The patches have been in linux-next for a while, *except* the last two
from Nick which came a bit later. Since AFAIU there will be no linux-next
this week, I included them here; but let me know if you prefer
to take them out.

You may see merge conflicts from a few other trees, from what we have
seen in linux-next.

I am not sure if you followed this, so please let me know if there is
anything you don't agree with.

And welcome back!

Cheers,
Miguel

The following changes since commit 17b57b1883c1285f3d0dc2266e8f79286a7bef38:

  Linux 4.19-rc6 (2018-09-30 07:15:35 -0700)

are available in the Git repository at:

  https://github.com/ojeda/linux.git tags/compiler-attributes-for-linus-4.20-rc1

for you to fetch changes up to 1ff2fea5e30ca15752777441ecb64a169fe22e9e:

  compiler-gcc: remove comment about gcc 4.5 from unreachable() (2018-10-19 08:47:30 +0200)

----------------------------------------------------------------
The Compiler Attributes series

This is an effort to disentangle the include/linux/compiler*.h headers
and bring them up to date.

The main idea behind the series is to use feature checking macros
(i.e. __has_attribute) instead of compiler version checks (e.g. GCC_VERSION),
which are compiler-agnostic (so they can be shared, reducing the size
of compiler-specific headers) and version-agnostic.

Other related improvements have been performed in the headers as well,
which on top of the use of __has_attribute it has amounted to a significant
simplification of these headers (e.g. GCC_VERSION is now only guarding
a few non-attribute macros).

This series should also help the efforts to support compiling the kernel
with clang and icc. A fair amount of documentation and comments have also
been added, clarified or removed; and the headers are now more readable,
which should help kernel developers in general.

The series was triggered due to the move to gcc >= 4.6. In turn, this series
has also triggered Sparse to gain the ability to recognize __has_attribute
on its own.

Finally, the __nonstring variable attribute series has been also applied
on top; plus two related patches from Nick Desaulniers for unreachable()
that came a bit afterwards.

----------------------------------------------------------------
Miguel Ojeda (15):
      Compiler Attributes: remove unused attributes
      Compiler Attributes: always use the extra-underscores syntax
      Compiler Attributes: remove unneeded tests
      Compiler Attributes: homogenize __must_be_array
      Compiler Attributes: remove unneeded sparse (__CHECKER__) tests
      Compiler Attributes: add missing SPDX ID in compiler_types.h
      Compiler Attributes: use feature checks instead of version checks
      Compiler Attributes: KENTRY used twice the "used" attribute
      Compiler Attributes: remove uses of __attribute__ from compiler.h
      Compiler Attributes: add Doc/process/programming-language.rst
      Compiler Attributes: add MAINTAINERS entry
      Compiler Attributes: add support for __nonstring (gcc >= 8)
      Compiler Attributes: enable -Wstringop-truncation on W=1 (gcc >= 8)
      Compiler Attributes: auxdisplay: panel: use __nonstring
      Compiler Attributes: ext4: remove local __nonstring definition

ndesaulniers@google.com (2):
      compiler.h: update definition of unreachable()
      compiler-gcc: remove comment about gcc 4.5 from unreachable()

 Documentation/process/index.rst                |   1 +
 Documentation/process/programming-language.rst |  45 +++++
 MAINTAINERS                                    |   5 +
 drivers/auxdisplay/panel.c                     |   7 +-
 fs/ext4/ext4.h                                 |   9 -
 include/linux/compiler-clang.h                 |   5 -
 include/linux/compiler-gcc.h                   |  74 +------
 include/linux/compiler-intel.h                 |   9 -
 include/linux/compiler.h                       |  24 +--
 include/linux/compiler_attributes.h            | 258 +++++++++++++++++++++++++
 include/linux/compiler_types.h                 | 101 ++--------
 scripts/Makefile.extrawarn                     |   1 +
 12 files changed, 345 insertions(+), 194 deletions(-)
 create mode 100644 Documentation/process/programming-language.rst
 create mode 100644 include/linux/compiler_attributes.h

Re: [GIT PULL] Compiler Attributes for v4.20-rc1

[Linus Torvalds]

On Mon, Oct 22, 2018 at 3:59 AM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> Here it is the Compiler Attributes series/tree, which tries to disentangle
> the include/linux/compiler*.h headers and bring them up to date.

I've finally emptied the "normal" pull queues, and am looking at the
odd ones that I felt I needed to review more in-depth.

This looked fine to me, and I already pulled, but when looking at the
conflict for __no_sanitize_address_or_inline, I also noticed that you
actually changed the gcc version logic.

The logic for using __no_sanitize_address *used* to be

    #if GCC_VERSION >= 40902

but you have changed it to

  # define __GCC4_has_attribute___no_sanitize_address__ (__GNUC_MINOR__ >= 8)

so now it's gcc-4.8+ rather than gcc-4.9.2+.

I'm not sure how much that matters (maybe the original check for 4.9.2
was just a random pick by Andrey? Added to cc), but together with the
movement to <linux/compiler_attributes.h> that looks like it also
wouldn't want the CONFIG_KASAN tests, I wonder what the right merge
resolution would be.

Yes, I see the resolution in linux-next, and I think that one is odd
and dubious, and now it *mixes* that old test of gcc-4.9.2 with the
different test in compiler-attributes.

I'm _inclined_ to just do

#ifdef CONFIG_KASAN
 #define __no_sanitize_address_or_inline \
  __no_sanitize_address __maybe_unused notrace
#else
 #define __no_sanitize_address_or_inline inline
#endif

without any compiler versions at all, because I don't think it matters
(maybe we won't get address sanitation, and we will not get inlining
either, but do we care?).

But I'm also unsure whether you meant for the "__has_attribute()" test
to be usable outside the linux/compiler_attributes.h file, in which
case I could just do

  #if defined(CONFIG_KASAN) && __has_attribute(__no_sanitize_address__)

instead.

End result: it's not the merge resolution itself that raises
questions, it's just semantics in general.

So I've dropped this for now, in the hope that you/Andrey can answer
these questions.

                   Linus

Re: [GIT PULL] Compiler Attributes for v4.20-rc1

[Miguel Ojeda]

On Thu, Nov 1, 2018 at 6:06 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> I'm not sure how much that matters (maybe the original check for 4.9.2
> was just a random pick by Andrey? Added to cc), but together with the
> movement to <linux/compiler_attributes.h> that looks like it also
> wouldn't want the CONFIG_KASAN tests, I wonder what the right merge
> resolution would be.

Good catch. I don't recall any special logic when I did that change,
so most likely I simply did like for the rest of the attributes and
took a look at when it was first supported (and documentation in gcc's
docs) in order to implement __has_attribute by hand.

But indeed, it *may* be that there is an (undocumented) problem
between 4.8 <= gcc < 4.9.2 with it. If so, we should document it down
and fix it. Andrey?

> Yes, I see the resolution in linux-next, and I think that one is odd
> and dubious, and now it *mixes* that old test of gcc-4.9.2 with the
> different test in compiler-attributes.

I missed that conflict completely, my bad (I did not miss all of them,
at least; one required fixing).

Hm.... at a quick look, why is it only on compiler-gcc.h? It should
either have a corresponding #define elsewhere or just be put directly
in another common header, no? (Adding Vasily & Martin to CC.)

> But I'm also unsure whether you meant for the "__has_attribute()" test
> to be usable outside the linux/compiler_attributes.h file, in which
> case I could just do
>
>   #if defined(CONFIG_KASAN) && __has_attribute(__no_sanitize_address__)
>
> instead.

I think that (using __has_attribute() outside) may be a good idea: I
wanted to keep compiler_attributes.h as simple as possible by avoiding
#ifdefs inside that header (except for __has_attribute itself), as an
attempt to avoid going back to the mess of #ifdefs we had previously.
Basically, keeping the attributes in compiler_attributes.h that do not
depend on complex logic. So using __has_attribute *outside* the header
actually goes well with that principle, because it helps keeping stuff
out of it if they depend on other config options; without having to
rely on GCC_VERSION either.

[By the way, in case it clarifies: note that "optional" in that file
actually is a bit of a misnomer. I meant to say "optional" as in "not
supported by all compilers, so conditionally defined" ("optionally"
defined); rather than "optional" in the sense of "code still works
without the attribute". It caught Rasmus in one of his patches sent a
few days ago on top of this tree, so I want to change it or explain it
to avoid confusion.]

Cheers,
Miguel

Re: [GIT PULL] Compiler Attributes for v4.20-rc1

[Linus Torvalds]

On Thu, Nov 1, 2018 at 10:06 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> The logic for using __no_sanitize_address *used* to be
>
>     #if GCC_VERSION >= 40902

Ok, looking around, I think this has less to do with the attribute
being recognized, and simply just being because KASAN itself wants
gcc-4.9.2.

I'm actually not seeing that KASAN dependency in the Kconfig scripts
(and it probably _should_ be now that we can just add compiler version
dependencies there), but that explains why the gcc version check is
different from "gcc supports the attribute".

Anyway, I decided to do the merge by just getting rid of the
GCC_VERSION check around __no_sanitize_address_or_inline entirely. If
you enable KASAN, then a function with that marking just won't be
marked inline.

End result: pulled. I'm as confused as you are as to why
__no_sanitize_address_or_inline is in the gcc header, but I guess it
ends up being the same issue: KASAN depends on gcc even if that
dependency doesn't seem to be spelled out in lib/Kconfig.kasan.

So I _think_ the KASAN config should have a

    depends on CC_IS_GCC && GCC_VERSION >= 40902

on it, but maybe there is something I'm missing.

But from a pull standpoint, I don't want to mess with those
(unrelated) issues, so I just kept the merge resolution as simple and
straightforward as possible.

Miguel, please do double-check the merge (it's not pushed out yet, I'm
doing the usual build tests etc first).

                    Linus

Re: [GIT PULL] Compiler Attributes for v4.20-rc1

[Miguel Ojeda]

On Fri, Nov 2, 2018 at 2:52 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Anyway, I decided to do the merge by just getting rid of the
> GCC_VERSION check around __no_sanitize_address_or_inline entirely. If
> you enable KASAN, then a function with that marking just won't be
> marked inline.

I was a bit confused when reading the gcc bug reports, i.e. why gcc
did *not* complain in 4.9 but it did in 5.1 (when it was supposed to
complain also in 4.9). It turns out that gcc 5.1 takes into account
who is the actual caller due to this change:

+  cgraph_node *caller = e->caller->global.inlined_to
+                       ? e->caller->global.inlined_to : e->caller;
...
-  else if (!sanitize_attrs_match_for_inline_p (e->caller->decl, callee->decl))
+  else if (!sanitize_attrs_match_for_inline_p (caller->decl, callee->decl))

change; e.g. this:

#define really_inline inline __attribute__((always_inline))
#define no_sanitize __attribute__((no_sanitize_address))

really_inline void f() {}
really_inline void g() { f(); }
no_sanitize void h() { g(); }

Complains in gcc 4.9 -O0, 5.1 -O0 and 5.1 -O2; but *not* in 4.9 -O2.
https://godbolt.org/z/kNApqk

Anyway, this is orthogonal but in case it clarifies that for someone else...

> Miguel, please do double-check the merge (it's not pushed out yet, I'm
> doing the usual build tests etc first).

I was sleeping, didn't manage to see it (in your GitHub, I guess?).

I did the merge myself, and arrived at the same thing as you. I
quickly inspected the rest and seems fine. By the way, I spotted an
extra space at:

+ * we do one or the other.

Cheers,
Miguel

Re: [GIT PULL] Compiler Attributes for v4.20-rc1

[Andrey Ryabinin]

On 11/02/2018 04:46 AM, Linus Torvalds wrote:
> On Thu, Nov 1, 2018 at 10:06 AM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> The logic for using __no_sanitize_address *used* to be
>>
>>     #if GCC_VERSION >= 40902
> 
> Ok, looking around, I think this has less to do with the attribute
> being recognized, and simply just being because KASAN itself wants
> gcc-4.9.2.
> 
> I'm actually not seeing that KASAN dependency in the Kconfig scripts
> (and it probably _should_ be now that we can just add compiler version
> dependencies there), but that explains why the gcc version check is
> different from "gcc supports the attribute".
> 
> Anyway, I decided to do the merge by just getting rid of the
> GCC_VERSION check around __no_sanitize_address_or_inline entirely. If
> you enable KASAN, then a function with that marking just won't be
> marked inline.
> 
> End result: pulled. I'm as confused as you are as to why
> __no_sanitize_address_or_inline is in the gcc header, but I guess it
> ends up being the same issue: KASAN depends on gcc even if that
> dependency doesn't seem to be spelled out in lib/Kconfig.kasan.
> 
> So I _think_ the KASAN config should have a
> 
>     depends on CC_IS_GCC && GCC_VERSION >= 40902
> 
> on it, but maybe there is something I'm missing.
> 

I'd rather use cc-option instead of version check, since we also support clang.

It should be possible to express compiler requirements for subfeatures
like stack/inline instrumentation in Kconfig. But that would be not that trivial, 
see the scripts/Makefile.kasan

> But from a pull standpoint, I don't want to mess with those
> (unrelated) issues, so I just kept the merge resolution as simple and
> straightforward as possible.
> 
> Miguel, please do double-check the merge (it's not pushed out yet, I'm
> doing the usual build tests etc first).
> 
>                     Linus
> 

Re: [GIT PULL] Compiler Attributes for v4.20-rc1

[Linus Torvalds]

On Fri, Nov 2, 2018 at 6:16 AM Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
>
> On 11/02/2018 04:46 AM, Linus Torvalds wrote:
> >
> > So I _think_ the KASAN config should have a
> >
> >     depends on CC_IS_GCC && GCC_VERSION >= 40902
> >
> > on it, but maybe there is something I'm missing.
>
> I'd rather use cc-option instead of version check, since we also support clang.

That would be even better, but I thought the requirement for 4.9.2
came not from the option existing, but because of some bugs getting
fixed?

But if we can do it without version checks, that would be lovely.

                    Linus

Re: [GIT PULL] Compiler Attributes for v4.20-rc1

[Andrey Ryabinin]

On 11/02/2018 07:11 PM, Linus Torvalds wrote:
> On Fri, Nov 2, 2018 at 6:16 AM Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
>>
>> On 11/02/2018 04:46 AM, Linus Torvalds wrote:
>>>
>>> So I _think_ the KASAN config should have a
>>>
>>>     depends on CC_IS_GCC && GCC_VERSION >= 40902
>>>
>>> on it, but maybe there is something I'm missing.
>>
>> I'd rather use cc-option instead of version check, since we also support clang.
> 
> That would be even better, but I thought the requirement for 4.9.2
> came not from the option existing, but because of some bugs getting
> fixed?

It looks unusual but -fsanitize=kernel-address really showed up only in 4.9.2.
It was actually backported from 5.0 branch to 4.9 for whatever reason.

> 
> But if we can do it without version checks, that would be lovely.
> 
>                     Linus
> 

Re: [GIT PULL] Compiler Attributes for v4.20-rc1

[Andrey Ryabinin]

On 11/01/2018 08:06 PM, Linus Torvalds wrote:
> On Mon, Oct 22, 2018 at 3:59 AM Miguel Ojeda
> <miguel.ojeda.sandonis@gmail.com> wrote:
>>
>> Here it is the Compiler Attributes series/tree, which tries to disentangle
>> the include/linux/compiler*.h headers and bring them up to date.
> 
> I've finally emptied the "normal" pull queues, and am looking at the
> odd ones that I felt I needed to review more in-depth.
> 
> This looked fine to me, and I already pulled, but when looking at the
> conflict for __no_sanitize_address_or_inline, I also noticed that you
> actually changed the gcc version logic.
> 
> The logic for using __no_sanitize_address *used* to be
> 
>     #if GCC_VERSION >= 40902
> 
> but you have changed it to
> 
>   # define __GCC4_has_attribute___no_sanitize_address__ (__GNUC_MINOR__ >= 8)
> 
> so now it's gcc-4.8+ rather than gcc-4.9.2+.

As you said in other email - "this has less to do with the attribute
being recognized, and simply just being because KASAN itself wants
gcc-4.9.2."

gcc < 4.9.2 simply doesn't have -fsanitize=kernel-address.
But no_sanitize attribute originally comes with user space ASAN (-fsanitize=address)
which exist in earlier GCCs like 4.8.
Checking against 4.8 gcc should be fine. If we compile the kernel with gcc 4.8
it will be compiled without instrumentation, so the attribute won't have any effect.

> 
> I'm not sure how much that matters (maybe the original check for 4.9.2
> was just a random pick by Andrey? Added to cc), but together with the
> movement to <linux/compiler_attributes.h> that looks like it also
> wouldn't want the CONFIG_KASAN tests, I wonder what the right merge
> resolution would be.
> 
> Yes, I see the resolution in linux-next, and I think that one is odd
> and dubious, and now it *mixes* that old test of gcc-4.9.2 with the
> different test in compiler-attributes.
> 
> I'm _inclined_ to just do
> 
> #ifdef CONFIG_KASAN
>  #define __no_sanitize_address_or_inline \
>   __no_sanitize_address __maybe_unused notrace
> #else
>  #define __no_sanitize_address_or_inline inline
> #endif
> 
> without any compiler versions at all, because I don't think it matters
> (maybe we won't get address sanitation, and we will not get inlining
> either, but do we care?).

You're right, version checks shouldn't matter here. But __no_sanitize_address_or_inline
shouldn't have been added in the first place, because we already have almost the same __no_kasan_or_inline:

#ifdef CONFIG_KASAN
/*
 * We can't declare function 'inline' because __no_sanitize_address confilcts
 * with inlining. Attempt to inline it may cause a build failure.
 * 	https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368
 * '__maybe_unused' allows us to avoid defined-but-not-used warnings.
 */
# define __no_kasan_or_inline __no_sanitize_address __maybe_unused
#else
# define __no_kasan_or_inline __always_inline
#endif

There are small differences like absence of notrace, but notrace could be added
to the function together with __no_kasan_or_inline.
And inline is *not* redefined to __always_inline  only on x86 when CONFIG_OPTIMIZE_INLINING=y

So there is absolutely no reason to have both __no_kasan_or_inline and __no_sanitize_address_or_inline.

Re: [GIT PULL] Compiler Attributes for v4.20-rc1

[Linus Torvalds]

On Fri, Nov 2, 2018 at 2:43 AM Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
>
> You're right, version checks shouldn't matter here. But __no_sanitize_address_or_inline
> shouldn't have been added in the first place, because we already have almost the same
>__no_kasan_or_inline:

Ahh, very good.

Vasily, Martin - since __no_sanitize_address_or_inline was added just
for s390, would you mind replacing it with __no_kasan_or_inline
instead, and testing that in whatever failed before?

Then we can just remove that unnecessary symbol #define entirely..

Thanks,

                 Linus

Re: [GIT PULL] Compiler Attributes for v4.20-rc1

[Martin Schwidefsky]

On Fri, 2 Nov 2018 09:09:32 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Fri, Nov 2, 2018 at 2:43 AM Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:
> >
> > You're right, version checks shouldn't matter here. But __no_sanitize_address_or_inline
> > shouldn't have been added in the first place, because we already have almost the same
> >__no_kasan_or_inline:  
> 
> Ahh, very good.
> 
> Vasily, Martin - since __no_sanitize_address_or_inline was added just
> for s390, would you mind replacing it with __no_kasan_or_inline
> instead, and testing that in whatever failed before?
> 
> Then we can just remove that unnecessary symbol #define entirely..

Ok, will do.

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

Re: [GIT PULL] Compiler Attributes for v4.20-rc1

[Martin Schwidefsky]

On Mon, 5 Nov 2018 07:02:56 +0100
Martin Schwidefsky <schwidefsky@de.ibm.com> wrote:

> On Fri, 2 Nov 2018 09:09:32 -0700
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
> > On Fri, Nov 2, 2018 at 2:43 AM Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:  
> > >
> > > You're right, version checks shouldn't matter here. But __no_sanitize_address_or_inline
> > > shouldn't have been added in the first place, because we already have almost the same
> > >__no_kasan_or_inline:    
> > 
> > Ahh, very good.
> > 
> > Vasily, Martin - since __no_sanitize_address_or_inline was added just
> > for s390, would you mind replacing it with __no_kasan_or_inline
> > instead, and testing that in whatever failed before?
> > 
> > Then we can just remove that unnecessary symbol #define entirely..  
> 
> Ok, will do.
 
Follow-up question: the __no_sanitize_address_or_inline define has the 'notrace'
option that is missing for __no_kasan_or_inline. We need that option for
__load_psw_mask, if we do the replacement all users of __no_kasan_or_inline
would get the option as well. This affects __read_once_size_nocheck and
read_word_at_a_time. Do these function have to be traceable ?

This patch would work for me:
--
>From 4aaa09fe4b54e930edabac86606dee979b12647c Mon Sep 17 00:00:00 2001
From: Martin Schwidefsky <schwidefsky@de.ibm.com>
Date: Mon, 5 Nov 2018 07:36:28 +0100
Subject: [PATCH] compiler: remove __no_sanitize_address_or_inline again

The __no_sanitize_address_or_inline and __no_kasan_or_inline defines
are almost identical. The only difference is that __no_kasan_or_inline
does not have the 'notrace' attribute.

To be able to replace __no_sanitize_address_or_inline with the older
definition, add 'notrace' to __no_kasan_or_inline and change to two
users of __no_sanitize_address_or_inline in the s390 code.

The 'notrace' option is necessary for e.g. the __load_psw_mask function
in arch/s390/include/asm/processor.h. Without the option it is possible
to trace __load_psw_mask which leads to kernel stack overflow.

Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
---
 arch/s390/include/asm/processor.h |  4 ++--
 include/linux/compiler-gcc.h      | 12 ------------
 include/linux/compiler.h          |  2 +-
 3 files changed, 3 insertions(+), 15 deletions(-)

diff --git a/arch/s390/include/asm/processor.h b/arch/s390/include/asm/processor.h
index 302795c47c06..81038ab357ce 100644
--- a/arch/s390/include/asm/processor.h
+++ b/arch/s390/include/asm/processor.h
@@ -236,7 +236,7 @@ static inline unsigned long current_stack_pointer(void)
 	return sp;
 }
 
-static __no_sanitize_address_or_inline unsigned short stap(void)
+static __no_kasan_or_inline unsigned short stap(void)
 {
 	unsigned short cpu_address;
 
@@ -330,7 +330,7 @@ static inline void __load_psw(psw_t psw)
  * Set PSW mask to specified value, while leaving the
  * PSW addr pointing to the next instruction.
  */
-static __no_sanitize_address_or_inline void __load_psw_mask(unsigned long mask)
+static __no_kasan_or_inline void __load_psw_mask(unsigned long mask)
 {
 	unsigned long addr;
 	psw_t psw;
diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
index c0f5db3a9621..2010493e1040 100644
--- a/include/linux/compiler-gcc.h
+++ b/include/linux/compiler-gcc.h
@@ -143,18 +143,6 @@
 #define KASAN_ABI_VERSION 3
 #endif
 
-/*
- * Because __no_sanitize_address conflicts with inlining:
- *   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368
- * we do one or the other. 
- */
-#ifdef CONFIG_KASAN
-#define __no_sanitize_address_or_inline					\
-	__no_sanitize_address __maybe_unused notrace
-#else
-#define __no_sanitize_address_or_inline inline
-#endif
-
 #if GCC_VERSION >= 50100
 #define COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW 1
 #endif
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 18c80cfa4fc4..06396c1cf127 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -189,7 +189,7 @@ void __read_once_size(const volatile void *p, void *res, int size)
  * 	https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368
  * '__maybe_unused' allows us to avoid defined-but-not-used warnings.
  */
-# define __no_kasan_or_inline __no_sanitize_address __maybe_unused
+# define __no_kasan_or_inline __no_sanitize_address notrace __maybe_unused
 #else
 # define __no_kasan_or_inline __always_inline
 #endif
-- 
2.16.4
-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

Re: [GIT PULL] Compiler Attributes for v4.20-rc1

[Martin Schwidefsky]

On Mon, 5 Nov 2018 07:02:56 +0100
Martin Schwidefsky <schwidefsky@de.ibm.com> wrote:

> On Fri, 2 Nov 2018 09:09:32 -0700
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
> > On Fri, Nov 2, 2018 at 2:43 AM Andrey Ryabinin <aryabinin@virtuozzo.com> wrote:  
> > >
> > > You're right, version checks shouldn't matter here. But __no_sanitize_address_or_inline
> > > shouldn't have been added in the first place, because we already have almost the same
> > >__no_kasan_or_inline:    
> > 
> > Ahh, very good.
> > 
> > Vasily, Martin - since __no_sanitize_address_or_inline was added just
> > for s390, would you mind replacing it with __no_kasan_or_inline
> > instead, and testing that in whatever failed before?
> > 
> > Then we can just remove that unnecessary symbol #define entirely..  
> 
> Ok, will do.
 
Follow-up question: the __no_sanitize_address_or_inline define has the 'notrace'
option that is missing for __no_kasan_or_inline. We need that option for
__load_psw_mask, if we do the replacement all users of __no_kasan_or_inline
would get the option as well. This affects __read_once_size_nocheck and
read_word_at_a_time. Do these function have to be traceable ?

This patch would work for me:
--
From 4aaa09fe4b54e930edabac86606dee979b12647c Mon Sep 17 00:00:00 2001
From: Martin Schwidefsky <schwidefsky@de.ibm.com>
Date: Mon, 5 Nov 2018 07:36:28 +0100
Subject: [PATCH] compiler: remove __no_sanitize_address_or_inline again

The __no_sanitize_address_or_inline and __no_kasan_or_inline defines
are almost identical. The only difference is that __no_kasan_or_inline
does not have the 'notrace' attribute.

To be able to replace __no_sanitize_address_or_inline with the older
definition, add 'notrace' to __no_kasan_or_inline and change to two
users of __no_sanitize_address_or_inline in the s390 code.

The 'notrace' option is necessary for e.g. the __load_psw_mask function
in arch/s390/include/asm/processor.h. Without the option it is possible
to trace __load_psw_mask which leads to kernel stack overflow.

Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
---
 arch/s390/include/asm/processor.h |  4 ++--
 include/linux/compiler-gcc.h      | 12 ------------
 include/linux/compiler.h          |  2 +-
 3 files changed, 3 insertions(+), 15 deletions(-)

diff --git a/arch/s390/include/asm/processor.h b/arch/s390/include/asm/processor.h
index 302795c47c06..81038ab357ce 100644
--- a/arch/s390/include/asm/processor.h
+++ b/arch/s390/include/asm/processor.h
@@ -236,7 +236,7 @@ static inline unsigned long current_stack_pointer(void)
 	return sp;
 }
 
-static __no_sanitize_address_or_inline unsigned short stap(void)
+static __no_kasan_or_inline unsigned short stap(void)
 {
 	unsigned short cpu_address;
 
@@ -330,7 +330,7 @@ static inline void __load_psw(psw_t psw)
  * Set PSW mask to specified value, while leaving the
  * PSW addr pointing to the next instruction.
  */
-static __no_sanitize_address_or_inline void __load_psw_mask(unsigned long mask)
+static __no_kasan_or_inline void __load_psw_mask(unsigned long mask)
 {
 	unsigned long addr;
 	psw_t psw;
diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
index c0f5db3a9621..2010493e1040 100644
--- a/include/linux/compiler-gcc.h
+++ b/include/linux/compiler-gcc.h
@@ -143,18 +143,6 @@
 #define KASAN_ABI_VERSION 3
 #endif
 
-/*
- * Because __no_sanitize_address conflicts with inlining:
- *   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368
- * we do one or the other. 
- */
-#ifdef CONFIG_KASAN
-#define __no_sanitize_address_or_inline					\
-	__no_sanitize_address __maybe_unused notrace
-#else
-#define __no_sanitize_address_or_inline inline
-#endif
-
 #if GCC_VERSION >= 50100
 #define COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW 1
 #endif
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 18c80cfa4fc4..06396c1cf127 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -189,7 +189,7 @@ void __read_once_size(const volatile void *p, void *res, int size)
  * 	https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368
  * '__maybe_unused' allows us to avoid defined-but-not-used warnings.
  */
-# define __no_kasan_or_inline __no_sanitize_address __maybe_unused
+# define __no_kasan_or_inline __no_sanitize_address notrace __maybe_unused
 #else
 # define __no_kasan_or_inline __always_inline
 #endif
-- 
2.16.4
-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

Re: [GIT PULL] Compiler Attributes for v4.20-rc1

[Steven Rostedt]

On Mon, 5 Nov 2018 14:15:35 +0100
Martin Schwidefsky <schwidefsky@de.ibm.com> wrote:
>  
> Follow-up question: the __no_sanitize_address_or_inline define has the 'notrace'
> option that is missing for __no_kasan_or_inline. We need that option for
> __load_psw_mask, if we do the replacement all users of __no_kasan_or_inline
> would get the option as well. This affects __read_once_size_nocheck and
> read_word_at_a_time. Do these function have to be traceable ?

Some history on why I added notrace to inline. It was to make things
more consistent. Since gcc wont add a fentry/mcount call to inlined
functions, having those functions show up in the trace depending on
whether or not gcc honored the "inline" tag made things confusing. And
it even once caused a crash when local_irq_save() stopped being
inlined.

I added notrace to inline so that if you mark something as inline, it
would not show up in the trace regardless of gcc deciding to inline the
function or not.

> 
> This patch would work for me:
> --
> >From 4aaa09fe4b54e930edabac86606dee979b12647c Mon Sep 17 00:00:00 2001  
> From: Martin Schwidefsky <schwidefsky@de.ibm.com>
> Date: Mon, 5 Nov 2018 07:36:28 +0100
> Subject: [PATCH] compiler: remove __no_sanitize_address_or_inline again
> 
> The __no_sanitize_address_or_inline and __no_kasan_or_inline defines
> are almost identical. The only difference is that __no_kasan_or_inline
> does not have the 'notrace' attribute.
> 
> To be able to replace __no_sanitize_address_or_inline with the older
> definition, add 'notrace' to __no_kasan_or_inline and change to two
> users of __no_sanitize_address_or_inline in the s390 code.
> 
> The 'notrace' option is necessary for e.g. the __load_psw_mask function
> in arch/s390/include/asm/processor.h. Without the option it is possible
> to trace __load_psw_mask which leads to kernel stack overflow.

Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

-- Steve

> 
> Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com>
> ---
>  arch/s390/include/asm/processor.h |  4 ++--
>  include/linux/compiler-gcc.h      | 12 ------------
>  include/linux/compiler.h          |  2 +-
>  3 files changed, 3 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/s390/include/asm/processor.h b/arch/s390/include/asm/processor.h
> index 302795c47c06..81038ab357ce 100644
> --- a/arch/s390/include/asm/processor.h
> +++ b/arch/s390/include/asm/processor.h
> @@ -236,7 +236,7 @@ static inline unsigned long current_stack_pointer(void)
>  	return sp;
>  }
>  
> -static __no_sanitize_address_or_inline unsigned short stap(void)
> +static __no_kasan_or_inline unsigned short stap(void)
>  {
>  	unsigned short cpu_address;
>  
> @@ -330,7 +330,7 @@ static inline void __load_psw(psw_t psw)
>   * Set PSW mask to specified value, while leaving the
>   * PSW addr pointing to the next instruction.
>   */
> -static __no_sanitize_address_or_inline void __load_psw_mask(unsigned long mask)
> +static __no_kasan_or_inline void __load_psw_mask(unsigned long mask)
>  {
>  	unsigned long addr;
>  	psw_t psw;
> diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
> index c0f5db3a9621..2010493e1040 100644
> --- a/include/linux/compiler-gcc.h
> +++ b/include/linux/compiler-gcc.h
> @@ -143,18 +143,6 @@
>  #define KASAN_ABI_VERSION 3
>  #endif
>  
> -/*
> - * Because __no_sanitize_address conflicts with inlining:
> - *   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368
> - * we do one or the other. 
> - */
> -#ifdef CONFIG_KASAN
> -#define __no_sanitize_address_or_inline					\
> -	__no_sanitize_address __maybe_unused notrace
> -#else
> -#define __no_sanitize_address_or_inline inline
> -#endif
> -
>  #if GCC_VERSION >= 50100
>  #define COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW 1
>  #endif
> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index 18c80cfa4fc4..06396c1cf127 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -189,7 +189,7 @@ void __read_once_size(const volatile void *p, void *res, int size)
>   * 	https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368
>   * '__maybe_unused' allows us to avoid defined-but-not-used warnings.
>   */
> -# define __no_kasan_or_inline __no_sanitize_address __maybe_unused
> +# define __no_kasan_or_inline __no_sanitize_address notrace __maybe_unused
>  #else
>  # define __no_kasan_or_inline __always_inline
>  #endif

Re: [GIT PULL] Compiler Attributes for v4.20-rc1

[Linus Torvalds]

On Mon, Nov 5, 2018 at 5:15 AM Martin Schwidefsky
<schwidefsky@de.ibm.com> wrote:
>
> This patch would work for me:

Thanks, applied,

                 Linus