interrupted single step fixes

Hi,

I've been playing with Pratyush's watchpoint and interrupt reproducer, it looks like we have three bugs with the way these interact:
* PSTATE.SS is saved when we take an IRQ, but not restored when we ERET.
* We can context switch while single-step is enabled.
* We can end up single stepping the irq handler.

What does this cause? Instead of stepping over a watchpoint, we step the interrupt handler instead, re-enable the watchpoints and disable MDSCR_EL1.SS. On return we hit the watchpoint again. (the same will happen with breakpoints).

Akashi, Pratyush, do these fix (all!) the issues you've been seeing?

Patch 3 conflicts badly with my doomed attempt to enforce an order on the DAIF flags in the Serror/RAS/IESB series.

These three patches, and v3 of Pratyush's three are at:
git://linux-arm.org/linux-jm -b perf_single_step/v1

Enable CONFIG_SAMPLE_HW_BREAKPOINT, then:

With mainline you will hit the watchpoint forever, Pratyush's patches reduce this to ~10 times. These patches reduce that to the expected once.

Thanks,

James Morse (3):
arm64: entry: Allow SPSR_EL1.SS to be restored
arm64: debug-monitors: Disable preemption
arm64: entry: Exceptions from single-step should leave debug masked

 arch/arm64/include/asm/assembler.h | 18 ++++++++++++++++++
 arch/arm64/kernel/debug-monitors.c |  5 +++--
 arch/arm64/kernel/entry.S          | 14 +++++++-------
 3 files changed, 28 insertions(+), 9 deletions(-)

2.13.3

If we take an IRQ from the single-step state-machine's active-not-pending state, the PSTATE.SS bit is saved in SPSR_EL1.SS. This lets us restore the state machine when we return to the to-be-stepped instruction.

