Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1405758 > unrolled thread

Re: [PATCH v3 3/3] sched, x86: Check that we're on the right stack in schedule and __might_sleep

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 3/3] sched, x86: Check that we're on the right stack in  schedule and __might_sleep Andy Lutomirski <luto@amacapital.net> - 2016-05-24 03:30 +0200
    Re: [PATCH v3 3/3] sched, x86: Check that we're on the right stack in  schedule and __might_sleep Linus Torvalds <torvalds@linux-foundation.org> - 2016-05-24 03:50 +0200
      Re: [PATCH v3 3/3] sched, x86: Check that we're on the right stack in  schedule and __might_sleep Andy Lutomirski <luto@amacapital.net> - 2016-05-24 04:20 +0200
        Re: [PATCH v3 3/3] sched, x86: Check that we're on the right stack in  schedule and __might_sleep Linus Torvalds <torvalds@linux-foundation.org> - 2016-05-24 04:20 +0200

#1405758 — Re: [PATCH v3 3/3] sched, x86: Check that we're on the right stack in schedule and __might_sleep

On Sun, Feb 28, 2016 at 9:27 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Wed, Nov 19, 2014 at 11:44 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>> On Wed, Nov 19, 2014 at 11:29 AM, Andi Kleen <andi@firstfloor.org> wrote:
>>>
>>> The exception handlers which use the IST stacks don't necessarily
>>> set irq count. Maybe they should.
>>
>> Hmm. I think they should. Since they clearly must not schedule, as
>> they use a percpu stack.
>>
>> Which exceptions use IST?
>>
>> [ grep grep ]
>>
>> Looks like stack, doublefault, nmi, debug and mce. And yes, I really
>> think they should all raise the irq count if they don't already.
>> Rather than add random arch-specific "let's check that we're on the
>> right stack" code to the might-sleep stuff, just use the one we have.
>>
>
> Resurrecting an old thread:
>
> The outcome of this discussion was that ist_enter now raises
> HARDIRQ_COUNT.  I think this is causing a problem.  If a user program
> enables TF, it generates a bunch of debug exceptions.  The handlers
> raise the IRQ count and do stuff, and apparently some of that stuff
> can raise a softirq.  (I have no idea where the softirq is being
> raised.)  The softirq code notices that we're in_interrupt and doesn't
> wake ksoftirqd because it thinks we're about to exit the interrupt and
> process the softirq.  But we don't, which causes occasional warnings
> and confuses things (and me!).
>
> So how do we fix it?  If we stop raising HARDIRQ_COUNT (and apply
> $SUBJECT?), then raise_softirq will wake ksoftirqd and life is good.
> But this seems a bit silly, since, if we entered the ist exception
> handler from a context with irqs on and softirqs enabled, we *could*
> plausibly handle the softirq right away -- we're on an essentially
> empty stack.  (Of course, it's a *small* stack, since it could be the
> IST stack.)
>
> Or we could just let ksoftirqd do its thing and stop raising
> HARDIRQ_COUNT.  We could add a new preempt count field just for IST
> (yuck).  We could try to hijack a different preempt count field
> (NMI?).  But I kind of like the idea of just reinstating the original
> patch of explicitly checking that we're on a safe stack in schedule
> and __might_sleep, since that is the actual condition we care about.

Ping?  I can still trigger this fairly easily on 4.6.

--Andy

[toc] | [next] | [standalone]

#1405768

On Mon, May 23, 2016 at 6:23 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>>
>> Or we could just let ksoftirqd do its thing and stop raising
>> HARDIRQ_COUNT.  We could add a new preempt count field just for IST
>> (yuck).  We could try to hijack a different preempt count field
>> (NMI?).  But I kind of like the idea of just reinstating the original
>> patch of explicitly checking that we're on a safe stack in schedule
>> and __might_sleep, since that is the actual condition we care about.
>
> Ping?  I can still trigger this fairly easily on 4.6.

.. I haven't seen a patch from you, last I saw that was kind of what I expected.

That said, I still despise your patch. Why can't you just fix
"in_interrupt()" and be done with it. The original patch was like 50
lines of changes for somethinig that feels like it should be a
one-liner.

