x86_64: Make int3 non-magical

This will allow IRQ stacks to nest inside NMIs or similar entries that can happen during IRQ stack setup or teardown.

The Xen code here has a confusing comment.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/entry/entry_64.S    | 72 ++++++++++++++++++++++++++------------------
 arch/x86/kernel/cpu/common.c |  2 +-
 arch/x86/kernel/process_64.c |  4 +++
 3 files changed, 47 insertions(+), 31 deletions(-)

arch/x86/entry/entry_64.S +0 -0

index d3033183ed70..5f7df8949fa7 100644

arch/x86/entry/entry_64.S +8 -0

 /*
+ * Enters the IRQ stack if we're not already using it.  NMI-safe.  Clobbers
+ * flags and puts old RSP into old_rsp, and leaves all other GPRs alone.
+ * Requires kernel GSBASE.
+ *
+ * The invariant is that, if irq_count != 0, then we're either on the
+ * IRQ stack or an IST stack, even if an NMI interrupts IRQ stack entry
+ * or exit.
+ */

To be obviously safe against any local exception, we want a single instruction that will change %rsp and some in-memory flag at the same time. There aren't a whole lot of candidates. Cmpxchg isn't useful (cmpxchg with a memory operand doesn't modify its register operand).

Quoted 4 lines · Andy Lutomirski

To be obviously safe against any local exception, we want a single instruction that will change %rsp and some in-memory flag at the same time. There aren't a whole lot of candidates. Cmpxchg isn't useful (cmpxchg with a memory operand doesn't modify its register operand).

Why would you even need that?

You do LOCK; CMPXCHG on a per_cpu variable and then test ZF? I.e., use it as a mutex in asm. With ZF=1, you switch stacks, with ZF=0, you busy-wait ...

Or am I missing something?

So really the only difference between this simple approach (which is more or less what we do now) and my fancy approach is that a kernel instruction breakpoint will cause do_debug to run on the initial stack instead of the IRQ stack.

I'm still tempted to say we should use my overly paranoid atomic approach for now and optimize later,...

Quoted 4 lines · Andy Lutomirski

So really the only difference between this simple approach (which is more or less what we do now) and my fancy approach is that a kernel instruction breakpoint will cause do_debug to run on the initial stack instead of the IRQ stack.

Sounds ok to me. What would be the worst thing if we limited the #DB stack? Some breakpoints will get ignored? In an endless stream of breakpoints hammering? Doesn't sound like a valid use case to me, does it?

Quoted 2 lines

I'm still tempted to say we should use my overly paranoid atomic approach for now and optimize later,...

But why change it if the simple approach of incrementing irq_count first is still fine? I think we want to KISS here exactly because apparently complexity in that area is a serious PITA...

Yeah, I'm going to submit v2 with the simple approach. I admit I'm rather fond of xadd as a way to switch rsp and set a flag at the same time, though :)

Quoted 3 lines · Andy Lutomirski

Yeah, I'm going to submit v2 with the simple approach. I admit I'm rather fond of xadd as a way to switch rsp and set a flag at the same time, though :)

I know you are.

But people will rip your head out if you added 60 cycles to the IRQ path.

And people will give me five new heads if I ignore Linus and do RET even with IF=1, saving 300 cycles?

And people will give me five new heads if I ignore Linus and do RET even with IF=1, saving 300 cycles?

Quoted 3 lines · Andy Lutomirski

And people will give me five new heads if I ignore Linus and do RET even with IF=1, saving 300 cycles?

So I'm still nervous about that "sti; ret" when we're back on the original kernel stack that took the original fault or interrupt. But it's probably ok.

