Bug 217602

(In reply to Niklāvs Koļesņikovs from comment #0)
> Since yesterday my builds of the
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git no longer
> boot with a black screen immediately upon booting. Today I finished git
> bisecting the issue and arrived at the following:
> 
> 9df9d2f0471b4c4702670380b8d8a45b40b23a7d is the first bad commit
> commit 9df9d2f0471b4c4702670380b8d8a45b40b23a7d
> Author: Thomas Gleixner <tglx@linutronix.de>
> Date:   Wed Jun 14 01:39:39 2023 +0200
> 
>     init: Invoke arch_cpu_finalize_init() earlier
>     
>     X86 is reworking the boot process so that initializations which are not
>     required during early boot can be moved into the late boot process and
> out
>     of the fragile and restricted initial boot phase.
>     
>     arch_cpu_finalize_init() is the obvious place to do such initializations,
>     but arch_cpu_finalize_init() is invoked too late in start_kernel() e.g.
> for
>     initializing the FPU completely. fork_init() requires that the FPU is
>     initialized as the size of task_struct on X86 depends on the size of the
>     required FPU register buffer.
>     
>     Fortunately none of the init calls between calibrate_delay() and
>     arch_cpu_finalize_init() is relevant for the functionality of
>     arch_cpu_finalize_init().
>     
>     Invoke it right after calibrate_delay() where everything which is
> relevant
>     for arch_cpu_finalize_init() has been set up already.
>     
>     No functional change intended.
>     
>     Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
>     Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
>     Link: https://lore.kernel.org/r/20230613224545.612182854@linutronix.de
> 
>  init/main.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> Since it might be relevant, my CPU is Intel Core i5-12400 with UEFI from
> december 2022 and the compiler is gcc (Gentoo Hardened 13.1.1_p20230527 p3)
> 13.1.1 20230527. If additional information such as the kernel configuration
> is required, let me know.

Can you provide dmesg and your kernel config?

What you could also do is start moving the arch_cpu_finalize_init() call in start_kernel() downwards from the line it is now - 1042 - to where it was - 1068 - and test each kernel whether it boots.

Do that line by line. For example, move it after pid_idr_init(), test that then move it after anon_vma_init(); and test that too.

You could also bisect this and move it somewhere in the middle between those lines so that you can do a binary search of sorts and test less kernels.

Since those are all functions which init different kernel functionality, you'll be able to figure out what placement of arch_cpu_finalize_init() causes your machine to fail so that we can concentrate on the function which causes it.

Thx.

On Wed, Jun 28 2023 at 10:01, bugzilla-daemon@kernel.org wrote:
> --- Comment #2 from Borislav Petkov (bp@alien8.de) ---
> What you could also do is start moving the arch_cpu_finalize_init() call in
> start_kernel() downwards from the line it is now - 1042 - to where it was -
> 1068 - and test each kernel whether it boots.
>
> Do that line by line. For example, move it after pid_idr_init(), test that
> then
> move it after anon_vma_init(); and test that too.

But please do this on that offending commit 9df9d2f0471b.

Thanks,

        tglx

The system boots if *arch_cpu_finalize_init()* invocation is placed after the ifdef containing *efi_enter_virtual_mode()* call. Any earlier and the boot seems to immediately get stuck.

Because of the immediate failure there is no dmesg from the failed boot. If a successful bootup dmesg is needed, let me know. I did prepare one but due to its possibly sensitive nature, I'd rather be certain that it's required before sharing it. I'll upload the kernel config in a moment.

Created attachment 304496 [details]
Fedora Core based custom kernel configuration

Here's the kernel configuration I used. It's based on a Fedora distro kernel but has been modified extensively, so there could be something weird or wrong there now.

On Wed, Jun 28 2023 at 12:58, bugzilla-daemon@kernel.org wrote:
> The system boots if *arch_cpu_finalize_init()* invocation is placed after the
> ifdef containing *efi_enter_virtual_mode()* call. Any earlier and the boot
> seems to immediately get stuck.

Hmm. I can't reproduce.

> Because of the immediate failure there is no dmesg from the failed boot. If a
> successful bootup dmesg is needed, let me know. I did prepare one but due to
> its possibly sensitive nature, I'd rather be certain that it's required
> before
> sharing it. I'll upload the kernel config in a moment.

Can you please try the patch below?

If it boots, then please move alternative_instructions() up into
arch_cpu_finalize_init().

If not then it gets interesting ...

---
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -2346,7 +2346,10 @@ void __init arch_cpu_finalize_init(void)
 	 */
 	fpu__init_system();
 	fpu__init_cpu();
+}
 