The ARM-ARM has some rules about when ERET will restore this bit, (see ARM DDI 0487B.a D2.12.4 Entering the active-not-pending state'), in particular it requires 'debug exceptions are disabled from the current exception level'. el1_irq unmasks debug exceptions, and continues like this until ERET. The PSTATE.SS bit is not restored.

Fix this this by masking all exceptions on kernel_exit.

---
 arch/arm64/include/asm/assembler.h | 4 ++++
 arch/arm64/kernel/entry.S          | 4 ++--
 2 files changed, 6 insertions(+), 2 deletions(-)

index 1b67c3782d00..1c490c578a2e 100644
@@ -31,6 +31,10 @@
 #include <asm/ptrace.h>
 #include <asm/thread_info.h>
 
+	.macro disable_daif
+	msr	daifset, #0xf
+	.endm
+
 /*
  * Enable and disable interrupts.
  */

index b738880350f9..eed2d51e16e6 100644
@@ -163,6 +163,8 @@ alternative_else_nop_endif
 	.endm
 
 	.macro	kernel_exit, el
+	disable_daif
+
 	.if	\el != 0
 	/* Restore the task's original addr_limit. */
 	ldr	x20, [sp, #S_ORIG_ADDR_LIMIT]

@@ -438,8 +440,6 @@ el1_da:
 	mov	x2, sp				// struct pt_regs
 	bl	do_mem_abort
 
-	// disable interrupts before pulling preserved data off the stack
-	disable_irq
 	kernel_exit 1
 el1_sp_pc:
 	/*

2.13.3

The helpers kernel_{en,dis}able_single_step check that interrupts are disabled when they are called as they are setting the per-cpu MDSCR_EL1 register.

But we don't expect single step to have an effect until we ERET with SPSR.SS set. We may ERET to a context that has interrupts unmasked, take an interrupt and switch CPU. This leaves MDSCR_EL1.SS set on one CPU, and SPSR.SS set on another.

Stop this by wrapping the MDSCR_EL1 writes in preempt_{en,dis}able().

---
 arch/arm64/kernel/debug-monitors.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

index c7ef99904934..6ba315dc2935 100644
@@ -22,6 +22,7 @@
 #include <linux/debugfs.h>
 #include <linux/hardirq.h>
 #include <linux/init.h>
+#include <linux/preempt.h>
 #include <linux/ptrace.h>
 #include <linux/kprobes.h>
 #include <linux/stat.h>

@@ -399,7 +400,7 @@ void user_fastforward_single_step(struct task_struct *task)
 /* Kernel API */
 void kernel_enable_single_step(struct pt_regs *regs)
 {
-	WARN_ON(!irqs_disabled());
+	preempt_disable();
 	set_regs_spsr_ss(regs);
 	mdscr_write(mdscr_read() | DBG_MDSCR_SS);
 	enable_debug_monitors(DBG_ACTIVE_EL1);

@@ -408,9 +409,9 @@ NOKPROBE_SYMBOL(kernel_enable_single_step);
 
 void kernel_disable_single_step(void)
 {
-	WARN_ON(!irqs_disabled());
 	mdscr_write(mdscr_read() & ~DBG_MDSCR_SS);
 	disable_debug_monitors(DBG_ACTIVE_EL1);
+	preempt_enable();
 }
 NOKPROBE_SYMBOL(kernel_disable_single_step);
 

2.13.3

If we interrupted an instruction being single-stepped we may end up taking a single-step exception from the interrupt handler. This confuses single-step users who are typically just waiting for 'the next' single step exception before re-enabling {break,watch}points. Returning from the interrupt causes us to hit the {break,watch}point again.

For the least-surprising results, lets confine single-step to its intended context.

state's 'If the PE takes an exception' section, we enter the inactive state because the exception sets PSTATE.D.

D2.12.6 'Entering the active-pending state', from the inactive state, we re-enter active-pending if we clear PSTATE.D. This causes a debug single step exception and we we step the exception handler.

Change the EL1 entry.S handlers to inherit their debug state if the SPSR.SS bit is clear, instead of unconditionally unmasking it.

This bit will be set if we took this exception instead of stepping an instruction.

This isn't needed for the EL0 entry.S handlers as we will have cleared MDSCR_EL1.SS on entry from EL0.

---
 arch/arm64/include/asm/assembler.h | 14 ++++++++++++++
 arch/arm64/kernel/entry.S          | 10 +++++-----
 2 files changed, 19 insertions(+), 5 deletions(-)

index 1c490c578a2e..96f01cc33d0e 100644
@@ -25,6 +25,7 @@
 
 #include <asm/asm-offsets.h>
 #include <asm/cpufeature.h>
+#include <asm/debug-monitors.h>
 #include <asm/mmu_context.h>
 #include <asm/page.h>
 #include <asm/pgtable-hwdef.h>

@@ -66,6 +67,19 @@
 	msr	daifclr, #8
 	.endm
 
+	/*
+	 * If we interrupted single step from EL1 then we may end up stepping
+	 * the exception handler. Leave debug masked. Otherwise inherit
+	 * the value we interrupted.
+	 */
+	.macro	inherit_dbg, pstate, reg
+	mov_q	\reg, (PSR_D_BIT | DBG_SPSR_SS)
+	and	\reg, \reg, \pstate
+	cbnz	\reg, 9998f
+	enable_dbg
+9998:
+	.endm
+
 	.macro	disable_step_tsk, flgs, tmp
 	tbz	\flgs, #TIF_SINGLESTEP, 9990f
 	mrs	\tmp, mdscr_el1

index eed2d51e16e6..9788bb47a7f7 100644
@@ -431,7 +431,7 @@ el1_da:
 	 * Data abort handling
 	 */
 	mrs	x3, far_el1
-	enable_dbg
+	inherit_dbg, pstate=x23 reg=x0
 	// re-enable interrupts if they were enabled in the aborted context
 	tbnz	x23, #7, 1f			// PSR_I_BIT
 	enable_irq

@@ -446,14 +446,14 @@ el1_sp_pc:
 	 * Stack or PC alignment exception handling
 	 */
 	mrs	x0, far_el1
-	enable_dbg
+	inherit_dbg, pstate=x23 reg=x2
 	mov	x2, sp
 	b	do_sp_pc_abort
 el1_undef:
 	/*
 	 * Undefined instruction
 	 */
-	enable_dbg
+	inherit_dbg, pstate=x23 reg=x0
 	mov	x0, sp
 	b	do_undefinstr
 el1_dbg:

@@ -469,7 +469,7 @@ el1_dbg:
 	kernel_exit 1
 el1_inv:
 	// TODO: add support for undefined instructions in kernel mode
-	enable_dbg
+	inherit_dbg, pstate=x23 reg=x0
 	mov	x0, sp
 	mov	x2, x1
 	mov	x1, #BAD_SYNC

@@ -479,7 +479,7 @@ ENDPROC(el1_sync)
 	.align	6
 el1_irq:
 	kernel_entry 1
-	enable_dbg
+	inherit_dbg, pstate=x23 reg=x0
 #ifdef CONFIG_TRACE_IRQFLAGS
 	bl	trace_hardirqs_off
 #endif

2.13.3

Hi James,

Thanks a lot for looking into it.

So, when you say that it reduce to ~10 times with my patches, did you use all patches ie 1-4 or only 1-3. 1-3 is just the preparation and 4 was my actual solution. I had seen only once (which is expected) with all(1-4) of my patches. Please let me know if I missed any observation.

In my understanding, you have used 1-3 of mine and then replaced my 4th patch with your patches in this series.If I understood correctly then, now we can handle IRQ between breakpoint and step handler, I think thats good. Only concern seems that how does this approach behave when we put a hardware breakpoint on a ISR called between breakpoint and step exception.

Regards
Pratyush

Hi Pratyush,

Yes, your patches 1-3 were fixing <something> in perf to cause us to step the watchpoint, and 4&5 disabled/re-enabled IRQs in the interrupted context when we do the step.

For use with your example, yes, but I think these fix the existing hwbreakpoint/kgdb users. (unless none of this has ever worked?)

(I still haven't put together a test for kgdb)

Yes, this won't work because the IRQ:breakpoint occurs while the debug hardware is already in use to step the first breakpoint. (but this only works today because we take breakpoints 10s of times)

It looks like this is an important use-case for perf... this is more complicated than just surviving an IRQ during single step.

I think ignore these patches, I'll try and come up with a way to get that 'nested' case working too. Will's suggestion of separating what we do for perf and user space may make this easier[0].

Thanks,

James

[0] https://www.spinics.net/lists/arm-kernel/msg594339.html