Yes, it's irq-safe. But it's not NMI-safe, so if an NMI happens there, when the NMI returns, an interrupt might occur there too. But since we're back on the original stack where the original fault happened, and since interrupts were enabled, I don't see why that would be horrible. In theory, we might have a growing stack if this keeps happening, but since the only way to get that is to get the NMI in that one-instruction window (and apparently on at least _some_ microarchitectures the sti shadow stops even NMI's), I don't see how any kind of unbounded growth would happen.

So.

I think it would work, and it might even be good for "coverage" (ie the whole "iret-to-ret-conversion" will not have a lot of testing if it only happens for faults with interrupts disabled).

But it still worries me a bit.

What if we added something like:

if (regs->ip == ret_after_sti && !user_mode(regs) && (regs->flags & X86_EFLAGS_IF)) {
regs->ip--;
regs->flags &= ~X86_EFLAGS_IF;
}

to do_nmi, do_machine_check, and do_debug (the latter because kernel breakpoints, damnit)?

arch/x86/kernel/process_64.c __switch_to(struct task_struct *prev_p, struct task_struct *next_p) · line 285

Quoted 9 lines · Andy Lutomirski

arch/x86/kernel/process_64.c +3 -0

@@ -280,6 +280,10 @@ __switch_to(struct task_struct *prev_p, struct task_struct *next_p)
      unsigned fsindex, gsindex;
      fpu_switch_t fpu_switch;
 
+#ifdef CONFIG_DEBUG_ENTRY
+     WARN_ON(this_cpu_read(irq_count));
+#endif

Please introduce a less noisy (to the eyes) version of this, something like:

        WARN_ON_DEBUG_ENTRY(this_cpu_read(irq_count));

or so, similar to WARN_ON_FPU().

Thanks,

Ingo

Quoted 15 lines · Ingo Molnar

arch/x86/kernel/process_64.c __switch_to(struct task_struct *prev_p, struct task_struct *next_p) · line 285

Quoted 9 lines · Andy Lutomirski

arch/x86/kernel/process_64.c +3 -0

@@ -280,6 +280,10 @@ __switch_to(struct task_struct *prev_p, struct task_struct *next_p)
      unsigned fsindex, gsindex;
      fpu_switch_t fpu_switch;
 
+#ifdef CONFIG_DEBUG_ENTRY
+     WARN_ON(this_cpu_read(irq_count));
+#endif

Please introduce a less noisy (to the eyes) version of this, something like:

Quoted 3 lines

        WARN_ON_DEBUG_ENTRY(this_cpu_read(irq_count));

or so, similar to WARN_ON_FPU().

I can do that (or "DEBUG_ENTRY_WARN_ON"? we seem to be inconsistent about ordering).

Or would if (IS_ENABLED(CONFIG_DEBUG_ENTRY)) WARN_ON(...) be better?

Quoted 20 lines · On Wed, 5 Aug 2015 11:24:54 -0700 Andy Lutomirski <[email protected]> wrote:

Quoted 18 lines · Ingo Molnar

arch/x86/kernel/process_64.c __switch_to(struct task_struct *prev_p, struct task_struct *next_p) · line 285

Quoted 9 lines · Andy Lutomirski

arch/x86/kernel/process_64.c +3 -0

@@ -280,6 +280,10 @@ __switch_to(struct task_struct *prev_p, struct task_struct *next_p)
      unsigned fsindex, gsindex;
      fpu_switch_t fpu_switch;
 
+#ifdef CONFIG_DEBUG_ENTRY
+     WARN_ON(this_cpu_read(irq_count));
+#endif

Please introduce a less noisy (to the eyes) version of this, something like:

        WARN_ON_DEBUG_ENTRY(this_cpu_read(irq_count));

or so, similar to WARN_ON_FPU().

Quoted 5 lines

I can do that (or "DEBUG_ENTRY_WARN_ON"? we seem to be inconsistent about ordering).

Or would if (IS_ENABLED(CONFIG_DEBUG_ENTRY)) WARN_ON(...) be better?

Does WARN_ON(IS_ENABLED(CONFIG_DEBUG_ENTRY) && this_cpu_read(irq_count)) work?

-- Steve

Quoted 26 lines · Steven Rostedt

Quoted 23 lines · On Wed, 5 Aug 2015 11:24:54 -0700 Andy Lutomirski <[email protected]> wrote:

Quoted 15 lines · Ingo Molnar

arch/x86/kernel/process_64.c __switch_to(struct task_struct *prev_p, struct task_struct *next_p) · line 285

Quoted 9 lines · Andy Lutomirski

arch/x86/kernel/process_64.c +3 -0

@@ -280,6 +280,10 @@ __switch_to(struct task_struct *prev_p, struct task_struct *next_p)
      unsigned fsindex, gsindex;
      fpu_switch_t fpu_switch;
 
+#ifdef CONFIG_DEBUG_ENTRY
+     WARN_ON(this_cpu_read(irq_count));
+#endif

Please introduce a less noisy (to the eyes) version of this, something like:

Quoted 3 lines

        WARN_ON_DEBUG_ENTRY(this_cpu_read(irq_count));

or so, similar to WARN_ON_FPU().

I can do that (or "DEBUG_ENTRY_WARN_ON"? we seem to be inconsistent about ordering).

Quoted 5 lines

Quoted 2 lines

Or would if (IS_ENABLED(CONFIG_DEBUG_ENTRY)) WARN_ON(...) be better?

Does WARN_ON(IS_ENABLED(CONFIG_DEBUG_ENTRY) && this_cpu_read(irq_count)) work?

I'd be okay with it. Ingo?

There's nothing IST-worthy about #BP/int3. We don't allow kprobes in the small handful of places in the kernel that run at CPL0 with an invalid stack, and 32-bit kernels have used normal interrupt gates for #BP forever.

Furthermore, we don't allow kprobes in places that have usergs while in kernel mode, so "paranoid" is also unnecessary.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/entry/entry_64.S |  2 +-
 arch/x86/kernel/traps.c   | 26 +++++++++++++-------------
 2 files changed, 14 insertions(+), 14 deletions(-)

int3 uses IST and the paranoid gsbase path. Neither is necessary, although the IST stack may currently be necessary to avoid stack overruns.

Clean up IRQ stacks, make them NMI safe, teach idtentry to use irqstacks if requested, and move int3 to the IRQ stack.

This prepares us to return from int3 using RET. While we could, in principle, return from an IST entry using RET, making that work seems likely to be much messier and more fragile than this approach.