[PATCH] kasan: remove hwasan-kernel-mem-intrinsic-prefix=1 for clang-14

[PATCH] kasan: remove hwasan-kernel-mem-intrinsic-prefix=1 for clang-14

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

Unknown -mllvm options don't cause an error to be returned by clang, so
the cc-option helper adds the unknown hwasan-kernel-mem-intrinsic-prefix=1
flag to CFLAGS with compilers that are new enough for hwasan but too
old for this option. This causes a rather unreadable build failure:

fixdep: error opening file: scripts/mod/.empty.o.d: No such file or directory
make[4]: *** [/home/arnd/arm-soc/scripts/Makefile.build:252: scripts/mod/empty.o] Error 2
fixdep: error opening file: scripts/mod/.devicetable-offsets.s.d: No such file or directory
make[4]: *** [/home/arnd/arm-soc/scripts/Makefile.build:114: scripts/mod/devicetable-offsets.s] Error 2

Add a version check to only allow this option with clang-15, gcc-13
or later versions.

Fixes: 51287dcb00cc ("kasan: emit different calls for instrumentable memintrinsics")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
There is probably a better way to do this than to add version checks,
but I could not figure it out.
---
 scripts/Makefile.kasan | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/scripts/Makefile.kasan b/scripts/Makefile.kasan
index c186110ffa20..2cea0592e343 100644
--- a/scripts/Makefile.kasan
+++ b/scripts/Makefile.kasan
@@ -69,7 +69,12 @@ CFLAGS_KASAN := -fsanitize=kernel-hwaddress \
 		$(instrumentation_flags)
 
 # Instrument memcpy/memset/memmove calls by using instrumented __hwasan_mem*().
+ifeq ($(call clang-min-version, 150000),y)
 CFLAGS_KASAN += $(call cc-param,hwasan-kernel-mem-intrinsic-prefix=1)
+endif
+ifeq ($(call gcc-min-version, 130000),y)
+CFLAGS_KASAN += $(call cc-param,hwasan-kernel-mem-intrinsic-prefix=1)
+endif
 
 endif # CONFIG_KASAN_SW_TAGS
 
-- 
2.39.2

Re: [PATCH] kasan: remove hwasan-kernel-mem-intrinsic-prefix=1 for clang-14

[Nathan Chancellor]

On Fri, Apr 14, 2023 at 10:29:27AM +0200, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> Unknown -mllvm options don't cause an error to be returned by clang, so
> the cc-option helper adds the unknown hwasan-kernel-mem-intrinsic-prefix=1
> flag to CFLAGS with compilers that are new enough for hwasan but too