+void __init arch_cpu_finalize_init2(void)
+{
 	alternative_instructions();
 
 	if (IS_ENABLED(CONFIG_X86_64)) {
--- a/init/main.c
+++ b/init/main.c
@@ -873,6 +873,8 @@ static void __init print_unknown_bootopt
 	memblock_free(unknown_options, len);
 }
 
+void arch_cpu_finalize_init2(void);
+
 asmlinkage __visible __init __no_sanitize_address __noreturn __no_stack_protector
 void start_kernel(void)
 {
@@ -1047,6 +1049,9 @@ void start_kernel(void)
 	if (efi_enabled(EFI_RUNTIME_SERVICES))
 		efi_enter_virtual_mode();
 #endif
+
+	arch_cpu_finalize_init2();
+
 	thread_stack_cache_init();
 	cred_init();
 	fork_init();

It seems like the patch is for current git master? Unfortunately I can't make it boot irrespective of where the *arch_cpu_finalize_init()* is placed. The placement after *efi_enter_virtual_mode()* only helps with commit 9df9d2f0471b but not git master.

On Wed, Jun 28 2023 at 21:43, bugzilla-daemon@kernel.org wrote:
> https://bugzilla.kernel.org/show_bug.cgi?id=217602
>
> --- Comment #7 from Niklāvs Koļesņikovs (pinkflames.linux@gmail.com) ---
> It seems like the patch is for current git master? Unfortunately I can't make
> it boot irrespective of where the *arch_cpu_finalize_init()* is placed. The
> placement after *efi_enter_virtual_mode()* only helps with commit
> 9df9d2f0471b
> but not git master.

That means we have two issues here:

  1) 9df9d2f0471b is problematic on its own independent of the FPU
     initialization. Updated patch against 9df9d2f0471b below.

     Sorry for pestering you, but we really need to understand which
     part of that change causes this.

  2) Initializing FPU late is problematic too.

     According to my debugging EFI needs FPU to be initialized _before_
     efi_enter_virtual_mode() as that issues some EFI call which is guarded
     by efi_fpu_begin(), which in turn is a wrapper around
     kernel_fpu_begin_mask().

	x86/fpu: Enabled xstate features 0x2ff, context size is 2568 bytes, using 'compacted' format.
	Freeing SMP alternatives memory: 40K
	pid_max: default: 114688 minimum: 896
	EFI enter virtual...
	FPU begin ... end
	FPU begin ... end
	EFI enter virtual done

     According to Ard, efi_enter_virtual_mode() is the first place where
     efi_fpu_begin() is used. There is no other (non-EFI) FPU register
     usage before that point either.

So I think the key point is not the FPU initialization. The key is
moving 9df9d2f0471b before that EFI initialization, but I have zero clue
which part of arch_cpu_finalize_init() causes the issue.

Thanks,

        tglx
---
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -2397,7 +2397,10 @@ void __init arch_cpu_finalize_init(void)
 		init_utsname()->machine[1] =
 			'0' + (boot_cpu_data.x86 > 6 ? 6 : boot_cpu_data.x86);
 	}
+}
 
