| Message ID | 8ffecfffe04088c52c42b92739c2bd8a0bcb3f5e.1516384594.git.andreyknvl@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hmm... I had mentioned this patch to some coworkers who have much more knowledge about LLVM than me. They had concern that LLVM needs memset to be defined, and that there were discussions on the llvm mailing list about this. I'm digging through trying to find anything relevant, maybe this one about memcpy: https://lists.llvm.org/pipermail/llvm-dev/2012-May/050108.html I wonder if -ffreestanding is more appropriate? -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
sorry, for new people I just cc'ed: https://patchwork.kernel.org/patch/10175831/ On Fri, Jan 19, 2018 at 2:06 PM, Nick Desaulniers <ndesaulniers@google.com> wrote: > Hmm... I had mentioned this patch to some coworkers who have much more > knowledge about LLVM than me. They had concern that LLVM needs memset > to be defined, and that there were discussions on the llvm mailing > list about this. > > I'm digging through trying to find anything relevant, maybe this one > about memcpy: https://lists.llvm.org/pipermail/llvm-dev/2012-May/050108.html > > I wonder if -ffreestanding is more appropriate?
On Fri, Jan 19, 2018 at 8:06 PM, Nick Desaulniers <ndesaulniers@google.com> wrote: > Hmm... I had mentioned this patch to some coworkers who have much more > knowledge about LLVM than me. They had concern that LLVM needs memset > to be defined, and that there were discussions on the llvm mailing > list about this. > > I'm digging through trying to find anything relevant, maybe this one > about memcpy: https://lists.llvm.org/pipermail/llvm-dev/2012-May/050108.html > > I wonder if -ffreestanding is more appropriate? I don't mind using either of those, they both fix the issue. I'm struggling to understand the difference though. GCC documentation doesn't really explain it [1] and Clang documentation [2] is completely useless in this case. [1] https://gcc.gnu.org/onlinedocs/gcc/Standards.html [2] https://clang.llvm.org/docs/CommandGuide/clang.html -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jan 22, 2018 at 12:22 PM, Andrey Konovalov <andreyknvl@google.com> wrote: > On Fri, Jan 19, 2018 at 8:06 PM, Nick Desaulniers > <ndesaulniers@google.com> wrote: >> Hmm... I had mentioned this patch to some coworkers who have much more >> knowledge about LLVM than me. They had concern that LLVM needs memset >> to be defined, and that there were discussions on the llvm mailing >> list about this. >> >> I'm digging through trying to find anything relevant, maybe this one >> about memcpy: https://lists.llvm.org/pipermail/llvm-dev/2012-May/050108.html >> >> I wonder if -ffreestanding is more appropriate? > > I don't mind using either of those, they both fix the issue. > > I'm struggling to understand the difference though. GCC documentation > doesn't really explain it [1] and Clang documentation [2] is > completely useless in this case. > > [1] https://gcc.gnu.org/onlinedocs/gcc/Standards.html > [2] https://clang.llvm.org/docs/CommandGuide/clang.html There's always the source, which is the ultimate source of truth. In LLVM, lib/Transforms/Utils/SimplifyLibCalls.cpp lib/Transforms/Scalar/MemCpyOptimizer.cpp include/llvm/Analysis/TargetLibraryInfo.h are the only things with mentions of `no-builtin` or `freestanding`. The driver is what parses these options: build/tools/clang/include/clang/Driver/Options.inc: 774 OPTION(prefix_1, "ffreestanding", ffreestanding, Flag, f_Group, INVALID, nullptr, CC1Option, 0, 775 "Assert that the compilation takes place in a freestanding environment", nullptr, nullptr) ... 1004 OPTION(prefix_1, "fno-builtin-", fno_builtin_, Joined, f_Group, INVALID, nullptr, CC1Option, 0, 1005 "Disable implicit builtin knowledge of a specific function", nullptr, nullptr) 1006 OPTION(prefix_1, "fno-builtin", fno_builtin, Flag, f_Group, INVALID, nullptr, CC1Option, 0, 1007 "Disable implicit builtin knowledge of functions", nullptr, nullptr) It looks like the TargetLibraryInfo#disableAllFunctions() method is called in a few different drivers. The options table generates all kinds of code, but places that parse command line flags look for `OPT_` + 3rd arg to OPTION. In Clang, looking for OPT_no_fbuiltin and OPT_ffreestanding: lib/Driver/ToolChains/Clang.cpp lib/Frontend/CompilerInvocation.cpp the second seems to set: 559 Opts.SimplifyLibCalls = !(Args.hasArg(OPT_fno_builtin) || 560 Args.hasArg(OPT_ffreestanding)); ... 2201 Opts.Freestanding = Args.hasArg(OPT_ffreestanding); which are then used back in LLVM. Opts.Freestanding seems to be used from lots of LTO handling code in LLVM. If ffreestanding implies fno-builtin, but does additional things with LTO, then we might regress LTO (there's 2 patch sets in the works that I know of) with KASAN by using ffreestanding rather than fno-builtin. But lets cross that bridge when we get there (if we decide for some reason that we need LTO+KASAN; I like both and if we start shipping LTO kernels then you can bet we'd want to test them with KASAN). So I don't think it hurts to keep v2 as is (sorry for the noise). Acked-by: Nick Desaulniers <ndesaulniers@google.com>
On 01/19/2018 08:58 PM, Andrey Konovalov wrote: > With KASAN enabled the kernel has two different memset() functions, one > with KASAN checks (memset) and one without (__memset). KASAN uses some > macro tricks to use the proper version where required. For example memset() > calls in mm/slub.c are without KASAN checks, since they operate on poisoned > slab object metadata. > > The issue is that clang emits memset() calls even when there is no memset() > in the source code. They get linked with improper memset() implementation > and the kernel fails to boot due to a huge amount of KASAN reports during > early boot stages. > So how did you observe this? Why am I not seeing this problem? > The solution is to add -fno-builtin flag for files with KASAN_SANITIZE := n > marker. > I'm not sure I understand how is this solves the problem. And what clang does instead of memset()? Does it inlines memset()? But according to GCC's man (clang's man is very vague about this) -fno-builtin should do exactly the opposite - prevent inlining builtin functions and always generate a function call. -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jan 23, 2018 at 10:24 AM, Andrey Ryabinin <aryabinin@virtuozzo.com> wrote: > > > On 01/19/2018 08:58 PM, Andrey Konovalov wrote: >> With KASAN enabled the kernel has two different memset() functions, one >> with KASAN checks (memset) and one without (__memset). KASAN uses some >> macro tricks to use the proper version where required. For example memset() >> calls in mm/slub.c are without KASAN checks, since they operate on poisoned >> slab object metadata. >> >> The issue is that clang emits memset() calls even when there is no memset() >> in the source code. They get linked with improper memset() implementation >> and the kernel fails to boot due to a huge amount of KASAN reports during >> early boot stages. >> > > So how did you observe this? Why am I not seeing this problem? I only observed this when I tried to cross-compile the kernel for arm64 with clang. I suspect this can happen on x86 as well. > >> The solution is to add -fno-builtin flag for files with KASAN_SANITIZE := n >> marker. >> > > I'm not sure I understand how is this solves the problem. And what clang does > instead of memset()? Does it inlines memset()? But according to GCC's man (clang's man is very vague about this) > -fno-builtin should do exactly the opposite - prevent inlining builtin functions > and always generate a function call. The issue is that the compiler is allowed to replace plain assignment with memset() calls. And -fno-builtin suppresses that behavior, but there are no mentions of this in the documentation that I could find. I also failed to find the exact place in the source code that leads to this, perhaps Nick can help with that. -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Makefile b/Makefile index bf5b8cbb9469..d45e31b293d0 100644 --- a/Makefile +++ b/Makefile @@ -432,7 +432,8 @@ export MAKE AWK GENKSYMS INSTALLKERNEL PERL PYTHON UTS_MACHINE export HOSTCXX HOSTCXXFLAGS LDFLAGS_MODULE CHECK CHECKFLAGS export KBUILD_CPPFLAGS NOSTDINC_FLAGS LINUXINCLUDE OBJCOPYFLAGS LDFLAGS -export KBUILD_CFLAGS CFLAGS_KERNEL CFLAGS_MODULE CFLAGS_KASAN CFLAGS_UBSAN +export KBUILD_CFLAGS CFLAGS_KERNEL CFLAGS_MODULE +export CFLAGS_KASAN CFLAGS_KASAN_NOSANITIZE CFLAGS_UBSAN export KBUILD_AFLAGS AFLAGS_KERNEL AFLAGS_MODULE export KBUILD_AFLAGS_MODULE KBUILD_CFLAGS_MODULE KBUILD_LDFLAGS_MODULE export KBUILD_AFLAGS_KERNEL KBUILD_CFLAGS_KERNEL diff --git a/scripts/Makefile.kasan b/scripts/Makefile.kasan index dbbd4382f15a..3fb382c69ff6 100644 --- a/scripts/Makefile.kasan +++ b/scripts/Makefile.kasan @@ -39,4 +39,7 @@ else endif CFLAGS_KASAN += $(call cc-option, -fsanitize-address-use-after-scope) + +CFLAGS_KASAN_NOSANITIZE := -fno-builtin + endif diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib index 1ca4dcd2d500..015aa9dbad86 100644 --- a/scripts/Makefile.lib +++ b/scripts/Makefile.lib @@ -121,7 +121,7 @@ endif ifeq ($(CONFIG_KASAN),y) _c_flags += $(if $(patsubst n%,, \ $(KASAN_SANITIZE_$(basetarget).o)$(KASAN_SANITIZE)y), \ - $(CFLAGS_KASAN)) + $(CFLAGS_KASAN), $(CFLAGS_KASAN_NOSANITIZE)) endif ifeq ($(CONFIG_UBSAN),y)
With KASAN enabled the kernel has two different memset() functions, one with KASAN checks (memset) and one without (__memset). KASAN uses some macro tricks to use the proper version where required. For example memset() calls in mm/slub.c are without KASAN checks, since they operate on poisoned slab object metadata. The issue is that clang emits memset() calls even when there is no memset() in the source code. They get linked with improper memset() implementation and the kernel fails to boot due to a huge amount of KASAN reports during early boot stages. The solution is to add -fno-builtin flag for files with KASAN_SANITIZE := n marker. Signed-off-by: Andrey Konovalov <andreyknvl@google.com> --- Changed in v2: - dropped cc-option for -fno-builtin Makefile | 3 ++- scripts/Makefile.kasan | 3 +++ scripts/Makefile.lib | 2 +- 3 files changed, 6 insertions(+), 2 deletions(-)