diff mbox series

kbuild: Disable KCSAN for autogenerated *.mod.c intermediaries

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

Commit Message

Borislav Petkov March 26, 2024, 8:25 p.m. UTC
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(-)

Comments

Marco Elver March 26, 2024, 8:41 p.m. UTC | #1
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
Nikolay Borisov March 27, 2024, 6:31 a.m. UTC | #2
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>
Paul Menzel March 28, 2024, 6:07 a.m. UTC | #3
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.
Masahiro Yamada March 30, 2024, 10:32 p.m. UTC | #4
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.
Borislav Petkov March 30, 2024, 10:48 p.m. UTC | #5
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 mbox series

Patch

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)