arm64: kgdb: fix single stepping

After entering kgdb mode, the first 'stepi' can succeed, but the following 'stepi' never executes the next instruction.

This is because a software step cannot get enabled as the software step bit(SS) in SPSR, which is cleared by the first single stepping, will not be set again for the following 's' commands.

Please note that this bit, as well as the software step control bit(SS) in MDSCR, must be set before resuming the execution. kernel_active_single_step() called by kgdb_arch_handle_exception() checks only for the bit in MDSCR, and so kgdb_enable_single_step() will never be called.

Quoted 6 lines · AKASHI Takahiro

After entering kgdb mode, the first 'stepi' can succeed, but the following 'stepi' never executes the next instruction.

This is because a software step cannot get enabled as the software step bit(SS) in SPSR, which is cleared by the first single stepping, will not be set again for the following 's' commands.

For userspace, we have user_rewind_single_step to re-arm the state machine on an unhandled step exception. It sounds like we need the kernel version of that?

Quoted 5 lines

Please note that this bit, as well as the software step control bit(SS) in MDSCR, must be set before resuming the execution. kernel_active_single_step() called by kgdb_arch_handle_exception() checks only for the bit in MDSCR, and so kgdb_enable_single_step() will never be called.

MDSCR.SS shouldn't get cleared by the hardware, so I don't understand your point here.

Will

After entering kgdb mode, 'stepi' may unexpectedly breaks the execution somewhere in el1_irq.

This happens because a debug exception is always enabled in el1_irq due to the following commit merged in v3.16:
commit 2a2830703a23 ("arm64: debug: avoid accessing mdscr_el1 on fault
paths where possible")
A pending interrupt can be taken after kgdb has enabled a software step, but before a debug exception is actually taken.

This patch enforces interrupts to be masked while single stepping.

Signed-off-by AKASHI Takahiro <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Will Deacon <[email protected]>

Cc Jason Wessel <[email protected]>

---
 arch/arm64/kernel/kgdb.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

arch/arm64/kernel/kgdb.c +16 -0

index b9176b324e5a..fddbc6be3780 100644
@@ -28,6 +28,7 @@
 
 #include <asm/debug-monitors.h>
 #include <asm/insn.h>
+#include <asm/ptrace.h>
 #include <asm/traps.h>
 
 struct dbg_reg_def_t dbg_reg_def[DBG_MAX_REG_NUM] = {

@@ -111,6 +112,8 @@ struct dbg_reg_def_t dbg_reg_def[DBG_MAX_REG_NUM] = {
 	{ "fpcr", 4, -1 },
 };
 
+static DEFINE_PER_CPU(unsigned int, kgdb_pstate);
+
 char *dbg_get_reg(int regno, void *mem, struct pt_regs *regs)
 {
 	if (regno >= DBG_MAX_REG_NUM || regno < 0)

@@ -200,6 +203,10 @@ int kgdb_arch_handle_exception(int exception_vector, int signo,
 		err = 0;
 		break;
 	case 's':
+		/* mask interrupts while single stepping */
+		__this_cpu_write(kgdb_pstate, linux_regs->pstate);
+		linux_regs->pstate |= PSR_I_BIT;
+
 		/*
 		 * Update step address value with address passed
 		 * with step packet.

@@ -242,11 +249,20 @@ NOKPROBE_SYMBOL(kgdb_compiled_brk_fn);
 
 static int kgdb_step_brk_fn(struct pt_regs *regs, unsigned int esr)
 {
+	unsigned int pstate;
+
 	if (!kgdb_single_step)
 		return DBG_HOOK_ERROR;
 
 	kernel_disable_single_step();
 
+	/* restore interrupt mask status */
+	pstate = __this_cpu_read(kgdb_pstate);
+	if (pstate & PSR_I_BIT)
+		regs->pstate |= PSR_I_BIT;
+	else
+		regs->pstate &= ~PSR_I_BIT;
+
 	kgdb_handle_exception(1, SIGTRAP, 0, regs);
 	return 0;
 }

Signature · 1 line

2.11.1

Quoted 10 lines · AKASHI Takahiro

After entering kgdb mode, 'stepi' may unexpectedly breaks the execution somewhere in el1_irq.

This happens because a debug exception is always enabled in el1_irq due to the following commit merged in v3.16:
commit 2a2830703a23 ("arm64: debug: avoid accessing mdscr_el1 on fault
paths where possible")
A pending interrupt can be taken after kgdb has enabled a software step, but before a debug exception is actually taken.

Quoted 1 line

This patch enforces interrupts to be masked while single stepping.

The desired behaviour here boils down to whether or not KGDB expects to step into or over interrupts in response a stepi instruction. What do other architectures do?

What happens if the instruction being stepped faults?

Will

Quoted 64 lines

Signed-off-by AKASHI Takahiro <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Will Deacon <[email protected]>

Cc Jason Wessel <[email protected]>

---
 arch/arm64/kernel/kgdb.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

arch/arm64/kernel/kgdb.c +16 -0

index b9176b324e5a..fddbc6be3780 100644
@@ -28,6 +28,7 @@
 
 #include <asm/debug-monitors.h>
 #include <asm/insn.h>
+#include <asm/ptrace.h>
 #include <asm/traps.h>
 
 struct dbg_reg_def_t dbg_reg_def[DBG_MAX_REG_NUM] = {

@@ -111,6 +112,8 @@ struct dbg_reg_def_t dbg_reg_def[DBG_MAX_REG_NUM] = {
 	{ "fpcr", 4, -1 },
 };
 
+static DEFINE_PER_CPU(unsigned int, kgdb_pstate);
+
 char *dbg_get_reg(int regno, void *mem, struct pt_regs *regs)
 {
 	if (regno >= DBG_MAX_REG_NUM || regno < 0)

@@ -200,6 +203,10 @@ int kgdb_arch_handle_exception(int exception_vector, int signo,
 		err = 0;
 		break;
 	case 's':
+		/* mask interrupts while single stepping */
+		__this_cpu_write(kgdb_pstate, linux_regs->pstate);
+		linux_regs->pstate |= PSR_I_BIT;
+
 		/*
 		 * Update step address value with address passed
 		 * with step packet.

@@ -242,11 +249,20 @@ NOKPROBE_SYMBOL(kgdb_compiled_brk_fn);
 
 static int kgdb_step_brk_fn(struct pt_regs *regs, unsigned int esr)
 {
+	unsigned int pstate;
+
 	if (!kgdb_single_step)
 		return DBG_HOOK_ERROR;
 
 	kernel_disable_single_step();
 
+	/* restore interrupt mask status */
+	pstate = __this_cpu_read(kgdb_pstate);
+	if (pstate & PSR_I_BIT)
+		regs->pstate |= PSR_I_BIT;
+	else
+		regs->pstate &= ~PSR_I_BIT;
+
 	kgdb_handle_exception(1, SIGTRAP, 0, regs);
 	return 0;
 }

Signature · 1 line

2.11.1

Quoted 11 lines · Will Deacon

Quoted 10 lines · AKASHI Takahiro

After entering kgdb mode, 'stepi' may unexpectedly breaks the execution somewhere in el1_irq.

This happens because a debug exception is always enabled in el1_irq due to the following commit merged in v3.16:
commit 2a2830703a23 ("arm64: debug: avoid accessing mdscr_el1 on fault
paths where possible")
A pending interrupt can be taken after kgdb has enabled a software step, but before a debug exception is actually taken.

Quoted 5 lines

Quoted 1 line

This patch enforces interrupts to be masked while single stepping.

The desired behaviour here boils down to whether or not KGDB expects to step into or over interrupts in response a stepi instruction. What do other architectures do?

I don't know x86 case, but if we step into interrupt code here, doing stepi on a normal function will be almost useless as every attempt of stepi will end up falling into irq code (mostly for timer interrupt).

Quoted 1 line

What happens if the instruction being stepped faults?

Well, as a matter of fact, we get a gdb control somewhere in exception code (actually just after 'enable_dbg' in el1_sync).

Quoted 23 lines · AKASHI Takahiro

Quoted 11 lines · Will Deacon

Quoted 10 lines · AKASHI Takahiro

After entering kgdb mode, 'stepi' may unexpectedly breaks the execution somewhere in el1_irq.

This happens because a debug exception is always enabled in el1_irq due to the following commit merged in v3.16:
commit 2a2830703a23 ("arm64: debug: avoid accessing mdscr_el1 on fault
paths where possible")
A pending interrupt can be taken after kgdb has enabled a software step, but before a debug exception is actually taken.

Quoted 5 lines

Quoted 1 line

This patch enforces interrupts to be masked while single stepping.

The desired behaviour here boils down to whether or not KGDB expects to step into or over interrupts in response a stepi instruction. What do other architectures do?

I don't know x86 case, but if we step into interrupt code here, doing stepi on a normal function will be almost useless as every attempt of stepi will end up falling into irq code (mostly for timer interrupt).

Quoted 4 lines

Quoted 1 line

What happens if the instruction being stepped faults?

Well, as a matter of fact, we get a gdb control somewhere in exception code (actually just after 'enable_dbg' in el1_sync).

Ok, but don't we need to re-enable interrupts, otherwise we can't safely handle the fault (which might involve blocking)?

Will

Quoted 24 lines · Will Deacon

Quoted 23 lines · AKASHI Takahiro

Quoted 11 lines · Will Deacon

Quoted 10 lines · AKASHI Takahiro

After entering kgdb mode, 'stepi' may unexpectedly breaks the execution somewhere in el1_irq.

This happens because a debug exception is always enabled in el1_irq due to the following commit merged in v3.16:
commit 2a2830703a23 ("arm64: debug: avoid accessing mdscr_el1 on fault
paths where possible")
A pending interrupt can be taken after kgdb has enabled a software step, but before a debug exception is actually taken.

Quoted 5 lines

Quoted 1 line

This patch enforces interrupts to be masked while single stepping.

The desired behaviour here boils down to whether or not KGDB expects to step into or over interrupts in response a stepi instruction. What do other architectures do?

I don't know x86 case, but if we step into interrupt code here, doing stepi on a normal function will be almost useless as every attempt of stepi will end up falling into irq code (mostly for timer interrupt).

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

What happens if the instruction being stepped faults?

Well, as a matter of fact, we get a gdb control somewhere in exception code (actually just after 'enable_dbg' in el1_sync).

Ok, but don't we need to re-enable interrupts, otherwise we can't safely handle the fault (which might involve blocking)?

I thought a lot, but have got no other way to solve the issue, which totally makes stepi in vain.
In theory, you might be right, but in practice, people don't always expect to step through the whole sequence of fault recovery with single stepping. Once we do 'c(ontinue),' interrupts are enabled again and the execution will follow as expected.

If you want to 'step over' a faulted instruction here, or to break somewhere in a middle of exception handler, you need to manage to set a breakpoint explicitly. But it will, I believe, be much better than useless stepi from day-1 :)

Meanwhile, kprobes also disables interrupts while single stepping. See setup_singlestep().

Quoted 6 lines · AKASHI Takahiro

Quoted 4 lines · Will Deacon

On Wed, Jun 07, 2017 at 02:34:50PM +0900, AKASHI Takahiro wrote:

Ok, but don't we need to re-enable interrupts, otherwise we can't safely handle the fault (which might involve blocking)?

Quoted 6 lines

I thought a lot, but have got no other way to solve the issue, which totally makes stepi in vain.
In theory, you might be right, but in practice, people don't always expect to step through the whole sequence of fault recovery with single stepping. Once we do 'c(ontinue),' interrupts are enabled again and the execution will follow as expected.

It's not the stepping guarantees I'm worried about. I'm more worried that the fault handler panics because it's called with IRQs disabled,

so the
debugger has ended up changing the behaviour of the kernel which is absolutely not what you want!

Quoted 7 lines

If you want to 'step over' a faulted instruction here, or to break somewhere in a middle of exception handler, you need to manage to set a breakpoint explicitly. But it will, I believe, be much better than useless stepi from day-1 :)

Meanwhile, kprobes also disables interrupts while single stepping. See setup_singlestep().

Sure, but I don't think those instructions can fault.

Can KGDB make the same
guarantees?

Will

Quoted 14 lines · Will Deacon

Quoted 12 lines · AKASHI Takahiro

Quoted 4 lines · Will Deacon

On Wed, Jun 07, 2017 at 02:34:50PM +0900, AKASHI Takahiro wrote:

Ok, but don't we need to re-enable interrupts, otherwise we can't safely handle the fault (which might involve blocking)?

I thought a lot, but have got no other way to solve the issue, which totally makes stepi in vain.
In theory, you might be right, but in practice, people don't always expect to step through the whole sequence of fault recovery with single stepping. Once we do 'c(ontinue),' interrupts are enabled again and the execution will follow as expected.

Quoted 2 lines

It's not the stepping guarantees I'm worried about. I'm more worried that the fault handler panics because it's called with IRQs disabled,

I'm still wondering how it can cause panic.