| Message ID | 20240506133544.2861555-2-masahiroy@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | kbuild: remove many tool coverage variables | expand |
On Mon, May 6, 2024, at 15:35, Masahiro Yamada wrote: > The objtool, sanitizers (KASAN, UBSAN, etc.), and profilers (GCOV, etc.) > are intended for kernel space objects. To exclude objects from their > coverage, you need to set variables such as OBJECT_FILES_NON_STNDARD=y, > KASAN_SANITIZE=n, etc. > > For instance, the following are not kernel objects, and therefore should > opt out of coverage: > > - vDSO > - purgatory > - bootloader (arch/*/boot/) > > Kbuild can detect these cases without relying on such variables because > objects not directly linked to vmlinux or modules are considered > "non-standard objects". > > Detecting objects linked to vmlinux or modules is straightforward: > > - objects added to obj-y are linked to vmlinux > - objects added to lib-y are linked to vmlinux > - objects added to obj-m are linked to modules > I noticed new randconfig build warnings and bisected them down to this patch: warning: unsafe memchr_inv() usage lacked '__read_overflow' symbol in lib/test_fortify/read_overflow-memchr_inv.c warning: unsafe memchr() usage lacked '__read_overflow' warning in lib/test_fortify/read_overflow-memchr.c warning: unsafe memscan() usage lacked '__read_overflow' symbol in lib/test_fortify/read_overflow-memscan.c warning: unsafe memcmp() usage lacked '__read_overflow' warning in lib/test_fortify/read_overflow-memcmp.c warning: unsafe memcpy() usage lacked '__read_overflow2' symbol in lib/test_fortify/read_overflow2-memcpy.c warning: unsafe memcmp() usage lacked '__read_overflow2' warning in lib/test_fortify/read_overflow2-memcmp.c warning: unsafe memmove() usage lacked '__read_overflow2' symbol in lib/test_fortify/read_overflow2-memmove.c warning: unsafe memcpy() usage lacked '__read_overflow2_field' symbol in lib/test_fortify/read_overflow2_field-memcpy.c warning: unsafe memmove() usage lacked '__read_overflow2_field' symbol in lib/test_fortify/read_overflow2_field-memmove.c warning: unsafe memcpy() usage lacked '__write_overflow' symbol in lib/test_fortify/write_overflow-memcpy.c warning: unsafe memmove() usage lacked '__write_overflow' symbol in lib/test_fortify/write_overflow-memmove.c warning: unsafe memset() usage lacked '__write_overflow' symbol in lib/test_fortify/write_overflow-memset.c warning: unsafe strcpy() usage lacked '__write_overflow' symbol in lib/test_fortify/write_overflow-strcpy-lit.c warning: unsafe strcpy() usage lacked '__write_overflow' symbol in lib/test_fortify/write_overflow-strcpy.c warning: unsafe strncpy() usage lacked '__write_overflow' symbol in lib/test_fortify/write_overflow-strncpy-src.c warning: unsafe strncpy() usage lacked '__write_overflow' symbol in lib/test_fortify/write_overflow-strncpy.c warning: unsafe strscpy() usage lacked '__write_overflow' symbol in lib/test_fortify/write_overflow-strscpy.c warning: unsafe memcpy() usage lacked '__write_overflow_field' symbol in lib/test_fortify/write_overflow_field-memcpy.c warning: unsafe memmove() usage lacked '__write_overflow_field' symbol in lib/test_fortify/write_overflow_field-memmove.c warning: unsafe memset() usage lacked '__write_overflow_field' symbol in lib/test_fortify/write_overflow_field-memset.c I don't understand the nature of this warning, but I see that your patch ended up dropping -fsanitize=kernel-address from the compiler flags because the lib/test_fortify/*.c files don't match the $(is-kernel-object) rule. Adding back -fsanitize=kernel-address shuts up these warnings. I've applied a local workaround in my randconfig tree diff --git a/lib/Makefile b/lib/Makefile index ddcb76b294b5..d7b8fab64068 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -425,5 +425,7 @@ $(obj)/$(TEST_FORTIFY_LOG): $(addprefix $(obj)/, $(TEST_FORTIFY_LOGS)) FORCE # Fake dependency to trigger the fortify tests. ifeq ($(CONFIG_FORTIFY_SOURCE),y) +ifndef CONFIG_KASAN $(obj)/string.o: $(obj)/$(TEST_FORTIFY_LOG) +endif endif which I don't think we want upstream. Can you and Kees come up with a proper fix instead? Arnd
On Tue, May 28, 2024 at 8:36 PM Arnd Bergmann <arnd@arndb.de> wrote: > > On Mon, May 6, 2024, at 15:35, Masahiro Yamada wrote: > > The objtool, sanitizers (KASAN, UBSAN, etc.), and profilers (GCOV, etc.) > > are intended for kernel space objects. To exclude objects from their > > coverage, you need to set variables such as OBJECT_FILES_NON_STNDARD=y, > > KASAN_SANITIZE=n, etc. > > > > For instance, the following are not kernel objects, and therefore should > > opt out of coverage: > > > > - vDSO > > - purgatory > > - bootloader (arch/*/boot/) > > > > Kbuild can detect these cases without relying on such variables because > > objects not directly linked to vmlinux or modules are considered > > "non-standard objects". > > > > Detecting objects linked to vmlinux or modules is straightforward: > > > > - objects added to obj-y are linked to vmlinux > > - objects added to lib-y are linked to vmlinux > > - objects added to obj-m are linked to modules > > > > I noticed new randconfig build warnings and bisected them > down to this patch: > > warning: unsafe memchr_inv() usage lacked '__read_overflow' symbol in lib/test_fortify/read_overflow-memchr_inv.c > warning: unsafe memchr() usage lacked '__read_overflow' warning in lib/test_fortify/read_overflow-memchr.c > warning: unsafe memscan() usage lacked '__read_overflow' symbol in lib/test_fortify/read_overflow-memscan.c > warning: unsafe memcmp() usage lacked '__read_overflow' warning in lib/test_fortify/read_overflow-memcmp.c > warning: unsafe memcpy() usage lacked '__read_overflow2' symbol in lib/test_fortify/read_overflow2-memcpy.c > warning: unsafe memcmp() usage lacked '__read_overflow2' warning in lib/test_fortify/read_overflow2-memcmp.c > warning: unsafe memmove() usage lacked '__read_overflow2' symbol in lib/test_fortify/read_overflow2-memmove.c > warning: unsafe memcpy() usage lacked '__read_overflow2_field' symbol in lib/test_fortify/read_overflow2_field-memcpy.c > warning: unsafe memmove() usage lacked '__read_overflow2_field' symbol in lib/test_fortify/read_overflow2_field-memmove.c > warning: unsafe memcpy() usage lacked '__write_overflow' symbol in lib/test_fortify/write_overflow-memcpy.c > warning: unsafe memmove() usage lacked '__write_overflow' symbol in lib/test_fortify/write_overflow-memmove.c > warning: unsafe memset() usage lacked '__write_overflow' symbol in lib/test_fortify/write_overflow-memset.c > warning: unsafe strcpy() usage lacked '__write_overflow' symbol in lib/test_fortify/write_overflow-strcpy-lit.c > warning: unsafe strcpy() usage lacked '__write_overflow' symbol in lib/test_fortify/write_overflow-strcpy.c > warning: unsafe strncpy() usage lacked '__write_overflow' symbol in lib/test_fortify/write_overflow-strncpy-src.c > warning: unsafe strncpy() usage lacked '__write_overflow' symbol in lib/test_fortify/write_overflow-strncpy.c > warning: unsafe strscpy() usage lacked '__write_overflow' symbol in lib/test_fortify/write_overflow-strscpy.c > warning: unsafe memcpy() usage lacked '__write_overflow_field' symbol in lib/test_fortify/write_overflow_field-memcpy.c > warning: unsafe memmove() usage lacked '__write_overflow_field' symbol in lib/test_fortify/write_overflow_field-memmove.c > warning: unsafe memset() usage lacked '__write_overflow_field' symbol in lib/test_fortify/write_overflow_field-memset.c > > I don't understand the nature of this warning, but I see > that your patch ended up dropping -fsanitize=kernel-address > from the compiler flags because the lib/test_fortify/*.c files > don't match the $(is-kernel-object) rule. Adding back > -fsanitize=kernel-address shuts up these warnings. In my understanding, fortify-string is independent of KASAN. I do not understand why -fsanitize=kernel-address matters. > I've applied a local workaround in my randconfig tree > > diff --git a/lib/Makefile b/lib/Makefile > index ddcb76b294b5..d7b8fab64068 100644 > --- a/lib/Makefile > +++ b/lib/Makefile > @@ -425,5 +425,7 @@ $(obj)/$(TEST_FORTIFY_LOG): $(addprefix $(obj)/, $(TEST_FORTIFY_LOGS)) FORCE > > # Fake dependency to trigger the fortify tests. > ifeq ($(CONFIG_FORTIFY_SOURCE),y) > +ifndef CONFIG_KASAN > $(obj)/string.o: $(obj)/$(TEST_FORTIFY_LOG) > +endif > endif > > > which I don't think we want upstream. Can you and Kees come > up with a proper fix instead? > I set CONFIG_FORTIFY_SOURCE=y and CONFIG_KASAN=y, but I did not observe such warnings. Is this arch or compiler-specific? Could you provide me with the steps to reproduce it? -- Best Regards Masahiro Yamada
On Fri, May 31, 2024, at 10:52, Masahiro Yamada wrote: > On Tue, May 28, 2024 at 8:36 PM Arnd Bergmann <arnd@arndb.de> wrote: >> I don't understand the nature of this warning, but I see >> that your patch ended up dropping -fsanitize=kernel-address >> from the compiler flags because the lib/test_fortify/*.c files >> don't match the $(is-kernel-object) rule. Adding back >> -fsanitize=kernel-address shuts up these warnings. > > > In my understanding, fortify-string is independent of KASAN. > > I do not understand why -fsanitize=kernel-address matters. Right, this is something I've failed to understand as well so far. >> I've applied a local workaround in my randconfig tree >> >> diff --git a/lib/Makefile b/lib/Makefile >> index ddcb76b294b5..d7b8fab64068 100644 >> --- a/lib/Makefile >> +++ b/lib/Makefile >> @@ -425,5 +425,7 @@ $(obj)/$(TEST_FORTIFY_LOG): $(addprefix $(obj)/, $(TEST_FORTIFY_LOGS)) FORCE >> >> # Fake dependency to trigger the fortify tests. >> ifeq ($(CONFIG_FORTIFY_SOURCE),y) >> +ifndef CONFIG_KASAN >> $(obj)/string.o: $(obj)/$(TEST_FORTIFY_LOG) >> +endif >> endif >> >> >> which I don't think we want upstream. Can you and Kees come >> up with a proper fix instead? > > I set CONFIG_FORTIFY_SOURCE=y and CONFIG_KASAN=y, > but I did not observe such warnings. > Is this arch or compiler-specific? > > > Could you provide me with the steps to reproduce it? This is a randconfig .config file that shows it, but I've seen it in a lot of others: https://pastebin.com/raw/ESVzUeth If this doesn't reproduce it for you, I can try to narrow it down further. Arnd
On Fri, May 31, 2024 at 6:06 PM Arnd Bergmann <arnd@arndb.de> wrote: > > On Fri, May 31, 2024, at 10:52, Masahiro Yamada wrote: > > On Tue, May 28, 2024 at 8:36 PM Arnd Bergmann <arnd@arndb.de> wrote: > > >> I don't understand the nature of this warning, but I see > >> that your patch ended up dropping -fsanitize=kernel-address > >> from the compiler flags because the lib/test_fortify/*.c files > >> don't match the $(is-kernel-object) rule. Adding back > >> -fsanitize=kernel-address shuts up these warnings. > > > > > > In my understanding, fortify-string is independent of KASAN. > > > > I do not understand why -fsanitize=kernel-address matters. > > Right, this is something I've failed to understand as well > so far. > > >> I've applied a local workaround in my randconfig tree > >> > >> diff --git a/lib/Makefile b/lib/Makefile > >> index ddcb76b294b5..d7b8fab64068 100644 > >> --- a/lib/Makefile > >> +++ b/lib/Makefile > >> @@ -425,5 +425,7 @@ $(obj)/$(TEST_FORTIFY_LOG): $(addprefix $(obj)/, $(TEST_FORTIFY_LOGS)) FORCE > >> > >> # Fake dependency to trigger the fortify tests. > >> ifeq ($(CONFIG_FORTIFY_SOURCE),y) > >> +ifndef CONFIG_KASAN > >> $(obj)/string.o: $(obj)/$(TEST_FORTIFY_LOG) > >> +endif > >> endif > >> > >> > >> which I don't think we want upstream. Can you and Kees come > >> up with a proper fix instead? > > > > I set CONFIG_FORTIFY_SOURCE=y and CONFIG_KASAN=y, > > but I did not observe such warnings. > > Is this arch or compiler-specific? > > > > > > Could you provide me with the steps to reproduce it? > > This is a randconfig .config file that shows it, but > I've seen it in a lot of others: > https://pastebin.com/raw/ESVzUeth > > If this doesn't reproduce it for you, I can try to narrow > it down further. > > Arnd Thanks, I was able to reproduce it. The issue happens with CONFIG_KASAN_SW_TAGS. I do not see the issue with CONFIG_KASAN_GENERIC.
On Fri, May 31, 2024 at 07:16:30PM +0900, Masahiro Yamada wrote: > On Fri, May 31, 2024 at 6:06 PM Arnd Bergmann <arnd@arndb.de> wrote: > > > > On Fri, May 31, 2024, at 10:52, Masahiro Yamada wrote: > > > On Tue, May 28, 2024 at 8:36 PM Arnd Bergmann <arnd@arndb.de> wrote: > > > > >> I don't understand the nature of this warning, but I see > > >> that your patch ended up dropping -fsanitize=kernel-address > > >> from the compiler flags because the lib/test_fortify/*.c files > > >> don't match the $(is-kernel-object) rule. Adding back > > >> -fsanitize=kernel-address shuts up these warnings. > > > > > > > > > In my understanding, fortify-string is independent of KASAN. > > > > > > I do not understand why -fsanitize=kernel-address matters. > > > > Right, this is something I've failed to understand as well > > so far. > > > > >> I've applied a local workaround in my randconfig tree > > >> > > >> diff --git a/lib/Makefile b/lib/Makefile > > >> index ddcb76b294b5..d7b8fab64068 100644 > > >> --- a/lib/Makefile > > >> +++ b/lib/Makefile > > >> @@ -425,5 +425,7 @@ $(obj)/$(TEST_FORTIFY_LOG): $(addprefix $(obj)/, $(TEST_FORTIFY_LOGS)) FORCE > > >> > > >> # Fake dependency to trigger the fortify tests. > > >> ifeq ($(CONFIG_FORTIFY_SOURCE),y) > > >> +ifndef CONFIG_KASAN > > >> $(obj)/string.o: $(obj)/$(TEST_FORTIFY_LOG) > > >> +endif > > >> endif > > >> > > >> > > >> which I don't think we want upstream. Can you and Kees come > > >> up with a proper fix instead? > > > > > > I set CONFIG_FORTIFY_SOURCE=y and CONFIG_KASAN=y, > > > but I did not observe such warnings. > > > Is this arch or compiler-specific? > > > > > > > > > Could you provide me with the steps to reproduce it? > > > > This is a randconfig .config file that shows it, but > > I've seen it in a lot of others: > > https://pastebin.com/raw/ESVzUeth > > > > If this doesn't reproduce it for you, I can try to narrow > > it down further. > > > > Arnd > > > Thanks, I was able to reproduce it. > > The issue happens with CONFIG_KASAN_SW_TAGS. > > I do not see the issue with CONFIG_KASAN_GENERIC. I'll try to figure this out. I suspect some kind of symbol name changes are happening? The fortify tests expect to find specifically-named symbols, so perhaps something is disrupting that?
diff --git a/scripts/Makefile.build b/scripts/Makefile.build index c9c07a6144eb..56bacd992a09 100644 --- a/scripts/Makefile.build +++ b/scripts/Makefile.build @@ -214,7 +214,7 @@ endif # CONFIG_FTRACE_MCOUNT_USE_RECORDMCOUNT # 'OBJECT_FILES_NON_STANDARD_foo.o := 'y': skip objtool checking for a file # 'OBJECT_FILES_NON_STANDARD_foo.o := 'n': override directory skip for a file -is-standard-object = $(if $(filter-out y%, $(OBJECT_FILES_NON_STANDARD_$(target-stem).o)$(OBJECT_FILES_NON_STANDARD)n),y) +is-standard-object = $(if $(filter-out y%, $(OBJECT_FILES_NON_STANDARD_$(target-stem).o)$(OBJECT_FILES_NON_STANDARD)n),$(is-kernel-object)) $(obj)/%.o: private objtool-enabled = $(if $(is-standard-object),$(if $(delay-objtool),$(is-single-obj-m),y)) diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib index 5972ec4ee29b..d3180182af47 100644 --- a/scripts/Makefile.lib +++ b/scripts/Makefile.lib @@ -154,7 +154,7 @@ _cpp_flags = $(KBUILD_CPPFLAGS) $(cppflags-y) $(CPPFLAGS_$(target-stem).lds) # ifeq ($(CONFIG_GCOV_KERNEL),y) _c_flags += $(if $(patsubst n%,, \ - $(GCOV_PROFILE_$(target-stem).o)$(GCOV_PROFILE)$(CONFIG_GCOV_PROFILE_ALL)), \ + $(GCOV_PROFILE_$(target-stem).o)$(GCOV_PROFILE)$(if $(is-kernel-object),$(CONFIG_GCOV_PROFILE_ALL))), \ $(CFLAGS_GCOV)) endif @@ -165,32 +165,32 @@ endif ifeq ($(CONFIG_KASAN),y) ifneq ($(CONFIG_KASAN_HW_TAGS),y) _c_flags += $(if $(patsubst n%,, \ - $(KASAN_SANITIZE_$(target-stem).o)$(KASAN_SANITIZE)y), \ + $(KASAN_SANITIZE_$(target-stem).o)$(KASAN_SANITIZE)$(is-kernel-object)), \ $(CFLAGS_KASAN), $(CFLAGS_KASAN_NOSANITIZE)) endif endif ifeq ($(CONFIG_KMSAN),y) _c_flags += $(if $(patsubst n%,, \ - $(KMSAN_SANITIZE_$(target-stem).o)$(KMSAN_SANITIZE)y), \ + $(KMSAN_SANITIZE_$(target-stem).o)$(KMSAN_SANITIZE)$(is-kernel-object)), \ $(CFLAGS_KMSAN)) _c_flags += $(if $(patsubst n%,, \ - $(KMSAN_ENABLE_CHECKS_$(target-stem).o)$(KMSAN_ENABLE_CHECKS)y), \ + $(KMSAN_ENABLE_CHECKS_$(target-stem).o)$(KMSAN_ENABLE_CHECKS)$(is-kernel-object)), \ , -mllvm -msan-disable-checks=1) endif ifeq ($(CONFIG_UBSAN),y) _c_flags += $(if $(patsubst n%,, \ - $(UBSAN_SANITIZE_$(target-stem).o)$(UBSAN_SANITIZE)y), \ + $(UBSAN_SANITIZE_$(target-stem).o)$(UBSAN_SANITIZE)$(is-kernel-object)), \ $(CFLAGS_UBSAN)) _c_flags += $(if $(patsubst n%,, \ - $(UBSAN_SIGNED_WRAP_$(target-stem).o)$(UBSAN_SANITIZE_$(target-stem).o)$(UBSAN_SIGNED_WRAP)$(UBSAN_SANITIZE)y), \ + $(UBSAN_SIGNED_WRAP_$(target-stem).o)$(UBSAN_SANITIZE_$(target-stem).o)$(UBSAN_SIGNED_WRAP)$(UBSAN_SANITIZE)$(is-kernel-object)), \ $(CFLAGS_UBSAN_SIGNED_WRAP)) endif ifeq ($(CONFIG_KCOV),y) _c_flags += $(if $(patsubst n%,, \ - $(KCOV_INSTRUMENT_$(target-stem).o)$(KCOV_INSTRUMENT)$(CONFIG_KCOV_INSTRUMENT_ALL)), \ + $(KCOV_INSTRUMENT_$(target-stem).o)$(KCOV_INSTRUMENT)$(if $(is-kernel-object),$(CONFIG_KCOV_INSTRUMENT_ALL))), \ $(CFLAGS_KCOV)) endif @@ -200,7 +200,7 @@ endif # ifeq ($(CONFIG_KCSAN),y) _c_flags += $(if $(patsubst n%,, \ - $(KCSAN_SANITIZE_$(target-stem).o)$(KCSAN_SANITIZE)y), \ + $(KCSAN_SANITIZE_$(target-stem).o)$(KCSAN_SANITIZE)$(is-kernel-object)), \ $(CFLAGS_KCSAN)) # Some uninstrumented files provide implied barriers required to avoid false # positives: set KCSAN_INSTRUMENT_BARRIERS for barrier instrumentation only. @@ -219,6 +219,10 @@ _cpp_flags += $(addprefix -I, $(src) $(obj)) endif endif +# If $(is-kernel-object) is 'y', this object will be linked to vmlinux or modules +is-kernel-object = $(or $(part-of-builtin),$(part-of-module)) + +part-of-builtin = $(if $(filter $(basename $@).o, $(real-obj-y) $(lib-y)),y) part-of-module = $(if $(filter $(basename $@).o, $(real-obj-m)),y) quiet_modtag = $(if $(part-of-module),[M], )
The objtool, sanitizers (KASAN, UBSAN, etc.), and profilers (GCOV, etc.) are intended for kernel space objects. To exclude objects from their coverage, you need to set variables such as OBJECT_FILES_NON_STNDARD=y, KASAN_SANITIZE=n, etc. For instance, the following are not kernel objects, and therefore should opt out of coverage: - vDSO - purgatory - bootloader (arch/*/boot/) Kbuild can detect these cases without relying on such variables because objects not directly linked to vmlinux or modules are considered "non-standard objects". Detecting objects linked to vmlinux or modules is straightforward: - objects added to obj-y are linked to vmlinux - objects added to lib-y are linked to vmlinux - objects added to obj-m are linked to modules In the past, there was yet another case: - object paths added to head-y were linked to vmlinux Commit ce697ccee1a8 ("kbuild: remove head-y syntax") eliminated this. There are still some exceptions. For example, arch/s390/boot/Makefile needlessly uses obj-y for the bootloader objects, which can be fixed later. Going forward, objects that are not listed in obj-y, lib-y, or obj-m will opt out of objtool, sanitizers, and profilers by default. You can still override the Kbuild decision by explicitly specifying OBJECT_FILES_NON_STANDARD, KASAN_SANITIZE, etc. but most of such Make variables can be removed. The next commit will clean up redundant variables. Note: The coverage for some objects will be changed: - exclude .vmlinux.export.o from UBSAN, KCOV - exclude arch/csky/kernel/vdso/vgettimeofday.o from UBSAN - exclude arch/parisc/kernel/vdso32/vdso32.so from UBSAN - exclude arch/parisc/kernel/vdso64/vdso64.so from UBSAN - exclude arch/x86/um/vdso/um_vdso.o from UBSAN - exclude drivers/misc/lkdtm/rodata.o from UBSAN, KCOV - exclude init/version-timestamp.o from UBSAN, KCOV - exclude lib/test_fortify/*.o from all santizers and profilers I believe these are positive effects. Signed-off-by: Masahiro Yamada <masahiroy@kernel.org> --- scripts/Makefile.build | 2 +- scripts/Makefile.lib | 20 ++++++++++++-------- 2 files changed, 13 insertions(+), 9 deletions(-)