LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Ingo Molnar <mingo@kernel.org>
To: Oleg Nesterov <oleg@redhat.com>
Cc: Dave Hansen <dave.hansen@intel.com>, Borislav Petkov <bp@suse.de>,
	Andy Lutomirski <luto@amacapital.net>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Pekka Riikonen <priikone@iki.fi>, Rik van Riel <riel@redhat.com>,
	Suresh Siddha <sbsiddha@gmail.com>,
	LKML <linux-kernel@vger.kernel.org>,
	"Yu, Fenghua" <fenghua.yu@intel.com>,
	Quentin Casasnovas <quentin.casasnovas@oracle.com>
Subject: Re: [PATCH 1/1] x86/fpu: math_state_restore() should not blindly disable irqs
Date: Fri, 6 Mar 2015 08:58:34 +0100	[thread overview]
Message-ID: <20150306075833.GA623@gmail.com> (raw)
In-Reply-To: <20150305212532.GA16890@redhat.com>


* Oleg Nesterov <oleg@redhat.com> wrote:

> On 03/05, Ingo Molnar wrote:
> >
> > * Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > > --- a/arch/x86/kernel/traps.c
> > > +++ b/arch/x86/kernel/traps.c
> > > @@ -774,7 +774,10 @@ void math_state_restore(void)
> > >  	struct task_struct *tsk = current;
> > >
> > >  	if (!tsk_used_math(tsk)) {
> > > -		local_irq_enable();
> > > +		bool disabled = irqs_disabled();
> > > +
> > > +		if (disabled)
> > > +			local_irq_enable();
> > >  		/*
> > >  		 * does a slab alloc which can sleep
> > >  		 */
> > > @@ -785,7 +788,9 @@ void math_state_restore(void)
> > >  			do_group_exit(SIGKILL);
> > >  			return;
> > >  		}
> > > -		local_irq_disable();
> > > +
> > > +		if (disabled)
> > > +			local_irq_disable();
> > >  	}
> >
> > Yuck!
> >
> > Is there a fundamental reason why we cannot simply enable irqs and
> > leave them enabled? Math state restore is not atomic and cannot really
> > be atomic.
> 
> You know, I didn't even try to verify ;) but see below.

So I'm thinking about the attached patch.

> Most probably we can simply enable irqs, yes. But what about older 
> kernels, how can we check?
>
> And let me repeat, I strongly believe that this !tsk_used_math() 
> case in math_state_restore() must die. And unlazy_fpu() in 
> init_fpu(). And both __restore_xstate_sig() and flush_thread() 
> should not use math_state_restore() at all. At least in its current 
> form.

Agreed.

> But this is obviously not -stable material.
> 
> That said, I'll try to look into git history tomorrow.

So I think the reasons are:

 - historic: because math_state_restore() started out as an interrupt 
   routine (from the IRQ13 days)

 - hardware imposed: the handler is executed with irqs off

 - it's probably the fastest implementation: we just run with the 
   natural irqs-off state the handler executes with.

So there's nothing outright wrong about executing with irqs off in a 
trap handler.

> [...] The patch above looks "obviously safe", but perhaps I am 
> paranoid too much...

