* [PATCH v2] x86/fpu: Disable BH while while loading FPU registers in __fpu__restore_sig() @ 2018-11-19 16:04 Sebastian Andrzej Siewior 2018-11-19 17:02 ` Dave Hansen ` (2 more replies) 0 siblings, 3 replies; 17+ messages in thread From: Sebastian Andrzej Siewior @ 2018-11-19 16:04 UTC (permalink / raw) To: Borislav Petkov, x86 Cc: Ingo Molnar, linux-kernel, Andy Lutomirski, Paolo Bonzini, Radim Krčmář, kvm, Jason A. Donenfeld, Rik van Riel, Dave Hansen The sequence fpu->initialized = 1; /* step A */ preempt_disable(); /* step B */ fpu__restore(fpu); preempt_enable(); is racy in regard to a context switch. For 32bit frames __fpu__restore_sig() prepares the FPU state within fpu->state. To ensure that a context switch (switch_fpu_prepare() in particular) does not modify fpu->state it uses fpu__drop() which sets fpu->initializes to 0. With this change the CPU's FPU state is not saved to fpu->state during a context switch. It then loads the state to fpu->state from userland and ensures it sane. The new state is loaded via fpu__restore(). The code sets then fpu->initializes to 1 in order to avoid fpu__initialize() doing anything (overwrite the new state) which is part of fpu__restore(). A context switch between step A and B would save CPU's current FPU registers to fpu->state and overwrite the newly prepared state. This looks like tiny race window but the Kernel Test Robot reported this back in 2016 while we had lazy FPU support. Borislav Petkov made the link between that report and another patch that has been posted. Since the removal of the lazy FPU support, this race goes unnoticed because the warning has been removed. Use local_bh_disable() around the restore sequence to avoid the race. BH needs to be disabled because BH is allowed to run (even with preemption disabled) and might invoke kernel_fpu_begin(). Link: https://lkml.kernel.org/r/20160226074940.GA28911@pd.tnic Cc: stable@vger.kernel.org Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> --- v1…v2: A more verbose commit as message. arch/x86/kernel/fpu/signal.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c index 61a949d84dfa5..d99a8ee9e185e 100644 --- a/arch/x86/kernel/fpu/signal.c +++ b/arch/x86/kernel/fpu/signal.c @@ -344,10 +344,10 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size) sanitize_restored_xstate(tsk, &env, xfeatures, fx_only); } + local_bh_disable(); fpu->initialized = 1; - preempt_disable(); fpu__restore(fpu); - preempt_enable(); + local_bh_enable(); return err; } else { -- 2.19.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2] x86/fpu: Disable BH while while loading FPU registers in __fpu__restore_sig() 2018-11-19 16:04 [PATCH v2] x86/fpu: Disable BH while while loading FPU registers in __fpu__restore_sig() Sebastian Andrzej Siewior @ 2018-11-19 17:02 ` Dave Hansen 2018-11-19 17:11 ` Sebastian Andrzej Siewior 2018-11-19 18:10 ` Borislav Petkov [not found] ` <20181120132531.6E80C206BB@mail.kernel.org> 2 siblings, 1 reply; 17+ messages in thread From: Dave Hansen @ 2018-11-19 17:02 UTC (permalink / raw) To: Sebastian Andrzej Siewior, Borislav Petkov, x86 Cc: Ingo Molnar, linux-kernel, Andy Lutomirski, Paolo Bonzini, Radim Krčmář, kvm, Jason A. Donenfeld, Rik van Riel, Dave Hansen On 11/19/18 8:04 AM, Sebastian Andrzej Siewior wrote: > v1…v2: A more verbose commit as message. I was really hoping for code comments. :) ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] x86/fpu: Disable BH while while loading FPU registers in __fpu__restore_sig() 2018-11-19 17:02 ` Dave Hansen @ 2018-11-19 17:11 ` Sebastian Andrzej Siewior 2018-11-19 17:27 ` Borislav Petkov 0 siblings, 1 reply; 17+ messages in thread From: Sebastian Andrzej Siewior @ 2018-11-19 17:11 UTC (permalink / raw) To: Dave Hansen Cc: Borislav Petkov, x86, Ingo Molnar, linux-kernel, Andy Lutomirski, Paolo Bonzini, Radim Krčmář, kvm, Jason A. Donenfeld, Rik van Riel, Dave Hansen On 2018-11-19 09:02:45 [-0800], Dave Hansen wrote: > On 11/19/18 8:04 AM, Sebastian Andrzej Siewior wrote: > > v1…v2: A more verbose commit as message. > > I was really hoping for code comments. :) I though we agreed to make those in the larger series because those comments in __fpu__restore_sig() would be removed anyway (as part of the series). Sebastian ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] x86/fpu: Disable BH while while loading FPU registers in __fpu__restore_sig() 2018-11-19 17:11 ` Sebastian Andrzej Siewior @ 2018-11-19 17:27 ` Borislav Petkov 2018-11-19 17:31 ` Sebastian Andrzej Siewior 2018-11-19 17:32 ` Dave Hansen 0 siblings, 2 replies; 17+ messages in thread From: Borislav Petkov @ 2018-11-19 17:27 UTC (permalink / raw) To: Sebastian Andrzej Siewior, Dave Hansen Cc: x86, Ingo Molnar, linux-kernel, Andy Lutomirski, Paolo Bonzini, Radim Krčmář, kvm, Jason A. Donenfeld, Rik van Riel, Dave Hansen On Mon, Nov 19, 2018 at 06:11:29PM +0100, Sebastian Andrzej Siewior wrote: > On 2018-11-19 09:02:45 [-0800], Dave Hansen wrote: > > On 11/19/18 8:04 AM, Sebastian Andrzej Siewior wrote: > > > v1…v2: A more verbose commit as message. > > > > I was really hoping for code comments. :) > > I though we agreed to make those in the larger series because those > comments in __fpu__restore_sig() would be removed anyway (as part of the > series). Also, over local_bh_disable() does not really fit as this is generic code and Sebastian said ARM does the whole thing a bit differently, for one. We probably should put that comment somewhere prominent in arch/x86/kernel/fpu/core.c or similar - somewhere people poking at FPU stuff will see it. -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] x86/fpu: Disable BH while while loading FPU registers in __fpu__restore_sig() 2018-11-19 17:27 ` Borislav Petkov @ 2018-11-19 17:31 ` Sebastian Andrzej Siewior 2018-11-19 17:41 ` Borislav Petkov 2018-11-19 17:32 ` Dave Hansen 1 sibling, 1 reply; 17+ messages in thread From: Sebastian Andrzej Siewior @ 2018-11-19 17:31 UTC (permalink / raw) To: Borislav Petkov Cc: Dave Hansen, x86, Ingo Molnar, linux-kernel, Andy Lutomirski, Paolo Bonzini, Radim Krčmář, kvm, Jason A. Donenfeld, Rik van Riel, Dave Hansen On 2018-11-19 18:27:43 [+0100], Borislav Petkov wrote: > On Mon, Nov 19, 2018 at 06:11:29PM +0100, Sebastian Andrzej Siewior wrote: > > On 2018-11-19 09:02:45 [-0800], Dave Hansen wrote: > > > On 11/19/18 8:04 AM, Sebastian Andrzej Siewior wrote: > > > > v1…v2: A more verbose commit as message. > > > > > > I was really hoping for code comments. :) > > > > I though we agreed to make those in the larger series because those > > comments in __fpu__restore_sig() would be removed anyway (as part of the > > series). > > Also, over local_bh_disable() does not really fit as this is generic > code and Sebastian said ARM does the whole thing a bit differently, for > one. > > We probably should put that comment somewhere prominent in > arch/x86/kernel/fpu/core.c or similar - somewhere people poking at FPU > stuff will see it. I though about __fpregs_changes_begin() in the last patch of the commit: https://git.kernel.org/pub/scm/linux/kernel/git/bigeasy/staging.git/commit/?h=x86_fpu_rtu_v4 Sebastian ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] x86/fpu: Disable BH while while loading FPU registers in __fpu__restore_sig() 2018-11-19 17:31 ` Sebastian Andrzej Siewior @ 2018-11-19 17:41 ` Borislav Petkov 0 siblings, 0 replies; 17+ messages in thread From: Borislav Petkov @ 2018-11-19 17:41 UTC (permalink / raw) To: Sebastian Andrzej Siewior Cc: Dave Hansen, x86, Ingo Molnar, linux-kernel, Andy Lutomirski, Paolo Bonzini, Radim Krčmář, kvm, Jason A. Donenfeld, Rik van Riel, Dave Hansen On Mon, Nov 19, 2018 at 06:31:36PM +0100, Sebastian Andrzej Siewior wrote: > I though about __fpregs_changes_begin() in the last patch of the commit: > https://git.kernel.org/pub/scm/linux/kernel/git/bigeasy/staging.git/commit/?h=x86_fpu_rtu_v4 Also a good spot - api.h talks about preemption already so sure, why not. Thx. -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] x86/fpu: Disable BH while while loading FPU registers in __fpu__restore_sig() 2018-11-19 17:27 ` Borislav Petkov 2018-11-19 17:31 ` Sebastian Andrzej Siewior @ 2018-11-19 17:32 ` Dave Hansen 1 sibling, 0 replies; 17+ messages in thread From: Dave Hansen @ 2018-11-19 17:32 UTC (permalink / raw) To: Borislav Petkov, Sebastian Andrzej Siewior Cc: x86, Ingo Molnar, linux-kernel, Andy Lutomirski, Paolo Bonzini, Radim Krčmář, kvm, Jason A. Donenfeld, Rik van Riel, Dave Hansen On 11/19/18 9:27 AM, Borislav Petkov wrote: >>> I was really hoping for code comments. :) >> I though we agreed to make those in the larger series because those >> comments in __fpu__restore_sig() would be removed anyway (as part of the >> series). > Also, over local_bh_disable() does not really fit as this is generic > code and Sebastian said ARM does the whole thing a bit differently, for > one. > > We probably should put that comment somewhere prominent in > arch/x86/kernel/fpu/core.c or similar - somewhere people poking at FPU > stuff will see it. Ahh, got it. arch/x86/kernel/fpu/core.c would be a fine spot. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] x86/fpu: Disable BH while while loading FPU registers in __fpu__restore_sig() 2018-11-19 16:04 [PATCH v2] x86/fpu: Disable BH while while loading FPU registers in __fpu__restore_sig() Sebastian Andrzej Siewior 2018-11-19 17:02 ` Dave Hansen @ 2018-11-19 18:10 ` Borislav Petkov 2018-11-20 10:26 ` [PATCH v3] " Sebastian Andrzej Siewior [not found] ` <20181120132531.6E80C206BB@mail.kernel.org> 2 siblings, 1 reply; 17+ messages in thread From: Borislav Petkov @ 2018-11-19 18:10 UTC (permalink / raw) To: Sebastian Andrzej Siewior Cc: x86, Ingo Molnar, linux-kernel, Andy Lutomirski, Paolo Bonzini, Radim Krčmář, kvm, Jason A. Donenfeld, Rik van Riel, Dave Hansen On Mon, Nov 19, 2018 at 05:04:10PM +0100, Sebastian Andrzej Siewior wrote: > The sequence > fpu->initialized = 1; /* step A */ > preempt_disable(); /* step B */ > fpu__restore(fpu); > preempt_enable(); > > is racy in regard to a context switch. > For 32bit frames __fpu__restore_sig() prepares the FPU state within > fpu->state. To ensure that a context switch (switch_fpu_prepare() in > particular) does not modify fpu->state it uses fpu__drop() which sets > fpu->initializes to 0. "... ->initialized to 0." Also, a new line here pls. > With this change the CPU's FPU state is not saved ^ comma: , Also, instead of "with this change" I think you mean: "After ->initialized is cleared, the CPU's FPU state..." > to fpu->state during a context switch. > It then loads the state to fpu->state from userland and ensures it > sane. "... and ensures it is sane." > The new state is loaded via fpu__restore(). The code sets then > fpu->initializes to 1 in order to avoid fpu__initialize() doing fpu->initialized > anything (overwrite the new state) which is part of fpu__restore(). <---- newline here. > A context switch between step A and B would save CPU's current FPU > registers to fpu->state and overwrite the newly prepared state. This > looks like tiny race window but the Kernel Test Robot reported this back > in 2016 while we had lazy FPU support. Borislav Petkov made the link > between that report and another patch that has been posted. > Since the removal of the lazy FPU support, this race goes unnoticed > because the warning has been removed. > > Use local_bh_disable() around the restore sequence to avoid the race. BH Let's write it out once: "Bottom halves need to be... " > needs to be disabled because BH is allowed to run (even with preemption > disabled) and might invoke kernel_fpu_begin(). ... and let's put the potential example here with IPsec and softirq. > Link: https://lkml.kernel.org/r/20160226074940.GA28911@pd.tnic > Cc: stable@vger.kernel.org > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > --- > v1…v2: A more verbose commit as message. Very much needed, thanks! > arch/x86/kernel/fpu/signal.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c > index 61a949d84dfa5..d99a8ee9e185e 100644 > --- a/arch/x86/kernel/fpu/signal.c > +++ b/arch/x86/kernel/fpu/signal.c > @@ -344,10 +344,10 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size) > sanitize_restored_xstate(tsk, &env, xfeatures, fx_only); > } > > + local_bh_disable(); > fpu->initialized = 1; > - preempt_disable(); > fpu__restore(fpu); > - preempt_enable(); > + local_bh_enable(); > > return err; > } else { > -- -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v3] x86/fpu: Disable BH while while loading FPU registers in __fpu__restore_sig() 2018-11-19 18:10 ` Borislav Petkov @ 2018-11-20 10:26 ` Sebastian Andrzej Siewior 0 siblings, 0 replies; 17+ messages in thread From: Sebastian Andrzej Siewior @ 2018-11-20 10:26 UTC (permalink / raw) To: Borislav Petkov Cc: x86, Ingo Molnar, linux-kernel, Andy Lutomirski, Paolo Bonzini, Radim Krčmář, kvm, Jason A. Donenfeld, Rik van Riel, Dave Hansen The sequence fpu->initialized = 1; /* step A */ preempt_disable(); /* step B */ fpu__restore(fpu); preempt_enable(); is racy in regard to a context switch. For 32bit frames __fpu__restore_sig() prepares the FPU state within fpu->state. To ensure that a context switch (switch_fpu_prepare() in particular) does not modify fpu->state it uses fpu__drop() which sets fpu->initialized to 0. After fpu->initialized is cleared, the CPU's FPU state is not saved to fpu->state during a context switch. It then loads the new state to fpu->state from userland and ensures it is sane. The new state is loaded via fpu__restore(). The code sets then fpu->initializes to 1 in order to avoid fpu__initialize() doing anything (overwrite the new state) which is part of fpu__restore(). A context switch between step A and B would save CPU's current FPU registers to fpu->state and overwrite the newly prepared state. This looks like tiny race window but the Kernel Test Robot reported this back in 2016 while we had lazy FPU support. Borislav Petkov made the link between that report and another patch that has been posted. Since the removal of the lazy FPU support, this race goes unnoticed because the warning has been removed. Disable bottom halfes around the restore sequence to avoid the race. BH needs to be disabled because BH is allowed to run (even with preemption disabled) and might invoke kernel_fpu_begin() by doing IPsec. Link: https://lkml.kernel.org/r/20160226074940.GA28911@pd.tnic Cc: stable@vger.kernel.org Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> --- v2…v3: Rewording parts of the commit message as per Borislav Petkov. v1…v2: A more verbose commit as message. arch/x86/kernel/fpu/signal.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c index 61a949d84dfa5..d99a8ee9e185e 100644 --- a/arch/x86/kernel/fpu/signal.c +++ b/arch/x86/kernel/fpu/signal.c @@ -344,10 +344,10 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size) sanitize_restored_xstate(tsk, &env, xfeatures, fx_only); } + local_bh_disable(); fpu->initialized = 1; - preempt_disable(); fpu__restore(fpu); - preempt_enable(); + local_bh_enable(); return err; } else { -- 2.19.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
[parent not found: <20181120132531.6E80C206BB@mail.kernel.org>]
* Re: [PATCH v2] x86/fpu: Disable BH while while loading FPU registers in __fpu__restore_sig() [not found] ` <20181120132531.6E80C206BB@mail.kernel.org> @ 2018-11-20 18:34 ` Borislav Petkov [not found] ` <CAMU5Q=72mNG4ikXKpLK3=SiDK3dRXtCw7wpHZ2i==awrtvRwdg@mail.gmail.com> 0 siblings, 1 reply; 17+ messages in thread From: Borislav Petkov @ 2018-11-20 18:34 UTC (permalink / raw) To: Sasha Levin, Victoria Anosova Cc: Sebastian Andrzej Siewior, x86, Ingo Molnar, linux-kernel, stable On Tue, Nov 20, 2018 at 01:25:30PM +0000, Sasha Levin wrote: > Hi, > > [This is an automated email] > > This commit has been processed because it contains a -stable tag. > The stable tag indicates that it's relevant for the following trees: all > > The bot has tested the following trees: v4.19.2, v4.18.19, v4.14.81, v4.9.137, v4.4.163, v3.18.125. > > v4.19.2: Build OK! > v4.18.19: Build OK! > v4.14.81: Build OK! > v4.9.137: Failed to apply! Possible dependencies: > Unable to calculate AFAIR, Victoria asked about this fix a while ago. CCed. Victoria, do you still need this patch in your 4.9? If so, you might wanna give it a try: https://git.kernel.org/tip/68239654acafe6aad5a3c1dc7237e60accfebc03 Let us know if you need help backporting - it should be easy though. HTH. -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply. ^ permalink raw reply [flat|nested] 17+ messages in thread
[parent not found: <CAMU5Q=72mNG4ikXKpLK3=SiDK3dRXtCw7wpHZ2i==awrtvRwdg@mail.gmail.com>]
* Re: [PATCH v2] x86/fpu: Disable BH while while loading FPU registers in __fpu__restore_sig() [not found] ` <CAMU5Q=72mNG4ikXKpLK3=SiDK3dRXtCw7wpHZ2i==awrtvRwdg@mail.gmail.com> @ 2018-11-21 10:56 ` Borislav Petkov [not found] ` <CAMU5Q=5LkypghWA=23YwscaXSx5yBv7LyW1WHbY_J_hQwryNpA@mail.gmail.com> 0 siblings, 1 reply; 17+ messages in thread From: Borislav Petkov @ 2018-11-21 10:56 UTC (permalink / raw) To: Victoria Anosova; +Cc: sashal, bigeasy, x86, mingo, linux-kernel, stable On Wed, Nov 21, 2018 at 11:11:18AM +0500, Victoria Anosova wrote: > Glad this come to kernel. We've already applied this patch. The current version, with the bottom half toggling or the original one with preempt_disable/enable? -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply. ^ permalink raw reply [flat|nested] 17+ messages in thread
[parent not found: <CAMU5Q=5LkypghWA=23YwscaXSx5yBv7LyW1WHbY_J_hQwryNpA@mail.gmail.com>]
* Re: [PATCH v2] x86/fpu: Disable BH while while loading FPU registers in __fpu__restore_sig() [not found] ` <CAMU5Q=5LkypghWA=23YwscaXSx5yBv7LyW1WHbY_J_hQwryNpA@mail.gmail.com> @ 2018-11-21 12:54 ` Boris Petkov 0 siblings, 0 replies; 17+ messages in thread From: Boris Petkov @ 2018-11-21 12:54 UTC (permalink / raw) To: Victoria Anosova; +Cc: sashal, bigeasy, x86, mingo, linux-kernel, stable On November 21, 2018 1:41:37 PM GMT+01:00, Victoria Anosova <v.s.anosova@gmail.com> wrote: >For v4.9 your first fix ( >https://lists.openwall.net/linux-kernel/2016/02/26/299) helped. Can you please not top-post? Thx. That old version is not quite right - see the commit message of the current fix. HTH. -- Sent from a small device: formatting sux and brevity is inevitable. ^ permalink raw reply [flat|nested] 17+ messages in thread
[parent not found: <87d1rk9str.fsf@yhuang-dev.intel.com>]
* Re: [lkp] [x86/fpu] 58122bf1d8: WARNING: CPU: 0 PID: 1 at arch/x86/include/asm/fpu/internal.h:529 fpu__restore+0x28f/0x9ab() [not found] <87d1rk9str.fsf@yhuang-dev.intel.com> @ 2016-02-26 7:49 ` Borislav Petkov 2016-02-27 12:02 ` Ingo Molnar ` (2 more replies) 0 siblings, 3 replies; 17+ messages in thread From: Borislav Petkov @ 2016-02-26 7:49 UTC (permalink / raw) To: kernel test robot Cc: Andy Lutomirski, lkp, LKML, yu-cheng yu, Thomas Gleixner, Sai Praneeth Prakhya, Rik van Riel, Quentin Casasnovas, Peter Zijlstra, Oleg Nesterov, Linus Torvalds, H. Peter Anvin, Fenghua Yu, Dave Hansen, Andy Lutomirski, Ingo Molnar On Fri, Feb 26, 2016 at 09:13:52AM +0800, kernel test robot wrote: > FYI, we noticed the below changes on > > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master > commit 58122bf1d856a4ea9581d62a07c557d997d46a19 ("x86/fpu: Default eagerfpu=on on all CPUs") Oh cool, so your bisection results point at Ingo's initial suspicion which I couldn't confirm with mine. > [ 17.097301] WARNING: CPU: 0 PID: 1 at arch/x86/include/asm/fpu/internal.h:529 fpu__restore+0x28f/0x9ab() > [ 17.099191] CPU: 0 PID: 1 Comm: init Not tainted 4.5.0-rc3-00015-g58122bf #1 > [ 17.100373] ffff88000ae17c38 ffffffff85405ca8 0000000000000002 0000000000000020 > [ 17.101747] 0000000000000001 0000000000000000 ffff88000ae17c70 ffffffff818f024e > [ 17.103110] ffff88000ae17cb0 ffffffff81138526 0000000900000000 ffff88000ae113c0 > [ 17.104543] Call Trace: > [ 17.104980] [<ffffffff818f024e>] dump_stack+0x19/0x1b > [ 17.105854] [<ffffffff81138526>] warn_slowpath_common+0x1a5/0x1c0 > [ 17.106895] [<ffffffff81138613>] warn_slowpath_null+0x1a/0x1c > [ 17.107904] [<ffffffff810362bd>] fpu__restore+0x28f/0x9ab > [ 17.108834] [<ffffffff8103ac01>] __fpu__restore_sig+0xc3e/0x1a3a > [ 17.109862] [<ffffffff81003058>] ? ___preempt_schedule+0x12/0x14 > [ 17.110916] [<ffffffff8103c645>] fpu__restore_sig+0xf5/0x102 > [ 17.111887] [<ffffffff81123b73>] ia32_restore_sigcontext+0x586/0x5af > [ 17.112987] [<ffffffff811244fb>] sys32_sigreturn+0x246/0x317 > [ 17.113956] [<ffffffff811242b5>] ? get_sigframe+0x719/0x719 > [ 17.115108] [<ffffffff810076ef>] do_syscall_32_irqs_off+0x2b2/0x789 > [ 17.116179] [<ffffffff82a43528>] entry_INT80_compat+0x38/0x50 > [ 17.117161] ---[ end trace cb640c01126c054b ]--- ...also cool, 32-bit process on a 64-bit kernel. I triggered it once on a 32-bit kernel. Does this one help? --- From: Borislav Petkov <bp@suse.de> Date: Mon, 15 Feb 2016 19:50:33 +0100 Subject: [PATCH] x86/FPU: Fix double FPU regs activation sys_sigreturn() calls fpu__restore_sig() with interrupts enabled. When restoring a 32-bit signal frame. And it can happen that we get preempted right after setting ->fpstate_active in a task's FPU. After we get preempted, we switch between tasks merrily and eventually are about to switch to that task above whose ->fpstate_active we set. We enter __switch_to() and do switch_fpu_prepare(). Our task gets ->fpregs_active set, we find ourselves back on the call stack below and especially in __fpu__restore_sig() which sets ->fpregs_active again. Leading to that whoops below. So let's enlarge the preemption-off region so that we set ->fpstate_active with preemption disabled and thus not trigger fpu.preload: switch_fpu_prepare ... fpu.preload = static_cpu_has(X86_FEATURE_FPU) && new_fpu->fpstate_active && ^^^^^^^^^^^^^^^^^^^^^^ prematurely. WARNING: CPU: 0 PID: 3031 at ./arch/x86/include/asm/fpu/internal.h:530 fpu__restore+0x90/0x130() Modules linked in: ... CPU: 0 PID: 3031 Comm: bash Not tainted 4.5.0-rc3+ #1 ... Call Trace: dump_stack warn_slowpath_common ? fpu__restore ? fpu__restore warn_slowpath_null fpu__restore __fpu__restore_sig fpu__restore_sig restore_sigcontext sys_sigreturn do_syscall_32_irqs_on entry_INT80_32 Suggested-by: Andy Lutomirski <luto@amacapital.net> Signed-off-by: Borislav Petkov <bp@suse.de> --- arch/x86/kernel/fpu/signal.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c index 31c6a60505e6..408e5a1c6fdd 100644 --- a/arch/x86/kernel/fpu/signal.c +++ b/arch/x86/kernel/fpu/signal.c @@ -316,12 +316,11 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size) sanitize_restored_xstate(tsk, &env, xfeatures, fx_only); } + preempt_disable(); fpu->fpstate_active = 1; - if (use_eager_fpu()) { - preempt_disable(); + if (use_eager_fpu()) fpu__restore(fpu); - preempt_enable(); - } + preempt_enable(); return err; } else { -- 2.3.5 -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [lkp] [x86/fpu] 58122bf1d8: WARNING: CPU: 0 PID: 1 at arch/x86/include/asm/fpu/internal.h:529 fpu__restore+0x28f/0x9ab() 2016-02-26 7:49 ` [lkp] [x86/fpu] 58122bf1d8: WARNING: CPU: 0 PID: 1 at arch/x86/include/asm/fpu/internal.h:529 fpu__restore+0x28f/0x9ab() Borislav Petkov @ 2016-02-27 12:02 ` Ingo Molnar 2016-02-27 13:13 ` Borislav Petkov 2018-11-20 12:05 ` [tip:x86/urgent] x86/fpu: Disable bottom halves while loading FPU registers tip-bot for Sebastian Andrzej Siewior 2018-11-20 16:29 ` tip-bot for Sebastian Andrzej Siewior 2 siblings, 1 reply; 17+ messages in thread From: Ingo Molnar @ 2016-02-27 12:02 UTC (permalink / raw) To: Borislav Petkov Cc: kernel test robot, Andy Lutomirski, lkp, LKML, yu-cheng yu, Thomas Gleixner, Sai Praneeth Prakhya, Rik van Riel, Quentin Casasnovas, Peter Zijlstra, Oleg Nesterov, Linus Torvalds, H. Peter Anvin, Fenghua Yu, Dave Hansen, Andy Lutomirski * Borislav Petkov <bp@alien8.de> wrote: > From: Borislav Petkov <bp@suse.de> > Date: Mon, 15 Feb 2016 19:50:33 +0100 > Subject: [PATCH] x86/FPU: Fix double FPU regs activation > > sys_sigreturn() calls fpu__restore_sig() with interrupts enabled. When > restoring a 32-bit signal frame. And it can happen that we get preempted > right after setting ->fpstate_active in a task's FPU. > > After we get preempted, we switch between tasks merrily and eventually > are about to switch to that task above whose ->fpstate_active we > set. We enter __switch_to() and do switch_fpu_prepare(). Our task gets > ->fpregs_active set, we find ourselves back on the call stack below and > especially in __fpu__restore_sig() which sets ->fpregs_active again. > > Leading to that whoops below. > > So let's enlarge the preemption-off region so that we set > ->fpstate_active with preemption disabled and thus not trigger > fpu.preload: > > switch_fpu_prepare > > ... > > fpu.preload = static_cpu_has(X86_FEATURE_FPU) && > new_fpu->fpstate_active && > ^^^^^^^^^^^^^^^^^^^^^^ > > prematurely. So I'm wondering, why did this commit: 58122bf1d856 x86/fpu: Default eagerfpu=on on all CPUs trigger the warning, while it never triggered on CPUs that were already eagerfpu=on for years? There must be something we are still missing I think. Thanks, Ingo ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [lkp] [x86/fpu] 58122bf1d8: WARNING: CPU: 0 PID: 1 at arch/x86/include/asm/fpu/internal.h:529 fpu__restore+0x28f/0x9ab() 2016-02-27 12:02 ` Ingo Molnar @ 2016-02-27 13:13 ` Borislav Petkov 0 siblings, 0 replies; 17+ messages in thread From: Borislav Petkov @ 2016-02-27 13:13 UTC (permalink / raw) To: Ingo Molnar Cc: kernel test robot, Andy Lutomirski, lkp, LKML, yu-cheng yu, Thomas Gleixner, Sai Praneeth Prakhya, Rik van Riel, Quentin Casasnovas, Peter Zijlstra, Oleg Nesterov, Linus Torvalds, H. Peter Anvin, Fenghua Yu, Dave Hansen, Andy Lutomirski On Sat, Feb 27, 2016 at 01:02:11PM +0100, Ingo Molnar wrote: > So I'm wondering, why did this commit: > > 58122bf1d856 x86/fpu: Default eagerfpu=on on all CPUs > Hmm, so looking at switch_fpu_prepare(): /* * If the task has used the math, pre-load the FPU on xsave processors * or if the past 5 consecutive context-switches used math. */ fpu.preload = static_cpu_has(X86_FEATURE_FPU) && new_fpu->fpstate_active && (use_eager_fpu() || new_fpu->counter > 5); ^^^^^^^^^^^^^^ and later: if (old_fpu->fpregs_active) { ... /* Don't change CR0.TS if we just switch! */ if (fpu.preload) { ... __fpregs_activate(new_fpu); so I can see a possible link between 58122bf1d856 and what we're seeing. But as I've told you offlist, I couldn't confirm that this commit was the culprit due to my simulated reproducer. So I'm thinking the 0day guys have a more reliable one. > trigger the warning, while it never triggered on CPUs that were already > eagerfpu=on for years? That I can't explain... yet. FWIW, the one time splat I saw, happened on an IVB machine on 32-bit which has always been eagerfpu=on. > There must be something we are still missing I think. Yeah. -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [tip:x86/urgent] x86/fpu: Disable bottom halves while loading FPU registers 2016-02-26 7:49 ` [lkp] [x86/fpu] 58122bf1d8: WARNING: CPU: 0 PID: 1 at arch/x86/include/asm/fpu/internal.h:529 fpu__restore+0x28f/0x9ab() Borislav Petkov 2016-02-27 12:02 ` Ingo Molnar @ 2018-11-20 12:05 ` tip-bot for Sebastian Andrzej Siewior 2018-11-20 16:29 ` tip-bot for Sebastian Andrzej Siewior 2 siblings, 0 replies; 17+ messages in thread From: tip-bot for Sebastian Andrzej Siewior @ 2018-11-20 12:05 UTC (permalink / raw) To: linux-tip-commits Cc: x86, luto, mingo, hpa, tglx, bp, Jason, dave.hansen, riel, linux-kernel, kvm, pbonzini, rkrcmar, bigeasy Commit-ID: 265fb8fe4c69a94431a17e8b87a9613d30fefe79 Gitweb: https://git.kernel.org/tip/265fb8fe4c69a94431a17e8b87a9613d30fefe79 Author: Sebastian Andrzej Siewior <bigeasy@linutronix.de> AuthorDate: Tue, 20 Nov 2018 11:26:35 +0100 Committer: Borislav Petkov <bp@suse.de> CommitDate: Tue, 20 Nov 2018 13:01:32 +0100 x86/fpu: Disable bottom halves while loading FPU registers The sequence fpu->initialized = 1; /* step A */ preempt_disable(); /* step B */ fpu__restore(fpu); preempt_enable(); in __fpu__restore_sig() is racy in regard to a context switch. For 32bit frames, __fpu__restore_sig() prepares the FPU state within fpu->state. To ensure that a context switch (switch_fpu_prepare() in particular) does not modify fpu->state it uses fpu__drop() which sets fpu->initialized to 0. After fpu->initialized is cleared, the CPU's FPU state is not saved to fpu->state during a context switch. The new state is loaded via fpu__restore(). It gets loaded into fpu->state from userland and ensured it is sane. fpu->initialized is then set to 1 in order to avoid fpu__initialize() doing anything (overwrite the new state) which is part of fpu__restore(). A context switch between step A and B above would save CPU's current FPU registers to fpu->state and overwrite the newly prepared state. This looks like a tiny race window but the Kernel Test Robot reported this back in 2016 while we had lazy FPU support. Borislav Petkov made the link between that report and another patch that has been posted. Since the removal of the lazy FPU support, this race goes unnoticed because the warning has been removed. Disable bottom halves around the restore sequence to avoid the race. BH need to be disabled because BH is allowed to run (even with preemption disabled) and might invoke kernel_fpu_begin() by doing IPsec. [ bp: massage commit message a bit. ] Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Signed-off-by: Borislav Petkov <bp@suse.de> Acked-by: Ingo Molnar <mingo@kernel.org> Acked-by: Thomas Gleixner <tglx@linutronix.de> CC: Andy Lutomirski <luto@kernel.org> CC: Dave Hansen <dave.hansen@linux.intel.com> CC: "H. Peter Anvin" <hpa@zytor.com> CC: "Jason A. Donenfeld" <Jason@zx2c4.com> CC: kvm ML <kvm@vger.kernel.org> CC: Paolo Bonzini <pbonzini@redhat.com> CC: Radim Krčmář <rkrcmar@redhat.com> CC: Rik van Riel <riel@surriel.com> Cc: stable@vger.kernel.org CC: x86-ml <x86@kernel.org> Link: http://lkml.kernel.org/r/20181120102635.ddv3fvavxajjlfqk@linutronix.de Link: https://lkml.kernel.org/r/20160226074940.GA28911@pd.tnic --- arch/x86/kernel/fpu/signal.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c index 61a949d84dfa..d99a8ee9e185 100644 --- a/arch/x86/kernel/fpu/signal.c +++ b/arch/x86/kernel/fpu/signal.c @@ -344,10 +344,10 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size) sanitize_restored_xstate(tsk, &env, xfeatures, fx_only); } + local_bh_disable(); fpu->initialized = 1; - preempt_disable(); fpu__restore(fpu); - preempt_enable(); + local_bh_enable(); return err; } else { ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [tip:x86/urgent] x86/fpu: Disable bottom halves while loading FPU registers 2016-02-26 7:49 ` [lkp] [x86/fpu] 58122bf1d8: WARNING: CPU: 0 PID: 1 at arch/x86/include/asm/fpu/internal.h:529 fpu__restore+0x28f/0x9ab() Borislav Petkov 2016-02-27 12:02 ` Ingo Molnar 2018-11-20 12:05 ` [tip:x86/urgent] x86/fpu: Disable bottom halves while loading FPU registers tip-bot for Sebastian Andrzej Siewior @ 2018-11-20 16:29 ` tip-bot for Sebastian Andrzej Siewior 2 siblings, 0 replies; 17+ messages in thread From: tip-bot for Sebastian Andrzej Siewior @ 2018-11-20 16:29 UTC (permalink / raw) To: linux-tip-commits Cc: dave.hansen, Jason, riel, pbonzini, x86, hpa, mingo, kvm, bigeasy, bp, tglx, rkrcmar, luto, linux-kernel Commit-ID: 68239654acafe6aad5a3c1dc7237e60accfebc03 Gitweb: https://git.kernel.org/tip/68239654acafe6aad5a3c1dc7237e60accfebc03 Author: Sebastian Andrzej Siewior <bigeasy@linutronix.de> AuthorDate: Tue, 20 Nov 2018 11:26:35 +0100 Committer: Borislav Petkov <bp@suse.de> CommitDate: Tue, 20 Nov 2018 17:22:42 +0100 x86/fpu: Disable bottom halves while loading FPU registers The sequence fpu->initialized = 1; /* step A */ preempt_disable(); /* step B */ fpu__restore(fpu); preempt_enable(); in __fpu__restore_sig() is racy in regard to a context switch. For 32bit frames, __fpu__restore_sig() prepares the FPU state within fpu->state. To ensure that a context switch (switch_fpu_prepare() in particular) does not modify fpu->state it uses fpu__drop() which sets fpu->initialized to 0. After fpu->initialized is cleared, the CPU's FPU state is not saved to fpu->state during a context switch. The new state is loaded via fpu__restore(). It gets loaded into fpu->state from userland and ensured it is sane. fpu->initialized is then set to 1 in order to avoid fpu__initialize() doing anything (overwrite the new state) which is part of fpu__restore(). A context switch between step A and B above would save CPU's current FPU registers to fpu->state and overwrite the newly prepared state. This looks like a tiny race window but the Kernel Test Robot reported this back in 2016 while we had lazy FPU support. Borislav Petkov made the link between that report and another patch that has been posted. Since the removal of the lazy FPU support, this race goes unnoticed because the warning has been removed. Disable bottom halves around the restore sequence to avoid the race. BH need to be disabled because BH is allowed to run (even with preemption disabled) and might invoke kernel_fpu_begin() by doing IPsec. [ bp: massage commit message a bit. ] Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> Signed-off-by: Borislav Petkov <bp@suse.de> Acked-by: Ingo Molnar <mingo@kernel.org> Acked-by: Thomas Gleixner <tglx@linutronix.de> Cc: Andy Lutomirski <luto@kernel.org> Cc: Dave Hansen <dave.hansen@linux.intel.com> Cc: "H. Peter Anvin" <hpa@zytor.com> Cc: "Jason A. Donenfeld" <Jason@zx2c4.com> Cc: kvm ML <kvm@vger.kernel.org> Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: Radim Krčmář <rkrcmar@redhat.com> Cc: Rik van Riel <riel@surriel.com> Cc: stable@vger.kernel.org Cc: x86-ml <x86@kernel.org> Link: http://lkml.kernel.org/r/20181120102635.ddv3fvavxajjlfqk@linutronix.de Link: https://lkml.kernel.org/r/20160226074940.GA28911@pd.tnic --- arch/x86/kernel/fpu/signal.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c index 61a949d84dfa..d99a8ee9e185 100644 --- a/arch/x86/kernel/fpu/signal.c +++ b/arch/x86/kernel/fpu/signal.c @@ -344,10 +344,10 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size) sanitize_restored_xstate(tsk, &env, xfeatures, fx_only); } + local_bh_disable(); fpu->initialized = 1; - preempt_disable(); fpu__restore(fpu); - preempt_enable(); + local_bh_enable(); return err; } else { ^ permalink raw reply related [flat|nested] 17+ messages in thread
end of thread, other threads:[~2018-11-21 12:54 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-11-19 16:04 [PATCH v2] x86/fpu: Disable BH while while loading FPU registers in __fpu__restore_sig() Sebastian Andrzej Siewior 2018-11-19 17:02 ` Dave Hansen 2018-11-19 17:11 ` Sebastian Andrzej Siewior 2018-11-19 17:27 ` Borislav Petkov 2018-11-19 17:31 ` Sebastian Andrzej Siewior 2018-11-19 17:41 ` Borislav Petkov 2018-11-19 17:32 ` Dave Hansen 2018-11-19 18:10 ` Borislav Petkov 2018-11-20 10:26 ` [PATCH v3] " Sebastian Andrzej Siewior [not found] ` <20181120132531.6E80C206BB@mail.kernel.org> 2018-11-20 18:34 ` [PATCH v2] " Borislav Petkov [not found] ` <CAMU5Q=72mNG4ikXKpLK3=SiDK3dRXtCw7wpHZ2i==awrtvRwdg@mail.gmail.com> 2018-11-21 10:56 ` Borislav Petkov [not found] ` <CAMU5Q=5LkypghWA=23YwscaXSx5yBv7LyW1WHbY_J_hQwryNpA@mail.gmail.com> 2018-11-21 12:54 ` Boris Petkov [not found] <87d1rk9str.fsf@yhuang-dev.intel.com> 2016-02-26 7:49 ` [lkp] [x86/fpu] 58122bf1d8: WARNING: CPU: 0 PID: 1 at arch/x86/include/asm/fpu/internal.h:529 fpu__restore+0x28f/0x9ab() Borislav Petkov 2016-02-27 12:02 ` Ingo Molnar 2016-02-27 13:13 ` Borislav Petkov 2018-11-20 12:05 ` [tip:x86/urgent] x86/fpu: Disable bottom halves while loading FPU registers tip-bot for Sebastian Andrzej Siewior 2018-11-20 16:29 ` tip-bot for Sebastian Andrzej Siewior
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).