| Message ID | 20240326202548.GLZgMvTGpPfQcs2cQ_@fat_crate.local (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kbuild: Disable KCSAN for autogenerated *.mod.c intermediaries | expand |
On Tue, 26 Mar 2024 at 21:26, Borislav Petkov <bp@alien8.de> wrote: > > On Tue, Mar 26, 2024 at 08:33:31PM +0100, Marco Elver wrote: > > I think just removing instrumentation from the mod.c files is very reasonable. > > Thanks! > > @Masahiro: pls send this to Linus now as the commit which adds the > warning is in 6.9 so we should make sure we release it with all issues > fixed. > > Thx. > > --- > From: "Borislav Petkov (AMD)" <bp@alien8.de> > Date: Tue, 26 Mar 2024 21:11:01 +0100 > > When KCSAN and CONSTRUCTORS are enabled, one can trigger the > > "Unpatched return thunk in use. This should not happen!" > > catch-all warning. > > Usually, when objtool runs on the .o objects, it does generate a section > .return_sites which contains all offsets in the objects to the return > thunks of the functions present there. Those return thunks then get > patched at runtime by the alternatives. > > KCSAN and CONSTRUCTORS add this to the the object file's .text.startup > section: > > ------------------- > Disassembly of section .text.startup: > > ... > > 0000000000000010 <_sub_I_00099_0>: > 10: f3 0f 1e fa endbr64 > 14: e8 00 00 00 00 call 19 <_sub_I_00099_0+0x9> > 15: R_X86_64_PLT32 __tsan_init-0x4 > 19: e9 00 00 00 00 jmp 1e <__UNIQUE_ID___addressable_cryptd_alloc_aead349+0x6> > 1a: R_X86_64_PLT32 __x86_return_thunk-0x4 > ------------------- > > which, if it is built as a module goes through the intermediary stage of > creating a <module>.mod.c file which, when translated, receives a second > constructor: > > ------------------- > Disassembly of section .text.startup: > > 0000000000000010 <_sub_I_00099_0>: > 10: f3 0f 1e fa endbr64 > 14: e8 00 00 00 00 call 19 <_sub_I_00099_0+0x9> > 15: R_X86_64_PLT32 __tsan_init-0x4 > 19: e9 00 00 00 00 jmp 1e <_sub_I_00099_0+0xe> > 1a: R_X86_64_PLT32 __x86_return_thunk-0x4 > > ... > > 0000000000000030 <_sub_I_00099_0>: > 30: f3 0f 1e fa endbr64 > 34: e8 00 00 00 00 call 39 <_sub_I_00099_0+0x9> > 35: R_X86_64_PLT32 __tsan_init-0x4 > 39: e9 00 00 00 00 jmp 3e <__ksymtab_cryptd_alloc_ahash+0x2> > 3a: R_X86_64_PLT32 __x86_return_thunk-0x4 > ------------------- > > in the .ko file. > > Objtool has run already so that second constructor's return thunk cannot > be added to the .return_sites section and thus the return thunk remains > unpatched and the warning rightfully fires. > > Drop KCSAN flags from the mod.c generation stage as those constructors > do not contain data races one would be interested about. > > Debugged together with David Kaplan <David.Kaplan@amd.com> and Nikolay > Borisov <nik.borisov@suse.com>. > > Reported-by: Paul Menzel <pmenzel@molgen.mpg.de> > Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de> > Link: https://lore.kernel.org/r/0851a207-7143-417e-be31-8bf2b3afb57d@molgen.mpg.de Reviewed-by: Marco Elver <elver@google.com> Thanks! > --- > scripts/Makefile.modfinal | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/scripts/Makefile.modfinal b/scripts/Makefile.modfinal > index 8568d256d6fb..79fcf2731686 100644 > --- a/scripts/Makefile.modfinal > +++ b/scripts/Makefile.modfinal > @@ -23,7 +23,7 @@ modname = $(notdir $(@:.mod.o=)) > part-of-module = y > > quiet_cmd_cc_o_c = CC [M] $@ > - cmd_cc_o_c = $(CC) $(filter-out $(CC_FLAGS_CFI) $(CFLAGS_GCOV), $(c_flags)) -c -o $@ $< > + cmd_cc_o_c = $(CC) $(filter-out $(CC_FLAGS_CFI) $(CFLAGS_GCOV) $(CFLAGS_KCSAN), $(c_flags)) -c -o $@ $< > > %.mod.o: %.mod.c FORCE > $(call if_changed_dep,cc_o_c) > -- > 2.43.0 > > > > -- > Regards/Gruss, > Boris. > > https://people.kernel.org/tglx/notes-about-netiquette
On 26.03.24 г. 22:25 ч., Borislav Petkov wrote: > On Tue, Mar 26, 2024 at 08:33:31PM +0100, Marco Elver wrote: >> I think just removing instrumentation from the mod.c files is very reasonable. > > Thanks! > > @Masahiro: pls send this to Linus now as the commit which adds the > warning is in 6.9 so we should make sure we release it with all issues > fixed. > > Thx. > > --- > From: "Borislav Petkov (AMD)" <bp@alien8.de> > Date: Tue, 26 Mar 2024 21:11:01 +0100 > > When KCSAN and CONSTRUCTORS are enabled, one can trigger the > > "Unpatched return thunk in use. This should not happen!" > > catch-all warning. > > Usually, when objtool runs on the .o objects, it does generate a section > .return_sites which contains all offsets in the objects to the return > thunks of the functions present there. Those return thunks then get > patched at runtime by the alternatives. > > KCSAN and CONSTRUCTORS add this to the the object file's .text.startup > section: > > ------------------- > Disassembly of section .text.startup: > > ... > > 0000000000000010 <_sub_I_00099_0>: > 10: f3 0f 1e fa endbr64 > 14: e8 00 00 00 00 call 19 <_sub_I_00099_0+0x9> > 15: R_X86_64_PLT32 __tsan_init-0x4 > 19: e9 00 00 00 00 jmp 1e <__UNIQUE_ID___addressable_cryptd_alloc_aead349+0x6> > 1a: R_X86_64_PLT32 __x86_return_thunk-0x4 > ------------------- > > which, if it is built as a module goes through the intermediary stage of > creating a <module>.mod.c file which, when translated, receives a second > constructor: > > ------------------- > Disassembly of section .text.startup: > > 0000000000000010 <_sub_I_00099_0>: > 10: f3 0f 1e fa endbr64 > 14: e8 00 00 00 00 call 19 <_sub_I_00099_0+0x9> > 15: R_X86_64_PLT32 __tsan_init-0x4 > 19: e9 00 00 00 00 jmp 1e <_sub_I_00099_0+0xe> > 1a: R_X86_64_PLT32 __x86_return_thunk-0x4 > > ... > > 0000000000000030 <_sub_I_00099_0>: > 30: f3 0f 1e fa endbr64 > 34: e8 00 00 00 00 call 39 <_sub_I_00099_0+0x9> > 35: R_X86_64_PLT32 __tsan_init-0x4 > 39: e9 00 00 00 00 jmp 3e <__ksymtab_cryptd_alloc_ahash+0x2> > 3a: R_X86_64_PLT32 __x86_return_thunk-0x4 > ------------------- > > in the .ko file. > > Objtool has run already so that second constructor's return thunk cannot > be added to the .return_sites section and thus the return thunk remains > unpatched and the warning rightfully fires. > > Drop KCSAN flags from the mod.c generation stage as those constructors > do not contain data races one would be interested about. > > Debugged together with David Kaplan <David.Kaplan@amd.com> and Nikolay > Borisov <nik.borisov@suse.com>. > > Reported-by: Paul Menzel <pmenzel@molgen.mpg.de> > Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de> > Link: https://lore.kernel.org/r/0851a207-7143-417e-be31-8bf2b3afb57d@molgen.mpg.de > --- > scripts/Makefile.modfinal | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/scripts/Makefile.modfinal b/scripts/Makefile.modfinal > index 8568d256d6fb..79fcf2731686 100644 > --- a/scripts/Makefile.modfinal > +++ b/scripts/Makefile.modfinal > @@ -23,7 +23,7 @@ modname = $(notdir $(@:.mod.o=)) > part-of-module = y > > quiet_cmd_cc_o_c = CC [M] $@ > - cmd_cc_o_c = $(CC) $(filter-out $(CC_FLAGS_CFI) $(CFLAGS_GCOV), $(c_flags)) -c -o $@ $< > + cmd_cc_o_c = $(CC) $(filter-out $(CC_FLAGS_CFI) $(CFLAGS_GCOV) $(CFLAGS_KCSAN), $(c_flags)) -c -o $@ $< > > %.mod.o: %.mod.c FORCE > $(call if_changed_dep,cc_o_c) Reviewed-by: Nikolay Borisov <nik.borisov@suse.com>
Dear Borislav,
Thank you and the others very much for coming up with a patch so quickly.
Tested-by: Paul Menzel <pmenzel@molgen.mpg.de> # Dell XPS 13
9360/0596KF, BIOS 2.21.0 06/02/2022
Kind regards,
Paul
PS: Without your patch, I also so it one in QEMU q35, but not consistently.
On Wed, Mar 27, 2024 at 5:26 AM Borislav Petkov <bp@alien8.de> wrote: > > On Tue, Mar 26, 2024 at 08:33:31PM +0100, Marco Elver wrote: > > I think just removing instrumentation from the mod.c files is very reasonable. > > Thanks! > > @Masahiro: pls send this to Linus now as the commit which adds the > warning is in 6.9 so we should make sure we release it with all issues > fixed. > > Thx. > > --- > From: "Borislav Petkov (AMD)" <bp@alien8.de> > Date: Tue, 26 Mar 2024 21:11:01 +0100 > > When KCSAN and CONSTRUCTORS are enabled, one can trigger the > > "Unpatched return thunk in use. This should not happen!" > > catch-all warning. > > Usually, when objtool runs on the .o objects, it does generate a section > .return_sites which contains all offsets in the objects to the return > thunks of the functions present there. Those return thunks then get > patched at runtime by the alternatives. > > KCSAN and CONSTRUCTORS add this to the the object file's .text.startup > section: > > ------------------- > Disassembly of section .text.startup: > > ... > > 0000000000000010 <_sub_I_00099_0>: > 10: f3 0f 1e fa endbr64 > 14: e8 00 00 00 00 call 19 <_sub_I_00099_0+0x9> > 15: R_X86_64_PLT32 __tsan_init-0x4 > 19: e9 00 00 00 00 jmp 1e <__UNIQUE_ID___addressable_cryptd_alloc_aead349+0x6> > 1a: R_X86_64_PLT32 __x86_return_thunk-0x4 > ------------------- > > which, if it is built as a module goes through the intermediary stage of > creating a <module>.mod.c file which, when translated, receives a second > constructor: > > ------------------- > Disassembly of section .text.startup: > > 0000000000000010 <_sub_I_00099_0>: > 10: f3 0f 1e fa endbr64 > 14: e8 00 00 00 00 call 19 <_sub_I_00099_0+0x9> > 15: R_X86_64_PLT32 __tsan_init-0x4 > 19: e9 00 00 00 00 jmp 1e <_sub_I_00099_0+0xe> > 1a: R_X86_64_PLT32 __x86_return_thunk-0x4 > > ... > > 0000000000000030 <_sub_I_00099_0>: > 30: f3 0f 1e fa endbr64 > 34: e8 00 00 00 00 call 39 <_sub_I_00099_0+0x9> > 35: R_X86_64_PLT32 __tsan_init-0x4 > 39: e9 00 00 00 00 jmp 3e <__ksymtab_cryptd_alloc_ahash+0x2> > 3a: R_X86_64_PLT32 __x86_return_thunk-0x4 > ------------------- > > in the .ko file. > > Objtool has run already so that second constructor's return thunk cannot > be added to the .return_sites section and thus the return thunk remains > unpatched and the warning rightfully fires. > > Drop KCSAN flags from the mod.c generation stage as those constructors > do not contain data races one would be interested about. > > Debugged together with David Kaplan <David.Kaplan@amd.com> and Nikolay > Borisov <nik.borisov@suse.com>. > > Reported-by: Paul Menzel <pmenzel@molgen.mpg.de> > Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de> > Link: https://lore.kernel.org/r/0851a207-7143-417e-be31-8bf2b3afb57d@molgen.mpg.de > --- > scripts/Makefile.modfinal | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/scripts/Makefile.modfinal b/scripts/Makefile.modfinal > index 8568d256d6fb..79fcf2731686 100644 > --- a/scripts/Makefile.modfinal > +++ b/scripts/Makefile.modfinal > @@ -23,7 +23,7 @@ modname = $(notdir $(@:.mod.o=)) > part-of-module = y > > quiet_cmd_cc_o_c = CC [M] $@ > - cmd_cc_o_c = $(CC) $(filter-out $(CC_FLAGS_CFI) $(CFLAGS_GCOV), $(c_flags)) -c -o $@ $< > + cmd_cc_o_c = $(CC) $(filter-out $(CC_FLAGS_CFI) $(CFLAGS_GCOV) $(CFLAGS_KCSAN), $(c_flags)) -c -o $@ $< > > %.mod.o: %.mod.c FORCE > $(call if_changed_dep,cc_o_c) > -- > 2.43.0 > > > > -- > Regards/Gruss, > Boris. > > https://people.kernel.org/tglx/notes-about-netiquette I applied. I fixed the typo "the the" and replaced Link: with Closes: to address the following checkpatch warnings: WARNING: Possible repeated word: 'the' #18: KCSAN and CONSTRUCTORS add this to the the object file's .text.startup WARNING: Reported-by: should be immediately followed by Closes: with a URL to the report #70: Reported-by: Paul Menzel <pmenzel@molgen.mpg.de> Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de> Instead of filter-out, you could add KCSAN_SANITIZE := n to scripts/Makefile.modfinal because it is the reason why KCSAN_SANITIZE exists. But, that is not a big deal. GCOV flag is also filtered away instead of GCOV_PROFILE := n I will probably use a different approach later.
On Sun, Mar 31, 2024 at 07:32:30AM +0900, Masahiro Yamada wrote: > I applied. > > I fixed the typo "the the" and replaced Link: with Closes: > to address the following checkpatch warnings: Thanks! > Instead of filter-out, you could add > KCSAN_SANITIZE := n > to scripts/Makefile.modfinal because > it is the reason why KCSAN_SANITIZE exists. > > But, that is not a big deal. > GCOV flag is also filtered away instead of > GCOV_PROFILE := n Ah, that would've been more readable, yap. > I will probably use a different approach later. Right. Thx.
diff --git a/scripts/Makefile.modfinal b/scripts/Makefile.modfinal index 8568d256d6fb..79fcf2731686 100644 --- a/scripts/Makefile.modfinal +++ b/scripts/Makefile.modfinal @@ -23,7 +23,7 @@ modname = $(notdir $(@:.mod.o=)) part-of-module = y quiet_cmd_cc_o_c = CC [M] $@ - cmd_cc_o_c = $(CC) $(filter-out $(CC_FLAGS_CFI) $(CFLAGS_GCOV), $(c_flags)) -c -o $@ $< + cmd_cc_o_c = $(CC) $(filter-out $(CC_FLAGS_CFI) $(CFLAGS_GCOV) $(CFLAGS_KCSAN), $(c_flags)) -c -o $@ $< %.mod.o: %.mod.c FORCE $(call if_changed_dep,cc_o_c)