| Message ID | 20240506133544.2861555-1-masahiroy@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | kbuild: remove many tool coverage variables | expand |
In the future can you CC the various maintainers of the affected tooling? :) On Mon, May 06, 2024 at 10:35:41PM +0900, Masahiro Yamada wrote: > > This patch set removes many instances of the following variables: > > - OBJECT_FILES_NON_STANDARD > - KASAN_SANITIZE > - UBSAN_SANITIZE > - KCSAN_SANITIZE > - KMSAN_SANITIZE > - GCOV_PROFILE > - KCOV_INSTRUMENT > > Such tools are intended only for kernel space objects, most of which > are listed in obj-y, lib-y, or obj-m. This is a reasonable assertion, and the changes really simplify things now and into the future. Thanks for finding such a clean solution! I note that it also immediately fixes the issue noticed and fixed here: https://lore.kernel.org/all/20240513122754.1282833-1-roberto.sassu@huaweicloud.com/ > The best guess is, objects in $(obj-y), $(lib-y), $(obj-m) can opt in > such tools. Otherwise, not. > > This works in most places. I am worried about the use of "guess" and "most", though. :) Before, we had some clear opt-out situations, and now it's more of a side-effect. I think this is okay, but I'd really like to know more about your testing. It seems like you did build testing comparing build flags, since you call out some of the explicit changes in patch 2, quoting: > - include arch/mips/vdso/vdso-image.o into UBSAN, GCOV, KCOV > - include arch/sparc/vdso/vdso-image-*.o into UBSAN > - include arch/sparc/vdso/vma.o into UBSAN > - include arch/x86/entry/vdso/extable.o into KASAN, KCSAN, UBSAN, GCOV, KCOV > - include arch/x86/entry/vdso/vdso-image-*.o into KASAN, KCSAN, UBSAN, GCOV, KCOV > - include arch/x86/entry/vdso/vdso32-setup.o into KASAN, KCSAN, UBSAN, GCOV, KCOV > - include arch/x86/entry/vdso/vma.o into GCOV, KCOV > - include arch/x86/um/vdso/vma.o into KASAN, GCOV, KCOV I would agree that these cases are all likely desirable. Did you find any cases where you found that instrumentation was _removed_ where not expected? -Kees
On Mon, 13 May 2024 at 20:48, Kees Cook <keescook@chromium.org> wrote: > > In the future can you CC the various maintainers of the affected > tooling? :) > > On Mon, May 06, 2024 at 10:35:41PM +0900, Masahiro Yamada wrote: > > > > This patch set removes many instances of the following variables: > > > > - OBJECT_FILES_NON_STANDARD > > - KASAN_SANITIZE > > - UBSAN_SANITIZE > > - KCSAN_SANITIZE > > - KMSAN_SANITIZE > > - GCOV_PROFILE > > - KCOV_INSTRUMENT > > > > Such tools are intended only for kernel space objects, most of which > > are listed in obj-y, lib-y, or obj-m. I welcome the simplification, but see below. > This is a reasonable assertion, and the changes really simplify things > now and into the future. Thanks for finding such a clean solution! I > note that it also immediately fixes the issue noticed and fixed here: > https://lore.kernel.org/all/20240513122754.1282833-1-roberto.sassu@huaweicloud.com/ > > > The best guess is, objects in $(obj-y), $(lib-y), $(obj-m) can opt in > > such tools. Otherwise, not. > > > > This works in most places. > > I am worried about the use of "guess" and "most", though. :) Before, we > had some clear opt-out situations, and now it's more of a side-effect. I > think this is okay, but I'd really like to know more about your testing. > > It seems like you did build testing comparing build flags, since you > call out some of the explicit changes in patch 2, quoting: > > > - include arch/mips/vdso/vdso-image.o into UBSAN, GCOV, KCOV > > - include arch/sparc/vdso/vdso-image-*.o into UBSAN > > - include arch/sparc/vdso/vma.o into UBSAN > > - include arch/x86/entry/vdso/extable.o into KASAN, KCSAN, UBSAN, GCOV, KCOV > > - include arch/x86/entry/vdso/vdso-image-*.o into KASAN, KCSAN, UBSAN, GCOV, KCOV > > - include arch/x86/entry/vdso/vdso32-setup.o into KASAN, KCSAN, UBSAN, GCOV, KCOV > > - include arch/x86/entry/vdso/vma.o into GCOV, KCOV > > - include arch/x86/um/vdso/vma.o into KASAN, GCOV, KCOV > > I would agree that these cases are all likely desirable. > > Did you find any cases where you found that instrumentation was _removed_ > where not expected? In addition, did you boot test these kernels? While I currently don't recall if the vdso code caused us problems (besides the linking problem for non-kernel objects), anything that is opted out from instrumentation in arch/ code needs to be carefully tested if it should be opted back into instrumentation. We had many fun hours debugging boot hangs or other recursion issues due to instrumented arch code. Thanks, -- Marco
On Tue, May 14, 2024 at 3:48 AM Kees Cook <keescook@chromium.org> wrote: > > In the future can you CC the various maintainers of the affected > tooling? :) Sorry, I was too lazy to add CC for treewide changes like this. Anyway, thanks for adding CC. > On Mon, May 06, 2024 at 10:35:41PM +0900, Masahiro Yamada wrote: > > > > This patch set removes many instances of the following variables: > > > > - OBJECT_FILES_NON_STANDARD > > - KASAN_SANITIZE > > - UBSAN_SANITIZE > > - KCSAN_SANITIZE > > - KMSAN_SANITIZE > > - GCOV_PROFILE > > - KCOV_INSTRUMENT > > > > Such tools are intended only for kernel space objects, most of which > > are listed in obj-y, lib-y, or obj-m. > > This is a reasonable assertion, and the changes really simplify things > now and into the future. Thanks for finding such a clean solution! I > note that it also immediately fixes the issue noticed and fixed here: > https://lore.kernel.org/all/20240513122754.1282833-1-roberto.sassu@huaweicloud.com/ > > > The best guess is, objects in $(obj-y), $(lib-y), $(obj-m) can opt in > > such tools. Otherwise, not. > > > > This works in most places. > > I am worried about the use of "guess" and "most", though. :) Before, we > had some clear opt-out situations, and now it's more of a side-effect. I > think this is okay, but I'd really like to know more about your testing. - defconfig for arc, hexagon, loongarch, microblaze, sh, xtensa - allmodconfig for the other architectures (IIRC, allmodconfig failed for the first case, for reasons unrelated to this patch set, so I used defconfig instead. I do not remember what errors I observed) I checked the diff of .*.cmd files. > > It seems like you did build testing comparing build flags, since you > call out some of the explicit changes in patch 2, quoting: > > > - include arch/mips/vdso/vdso-image.o into UBSAN, GCOV, KCOV > > - include arch/sparc/vdso/vdso-image-*.o into UBSAN > > - include arch/sparc/vdso/vma.o into UBSAN > > - include arch/x86/entry/vdso/extable.o into KASAN, KCSAN, UBSAN, GCOV, KCOV > > - include arch/x86/entry/vdso/vdso-image-*.o into KASAN, KCSAN, UBSAN, GCOV, KCOV > > - include arch/x86/entry/vdso/vdso32-setup.o into KASAN, KCSAN, UBSAN, GCOV, KCOV > > - include arch/x86/entry/vdso/vma.o into GCOV, KCOV > > - include arch/x86/um/vdso/vma.o into KASAN, GCOV, KCOV > > I would agree that these cases are all likely desirable. > > Did you find any cases where you found that instrumentation was _removed_ > where not expected? See the commit log of 1/3. > 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.
On Tue, May 14, 2024 at 4:55 AM Marco Elver <elver@google.com> wrote: > > On Mon, 13 May 2024 at 20:48, Kees Cook <keescook@chromium.org> wrote: > > > > In the future can you CC the various maintainers of the affected > > tooling? :) > > > > On Mon, May 06, 2024 at 10:35:41PM +0900, Masahiro Yamada wrote: > > > > > > This patch set removes many instances of the following variables: > > > > > > - OBJECT_FILES_NON_STANDARD > > > - KASAN_SANITIZE > > > - UBSAN_SANITIZE > > > - KCSAN_SANITIZE > > > - KMSAN_SANITIZE > > > - GCOV_PROFILE > > > - KCOV_INSTRUMENT > > > > > > Such tools are intended only for kernel space objects, most of which > > > are listed in obj-y, lib-y, or obj-m. > > I welcome the simplification, but see below. > > > This is a reasonable assertion, and the changes really simplify things > > now and into the future. Thanks for finding such a clean solution! I > > note that it also immediately fixes the issue noticed and fixed here: > > https://lore.kernel.org/all/20240513122754.1282833-1-roberto.sassu@huaweicloud.com/ > > > > > The best guess is, objects in $(obj-y), $(lib-y), $(obj-m) can opt in > > > such tools. Otherwise, not. > > > > > > This works in most places. > > > > I am worried about the use of "guess" and "most", though. :) Before, we > > had some clear opt-out situations, and now it's more of a side-effect. I > > think this is okay, but I'd really like to know more about your testing. > > > > It seems like you did build testing comparing build flags, since you > > call out some of the explicit changes in patch 2, quoting: > > > > > - include arch/mips/vdso/vdso-image.o into UBSAN, GCOV, KCOV > > > - include arch/sparc/vdso/vdso-image-*.o into UBSAN > > > - include arch/sparc/vdso/vma.o into UBSAN > > > - include arch/x86/entry/vdso/extable.o into KASAN, KCSAN, UBSAN, GCOV, KCOV > > > - include arch/x86/entry/vdso/vdso-image-*.o into KASAN, KCSAN, UBSAN, GCOV, KCOV > > > - include arch/x86/entry/vdso/vdso32-setup.o into KASAN, KCSAN, UBSAN, GCOV, KCOV > > > - include arch/x86/entry/vdso/vma.o into GCOV, KCOV > > > - include arch/x86/um/vdso/vma.o into KASAN, GCOV, KCOV > > > > I would agree that these cases are all likely desirable. > > > > Did you find any cases where you found that instrumentation was _removed_ > > where not expected? > > In addition, did you boot test these kernels? No. I didn't. > While I currently don't > recall if the vdso code caused us problems (besides the linking > problem for non-kernel objects), anything that is opted out from > instrumentation in arch/ code needs to be carefully tested if it > should be opted back into instrumentation. We had many fun hours > debugging boot hangs or other recursion issues due to instrumented > arch code. As I replied to Kees, I checked the diff of .*.cmd files. I believe checking the compiler flags for every object is comprehensive testing. If the same set of compiler flags is passed, the same build artifact is generated.
On Tue, May 14, 2024 at 07:39:31AM +0900, Masahiro Yamada wrote: > On Tue, May 14, 2024 at 3:48 AM Kees Cook <keescook@chromium.org> wrote: > > I am worried about the use of "guess" and "most", though. :) Before, we > > had some clear opt-out situations, and now it's more of a side-effect. I > > think this is okay, but I'd really like to know more about your testing. > > - defconfig for arc, hexagon, loongarch, microblaze, sh, xtensa > - allmodconfig for the other architectures > > (IIRC, allmodconfig failed for the first case, for reasons unrelated > to this patch set, so I used defconfig instead. > I do not remember what errors I observed) > > I checked the diff of .*.cmd files. Ah-ha, perfect! Thanks. :) > > Did you find any cases where you found that instrumentation was _removed_ > > where not expected? > > See the commit log of 1/3. Okay, thanks. I wasn't sure if that was the complete set or just part of the "most" bit. :) Thanks! I think this should all be fine. I'm not aware of anything melting down yet from these changes being in -next, so: Reviewed-by: Kees Cook <keescook@chromium.org>
On Mon, 2024-05-13 at 11:48 -0700, Kees Cook wrote: > In the future can you CC the various maintainers of the affected > tooling? :) > > On Mon, May 06, 2024 at 10:35:41PM +0900, Masahiro Yamada wrote: > > > > This patch set removes many instances of the following variables: > > > > - OBJECT_FILES_NON_STANDARD > > - KASAN_SANITIZE > > - UBSAN_SANITIZE > > - KCSAN_SANITIZE > > - KMSAN_SANITIZE > > - GCOV_PROFILE > > - KCOV_INSTRUMENT > > > > Such tools are intended only for kernel space objects, most of which > > are listed in obj-y, lib-y, or obj-m. > > This is a reasonable assertion, and the changes really simplify things > now and into the future. Thanks for finding such a clean solution! I > note that it also immediately fixes the issue noticed and fixed here: > https://lore.kernel.org/all/20240513122754.1282833-1-roberto.sassu@huaweicloud.com/ Yes, this patch set fixes the issue too. Tested-by: Roberto Sassu <roberto.sassu@huawei.com> Now UBSAN complains about misaligned address, such as: [ 0.150000][ T1] UBSAN: misaligned-access in kernel/workqueue.c:5514:3 [ 0.150000][ T1] member access within misaligned address 0000000064c36f78 for type 'struct pool_workqueue' [ 0.150000][ T1] which requires 512 byte alignment [ 0.150000][ T1] CPU: 0 PID: 1 Comm: swapper Not tainted 6.9.0-dont-use-00003-g3b621c71dc5e #2244 But I guess this is for a separate thread. Thanks Roberto > > The best guess is, objects in $(obj-y), $(lib-y), $(obj-m) can opt in > > such tools. Otherwise, not. > > > > This works in most places. > > I am worried about the use of "guess" and "most", though. :) Before, we > had some clear opt-out situations, and now it's more of a side-effect. I > think this is okay, but I'd really like to know more about your testing. > > It seems like you did build testing comparing build flags, since you > call out some of the explicit changes in patch 2, quoting: > > > - include arch/mips/vdso/vdso-image.o into UBSAN, GCOV, KCOV > > - include arch/sparc/vdso/vdso-image-*.o into UBSAN > > - include arch/sparc/vdso/vma.o into UBSAN > > - include arch/x86/entry/vdso/extable.o into KASAN, KCSAN, UBSAN, GCOV, KCOV > > - include arch/x86/entry/vdso/vdso-image-*.o into KASAN, KCSAN, UBSAN, GCOV, KCOV > > - include arch/x86/entry/vdso/vdso32-setup.o into KASAN, KCSAN, UBSAN, GCOV, KCOV > > - include arch/x86/entry/vdso/vma.o into GCOV, KCOV > > - include arch/x86/um/vdso/vma.o into KASAN, GCOV, KCOV > > I would agree that these cases are all likely desirable. > > Did you find any cases where you found that instrumentation was _removed_ > where not expected? > > -Kees >
This patch set removes many instances of the following variables: - OBJECT_FILES_NON_STANDARD - KASAN_SANITIZE - UBSAN_SANITIZE - KCSAN_SANITIZE - KMSAN_SANITIZE - GCOV_PROFILE - KCOV_INSTRUMENT Such tools are intended only for kernel space objects, most of which are listed in obj-y, lib-y, or obj-m. The best guess is, objects in $(obj-y), $(lib-y), $(obj-m) can opt in such tools. Otherwise, not. This works in most places. Masahiro Yamada (3): kbuild: provide reasonable defaults for tool coverage Makefile: remove redundant tool coverage variables kbuild: use GCOV_PROFILE and KCSAN_SANITIZE in scripts/Makefile.modfinal arch/arm/boot/bootp/Makefile | 1 - arch/arm/boot/compressed/Makefile | 7 ------- arch/arm/vdso/Makefile | 9 --------- arch/arm64/kernel/pi/Makefile | 6 ------ arch/arm64/kernel/vdso/Makefile | 8 -------- arch/arm64/kvm/hyp/nvhe/Makefile | 13 ------------- arch/csky/kernel/vdso/Makefile | 4 ---- arch/loongarch/vdso/Makefile | 7 ------- arch/mips/boot/compressed/Makefile | 6 ------ arch/mips/vdso/Makefile | 7 ------- arch/parisc/boot/compressed/Makefile | 4 ---- arch/powerpc/kernel/vdso/Makefile | 8 -------- arch/powerpc/purgatory/Makefile | 3 --- arch/riscv/boot/Makefile | 2 -- arch/riscv/kernel/compat_vdso/Makefile | 6 ------ arch/riscv/kernel/pi/Makefile | 6 ------ arch/riscv/kernel/vdso/Makefile | 6 ------ arch/riscv/purgatory/Makefile | 8 -------- arch/s390/boot/Makefile | 2 ++ arch/s390/kernel/vdso32/Makefile | 8 -------- arch/s390/kernel/vdso64/Makefile | 8 -------- arch/s390/purgatory/Makefile | 8 -------- arch/sh/boot/compressed/Makefile | 3 --- arch/sparc/vdso/Makefile | 2 -- arch/x86/boot/Makefile | 15 --------------- arch/x86/boot/compressed/Makefile | 11 ----------- arch/x86/entry/vdso/Makefile | 26 -------------------------- arch/x86/purgatory/Makefile | 9 --------- arch/x86/realmode/rm/Makefile | 11 ----------- arch/x86/um/vdso/Makefile | 7 ------- arch/xtensa/boot/lib/Makefile | 5 ----- drivers/firmware/efi/libstub/Makefile | 11 ----------- drivers/misc/lkdtm/Makefile | 4 ---- init/Makefile | 3 --- scripts/Makefile.build | 2 +- scripts/Makefile.lib | 20 ++++++++++++-------- scripts/Makefile.modfinal | 4 +++- scripts/Makefile.vmlinux | 3 --- scripts/mod/Makefile | 1 - 39 files changed, 18 insertions(+), 256 deletions(-)