| Message ID | 20230414082943.1341757-1-arnd@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kasan: remove hwasan-kernel-mem-intrinsic-prefix=1 for clang-14 | expand |
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
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
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
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
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
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