Received-SPF: pass (google.com: domain of linux-kernel+bounces-119773-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Tue, 26 Mar 2024 20:12:29 +0100
From: Borislav Petkov <bp@alien8.de>
To: Nikolay Borisov <nik.borisov@suse.com>, Marco Elver <elver@google.com>,
	Josh Poimboeuf <jpoimboe@kernel.org>
Cc: Paul Menzel <pmenzel@molgen.mpg.de>,
	Thomas Gleixner <tglx@linutronix.de>,
	Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@redhat.com>,
	Dave Hansen <dave.hansen@linux.intel.com>, x86@kernel.org,
	LKML <linux-kernel@vger.kernel.org>, kasan-dev@googlegroups.com,
	David Kaplan <David.Kaplan@amd.com>
Subject: Re: Unpatched return thunk in use. This should not happen!
Message-ID: <20240326191211.GKZgMeC21uxi7H16o_@fat_crate.local>
References: <0851a207-7143-417e-be31-8bf2b3afb57d@molgen.mpg.de>
 <47e032a0-c9a0-4639-867b-cb3d67076eaf@suse.com>
 <20240326155247.GJZgLvT_AZi3XPPpBM@fat_crate.local>
 <80582244-8c1c-4eb4-8881-db68a1428817@suse.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <80582244-8c1c-4eb4-8881-db68a1428817@suse.com>

On Tue, Mar 26, 2024 at 06:04:26PM +0200, Nikolay Borisov wrote:
> So this       _sub_I_00099_0 is the compiler generated ctors that is likely
> not patched. What's strange is that when adding debugging code I see that 2
> ctors are being executed and only the 2nd one fires:
> 
> [    7.635418] in do_mod_ctors
> [    7.635425] calling 0 ctor 00000000aa7a443a
> [    7.635430] called 0 ctor
> [    7.635433] calling 1 ctor 00000000fe9d0d54
> [    7.635437] ------------[ cut here ]------------
> [    7.635441] Unpatched return thunk in use. This should not happen!

.. and this is just the beginning of the rabbit hole. David and I went
all the way down.

Turns out that objtool runs on the .o files and creates the
return_sites just fine but then the module building dance creates an
intermediary *.mod.c file and when that thing is built, KCSAN would
cause the addition of *another* constructor to .text.startup in the
module.

The .o file has one:

-------------------
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
-------------------


while the .ko file has two:

-------------------
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
-------------------

Once we've figured that out, finding a fix is easy:

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)

However, I'm not sure.

I wanna say that since those are constructors then we don't care about
dynamic races there so we could exclude them from KCSAN.

If not, I could disable the warning on KCSAN. I'm thinking no one would
run KCSAN in production...

A third option would be to make objtool run on .ko files. Yeah, that
would be fun for Josh. :-P

I'd look into the direction of melver for suggestions here.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette