Re: [PATCH 2/2] arm64: Fix watchpoint recursion when single-step is wrongly triggered in irq
From: Li Bin
Date: Thu Apr 07 2016 - 07:35:23 EST
Hi Pratyush,
on 2016/4/4 13:17, Pratyush Anand wrote:
> Hi Li,
>
> On 31/03/2016:08:45:05 PM, Li Bin wrote:
>> Hi Pratyush,
>>
>> on 2016/3/21 18:24, Pratyush Anand wrote:
>>> On 21/03/2016:08:37:50 AM, He Kuang wrote:
>>>> On arm64, watchpoint handler enables single-step to bypass the next
>>>> instruction for not recursive enter. If an irq is triggered right
>>>> after the watchpoint, a single-step will be wrongly triggered in irq
>>>> handler, which causes the watchpoint address not stepped over and
>>>> system hang.
>>>
>>> Does patch [1] resolves this issue as well? I hope it should. Patch[1] has still
>>> not been sent for review. Your test result will be helpful.
>>>
>>> ~Pratyush
>>>
>>> [1] https://github.com/pratyushanand/linux/commit/7623c8099ac22eaa00e7e0f52430f7a4bd154652
>>
>> This patch did not consider that, when excetpion return, the singlestep flag
>> should be restored, otherwise the right singlestep will not triggered.
>> Right?
>
> Yes, you are right, and there are other problems as well. Will Deacon pointed
> out [1] that kernel debugging is per-cpu rather than per-task. So, I did thought
> of a per-cpu implementation by introducing a new element "flags" in struct
> pt_regs. But even with that I see issues. For example:
> - While executing single step instruction, we get a data abort
> - In the kernel_entry of data abort we disable single stepping based on "flags"
> bit field
> - While handling data abort we receive anther interrupt, so we are again in
> kernel_entry (for el1_irq). Single stepping will be disabled again (although
> it does not matter).
>
> Now the issue is that, what condition should be verified in kernel_exit for
> enabling single step again? In the above scenario, kernel_exit for el1_irq
> should not enable single stepping, but how to prevent that elegantly?
The condition for kernel_entry to disable the single step is that MDSCR_EL1.SS
has been set. And only when the corresponding kernel_entry has disabled the single
step, the kernel_exit should enable it, but the kernel_exit of single-step exception
should be handled specially, that when disable single step in single-step exception
handler, flag of pt_regs stored in stack should be cleard to prevent to be re-enabled
by kernel_exit.
Thanks,
Li Bin
---
arch/arm64/include/asm/assembler.h | 11 +++++++++++
arch/arm64/include/asm/debug-monitors.h | 2 +-
arch/arm64/include/asm/ptrace.h | 2 ++
arch/arm64/kernel/asm-offsets.c | 1 +
arch/arm64/kernel/debug-monitors.c | 3 ++-
arch/arm64/kernel/entry.S | 18 ++++++++++++++++++
arch/arm64/kernel/hw_breakpoint.c | 2 +-
arch/arm64/kernel/kgdb.c | 2 +-
8 files changed, 37 insertions(+), 4 deletions(-)
diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
index 70f7b9e..56a4335 100644
--- a/arch/arm64/include/asm/assembler.h
+++ b/arch/arm64/include/asm/assembler.h
@@ -60,6 +60,17 @@
msr daifclr, #8
.endm
+ .macro disable_step, orig
+ bic \orig, \orig, #1
+ msr mdscr_el1, \orig
+ .endm
+
+ .macro enable_step, orig
+ disable_dbg
+ orr \orig, \orig, #1
+ msr mdscr_el1, \orig
+ .endm
+
.macro disable_step_tsk, flgs, tmp
tbz \flgs, #TIF_SINGLESTEP, 9990f
mrs \tmp, mdscr_el1
diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
index 2fcb9b7..50f114c 100644
--- a/arch/arm64/include/asm/debug-monitors.h
+++ b/arch/arm64/include/asm/debug-monitors.h
@@ -115,7 +115,7 @@ void user_rewind_single_step(struct task_struct *task);
void user_fastforward_single_step(struct task_struct *task);
void kernel_enable_single_step(struct pt_regs *regs);
-void kernel_disable_single_step(void);
+void kernel_disable_single_step(struct pt_regs *regs);
int kernel_active_single_step(void);
#ifdef CONFIG_HAVE_HW_BREAKPOINT
diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
index a307eb6..da00cd8 100644
--- a/arch/arm64/include/asm/ptrace.h
+++ b/arch/arm64/include/asm/ptrace.h
@@ -113,6 +113,8 @@ struct pt_regs {
u64 sp;
u64 pc;
u64 pstate;
+ u64 flags;
+ u64 padding;
};
};
u64 orig_x0;
diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
index 3ae6b31..7936ba9 100644
--- a/arch/arm64/kernel/asm-offsets.c
+++ b/arch/arm64/kernel/asm-offsets.c
@@ -56,6 +56,7 @@ int main(void)
DEFINE(S_COMPAT_SP, offsetof(struct pt_regs, compat_sp));
#endif
DEFINE(S_PSTATE, offsetof(struct pt_regs, pstate));
+ DEFINE(S_FLAGS, offsetof(struct pt_regs, flags));
DEFINE(S_PC, offsetof(struct pt_regs, pc));
DEFINE(S_ORIG_X0, offsetof(struct pt_regs, orig_x0));
DEFINE(S_SYSCALLNO, offsetof(struct pt_regs, syscallno));
diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
index c45f296..9d2bdbf 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -403,9 +403,10 @@ void kernel_enable_single_step(struct pt_regs *regs)
enable_debug_monitors(DBG_ACTIVE_EL1);
}
-void kernel_disable_single_step(void)
+void kernel_disable_single_step(struct pt_regs *regs)
{
WARN_ON(!irqs_disabled());
+ regs->flags = 0;
mdscr_write(mdscr_read() & ~DBG_MDSCR_SS);
disable_debug_monitors(DBG_ACTIVE_EL1);
}
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index 12e8d2b..753bef2 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -87,6 +87,15 @@
stp x26, x27, [sp, #16 * 13]
stp x28, x29, [sp, #16 * 14]
+ .if \el == 1
+ mrs x19, mdscr_el1
+ and x20, x19, #1
+ str x20, [sp, #S_FLAGS]
+ tbz x20, #0, 1f
+ disable_step x19
+1:
+ .endif
+
.if \el == 0
mrs x21, sp_el0
mov tsk, sp
@@ -154,6 +163,15 @@ alternative_endif
.endif
msr elr_el1, x21 // set up the return data
msr spsr_el1, x22
+
+ .if \el == 1
+ ldr x19, [sp, #S_FLAGS]
+ tbz x19, #0, 1f
+ mrs x19, mdscr_el1
+ enable_step x19
+1:
+ .endif
+
ldp x0, x1, [sp, #16 * 0]
ldp x2, x3, [sp, #16 * 1]
ldp x4, x5, [sp, #16 * 2]
diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c
index b45c95d..6afbb80 100644
--- a/arch/arm64/kernel/hw_breakpoint.c
+++ b/arch/arm64/kernel/hw_breakpoint.c
@@ -802,7 +802,7 @@ int reinstall_suspended_bps(struct pt_regs *regs)
toggle_bp_registers(AARCH64_DBG_REG_WCR, DBG_ACTIVE_EL0, 1);
if (*kernel_step != ARM_KERNEL_STEP_SUSPEND) {
- kernel_disable_single_step();
+ kernel_disable_single_step(regs);
handled_exception = 1;
} else {
handled_exception = 0;
diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
index b67531a..f92c5fb 100644
--- a/arch/arm64/kernel/kgdb.c
+++ b/arch/arm64/kernel/kgdb.c
@@ -183,7 +183,7 @@ int kgdb_arch_handle_exception(int exception_vector, int signo,
* Received continue command, disable single step
*/
if (kernel_active_single_step())
- kernel_disable_single_step();
+ kernel_disable_single_step(linux_regs);
err = 0;
break;
--
1.7.1
>
> [1] http://www.spinics.net/lists/arm-kernel/msg491844.html
>
> .
>