+void __init arch_cpu_finalize_init2(void)
+{
 	alternative_instructions();
 
 	if (IS_ENABLED(CONFIG_X86_64)) {
--- a/init/main.c
+++ b/init/main.c
@@ -140,6 +140,8 @@ extern void time_init(void);
 /* Default late time init is NULL. archs can override this later. */
 void (*__initdata late_time_init)(void);
 
+void arch_cpu_finalize_init2(void);
+
 /* Untouched command line saved by arch-specific code. */
 char __initdata boot_command_line[COMMAND_LINE_SIZE];
 /* Untouched saved command line (eg. for /proc) */
@@ -1059,6 +1061,9 @@ asmlinkage __visible void __init __no_sa
 	if (efi_enabled(EFI_RUNTIME_SERVICES))
 		efi_enter_virtual_mode();
 #endif
+
+	arch_cpu_finalize_init2();
+
 	thread_stack_cache_init();
 	cred_init();
 	fork_init();

On Thu, Jun 29 2023 at 11:06, Thomas Gleixner wrote:
> So I think the key point is not the FPU initialization. The key is
> moving 9df9d2f0471b before that EFI initialization, but I have zero clue
> which part of arch_cpu_finalize_init() causes the issue.

Can you please try to boot with 'ibt=off' on the kernel command line?

Thanks,

        tglx

Yes, ibt=off makes the kernel bootable.

I'll report back on the patched 9df9d2f0471b a bit later.

The patch for 9df9d2f0471b did not boot with ibt enabled. I can hopefully later today/evening try moving the point where arch_cpu_finalize_init2 begins but looking at the code, I'm a bit unsure, if there's some gotchas to beware when doing that.

On Thu, Jun 29 2023 at 13:02, bugzilla-daemon@kernel.org wrote:
> --- Comment #11 from Niklāvs Koļesņikovs (pinkflames.linux@gmail.com) ---
> The patch for 9df9d2f0471b did not boot with ibt enabled. I can hopefully
> later
> today/evening try moving the point where arch_cpu_finalize_init2 begins but
> looking at the code, I'm a bit unsure, if there's some gotchas to beware when
> doing that.

Don't bother. IBT is enabled in identify_cpu(). alternatives() is just
converting the conditional branches to unconditional jumps or nops and
poisoning some ENDBR locations which have been identified by objtool.

But between enable and poisoning it just works. Not any different to
what we had before. Just earlier :)

Though I found the culprit. Can you test the patch below please?

Thanks,

        tglx
---
diff --git a/arch/x86/platform/efi/efi_64.c b/arch/x86/platform/efi/efi_64.c
index 232acf418cfb..77f7ac3668cb 100644
--- a/arch/x86/platform/efi/efi_64.c
+++ b/arch/x86/platform/efi/efi_64.c
@@ -853,9 +853,9 @@ efi_set_virtual_address_map(unsigned long memory_map_size,
 
 	/* Disable interrupts around EFI calls: */
 	local_irq_save(flags);
-	status = efi_call(efi.runtime->set_virtual_address_map,
-			  memory_map_size, descriptor_size,
-			  descriptor_version, virtual_map);
+	status = arch_efi_call_virt(efi.runtime, set_virtual_address_map,
+				    memory_map_size, descriptor_size,
+				    descriptor_version, virtual_map);
 	local_irq_restore(flags);
 
 	efi_fpu_end();

I can confirm that applying the latest patch to git master results in a bootable kernel even with IBT enabled. Thank you for looking into this and creating the multiple test patches.

On Thu, Jun 29 2023 at 19:11, bugzilla-daemon@kernel.org wrote:
> --- Comment #13 from Niklāvs Koļesņikovs (pinkflames.linux@gmail.com) ---
> I can confirm that applying the latest patch to git master results in a
> bootable kernel even with IBT enabled. Thank you for looking into this and
> creating the multiple test patches.

Thank you for bisecting, testing and chasing this.

Let me cook a proper patch, so this gets fixed upstream ASAP.

Thanks,

        tglx

The fix has landed in the https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git and, unless it was backported to a stable tree, the boot failure shouldn't be affecting any release version, so I'm closing this as fixed. If that was too hasty, sorry and feel free to re-open.