Message ID | 20230712101344.2714626-1-chenhuacai@loongson.cn |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:a6b2:0:b0:3e4:2afc:c1 with SMTP id c18csp1050969vqm; Wed, 12 Jul 2023 03:33:32 -0700 (PDT) X-Google-Smtp-Source: APBJJlEJvXruoEigKGdD1cKYhWuOK+gZVRB6cdU38kTRtOAEZO5hXSoTCYqv5eD09DNiCWeuiLTv X-Received: by 2002:a05:6a20:7fa3:b0:12c:ed6f:c114 with SMTP id d35-20020a056a207fa300b0012ced6fc114mr22853895pzj.50.1689158011683; Wed, 12 Jul 2023 03:33:31 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1689158011; cv=none; d=google.com; s=arc-20160816; b=JBq7hM3E4mmjpRfVYiSSlMW73c0x9+4IkxxzGz84b4k247Kjb7bGG3hVaVjedVOuI8 BHqgIKXtI5yF1ab69AzIU9myLk52YK5o2p4h2LSvrzj18l4eNjPJHAOWOqPuWtuBTiiZ VkEvf6Dq/+XP0a2gSBkqU5eKKp8EPCyd0soJf3kAt9hGKc5AunSyZKqRuY6X/+Rx0QRk +yDVFw0Cs1fycdjPf7FDZz4EPcXd9m/HihRzaE1jv+GmD4JrjW6z+INfbUs8uqga2Pvz MiNOd5TEY+gnthb9wrH6tsFxN8Q9iYRTaEYOktEjCNswe9Xml/ErQNzRQbVclnyZaVQQ 4avw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from; bh=sCIaPYCECVd0cE+mg+HtMqz+ndFV6us858GDoMecbgo=; fh=Yj/bD7TgMfhKFl6g+uOJDdzvXjDmfV75Wg5uGMySJLA=; b=bQPOh6lexoaaCpkZkW+09SYlP4fE5zSuYkDNVGV2gOPnoX6XH8+8jkIZ3vHt1/rtbX 8/bOxDBRo+JfvZWQXGo4C4lnzy1Xh5h8Zk2uvR3pqRh++s7q4FjZVwiKX4mzFRuaNpvN T1CtXEDH7yQAB0HlpdcR62y0ytfWM7zCKohnqTjmy7OpLVlTNvmTan8TmJ9ROaDid+yg fcDfJa8YPrPLnnZkbDuyLrF1RVN7SBo2PR+N2QPfn6WQt1QRvF5DtQbIFyHTPp9mw9Bm BMZD2oz1jpehEe2xa56l8cTjTWrqGflDrfpaLZfbPG2R5d7EWFpskNNLG+5yRQBfDLoO 8EBg== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id u64-20020a638543000000b0055bf3d0c991si3062631pgd.5.2023.07.12.03.33.19; Wed, 12 Jul 2023 03:33:31 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232428AbjGLKOK (ORCPT <rfc822;ybw1215001957@gmail.com> + 99 others); Wed, 12 Jul 2023 06:14:10 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:41264 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230504AbjGLKOI (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 12 Jul 2023 06:14:08 -0400 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id D68D01991 for <linux-kernel@vger.kernel.org>; Wed, 12 Jul 2023 03:14:07 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 7586F61725 for <linux-kernel@vger.kernel.org>; Wed, 12 Jul 2023 10:14:07 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id A04AAC433C8; Wed, 12 Jul 2023 10:14:04 +0000 (UTC) From: Huacai Chen <chenhuacai@loongson.cn> To: Andrey Ryabinin <ryabinin.a.a@gmail.com>, Alexander Potapenko <glider@google.com> Cc: Andrey Konovalov <andreyknvl@gmail.com>, Dmitry Vyukov <dvyukov@google.com>, Vincenzo Frascino <vincenzo.frascino@arm.com>, Huacai Chen <chenhuacai@kernel.org>, kasan-dev@googlegroups.com, linux-kernel@vger.kernel.org, Huacai Chen <chenhuacai@loongson.cn> Subject: [PATCH] kasan: Fix tests by removing -ffreestanding Date: Wed, 12 Jul 2023 18:13:44 +0800 Message-Id: <20230712101344.2714626-1-chenhuacai@loongson.cn> X-Mailer: git-send-email 2.39.3 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-6.7 required=5.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,RCVD_IN_DNSWL_HI,SPF_HELO_NONE,SPF_PASS, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1771210551475977524 X-GMAIL-MSGID: 1771210551475977524 |
Series |
kasan: Fix tests by removing -ffreestanding
|
|
Commit Message
Huacai Chen
July 12, 2023, 10:13 a.m. UTC
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 <chenhuacai@loongson.cn>
---
mm/kasan/Makefile | 2 ++
1 file changed, 2 insertions(+)
Comments
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? > > Signed-off-by: Huacai Chen <chenhuacai@loongson.cn> > --- > 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 <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. [1] https://github.com/chenhuacai/linux/commit/af2da91541a8899b502bece9b1fde225b71f37a8 Huacai > > > > > Signed-off-by: Huacai Chen <chenhuacai@loongson.cn> > > --- > > 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 <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.
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. Huacai
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.
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
Hi Huacai, On Fri, Jul 14, 2023 at 8:23 AM Huacai Chen <chenhuacai@kernel.org> wrote: > 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: > > > 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: > > > > > 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. 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
Hi, Geert, On Fri, Jul 14, 2023 at 9:44 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Huacai, > > On Fri, Jul 14, 2023 at 8:23 AM Huacai Chen <chenhuacai@kernel.org> wrote: > > 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: > > > > 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: > > > > > > 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. > > 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 -- geert@linux-m68k.org > > 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
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