lkml.org 
[lkml]   [2015]   [Mar]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH 1/1] x86/fpu: math_state_restore() should not blindly disable irqs
    On Fri, Mar 6, 2015 at 9:33 AM, Linus Torvalds
    <torvalds@linux-foundation.org> wrote:
    > 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());

    I like this a lot better. Ingo's patch scares me because in-kernel
    FPU users are fundamentally atomic: if we're using kernel FPU *and*
    current has important FPU state, we can't schedule safely because
    there's nowhere to save the in-kernel FPU state. I don't see how we
    would end up in this situation with CR0.TS set, but if we somehow do,
    then we'll corrupt something with your patch.

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

    Please don't. IMO it's really nice that we don't use trap gates at
    all on x86_64, and I find the conditional_sti thing much nicer than
    having to audit all of the entry code to see whether it's safe to run
    it with IRQs on.

    This isn't to say that using trap gates would be a terrible idea in
    general. I think I'd be okay with saying that non-IST
    synchronous-only never-from-random-places entries should be trap gates
    in general. We could then audit the entry code and convert various
    entries.

    Thinks that maybe shouldn't be trap gates:

    - Anything asynchronous.
    - Page faults. The entry code can cause page faults recursively due
    to lazy vmap page table fills. Maybe this doesn't matter.
    - Anything using IST. That way lies madness.

    FWIW, I just started auditing the entry code a bit, and it's currently
    unsafe to use trap gates. The IRQ entry code in the "interrupt" macro
    does this:

    testl $3, CS-RBP(%rsp)
    je 1f
    SWAPGS

    That will go boom quite nicely if it happens very early in device_not_available.

    I suspect that fixing that will be slow and unpleasant and will
    significantly outweigh any benefit.

    --Andy


    \
     
     \ /
      Last update: 2015-03-06 20:41    [W:2.477 / U:0.392 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site