Hmmm, how did a change like commit 0e1aa5b62160 ("kcsan: Restrict
supported compilers") work if cc-option does not work with unknown
'-mllvm' flags (or did it)? That definitely seems like a problem, as I
see a few different places where '-mllvm' options are used with
cc-option. I guess I will leave that up to the sanitizer folks to
comment on that further, one small comment below.

> old for this option. This causes a rather unreadable build failure:
> 
> fixdep: error opening file: scripts/mod/.empty.o.d: No such file or directory
> make[4]: *** [/home/arnd/arm-soc/scripts/Makefile.build:252: scripts/mod/empty.o] Error 2
> fixdep: error opening file: scripts/mod/.devicetable-offsets.s.d: No such file or directory
> make[4]: *** [/home/arnd/arm-soc/scripts/Makefile.build:114: scripts/mod/devicetable-offsets.s] Error 2
> 
> Add a version check to only allow this option with clang-15, gcc-13
> or later versions.
> 
> Fixes: 51287dcb00cc ("kasan: emit different calls for instrumentable memintrinsics")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> There is probably a better way to do this than to add version checks,
> but I could not figure it out.
> ---
>  scripts/Makefile.kasan | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/scripts/Makefile.kasan b/scripts/Makefile.kasan
> index c186110ffa20..2cea0592e343 100644
> --- a/scripts/Makefile.kasan
> +++ b/scripts/Makefile.kasan
> @@ -69,7 +69,12 @@ CFLAGS_KASAN := -fsanitize=kernel-hwaddress \
>  		$(instrumentation_flags)
>  
>  # Instrument memcpy/memset/memmove calls by using instrumented __hwasan_mem*().
> +ifeq ($(call clang-min-version, 150000),y)
>  CFLAGS_KASAN += $(call cc-param,hwasan-kernel-mem-intrinsic-prefix=1)
> +endif
> +ifeq ($(call gcc-min-version, 130000),y)
> +CFLAGS_KASAN += $(call cc-param,hwasan-kernel-mem-intrinsic-prefix=1)
> +endif

I do not think you need to duplicate this block, I think

  ifeq ($(call clang-min-version, 150000)$(call gcc-min-version, 130000),y)
  CFLAGS_KASAN += $(call cc-param,hwasan-kernel-mem-intrinsic-prefix=1)
  endif

would work, as only one of those conditions can be true at a time.

Cheers,
Nathan

Re: [PATCH] kasan: remove hwasan-kernel-mem-intrinsic-prefix=1 for clang-14

[Arnd Bergmann]

On Fri, Apr 14, 2023, at 18:26, Nathan Chancellor wrote:
> On Fri, Apr 14, 2023 at 10:29:27AM +0200, Arnd Bergmann wrote:
>> From: Arnd Bergmann <arnd@arndb.de>
>> 
>> Unknown -mllvm options don't cause an error to be returned by clang, so
>> the cc-option helper adds the unknown hwasan-kernel-mem-intrinsic-prefix=1
>> flag to CFLAGS with compilers that are new enough for hwasan but too
>
> Hmmm, how did a change like commit 0e1aa5b62160 ("kcsan: Restrict
> supported compilers") work if cc-option does not work with unknown
> '-mllvm' flags (or did it)? That definitely seems like a problem, as I
> see a few different places where '-mllvm' options are used with
> cc-option. I guess I will leave that up to the sanitizer folks to
> comment on that further, one small comment below.

That one adds both "-fsanitize=thread" and "-mllvm
-tsan-distinguish-volatile=1". If the first one is missing in the
compiler, neither will be set. If only the second one fails, I assume
you'd get the same result I see with hwasan-kernel-mem-intrinsic-prefix=1.

>>  # Instrument memcpy/memset/memmove calls by using instrumented __hwasan_mem*().
>> +ifeq ($(call clang-min-version, 150000),y)
>>  CFLAGS_KASAN += $(call cc-param,hwasan-kernel-mem-intrinsic-prefix=1)
>> +endif
>> +ifeq ($(call gcc-min-version, 130000),y)
>> +CFLAGS_KASAN += $(call cc-param,hwasan-kernel-mem-intrinsic-prefix=1)
>> +endif
>
> I do not think you need to duplicate this block, I think
>
>   ifeq ($(call clang-min-version, 150000)$(call gcc-min-version, 130000),y)
>   CFLAGS_KASAN += $(call cc-param,hwasan-kernel-mem-intrinsic-prefix=1)
>   endif
>
> would work, as only one of those conditions can be true at a time.

Are you sure that clang-min-version evaluates to an empty string
rather than "n" or something else? I haven't found a documentation
that says anything about it other than it returning "y" if the condition
is true.

      Arnd

Re: [PATCH] kasan: remove hwasan-kernel-mem-intrinsic-prefix=1 for clang-14

[Nathan Chancellor]

On Fri, Apr 14, 2023 at 08:53:49PM +0200, Arnd Bergmann wrote:
> On Fri, Apr 14, 2023, at 18:26, Nathan Chancellor wrote:
> > On Fri, Apr 14, 2023 at 10:29:27AM +0200, Arnd Bergmann wrote:
> >> From: Arnd Bergmann <arnd@arndb.de>
> >> 
> >> Unknown -mllvm options don't cause an error to be returned by clang, so
> >> the cc-option helper adds the unknown hwasan-kernel-mem-intrinsic-prefix=1
> >> flag to CFLAGS with compilers that are new enough for hwasan but too
> >
> > Hmmm, how did a change like commit 0e1aa5b62160 ("kcsan: Restrict
> > supported compilers") work if cc-option does not work with unknown
> > '-mllvm' flags (or did it)? That definitely seems like a problem, as I
> > see a few different places where '-mllvm' options are used with
> > cc-option. I guess I will leave that up to the sanitizer folks to
> > comment on that further, one small comment below.
> 
> That one adds both "-fsanitize=thread" and "-mllvm
> -tsan-distinguish-volatile=1". If the first one is missing in the
> compiler, neither will be set. If only the second one fails, I assume
> you'd get the same result I see with hwasan-kernel-mem-intrinsic-prefix=1.

I did not look close enough but it turns out that this check is always
true for the versions of clang that the kernel currently supports, so it
could not fail even if '-mllvm' flag checking worked.

$ git grep tsan-distinguish-volatile llvmorg-11.0.0
llvmorg-11.0.0:llvm/lib/Transforms/Instrumentation/ThreadSanitizer.cpp:    "tsan-distinguish-volatile", cl::init(false),
llvmorg-11.0.0:llvm/test/Instrumentation/ThreadSanitizer/volatile.ll:; RUN: opt < %s -tsan -tsan-distinguish-volatile -S | FileCheck %s

At the time of the Linux change though, we did not have a minimum
supported version, so that check was necessary. I wonder if LLVM
regressed with regards to '-mllvm' flag checking at some point...

> >>  # Instrument memcpy/memset/memmove calls by using instrumented __hwasan_mem*().
> >> +ifeq ($(call clang-min-version, 150000),y)
> >>  CFLAGS_KASAN += $(call cc-param,hwasan-kernel-mem-intrinsic-prefix=1)
> >> +endif
> >> +ifeq ($(call gcc-min-version, 130000),y)
> >> +CFLAGS_KASAN += $(call cc-param,hwasan-kernel-mem-intrinsic-prefix=1)
> >> +endif
> >
> > I do not think you need to duplicate this block, I think
> >
> >   ifeq ($(call clang-min-version, 150000)$(call gcc-min-version, 130000),y)
> >   CFLAGS_KASAN += $(call cc-param,hwasan-kernel-mem-intrinsic-prefix=1)
> >   endif
> >
> > would work, as only one of those conditions can be true at a time.
> 
> Are you sure that clang-min-version evaluates to an empty string
> rather than "n" or something else? I haven't found a documentation
> that says anything about it other than it returning "y" if the condition
> is true.

Yes, see the test-ge and test-gt macros in scripts/Kbuild.include, they
will only ever print the empty string or 'y'.

Cheers,
Nathan

Re: [PATCH] kasan: remove hwasan-kernel-mem-intrinsic-prefix=1 for clang-14

[Marco Elver]

On Fri, 14 Apr 2023 at 18:26, Nathan Chancellor <nathan@kernel.org> wrote:
>
> On Fri, Apr 14, 2023 at 10:29:27AM +0200, Arnd Bergmann wrote:
> > From: Arnd Bergmann <arnd@arndb.de>
> >
> > Unknown -mllvm options don't cause an error to be returned by clang, so
> > the cc-option helper adds the unknown hwasan-kernel-mem-intrinsic-prefix=1
> > flag to CFLAGS with compilers that are new enough for hwasan but too
>
> Hmmm, how did a change like commit 0e1aa5b62160 ("kcsan: Restrict
> supported compilers") work if cc-option does not work with unknown
> '-mllvm' flags (or did it)? That definitely seems like a problem, as I
> see a few different places where '-mllvm' options are used with
> cc-option. I guess I will leave that up to the sanitizer folks to
> comment on that further, one small comment below.

Urgh, this one turns out to be rather ridiculous. It's only a problem
with hwasan...

If you try it for yourself, e.g. with something "normal" like:

> clang -Werror -mllvm -asan-does-not-exist -c -x c /dev/null -o /dev/null

It errors as expected. But with:

> clang -Werror -mllvm -hwasan-does-not-exist -c -x c /dev/null -o /dev/null

It ends up printing _help_ text, because anything "-h..." (if it
doesn't recognize it as a long-form argument), will make it produce
the help text.

> > old for this option. This causes a rather unreadable build failure:
> >
> > fixdep: error opening file: scripts/mod/.empty.o.d: No such file or directory
> > make[4]: *** [/home/arnd/arm-soc/scripts/Makefile.build:252: scripts/mod/empty.o] Error 2
> > fixdep: error opening file: scripts/mod/.devicetable-offsets.s.d: No such file or directory
> > make[4]: *** [/home/arnd/arm-soc/scripts/Makefile.build:114: scripts/mod/devicetable-offsets.s] Error 2
> >
> > Add a version check to only allow this option with clang-15, gcc-13
> > or later versions.
> >
> > Fixes: 51287dcb00cc ("kasan: emit different calls for instrumentable memintrinsics")
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > ---
> > There is probably a better way to do this than to add version checks,
> > but I could not figure it out.
> > ---
> >  scripts/Makefile.kasan | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/scripts/Makefile.kasan b/scripts/Makefile.kasan
> > index c186110ffa20..2cea0592e343 100644
> > --- a/scripts/Makefile.kasan
> > +++ b/scripts/Makefile.kasan
> > @@ -69,7 +69,12 @@ CFLAGS_KASAN := -fsanitize=kernel-hwaddress \
> >               $(instrumentation_flags)
> >
> >  # Instrument memcpy/memset/memmove calls by using instrumented __hwasan_mem*().
> > +ifeq ($(call clang-min-version, 150000),y)
> >  CFLAGS_KASAN += $(call cc-param,hwasan-kernel-mem-intrinsic-prefix=1)
> > +endif
> > +ifeq ($(call gcc-min-version, 130000),y)
> > +CFLAGS_KASAN += $(call cc-param,hwasan-kernel-mem-intrinsic-prefix=1)
> > +endif
>
> I do not think you need to duplicate this block, I think
>
>   ifeq ($(call clang-min-version, 150000)$(call gcc-min-version, 130000),y)
>   CFLAGS_KASAN += $(call cc-param,hwasan-kernel-mem-intrinsic-prefix=1)
>   endif

We just need the clang version check. If the compiler is gcc, it'll do
the "right thing" (i.e. not print help text). So at a minimum, we need
if "clang version >= 15 or gcc". Checking if gcc is 13 or later
doesn't hurt though, so I don't mind either way.

So on a whole this patch is the right thing to do because fixing old
clang versions to not interpret unrecognized options that start with
"-h.." as help isn't something we can realistically do.

Thanks,
-- Marco

Re: [PATCH] kasan: remove hwasan-kernel-mem-intrinsic-prefix=1 for clang-14

[Arnd Bergmann]

On Tue, Apr 18, 2023, at 14:06, Marco Elver wrote:
> On Fri, 14 Apr 2023 at 18:26, Nathan Chancellor <nathan@kernel.org> wrote:
>> On Fri, Apr 14, 2023 at 10:29:27AM +0200, Arnd Bergmann wrote:
> It errors as expected. But with:
>
>> clang -Werror -mllvm -hwasan-does-not-exist -c -x c /dev/null -o /dev/null
>
> It ends up printing _help_ text, because anything "-h..." (if it
> doesn't recognize it as a long-form argument), will make it produce
> the help text.

Ah, that explains a lot. I think I actually tried a few other options, but
probably only edited part of the option name, and not the beginning, so
I always saw the help text.

>> >  # Instrument memcpy/memset/memmove calls by using instrumented __hwasan_mem*().
>> > +ifeq ($(call clang-min-version, 150000),y)
>> >  CFLAGS_KASAN += $(call cc-param,hwasan-kernel-mem-intrinsic-prefix=1)
>> > +endif
>> > +ifeq ($(call gcc-min-version, 130000),y)
>> > +CFLAGS_KASAN += $(call cc-param,hwasan-kernel-mem-intrinsic-prefix=1)
>> > +endif
>>
>> I do not think you need to duplicate this block, I think
>>
>>   ifeq ($(call clang-min-version, 150000)$(call gcc-min-version, 130000),y)
>>   CFLAGS_KASAN += $(call cc-param,hwasan-kernel-mem-intrinsic-prefix=1)
>>   endif
>
> We just need the clang version check. If the compiler is gcc, it'll do
> the "right thing" (i.e. not print help text). So at a minimum, we need
> if "clang version >= 15 or gcc". Checking if gcc is 13 or later
> doesn't hurt though, so I don't mind either way.

I've sent a v2 now, with an updated help text and the simplified
version check.

It might be possible to change the cc-option check in a way that
parses the output, this variant should do that, if we care:

echo "char *str = \"check that assembler works\";" | clang -Werror -mllvm -hwasan-does-not-exist  -S -x c - -o - | grep -q "check that assembler works"

      Arnd