Non-booting current Linus' tree

Non-booting current Linus' tree

[Jan Kara]

  Hello,

  so I was wondering why I cannot boot current Linus' tree in my kvm
instance. The boot dies after writing "Booting the kernel" message. After
some bisection I have identified the culprit is in commit
91a8c2a5b43fc4be4adb4bda50cd331697e289e0 (x86/fpu: Clean up and fix MXCSR
handling). After applying that commit I start to get oopses like:

general protection fault: 0000 [#1] SMP 
Modules linked in:
CPU: 0 PID: 0 Comm: swapper Not tainted 4.1.0-rc4-xen+ #18
Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
task: ffffffff8180d4c0 ti: ffffffff81800000 task.ti: ffffffff81800000
RIP: 0010:[<ffffffff81048a6c>]  [<ffffffff81048a6c>] mxcsr_feature_mask_init+0x1c/0x40
RSP: 0000:ffffffff81803ba8  EFLAGS: 00010082
RAX: 0000000000000000 RBX: 00000000ffff8800 RCX: 0000000000000000
RDX: 0020400000000000 RSI: 0000000000000000 RDI: ffffffff81803da8
RBP: ffffffff81803da8 R08: 7f0089011d402087 R09: 00000000ffff8800
R10: 7f0089011d402087 R11: 00000000ffff8800 R12: ffff88007f004000
R13: 0000000000000000 R14: 0000000000000004 R15: 0000000000000008
FS:  0000000000000000(0000) GS:ffff88007f000000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: ffff88000269a000 CR3: 0000000001808000 CR4: 00000000000006b0
Stack:
 0000000000000000 0000000000000000 0000000000000000 0000000000000000
 0000000000000000 0000000000000000 0000000000000000 0000000000000000
 0000000000000000 0000000000000000 0000000000000000 0000000000000000
Call Trace:
 [<ffffffff81048beb>] fpu__init_system+0x2b/0x190
 [<ffffffff81048d5e>] fpu__cpu_init+0xe/0x10
 [<ffffffff8104eac3>] cpu_init+0x3a3/0x430
 [<ffffffff810794b1>] ? fill_pte+0x31/0x140
 [<ffffffff8107975d>] ? set_pte_vaddr_pud+0x4d/0x60
 [<ffffffff814058c0>] ? do_softirq_own_stack+0x30/0x30
 [<ffffffff81881519>] trap_init+0x4bb/0x602
 [<ffffffff8189d583>] ? inode_init_early+0x5a/0x98
 [<ffffffff8187efea>] start_kernel+0x265/0x4e1
 [<ffffffff8187eb7d>] ? set_init_arg+0x6a/0x6a
 [<ffffffff8187e565>] x86_64_start_reservations+0x1b/0x32
 [<ffffffff8187e6c3>] x86_64_start_kernel+0x147/0x156
 [<ffffffff8187e120>] ? early_idt_handlers+0x120/0x120
Code: 05 00 c9 c3 66 2e 0f 1f 84 00 00 00 00 00 90 55 31 c0 b9 40 00 00 00 48 89 e5 48 81 ec 00 02 00 00 48 8d bd 00 fe ff ff f3 48 ab <0f> ae 85 00 fe ff ff 8b 85 1c fe ff ff ba bf ff 00 00 85 c0 0f 
RIP  [<ffffffff81048a6c>] mxcsr_feature_mask_init+0x1c/0x40
 RSP <ffffffff81803ba8>

And indeed the oops happens at:
  2b:*	0f ae 85 00 fe ff ff 	fxsave -0x200(%rbp)		<--
trapping instruction

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. Now my build
host is rather old installation (SLES 11 SP3) running gcc 4.3.4 but there
are quite a few installations of it still running. So do we care or should
I just upgrade the build host?

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: Non-booting current Linus' tree

[Thomas Gleixner]

On Fri, 3 Jul 2015, Jan Kara wrote:
>   so I was wondering why I cannot boot current Linus' tree in my kvm
> instance. The boot dies after writing "Booting the kernel" message. After
> some bisection I have identified the culprit is in commit
> 91a8c2a5b43fc4be4adb4bda50cd331697e289e0 (x86/fpu: Clean up and fix MXCSR
> handling). After applying that commit I start to get oopses like:

At least you get oopses. My hardware simply refused to spit out
anything useful and my bisect just ended up at the same commit.
 
> And indeed the oops happens at:
>   2b:*	0f ae 85 00 fe ff ff 	fxsave -0x200(%rbp)		<--
> trapping instruction
> 
> 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. Now my build
> host is rather old installation (SLES 11 SP3) running gcc 4.3.4 but there
> are quite a few installations of it still running. So do we care or should
> I just upgrade the build host?

My gcc is old as well. 4.3.0-8. 

Thanks,

	tglx

Re: Non-booting current Linus' tree

[Linus Torvalds]

On Fri, Jul 3, 2015 at 8:23 AM, Jan Kara <jack@suse.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.

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.

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

Ingo?

               Linus

[PATCH] x86/fpu: Fix boot crash in the early FPU code

[Ingo Molnar]

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

> On Fri, Jul 3, 2015 at 8:23 AM, Jan Kara <jack@suse.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@kernel.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@suse.cz>
Reported-and-bisected-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Ingo Molnar <mingo@kernel.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,

Re: [PATCH] x86/fpu: Fix boot crash in the early FPU code

[Thomas Gleixner]

On Sat, 4 Jul 2015, Ingo Molnar wrote:
> Reported-and-bisected-by: Jan Kara <jack@suse.cz>
> Reported-and-bisected-by: Thomas Gleixner <tglx@linutronix.de>

Tested-by-me

Re: [PATCH] x86/fpu: Fix boot crash in the early FPU code

[Ingo Molnar]

* Thomas Gleixner <tglx@linutronix.de> wrote:

> On Sat, 4 Jul 2015, Ingo Molnar wrote:
> > Reported-and-bisected-by: Jan Kara <jack@suse.cz>
> > Reported-and-bisected-by: Thomas Gleixner <tglx@linutronix.de>
> 
> Tested-by-me

Ok, thanks, I've pushed it out!

	Ingo

[tip:x86/urgent] x86/fpu: Fix boot crash in the early FPU code

[tip-bot for Ingo Molnar]

Commit-ID:  b96fecbfa8c88b057e2bbf10021521c232bb3650
Gitweb:     http://git.kernel.org/tip/b96fecbfa8c88b057e2bbf10021521c232bb3650
Author:     Ingo Molnar <mingo@kernel.org>
AuthorDate: Sat, 4 Jul 2015 09:58:19 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Sat, 4 Jul 2015 10:05:56 +0200

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@suse.cz>
Reported-bisected-and-tested-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Jan Kara <jack@suse.cz>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Quentin Casasnovas <quentin.casasnovas@oracle.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/20150704075819.GA9201@gmail.com
Signed-off-by: Ingo Molnar <mingo@kernel.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 fc878fe..3282679 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,

Re: [PATCH] x86/fpu: Fix boot crash in the early FPU code

[Maciej W. Rozycki]

On Sat, 4 Jul 2015, Ingo Molnar wrote:

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

 What's the stack alignment mandated by the x86 psABI?  Is that anything 
beyond what the PUSH instructions imply (i.e. 4 bytes for 32-bit and 8 
bytes for 64-bit targets)?  Otherwise GCC might not be smart enough to 
apply the right subtract and mask operations to the stack/frame pointer.  
This might even have been documented with the alignment attribute.

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

 You could have used `alloca' and manually aligned the structure within 
the area obtained too (to avoid static storage if desired).

 FWIW,

  Maciej

Re: Non-booting current Linus' tree

[Andy Lutomirski]

On 07/03/2015 02:40 PM, Linus Torvalds wrote:
> On Fri, Jul 3, 2015 at 8:23 AM, Jan Kara <jack@suse.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.
> 

Are you sure?

The 64-bit part of arch/x86/Makefile contains:

        # Use -mpreferred-stack-boundary=3 if supported.
        KBUILD_CFLAGS += $(call cc-option,-mpreferred-stack-boundary=3)

but make V=1 isn't showing -mpreferred-stack-boundary=3.  This may be
because:

error: -mpreferred-stack-boundary=3 is not between 4 and 12
 extern int bar(const char *);
 ^

I found:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53383

and, indeed, -mno-sse -mpreferred-stack-boundary=3 is accepted.  This
make me think that the makefile is broken -- cc-option isn't working
because it doesn't check -mpreferred-stack-boundary in conjunction with
-mno-sse.

Given that -mpreferred-stack-boundary=3 doesn't appear to being set, I
think this is really our bug: our asm code makes no effort to align the
stack to a 16-byte boundary as required by the ABI, and we're not
overriding the ABI correctly.

I'll send a patch for the Makefile issue.

--Andy