Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1176664 > unrolled thread
| Started by | Jan Kara <jack@suse.cz> |
|---|---|
| First post | 2015-07-03 17:30 +0200 |
| Last post | 2015-07-05 10:40 +0200 |
| Articles | 3 — 3 participants |
Back to article view | Back to linux.kernel
Non-booting current Linus' tree Jan Kara <jack@suse.cz> - 2015-07-03 17:30 +0200
[PATCH] x86/fpu: Fix boot crash in the early FPU code Ingo Molnar <mingo@kernel.org> - 2015-07-05 10:40 +0200
[tip:x86/urgent] x86/fpu: Fix boot crash in the early FPU code tip-bot for Ingo Molnar <tipbot@zytor.com> - 2015-07-05 10:40 +0200
| From | Jan Kara <jack@suse.cz> |
|---|---|
| Date | 2015-07-03 17:30 +0200 |
| Subject | Non-booting current Linus' tree |
| Message-ID | <pIb4C-1JT-5@gated-at.bofh.it> |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [next] | [standalone]
| From | Ingo Molnar <mingo@kernel.org> |
|---|---|
| Date | 2015-07-05 10:40 +0200 |
| Subject | [PATCH] x86/fpu: Fix boot crash in the early FPU code |
| Message-ID | <pINCX-wQ-25@gated-at.bofh.it> |
| In reply to | #1176664 |
* 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,
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | tip-bot for Ingo Molnar <tipbot@zytor.com> |
|---|---|
| Date | 2015-07-05 10:40 +0200 |
| Subject | [tip:x86/urgent] x86/fpu: Fix boot crash in the early FPU code |
| Message-ID | <pINCX-wQ-27@gated-at.bofh.it> |
| In reply to | #1176862 |
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,
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web