And no, we don't add idiotic new config symbols for things like "I
have this one-liner trivial arch helper". What we do is to just test
for such a helper with "#ifdef" (and if it's a inline function we do
#define xyz xyz" so that the #ifdef works).

So the original patch in this thread is still off the table,
especially since there was absolutely no explanation for why it should
be such a crazy complicated thing.

What exactly is it you are nervous about scheduling in NMI's? I agree
that that would be disastrous, but it's not supposed to actually
happen.

              Linus

[toc] | [prev] | [next] | [standalone]

#1405779

On Mon, May 23, 2016 at 6:48 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Mon, May 23, 2016 at 6:23 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>>>
>>> Or we could just let ksoftirqd do its thing and stop raising
>>> HARDIRQ_COUNT.  We could add a new preempt count field just for IST
>>> (yuck).  We could try to hijack a different preempt count field
>>> (NMI?).  But I kind of like the idea of just reinstating the original
>>> patch of explicitly checking that we're on a safe stack in schedule
>>> and __might_sleep, since that is the actual condition we care about.
>>
>> Ping?  I can still trigger this fairly easily on 4.6.
>
> .. I haven't seen a patch from you, last I saw that was kind of what I expected.
>
> That said, I still despise your patch. Why can't you just fix
> "in_interrupt()" and be done with it. The original patch was like 50
> lines of changes for somethinig that feels like it should be a
> one-liner.
>
> And no, we don't add idiotic new config symbols for things like "I
> have this one-liner trivial arch helper". What we do is to just test
> for such a helper with "#ifdef" (and if it's a inline function we do
> #define xyz xyz" so that the #ifdef works).
>
> So the original patch in this thread is still off the table,
> especially since there was absolutely no explanation for why it should
> be such a crazy complicated thing.
>
> What exactly is it you are nervous about scheduling in NMI's? I agree
> that that would be disastrous, but it's not supposed to actually
> happen.

It's not the NMIs I'm worried about -- they do crazy stuff, but it's
self-contained crazy stuff, and NMIs can *never* schedule, so they're
unlikely to have issues here.  It's the things that are a bit more
ambiguous.  For example, MCE handlers sometimes do schedule, and we
allow it if they use the right helpers and check the right conditions.
The original patch lets us print a big fat warning if they screw it
up.

I can't modify in_interrupt for the same reason that the current code
is broken: if in_interrupt() returns true, that's a promise that we'll
call invoke_softirq via irq_exit in a timely manner.  Doing this from
a weird ultra-atomic context that interrupted kernel code that had
IF=0 would be bad.  IOW, the whole in_interrupt() mechanism was
designed until the entirely reasonably assumption that interrupts only
happen when interrupts are on.  These special interrupt-like things
(NMI, MCE, debug) can happen asynchronously with interrupts *off*, and
the result is a mess.

What about this silly fix?  (Pardon the probable whitespace damage.)

commit b3e89c652bdf6a7d5a23b094ae921f193e62c534
Author: Andy Lutomirski <luto@kernel.org>
Date:   Mon May 23 19:07:21 2016 -0700

    x86/traps: Don't for in_interrupt() to return true in IST handlers

    Forcing in_interrupt() to return true if we're not in a bona fide
    interrupt confuses the softirq code.

    Cc: stable@vger.kernel.org
    Fixes: 959274753857 ("x86, traps: Track entry into and exit from
IST context")
    Signed-off-by: Andy Lutomirski <luto@kernel.org>

diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index d1590486204a..9c1d948d5d8b 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -96,6 +96,19 @@ static inline void cond_local_irq_disable(struct
pt_regs *regs)
         local_irq_disable();
 }

+/*
+ * We want to cause in_atomic() to return true while in an IST handler
+ * so that attempts to schedule will warn.  We also want to detect buggy
+ * code that does preempt_enable(); preempt_disable() in an IST handler,
+ * so we want in_atomic to still return true if that happens.
+ *
+ * We cannot add use HARDIRQ_OFFSET or otherwise cause in_interrupt() to
+ * return true: the softirq code assumes that in_interrupt() only
+ * returns true if we will soon execute softirqs, and we can't do that
+ * if an IST entry interrupts kernel code with interrupts disabled.
+ */
+#define IST_OFFSET (3 * PREEMPT_OFFSET)
+
 void ist_enter(struct pt_regs *regs)
 {
     if (user_mode(regs)) {
@@ -116,7 +129,7 @@ void ist_enter(struct pt_regs *regs)
      * on x86_64 and entered from user mode, in which case we're
      * still atomic unless ist_begin_non_atomic is called.
      */
-    preempt_count_add(HARDIRQ_OFFSET);
+    preempt_count_add(IST_OFFSET);

     /* This code is a bit fragile.  Test it. */
     RCU_LOCKDEP_WARN(!rcu_is_watching(), "ist_enter didn't work");
@@ -124,7 +137,7 @@ void ist_enter(struct pt_regs *regs)

 void ist_exit(struct pt_regs *regs)
 {
-    preempt_count_sub(HARDIRQ_OFFSET);
+    preempt_count_sub(IST_OFFSET);

     if (!user_mode(regs))
         rcu_nmi_exit();

[toc] | [prev] | [next] | [standalone]

#1405780

On Mon, May 23, 2016 at 7:09 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>
> What about this silly fix?  (Pardon the probable whitespace damage.)

That looks fine to me, and has a reason for it.

That said, I'm not convinced about the preempt_enable/preempt_disable
excuse: that would be horribly buggy anyway, and has nothing to do
with IST's. Why protect against random insane bugs?

So the "3*" certainly doesn't hurt, but and I won't argue more against
it, but it looks pretty magical and made-up. Might as well just
add/subtract one. You can never protect against people who
intentionally write buggy code, and if it's protecting against
unintentional buggy code I don't see why the IST case is so special
when it's just a tiny fraction of code..

              Linus

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web