IMHO your hack above isn't really acceptable, even for a backport.
So lets test the patch below (assuming it's the right thing to do)
and move forward?

Thanks,

	Ingo

======================>
From: Ingo Molnar <mingo@kernel.org>
Date: Fri, 6 Mar 2015 08:37:57 +0100
Subject: [PATCH] x86/fpu: Don't disable irqs in math_state_restore()

math_state_restore() was historically called with irqs disabled, 
because that's how the hardware generates the trap, and also because 
back in the days it was possible for it to be an asynchronous 
interrupt and interrupt handlers run with irqs off.

These days it's always an instruction trap, and furthermore it does 
inevitably complex things such as memory allocation and signal 
processing, which is not done with irqs disabled.

So keep irqs enabled.

This might surprise in-kernel FPU users that somehow relied on
interrupts being disabled across FPU usage - but that's
fundamentally fragile anyway due to the inatomicity of FPU state
restores. The trap return will restore interrupts to its previous 
state, but if FPU ops trigger math_state_restore() there's no
guarantee of atomicity anymore.

To warn about in-kernel irqs-off users of FPU state we might want to 
pass 'struct pt_regs' to math_state_restore() and check the trapped 
state for irqs disabled (flags has IF cleared) and kernel context - 
but that's for a later patch.

Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Quentin Casasnovas <quentin.casasnovas@oracle.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/traps.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 950815a138e1..52f9e4057cee 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -844,8 +844,9 @@ void math_state_restore(void)
 {
 	struct task_struct *tsk = current;
 
+	local_irq_enable();
+
 	if (!tsk_used_math(tsk)) {
-		local_irq_enable();
 		/*
 		 * does a slab alloc which can sleep
 		 */
@@ -856,7 +857,6 @@ void math_state_restore(void)
 			do_group_exit(SIGKILL);
 			return;
 		}
-		local_irq_disable();
 	}
 
 	/* Avoid __kernel_fpu_begin() right after __thread_fpu_begin() */

  reply	other threads:[~2015-03-06  7:58 UTC|newest]

Thread overview: 126+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-03-04 18:30 Oops with tip/x86/fpu Dave Hansen
2015-03-04 19:06 ` Oleg Nesterov
2015-03-04 19:12   ` Dave Hansen
2015-03-04 20:06   ` Borislav Petkov
2015-03-05 15:14     ` Oleg Nesterov
     [not found]       ` <20150305182203.GA4203@redhat.com>
2015-03-05 18:34         ` Dave Hansen
2015-03-05 18:46           ` Oleg Nesterov
2015-03-05 18:41         ` Dave Hansen
2015-03-26 22:37         ` Yu, Fenghua
2015-03-26 22:43           ` Dave Hansen
2015-03-26 22:48             ` Yu, Fenghua
2015-03-27  7:30               ` Quentin Casasnovas
2015-03-27 19:06           ` Oleg Nesterov
2015-03-05  8:38   ` Quentin Casasnovas
2015-03-05 15:13     ` Oleg Nesterov
2015-03-05 18:42       ` Borislav Petkov
2015-03-05 22:16         ` Dave Hansen
2015-03-05 19:51 ` [PATCH 0/1] x86/fpu: math_state_restore() should not blindly disable irqs Oleg Nesterov
2015-03-05 19:51   ` [PATCH 1/1] " Oleg Nesterov
2015-03-05 20:11     ` Ingo Molnar
2015-03-05 21:25       ` Oleg Nesterov
2015-03-06  7:58         ` Ingo Molnar [this message]
2015-03-06 13:26           ` Oleg Nesterov
2015-03-06 13:39             ` Oleg Nesterov
2015-03-06 13:46             ` Ingo Molnar
2015-03-06 14:01               ` Oleg Nesterov
2015-03-06 14:17                 ` Oleg Nesterov
2015-03-06 15:00                 ` David Vrabel
2015-03-06 15:36                   ` Oleg Nesterov
2015-03-06 16:15                     ` David Vrabel
2015-03-06 16:31                       ` Oleg Nesterov
2015-03-06 17:33           ` Linus Torvalds
2015-03-06 18:15             ` Oleg Nesterov
2015-03-06 19:23             ` Andy Lutomirski
2015-03-06 22:00               ` Linus Torvalds
2015-03-06 22:28                 ` Andy Lutomirski
2015-03-07 10:36                   ` Ingo Molnar
2015-03-07 20:11                     ` Linus Torvalds
2015-03-08  8:55                       ` Ingo Molnar
2015-03-08 11:38                         ` Ingo Molnar
2015-03-08 13:59                         ` Andy Lutomirski
2015-03-08 14:38                           ` Andy Lutomirski
2015-03-07 10:32             ` Ingo Molnar
2015-03-07 15:38   ` [PATCH 0/1] x86/fpu: x86/fpu: avoid math_state_restore() without used_math() in __restore_xstate_sig() Oleg Nesterov
2015-03-07 15:38     ` [PATCH 1/1] " Oleg Nesterov
2015-03-09 14:07       ` Borislav Petkov
2015-03-09 14:34         ` Oleg Nesterov
2015-03-09 15:18           ` Borislav Petkov
2015-03-09 16:24             ` Oleg Nesterov
2015-03-09 16:53               ` Borislav Petkov
2015-03-09 17:05                 ` Oleg Nesterov
2015-03-09 17:23                   ` Borislav Petkov
2015-03-16 12:07       ` [tip:x86/urgent] x86/fpu: Avoid " tip-bot for Oleg Nesterov
2015-03-05 20:35 ` [PATCH 0/1] x86/fpu: math_state_restore() should not blindly disable irqs Oleg Nesterov
2015-03-09 17:10 ` [PATCH] x86/fpu: drop_fpu() should not assume that tsk == current Oleg Nesterov
2015-03-09 17:36   ` Rik van Riel
2015-03-09 17:48   ` Borislav Petkov
2015-03-09 18:06     ` Oleg Nesterov
2015-03-09 18:10       ` Borislav Petkov
2015-03-16 12:07   ` [tip:x86/urgent] x86/fpu: Drop_fpu() should not assume that tsk equals current tip-bot for Oleg Nesterov
2015-03-11 17:33 ` [PATCH 0/4] x86/fpu: avoid math_state_restore() on kthread exec Oleg Nesterov
2015-03-11 17:34   ` [PATCH 1/4] x86/fpu: document user_fpu_begin() Oleg Nesterov
2015-03-13  9:47     ` Borislav Petkov
2015-03-13 14:34       ` Oleg Nesterov
2015-03-23 12:20     ` [tip:x86/fpu] x86/fpu: Document user_fpu_begin() tip-bot for Oleg Nesterov
2015-03-11 17:34   ` [PATCH 2/4] x86/fpu: introduce restore_init_xstate() Oleg Nesterov
2015-03-13 10:34     ` Borislav Petkov
2015-03-13 14:39       ` Oleg Nesterov
2015-03-13 15:20         ` Borislav Petkov
2015-03-16 19:05           ` Rik van Riel
2015-03-23 12:20     ` [tip:x86/fpu] x86/fpu: Introduce restore_init_xstate() tip-bot for Oleg Nesterov
2015-03-11 17:34   ` [PATCH 3/4] x86/fpu: use restore_init_xstate() instead of math_state_restore() on kthread exec Oleg Nesterov
2015-03-13 10:48     ` Borislav Petkov
2015-03-13 14:45       ` Oleg Nesterov
2015-03-13 15:51         ` Borislav Petkov
2015-03-23 12:21     ` [tip:x86/fpu] x86/fpu: Use " tip-bot for Oleg Nesterov
2015-03-11 17:35   ` [PATCH 4/4] x86/fpu: don't abuse drop_init_fpu() in flush_thread() Oleg Nesterov
2015-03-13 10:52     ` Borislav Petkov
2015-03-13 14:55       ` Oleg Nesterov
2015-03-13 16:19         ` Borislav Petkov
2015-03-13 16:26           ` Oleg Nesterov
2015-03-13 19:27             ` Borislav Petkov
2015-03-14 14:48               ` Oleg Nesterov
2015-03-15 17:36                 ` Borislav Petkov
2015-03-15 18:16                   ` Oleg Nesterov
2015-03-15 18:50                     ` Borislav Petkov
2015-03-15 20:04                       ` Oleg Nesterov
2015-03-15 20:38                         ` Borislav Petkov
2015-03-16  9:35                           ` Borislav Petkov
2015-03-16 10:28                             ` Ingo Molnar
2015-03-16 14:39                             ` Oleg Nesterov
2015-03-16 15:26                               ` Borislav Petkov
2015-03-16 15:34                             ` Andy Lutomirski
2015-03-16 15:35                               ` Borislav Petkov
2015-03-13 17:30     ` [PATCH v2 " Oleg Nesterov
2015-03-14 10:55       ` Borislav Petkov
2015-03-14 10:57         ` [PATCH] x86/fpu: Fold __drop_fpu() into its sole user Borislav Petkov
2015-03-14 15:15           ` Oleg Nesterov
2015-03-16 10:27           ` Ingo Molnar
2015-03-23 12:21       ` [tip:x86/fpu] x86/fpu: Don't abuse drop_init_fpu() in flush_thread() tip-bot for Oleg Nesterov
2015-03-13 18:26 ` [PATCH 0/1] x86/cpu: don't allocate fpu->state for swapper/0 Oleg Nesterov
2015-03-13 18:27   ` [PATCH 1/1] " Oleg Nesterov
2015-03-16 10:18     ` Borislav Petkov
2015-03-23 12:22     ` [tip:x86/fpu] x86/fpu: Don't " tip-bot for Oleg Nesterov
2015-03-14 11:16   ` [PATCH 0/1] x86/cpu: don't " Borislav Petkov
2015-03-14 15:13     ` [PATCH 0/1] x86/cpu: kill eager_fpu_init_bp() Oleg Nesterov
2015-03-14 15:13       ` [PATCH 1/1] " Oleg Nesterov
2015-03-16 12:44         ` Borislav Petkov
2015-03-23 12:22         ` [tip:x86/fpu] x86/fpu: Kill eager_fpu_init_bp() tip-bot for Oleg Nesterov
2015-03-15 16:49 ` [PATCH RFC 0/2] x86/fpu: avoid "xstate_fault" in xsave_user/xrestore_user Oleg Nesterov
2015-03-15 16:50   ` [PATCH RFC 1/2] x86: introduce __user_insn() and __check_insn() Oleg Nesterov
2015-03-15 16:50   ` [PATCH RFC 2/2] x86/fpu: change xsave_user() and xrestore_user() to use __user_insn() Oleg Nesterov
2015-03-16 22:43     ` Quentin Casasnovas
2015-03-17  9:35       ` Borislav Petkov
2015-03-16 14:36   ` [PATCH RFC 0/2] x86/fpu: avoid "xstate_fault" in xsave_user/xrestore_user Borislav Petkov
2015-03-16 14:57     ` Oleg Nesterov
2015-03-16 17:58       ` Borislav Petkov
2015-03-16 22:37   ` Quentin Casasnovas
2015-03-17  9:47     ` Borislav Petkov
2015-03-17 10:00       ` Quentin Casasnovas
2015-03-17 11:20         ` Borislav Petkov
2015-03-17 11:36           ` Quentin Casasnovas
2015-03-17 12:07             ` Borislav Petkov
2015-03-18  9:06               ` Quentin Casasnovas
2015-03-18  9:53                 ` Borislav Petkov
2015-03-17 10:07       ` Quentin Casasnovas

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20150306075833.GA623@gmail.com \
    --to=mingo@kernel.org \
    --cc=bp@suse.de \
    --cc=dave.hansen@intel.com \
    --cc=fenghua.yu@intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=luto@amacapital.net \
    --cc=oleg@redhat.com \
    --cc=priikone@iki.fi \
    --cc=quentin.casasnovas@oracle.com \
    --cc=riel@redhat.com \
    --cc=sbsiddha@gmail.com \
    --cc=torvalds@linux-foundation.org \
    --subject='Re: [PATCH 1/1] x86/fpu: math_state_restore() should not blindly disable irqs' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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