Skip to contentSkip to conversation

irqstack changes for Josh

PATCH In review Andy Lutomirski 2 participants 4 messages Jun 30, 2017

Subsystems (2)
lore
Share & tools
lore

Andy Lutomirski

author maintainer
#0 lore raw

cover irqstack changes for Josh

Patch 1 consolidates code and patch 2 fixes a glitch that Josh noticed. These could go straight to -tip, but it may make more sense for Josh to add them to his undwarf series.

P.S. Should undwarf be called balrog instead? It seems that Josh has been delving very deeply indeed lately.

Andy Lutomirski (2):
x86/entry/64: Refactor IRQ stacks and make them NMI-safe x86/entry/64: Initialize the top of the IRQ stack before switching
stacks

 arch/x86/Kconfig.debug       |   2 -
 arch/x86/entry/entry_64.S    | 107 +++++++++++++++++++++++++++++++++----------
 arch/x86/kernel/process_64.c |   3 ++
 3 files changed, 86 insertions(+), 26 deletions(-)
Signature · 1 line
2.9.4
author maintainer
#1 lore raw

PATCH 1/2 x86/entry/64: Refactor IRQ stacks and make them NMI-safe

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

The new macros won't work correctly if they're invoked with IRQs on. Add a check under CONFIG_DEBUG_ENTRY to detect that.

---
 arch/x86/Kconfig.debug       |  2 --
 arch/x86/entry/entry_64.S    | 85 +++++++++++++++++++++++++++++++-------------
 arch/x86/kernel/process_64.c |  3 ++
 3 files changed, 64 insertions(+), 26 deletions(-)
arch/x86/Kconfig.debug +0 -2
index fcb7604172ce..353ed09f5fba 100644
@@ -305,8 +305,6 @@ config DEBUG_ENTRY
 	  Some of these sanity checks may slow down kernel entries and
 	  exits or otherwise impact performance.
 
-	  This is currently used to help test NMI code.
-
 	  If unsure, say N.
 
 config DEBUG_NMI_SELFTEST
arch/x86/entry/entry_64.S +61 -24

119 lines not on this page · raw message

arch/x86/kernel/process_64.c +3 -0
index b6840bf3940b..1d910d97c1c6 100644
@@ -279,6 +279,9 @@ __switch_to(struct task_struct *prev_p, struct task_struct *next_p)
 	struct tss_struct *tss = &per_cpu(cpu_tss, cpu);
 	unsigned prev_fsindex, prev_gsindex;
 
+	WARN_ON_ONCE(IS_ENABLED(CONFIG_DEBUG_ENTRY) &&
+		     this_cpu_read(irq_count) != -1);
+
 	switch_fpu_prepare(prev_fpu, cpu);
 
 	/* We must save %fs and %gs before load_TLS() because
Signature · 1 line
2.9.4
author maintainer
#2 lore raw

PATCH 2/2 x86/entry/64: Initialize the top of the IRQ stack before switching stacks

In reply to Andy Lutomirski

The OOPS unwinder wants the word at the top of the IRQ stack to point back to the previous stack at all times when the IRQ stack is in use. There's currently a one-instruction window in ENTER_IRQ_STACK during which this isn't the case. Fix it by writing the old RSP to the top of the IRQ stack before jumping.

This currently writes the pointer to the stack twice, which is a bit ugly. We could get rid of this by replacing irq_stack_ptr with irq_stack_ptr_minus_eight (better name welcome). OTOH, there may be all kinds of odd microarchitectural considerations in play that affect performance by a few cycles here.

---
 arch/x86/entry/entry_64.S | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)
arch/x86/entry/entry_64.S +23 -1
index 3ace85965242..f914a9207bcc 100644
@@ -468,6 +468,7 @@ END(irq_entries_start)
 	DEBUG_ENTRY_ASSERT_IRQS_OFF
 	movq	%rsp, \old_rsp
 	incl	PER_CPU_VAR(irq_count)
+	jnz	.Lirq_stack_push_old_rsp_\@
 
 	/*
 	 * Right now, if we just incremented irq_count to zero, we've
@@ -477,9 +478,30 @@ END(irq_entries_start)
 	 * it must be *extremely* careful to limit its stack usage.  This
 	 * could include kprobes and a hypothetical future IST-less #DB
 	 * handler.
+	 *
+	 * The OOPS unwinder relies on the word at the top of the IRQ
+	 * stack linking back to the previous RSP for the entire time we're
+	 * on the IRQ stack.  For this to work reliably, we need to write
+	 * it before we actually move ourselves to the IRQ stack.
+	 */
+
+	movq	\old_rsp, PER_CPU_VAR(irq_stack_union + IRQ_STACK_SIZE - 8)
+	movq	PER_CPU_VAR(irq_stack_ptr), %rsp
+
+#ifdef CONFIG_DEBUG_ENTRY
+	/*
+	 * If the first movq above becomes wrong due to IRQ stack layout
+	 * changes, the only way we'll notice is if we try to unwind right
+	 * here.  Assert that we set up the stack right to catch this type
+	 * of bug quickly.
 	 */
+	cmpq	-8(%rsp), \old_rsp
+	je	.Lirq_stack_okay\@
+	ud2
+	.Lirq_stack_okay\@:
+#endif
 
-	cmovzq	PER_CPU_VAR(irq_stack_ptr), %rsp
+.Lirq_stack_push_old_rsp_\@:
 	pushq	\old_rsp
 .endm
 
Signature · 1 line
2.9.4
#3 lore raw
In reply to Andy Lutomirski
Quoted 6 lines · Andy Lutomirski

Patch 1 consolidates code and patch 2 fixes a glitch that Josh noticed. These could go straight to -tip, but it may make more sense for Josh to add them to his undwarf series.

P.S. Should undwarf be called balrog instead? It seems that Josh has been delving very deeply indeed lately.

Who cares about that entry code stuff. I like the Balrog idea. The top contenders in my mind are:

- Balrog unwinder
- Orc unwinder
- Undwarf unwinder

Signature · 1 line
Josh