Re: [PATCH] kasan: Fix tests by removing -ffreestanding

[Geert Uytterhoeven]

Hi Huacai,

On Fri, Jul 14, 2023 at 8:23 AM Huacai Chen <chenhuacai@xxxxxxxxxx> wrote:
> On Thu, Jul 13, 2023 at 6:09 PM Marco Elver <elver@xxxxxxxxxx> wrote:
> > On Thu, 13 Jul 2023 at 11:58, Huacai Chen <chenhuacai@xxxxxxxxxx> wrote:
> > > On Thu, Jul 13, 2023 at 4:12 PM Marco Elver <elver@xxxxxxxxxx> wrote:
> > > > On Thu, 13 Jul 2023 at 06:33, Huacai Chen <chenhuacai@xxxxxxxxxx> wrote:
> > > > > On Thu, Jul 13, 2023 at 12:12 AM Andrey Konovalov <andreyknvl@xxxxxxxxx> wrote:
> > > > > > On Wed, Jul 12, 2023 at 12:14 PM Huacai Chen <chenhuacai@xxxxxxxxxxx> wrote:
> > > > > > >
> > > > > > > CONFIG_CC_HAS_KASAN_MEMINTRINSIC_PREFIX hopes -fbuiltin for memset()/
> > > > > > > memcpy()/memmove() if instrumentation is needed. This is the default
> > > > > > > behavior but some archs pass -ffreestanding which implies -fno-builtin,
> > > > > > > and then causes some kasan tests fail. So we remove -ffreestanding for
> > > > > > > kasan tests.
> > > > > >
> > > > > > Could you clarify on which architecture you observed tests failures?
> > > > > Observed on LoongArch [1], KASAN for LoongArch was planned to be
> > > > > merged in 6.5, but at the last minute I found some tests fail with
> > > > > GCC14 (CONFIG_CC_HAS_KASAN_MEMINTRINSIC_PREFIX) so the patches are
> > > > > dropped. After some debugging we found the root cause is
> > > > > -ffreestanding.
> > > > [...]
> > > > > > >  CFLAGS_kasan_test.o := $(CFLAGS_KASAN_TEST)
> > > > > > > +CFLAGS_REMOVE_kasan_test.o := -ffreestanding
> > > > > > >  CFLAGS_kasan_test_module.o := $(CFLAGS_KASAN_TEST)
> > > > > > > +CFLAGS_REMOVE_kasan_test_module.o := -ffreestanding
> > > >
> > > > It makes sense that if -ffreestanding is added everywhere, that this
> > > > patch fixes the test. Also see:
> > > > https://lkml.kernel.org/r/20230224085942.1791837-3-elver@xxxxxxxxxx
> > > >
> > > > -ffreestanding implies -fno-builtin, which used to be added to the
> > > > test where !CC_HAS_KASAN_MEMINTRINSIC_PREFIX (old compilers).
> > > >
> > > > But ideally, the test doesn't have any special flags to make it pass,
> > > > because ultimately we want the test setup to be as close to other
> > > > normal kernel code.
> > > >
> > > > What this means for LoongArch, is that the test legitimately is
> > > > pointing out an issue: namely that with newer compilers, your current
> > > > KASAN support for LoongArch is failing to detect bad accesses within
> > > > mem*() functions.
> > > >
> > > > The reason newer compilers should emit __asan_mem*() functions and
> > > > replace normal mem*() functions, is that making mem*() functions
> > > > always instrumented is not safe when e.g. called from uninstrumented
> > > > code. One problem is that compilers will happily generate
> > > > memcpy/memset calls themselves for e.g. variable initialization or
> > > > struct copies - and unfortunately -ffreestanding does _not_ prohibit
> > > > compilers from doing so: https://godbolt.org/z/hxGvdo4P9
> > > >
> > > > I would propose 2 options:
> > > >
> > > > 1. Removing -ffreestanding from LoongArch. It is unclear to me why
> > > > this is required. As said above, -ffreestanding does not actually
> > > > prohibit the compiler from generating implicit memset/memcpy. It
> > > > prohibits some other optimizations, but in the kernel, you might even
> > > > want those optimizations if common libcalls are already implemented
> > > > (which they should be?).
> > > >
> > > > 2. If KASAN is enabled on LoongArch, make memset/memcpy/memmove
> > > > aliases to __asan_memset/__asan_memcpy/__asan_memmove. That means
> > > > you'd have to invert how you currently set up __mem and mem functions:
> > > > the implementation is in __mem*, and mem* functions alias __mem* -or-
> > > > if KASAN is enabled __asan_mem* functions (ifdef
> > > > CONFIG_CC_HAS_KASAN_MEMINTRINSIC_PREFIX to make old compilers work as
> > > > well).
> > > >
> > > > If you go with option #2 you are accepting the risk of using
> > > > instrumented mem* functions from uninstrumented files/functions. This
> > > > has been an issue for other architectures. In many cases you might get
> > > > lucky enough that it doesn't cause issues, but that's not guaranteed.
> > > Thank you for your advice, but we should keep -ffreestanding for
> > > LoongArch, even if it may cause failing to detect bad accesses.
> > > Because now the __builtin_memset() assumes hardware supports unaligned
> > > access, which is not the case for Loongson-2K series. If removing
> > > -ffreestanding, Loongson-2K gets a poor performance.
> > >
> > > On the other hand, LoongArch is not the only architecture use
> > > -ffreestanding, e.g., MIPS, X86_32, M68K and Xtensa also use, so the
> > > tests should get fixed.
> >
> > That's fair - in which case, I would recommend option #2 or some
> > variant of it. Because fixing the test by removing -ffreestanding is
> > just hiding that there's a real issue that needs to be fixed to have
> > properly working KASAN on LoongArch.
>
> After some thinking, I found we can remove -ffreestanding in the arch
> Makefile when KASAN is enabled -- because it is not the performance
> critical configuration. And then, this patch can be dropped, thank
> you.

Doesn't this introduce an unwanted impact?

And it's not just arch makefiles:

crypto/Makefile:CFLAGS_aegis128-neon-inner.o += -ffreestanding
-march=armv8-a -mfloat-abi=softfp
crypto/Makefile:aegis128-cflags-y := -ffreestanding -mcpu=generic+crypto
lib/Makefile:CFLAGS_string.o := -ffreestanding
lib/raid6/Makefile:NEON_FLAGS := -ffreestanding

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds