[PATCH 0/3] interrupted single step fixes

[PATCH 0/3] interrupted single step fixes

[James Morse]

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:
> insmod data_breakpoint.ko ksym=__sysrq_enabled
> cat /proc/sys/kernel/sysrq

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

[PATCH 1/3] arm64: entry: Allow SPSR_EL1.SS to be restored

[James Morse]

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.

Signed-off-by: James Morse <james.morse@arm.com>
CC: Pratyush Anand <panand@redhat.com>
CC: AKASHI Takahiro <takahiro.akashi@linaro.org>

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

diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
index 1b67c3782d00..1c490c578a2e 100644
--- a/arch/arm64/include/asm/assembler.h
+++ b/arch/arm64/include/asm/assembler.h
@@ -31,6 +31,10 @@
 #include <asm/ptrace.h>
 #include <asm/thread_info.h>
 
+	.macro disable_daif
+	msr	daifset, #0xf
+	.endm
+
 /*
  * Enable and disable interrupts.
  */
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index b738880350f9..eed2d51e16e6 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -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

[PATCH 2/3] kernel: debug-monitors: Disable preemption

[James Morse]

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

Signed-off-by: James Morse <james.morse@arm.com>
CC: Pratyush Anand <panand@redhat.com>
CC: AKASHI Takahiro <takahiro.akashi@linaro.org>

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

diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
index c7ef99904934..6ba315dc2935 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -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

[PATCH 3/3] arm64: entry: Exceptions from single-step should leave debug masked

[James Morse]

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.

>From the ARM-ARM DDI 0487B.a, D.12.5 'Behaviour in the active-not-pending
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.

Signed-off-by: James Morse <james.morse@arm.com>
CC: Pratyush Anand <panand@redhat.com>
CC: AKASHI Takahiro <takahiro.akashi@linaro.org>

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

diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
index 1c490c578a2e..96f01cc33d0e 100644
--- a/arch/arm64/include/asm/assembler.h
+++ b/arch/arm64/include/asm/assembler.h
@@ -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
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index eed2d51e16e6..9788bb47a7f7 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -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

[PATCH 0/3] interrupted single step fixes

[Pratyush Anand]

Hi James,

Thanks a lot for looking into it.

On Thursday 03 August 2017 08:45 PM, James Morse wrote:
> Enable CONFIG_SAMPLE_HW_BREAKPOINT, then:
>> insmod data_breakpoint.ko ksym=__sysrq_enabled
>> cat /proc/sys/kernel/sysrq
> With mainline you will hit the watchpoint forever, Pratyush's patches
> reduce this to ~10 times. These patches reduce that to the expected
> once.

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

[PATCH 0/3] interrupted single step fixes

[James Morse]

Hi Pratyush,

On 04/08/17 13:49, Pratyush Anand wrote:
> On Thursday 03 August 2017 08:45 PM, James Morse wrote:
>> Enable CONFIG_SAMPLE_HW_BREAKPOINT, then:
>>> insmod data_breakpoint.ko ksym=__sysrq_enabled
>>> cat /proc/sys/kernel/sysrq
>> With mainline you will hit the watchpoint forever, Pratyush's patches
>> reduce this to ~10 times. These patches reduce that to the expected
>> once.
> 
> 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.

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.


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

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)


> Only concern
> seems that how does this approach behave when we put a hardware breakpoint on a
> ISR called between breakpoint and step exception.

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