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@xxxxxxxx>
> >> 
> >> 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