LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Linus Torvalds <torvalds@linux-foundation.org>
To: Ingo Molnar <mingo@kernel.org>
Cc: Oleg Nesterov <oleg@redhat.com>,
	Dave Hansen <dave.hansen@intel.com>, Borislav Petkov <bp@suse.de>,
	Andy Lutomirski <luto@amacapital.net>,
	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 09:33:46 -0800	[thread overview]
Message-ID: <CA+55aFzaQqR7xcHSoW_bwb_t6Cf4wiJ9LyRw3i_GSRHiGRvfDA@mail.gmail.com> (raw)
In-Reply-To: <20150306075833.GA623@gmail.com>

[-- Attachment #1: Type: text/plain, Size: 1677 bytes --]

On Thu, Mar 5, 2015 at 11:58 PM, Ingo Molnar <mingo@kernel.org> wrote:
>
> 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.

I agree with the "keep irqs enabled".

However, I do *not* agree with the actual patch, which doesn't do that at all.
> @@ -844,8 +844,9 @@ void math_state_restore(void)
>  {
>         struct task_struct *tsk = current;
>
> +       local_irq_enable();
> +

There's a big difference between "keep interrupts enabled" (ok) and
"explicitly enable interrupts in random contexts" (*NOT* ok).

So get rid of the "local_irq_enable()" entirely, and replace it with a

   WARN_ON_ONCE(irqs_disabled());

and let's just fix the cases where this actually gets called with
interrupts off. In particular, let's just make the
device_not_available thing use a trap gate, not an interrupt gate. And
then remove the "conditional_sti()" stuff.

IOW, I think the starting point should be something like the attached
(which doesn't do the WARN_ON_ONCE() - it should be added for
debugging).

*NOT* some kind of "let's just enable interrupts blindly" approach.

This is completely untested, of course. But I don't see why we should
use an interrupt gate for this. Is there anything in
"exception_enter()" that requires it?

                       Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/plain, Size: 1852 bytes --]

 arch/x86/kernel/traps.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 9d2073e2ecc9..f045ac026ff1 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -836,16 +836,12 @@ asmlinkage __visible void __attribute__((weak)) smp_threshold_interrupt(void)
  *
  * Careful.. There are problems with IBM-designed IRQ13 behaviour.
  * Don't touch unless you *really* know how it works.
- *
- * Must be called with kernel preemption disabled (eg with local
- * local interrupts as in the case of do_device_not_available).
  */
 void math_state_restore(void)
 {
 	struct task_struct *tsk = current;
 
 	if (!tsk_used_math(tsk)) {
-		local_irq_enable();
 		/*
 		 * does a slab alloc which can sleep
 		 */
@@ -856,7 +852,6 @@ void math_state_restore(void)
 			do_group_exit(SIGKILL);
 			return;
 		}
-		local_irq_disable();
 	}
 
 	/* Avoid __kernel_fpu_begin() right after __thread_fpu_begin() */
@@ -884,18 +879,13 @@ do_device_not_available(struct pt_regs *regs, long error_code)
 	if (read_cr0() & X86_CR0_EM) {
 		struct math_emu_info info = { };
 
-		conditional_sti(regs);
-
 		info.regs = regs;
 		math_emulate(&info);
 		exception_exit(prev_state);
 		return;
 	}
 #endif
-	math_state_restore(); /* interrupts still off */
-#ifdef CONFIG_X86_32
-	conditional_sti(regs);
-#endif
+	math_state_restore();
 	exception_exit(prev_state);
 }
 NOKPROBE_SYMBOL(do_device_not_available);
@@ -959,7 +949,7 @@ void __init trap_init(void)
 	set_system_intr_gate(X86_TRAP_OF, &overflow);
 	set_intr_gate(X86_TRAP_BR, bounds);
 	set_intr_gate(X86_TRAP_UD, invalid_op);
-	set_intr_gate(X86_TRAP_NM, device_not_available);
+	set_trap_gate(X86_TRAP_NM, device_not_available);
 #ifdef CONFIG_X86_32
 	set_task_gate(X86_TRAP_DF, GDT_ENTRY_DOUBLEFAULT_TSS);
 #else

  parent reply	other threads:[~2015-03-06 17:33 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
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 [this message]
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=CA+55aFzaQqR7xcHSoW_bwb_t6Cf4wiJ9LyRw3i_GSRHiGRvfDA@mail.gmail.com \
    --to=torvalds@linux-foundation.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=mingo@kernel.org \
    --cc=oleg@redhat.com \
    --cc=priikone@iki.fi \
    --cc=quentin.casasnovas@oracle.com \
    --cc=riel@redhat.com \
    --cc=sbsiddha@gmail.com \
    --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).