Date:	Sat, 4 Jul 2015 09:58:19 +0200
From:	Ingo Molnar <mingo@...nel.org>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	Jan Kara <jack@...e.cz>, Borislav Petkov <bp@...en8.de>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	the arch/x86 maintainers <x86@...nel.org>
Subject: [PATCH] x86/fpu: Fix boot crash in the early FPU code


* Linus Torvalds <torvalds@...ux-foundation.org> wrote:

> On Fri, Jul 3, 2015 at 8:23 AM, Jan Kara <jack@...e.cz> wrote:
> >
> > Because the address isn't 32-byte aligned (which I assume is the
> > requirement from looking into the code). So clearly my gcc messed up and
> > miscompiled the thing by ignoring the alignment attribute.
> 
> Well, it's probably a mistake to begin with to expect gcc to get stack
> alignment right. Especially since we tell gcc to not align the stack
> as much as it usually wants to with -mpreferred-stack-boundary=2.
> 
> The code is broken in other ways too. The fxsave alignment isn't 32 bytes. It's 
> 16 bytes. And that's already encoded in the "struct fxregs_state", so adding 
> that extra alignment is just bogus crud anyway.

Yeah.

> So I seriously think that the whole commit 91a8c2a5b43f is just fundamentally 
> broken and should probably be reverted. The theoretical issue it fixes is a 
> smaller problem than the broken code it introduces (and I'm not just talking 
> about gcc bugs).
> 
> Plus the code just sets up and writes to a global variable *anyway*, so the 
> alleged race with using a static allocation is bogus: if that's a real concern, 
> then the code is fundamentally buggy in other ways anyway.

So this function used to be called from multiple sites, from per CPU init for 
example, with unclear semantics so when I wrote the patch the claimed parallelism 
could occur, my worry was things like (future ...) parallel bootup sequences 
writing to the same variable at once.

Even in that hypothetical case it _should_ work even in the racy case but it 
seemed like a neat idea to move it to the stack and eliminate any possibility of 
interaction. Today it does not look like such a neat idea anymore!

But note that the FPU series cleaned the messy init dependencies up, and now this 
function (called fpu__init_system_mxcsr()) is only called once, during system init 
- and the name now clearly reflects that. So there are no race worries whatsoever.

> I don't think you can boot with different CPU's having different mxcsr features 
> anyway, so..

Yes.

Also, because the function is now __init, this can be __initdata - further 
weakening the original 'this is better on the stack' argument.

Btw., is it a GCC bug or a known GCC property that a structure with a 16-byte 
alignment attribute does not get properly aligned on the stack?

In any case the patch below should fix the bogosities. Only very lighly tested.

Thanks,

	Ingo

===========================>
>From 2d1bba21cb64d9181eea6a9ec3083983a77678bf Mon Sep 17 00:00:00 2001
From: Ingo Molnar <mingo@...nel.org>
Date: Sat, 4 Jul 2015 09:40:55 +0200
Subject: [PATCH] x86/fpu: Fix boot crash in the early FPU code

Jan Kara and Thomas Gleixner reported boot crashes in the FPU code:

  general protection fault: 0000 [#1] SMP
  RIP: 0010:[<ffffffff81048a6c>]  [<ffffffff81048a6c>] mxcsr_feature_mask_init+0x1c/0x40

  2b:*  0f ae 85 00 fe ff ff    fxsave -0x200(%rbp)

and bisected it down to the following FPU commit:

   91a8c2a5b43f ("x86/fpu: Clean up and fix MXCSR handling")

The reason is that the on-stack FPU registers state variable, used by
the FXSAVE instruction, did not have the required minimum alignment
of 16 bytes, causing the general protection fault.

This is most likely a GCC bug in older GCC versions, but the offending commit
also added a bogus extra 32-byte alignment (which GCC ignored too).

So fix this bug by making the variable static again, but also mark it
__initdata this time, because fpu__init_system_mxcsr() is now an
__init function.

Reported-and-bisected-by: Jan Kara <jack@...e.cz>
Reported-and-bisected-by: Thomas Gleixner <tglx@...utronix.de>
Cc: Andy Lutomirski <luto@...capital.net>
Cc: Borislav Petkov <bp@...en8.de>
Cc: Brian Gerst <brgerst@...il.com>
Cc: Denys Vlasenko <dvlasenk@...hat.com>
Cc: H. Peter Anvin <hpa@...or.com>
Cc: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: Peter Zijlstra <peterz@...radead.org>
Cc: linux-kernel@...r.kernel.org
Signed-off-by: Ingo Molnar <mingo@...nel.org>
---
 arch/x86/kernel/fpu/init.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/fpu/init.c b/arch/x86/kernel/fpu/init.c
index fc878fee6a51..32826791e675 100644
--- a/arch/x86/kernel/fpu/init.c
+++ b/arch/x86/kernel/fpu/init.c
@@ -95,11 +95,12 @@ static void __init fpu__init_system_mxcsr(void)
 	unsigned int mask = 0;
 
 	if (cpu_has_fxsr) {
-		struct fxregs_state fx_tmp __aligned(32) = { };
+		/* Static because GCC does not get 16-byte stack alignment right: */
+		static struct fxregs_state fxregs __initdata;
 
-		asm volatile("fxsave %0" : "+m" (fx_tmp));
+		asm volatile("fxsave %0" : "+m" (fxregs));
 
-		mask = fx_tmp.mxcsr_mask;
+		mask = fxregs.mxcsr_mask;
 
 		/*
 		 * If zero then use the default features mask,
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists