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.
Signed-off-by: Huacai Chen <[email protected]>
---
mm/kasan/Makefile | 2 ++
1 file changed, 2 insertions(+)
diff --git a/mm/kasan/Makefile b/mm/kasan/Makefile
index 7634dd2a6128..edd1977a6b88 100644
--- a/mm/kasan/Makefile
+++ b/mm/kasan/Makefile
@@ -45,7 +45,9 @@ CFLAGS_KASAN_TEST += -fno-builtin
endif
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
obj-y := common.o report.o
obj-$(CONFIG_KASAN_GENERIC) += init.o generic.o report_generic.o shadow.o quarantine.o
--
2.39.3
On Wed, Jul 12, 2023 at 12:14 PM Huacai Chen <[email protected]> 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?
>
> Signed-off-by: Huacai Chen <[email protected]>
> ---
> mm/kasan/Makefile | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/mm/kasan/Makefile b/mm/kasan/Makefile
> index 7634dd2a6128..edd1977a6b88 100644
> --- a/mm/kasan/Makefile
> +++ b/mm/kasan/Makefile
> @@ -45,7 +45,9 @@ CFLAGS_KASAN_TEST += -fno-builtin
> endif
>
> 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
>
> obj-y := common.o report.o
> obj-$(CONFIG_KASAN_GENERIC) += init.o generic.o report_generic.o shadow.o quarantine.o
> --
> 2.39.3
+Marco
Hi, Andrey,
On Thu, Jul 13, 2023 at 12:12 AM Andrey Konovalov <[email protected]> wrote:
>
> On Wed, Jul 12, 2023 at 12:14 PM Huacai Chen <[email protected]> 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.
[1] https://github.com/chenhuacai/linux/commit/af2da91541a8899b502bece9b1fde225b71f37a8
Huacai
>
> >
> > Signed-off-by: Huacai Chen <[email protected]>
> > ---
> > mm/kasan/Makefile | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/mm/kasan/Makefile b/mm/kasan/Makefile
> > index 7634dd2a6128..edd1977a6b88 100644
> > --- a/mm/kasan/Makefile
> > +++ b/mm/kasan/Makefile
> > @@ -45,7 +45,9 @@ CFLAGS_KASAN_TEST += -fno-builtin
> > endif
> >
> > 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
> >
> > obj-y := common.o report.o
> > obj-$(CONFIG_KASAN_GENERIC) += init.o generic.o report_generic.o shadow.o quarantine.o
> > --
> > 2.39.3
>
> +Marco
On Thu, 13 Jul 2023 at 06:33, Huacai Chen <[email protected]> wrote:
>
> Hi, Andrey,
>
> On Thu, Jul 13, 2023 at 12:12 AM Andrey Konovalov <[email protected]> wrote:
> > On Wed, Jul 12, 2023 at 12:14 PM Huacai Chen <[email protected]> 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/[email protected]
-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.
Hi, Marco,
On Thu, Jul 13, 2023 at 4:12 PM Marco Elver <[email protected]> wrote:
>
> On Thu, 13 Jul 2023 at 06:33, Huacai Chen <[email protected]> wrote:
> >
> > Hi, Andrey,
> >
> > On Thu, Jul 13, 2023 at 12:12 AM Andrey Konovalov <[email protected]> wrote:
> > > On Wed, Jul 12, 2023 at 12:14 PM Huacai Chen <[email protected]> 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/[email protected]
>
> -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.
Huacai
On Thu, 13 Jul 2023 at 11:58, Huacai Chen <[email protected]> wrote:
>
> Hi, Marco,
>
> On Thu, Jul 13, 2023 at 4:12 PM Marco Elver <[email protected]> wrote:
> >
> > On Thu, 13 Jul 2023 at 06:33, Huacai Chen <[email protected]> wrote:
> > >
> > > Hi, Andrey,
> > >
> > > On Thu, Jul 13, 2023 at 12:12 AM Andrey Konovalov <[email protected]> wrote:
> > > > On Wed, Jul 12, 2023 at 12:14 PM Huacai Chen <[email protected]> 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/[email protected]
> >
> > -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.
Hi, Marco,
On Thu, Jul 13, 2023 at 6:09 PM Marco Elver <[email protected]> wrote:
>
> On Thu, 13 Jul 2023 at 11:58, Huacai Chen <[email protected]> wrote:
> >
> > Hi, Marco,
> >
> > On Thu, Jul 13, 2023 at 4:12 PM Marco Elver <[email protected]> wrote:
> > >
> > > On Thu, 13 Jul 2023 at 06:33, Huacai Chen <[email protected]> wrote:
> > > >
> > > > Hi, Andrey,
> > > >
> > > > On Thu, Jul 13, 2023 at 12:12 AM Andrey Konovalov <[email protected]> wrote:
> > > > > On Wed, Jul 12, 2023 at 12:14 PM Huacai Chen <[email protected]> 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/[email protected]
> > >
> > > -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
Hi Huacai,
On Fri, Jul 14, 2023 at 8:23 AM Huacai Chen <[email protected]> wrote:
> On Thu, Jul 13, 2023 at 6:09 PM Marco Elver <[email protected]> wrote:
> > On Thu, 13 Jul 2023 at 11:58, Huacai Chen <[email protected]> wrote:
> > > On Thu, Jul 13, 2023 at 4:12 PM Marco Elver <[email protected]> wrote:
> > > > On Thu, 13 Jul 2023 at 06:33, Huacai Chen <[email protected]> wrote:
> > > > > On Thu, Jul 13, 2023 at 12:12 AM Andrey Konovalov <[email protected]> wrote:
> > > > > > On Wed, Jul 12, 2023 at 12:14 PM Huacai Chen <[email protected]> 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/[email protected]
> > > >
> > > > -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 -- [email protected]
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
Hi, Geert,
On Fri, Jul 14, 2023 at 9:44 PM Geert Uytterhoeven <[email protected]> wrote:
>
> Hi Huacai,
>
> On Fri, Jul 14, 2023 at 8:23 AM Huacai Chen <[email protected]> wrote:
> > On Thu, Jul 13, 2023 at 6:09 PM Marco Elver <[email protected]> wrote:
> > > On Thu, 13 Jul 2023 at 11:58, Huacai Chen <[email protected]> wrote:
> > > > On Thu, Jul 13, 2023 at 4:12 PM Marco Elver <[email protected]> wrote:
> > > > > On Thu, 13 Jul 2023 at 06:33, Huacai Chen <[email protected]> wrote:
> > > > > > On Thu, Jul 13, 2023 at 12:12 AM Andrey Konovalov <[email protected]> wrote:
> > > > > > > On Wed, Jul 12, 2023 at 12:14 PM Huacai Chen <[email protected]> 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/[email protected]
> > > > >
> > > > > -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
That's another story. What we are discussing in this thread is "global
-ffreestanding" which makes KASAN on mem*() globally uninstrumentable
(unexpected). On the other hand, what you mentioned here only makes
some specific files uninstrumentable, and this is an expected
behavior.
Huacai
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
>
> 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