Messages in this thread | | | From | Huacai Chen <> | Date | Fri, 14 Jul 2023 14:09:07 +0800 | Subject | Re: [PATCH] kasan: Fix tests by removing -ffreestanding |
| |
Hi, Marco,
On Thu, Jul 13, 2023 at 6:09 PM Marco Elver <elver@google.com> wrote: > > On Thu, 13 Jul 2023 at 11:58, Huacai Chen <chenhuacai@kernel.org> wrote: > > > > Hi, Marco, > > > > On Thu, Jul 13, 2023 at 4:12 PM Marco Elver <elver@google.com> wrote: > > > > > > On Thu, 13 Jul 2023 at 06:33, Huacai Chen <chenhuacai@kernel.org> wrote: > > > > > > > > Hi, Andrey, > > > > > > > > On Thu, Jul 13, 2023 at 12:12 AM Andrey Konovalov <andreyknvl@gmail.com> wrote: > > > > > On Wed, Jul 12, 2023 at 12:14 PM Huacai Chen <chenhuacai@loongson.cn> 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@google.com > > > > > > -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.
Huacai
| |