Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1361615 > unrolled thread

[PATCH 1/2] arm64: Store breakpoint single step state into pstate

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/2] arm64: Store breakpoint single step state into pstate He Kuang <hekuang@huawei.com> - 2016-03-21 09:50 +0100
    [PATCH 2/2] arm64: Fix watchpoint recursion when single-step is wrongly triggered in irq He Kuang <hekuang@huawei.com> - 2016-03-21 09:50 +0100
      Re: [PATCH 2/2] arm64: Fix watchpoint recursion when single-step is  wrongly triggered in irq Pratyush Anand <panand@redhat.com> - 2016-03-21 11:30 +0100
        Re: [PATCH 2/2] arm64: Fix watchpoint recursion when single-step is  wrongly triggered in irq "Wangnan (F)" <wangnan0@huawei.com> - 2016-03-21 11:50 +0100
          Re: [PATCH 2/2] arm64: Fix watchpoint recursion when single-step is  wrongly triggered in irq Pratyush Anand <panand@redhat.com> - 2016-03-21 12:10 +0100
        Re: [PATCH 2/2] arm64: Fix watchpoint recursion when single-step is  wrongly triggered in irq Li Bin <huawei.libin@huawei.com> - 2016-03-31 14:50 +0200
    Re: [PATCH 1/2] arm64: Store breakpoint single step state into pstate Will Deacon <will.deacon@arm.com> - 2016-03-21 17:10 +0100

#1361615 — [PATCH 1/2] arm64: Store breakpoint single step state into pstate

From: Wang Nan <wangnan0@huawei.com>

Store breakpoint single step state into pstate to fix the
recursion issue on ARM64.

Signed-off-by: Kaixu Xia <xiakaixu@huawei.com>
Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
---
 arch/arm64/include/asm/debug-monitors.h |  9 ++++++
 arch/arm64/include/uapi/asm/ptrace.h    | 10 +++++++
 arch/arm64/kernel/hw_breakpoint.c       | 49 +++++++++++++++++++++++++++++++++
 arch/arm64/kernel/signal.c              |  2 ++
 4 files changed, 70 insertions(+)

diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
index 279c85b5..b5902e8 100644
--- a/arch/arm64/include/asm/debug-monitors.h
+++ b/arch/arm64/include/asm/debug-monitors.h
@@ -132,11 +132,20 @@ int kernel_active_single_step(void);
 
 #ifdef CONFIG_HAVE_HW_BREAKPOINT
 int reinstall_suspended_bps(struct pt_regs *regs);
+u64 signal_single_step_enable_bps(void);
+void signal_reinstall_single_step(u64 pstate);
 #else
 static inline int reinstall_suspended_bps(struct pt_regs *regs)
 {
 	return -ENODEV;
 }
+
+static inline u64 signal_single_step_enable_bps(void)
+{
+	return 0;
+}
+
+static inline void signal_reinstall_single_step(u64 pstate) { }
 #endif
 
 int aarch32_break_handler(struct pt_regs *regs);
diff --git a/arch/arm64/include/uapi/asm/ptrace.h b/arch/arm64/include/uapi/asm/ptrace.h
index 208db3d..8dbfdac 100644
--- a/arch/arm64/include/uapi/asm/ptrace.h
+++ b/arch/arm64/include/uapi/asm/ptrace.h
@@ -52,6 +52,16 @@
 #define PSR_N_BIT	0x80000000
 
 /*
+ * pstat in pt_regs and user_pt_regs are 64 bits. The highest 32 bits
+ * of it can be used by kernel. One user of them is signal handler.
+ */
+#define PSR_LINUX_MASK        0xffffffff00000000UL
+#define PSR_LINUX_HW_BP_SS    0x0000000100000000UL    /* Single step and disable breakpoints */
+#define PSR_LINUX_HW_WP_SS    0x0000000200000000UL    /* Single step and disable watchpoints */
+
+#define PSR_LINUX_HW_SS    (PSR_LINUX_HW_BP_SS | PSR_LINUX_HW_WP_SS)
+
+/*
  * Groups of PSR bits
  */
 #define PSR_f		0xff000000	/* Flags		*/
diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c
index b45c95d..18fd3d3 100644
--- a/arch/arm64/kernel/hw_breakpoint.c
+++ b/arch/arm64/kernel/hw_breakpoint.c
@@ -954,3 +954,52 @@ int hw_breakpoint_exceptions_notify(struct notifier_block *unused,
 {
 	return NOTIFY_DONE;
 }
+
+u64 signal_single_step_enable_bps(void)
+{
+	struct debug_info *debug_info = &current->thread.debug;
+	u64 retval = 0;
+
+	if (likely(!debug_info->bps_disabled && !debug_info->wps_disabled))
+		return 0;
+
+	if (debug_info->bps_disabled) {
+		retval |= PSR_LINUX_HW_BP_SS;
+		toggle_bp_registers(AARCH64_DBG_REG_BCR, DBG_ACTIVE_EL0, 1);
+		debug_info->bps_disabled = 0;
+	}
+
+	if (debug_info->wps_disabled) {
+		retval |= PSR_LINUX_HW_WP_SS;
+		toggle_bp_registers(AARCH64_DBG_REG_WCR, DBG_ACTIVE_EL0, 1);
+		debug_info->wps_disabled = 0;
+	}
+
+	if (debug_info->suspended_step)
+		debug_info->suspended_step = 0;
+	else
+		user_disable_single_step(current);
+	return retval;
+}
+
+void signal_reinstall_single_step(u64 pstate)
+{
+	struct debug_info *debug_info = &current->thread.debug;
+
+	if (likely(!(pstate & PSR_LINUX_HW_SS)))
+		return;
+
+	if (pstate & PSR_LINUX_HW_BP_SS) {
+		debug_info->bps_disabled = 1;
+		toggle_bp_registers(AARCH64_DBG_REG_BCR, DBG_ACTIVE_EL0, 0);
+	}
+	if (pstate & PSR_LINUX_HW_WP_SS) {
+		debug_info->wps_disabled = 1;
+		toggle_bp_registers(AARCH64_DBG_REG_WCR, DBG_ACTIVE_EL0, 0);
+	}
+
+	if (test_thread_flag(TIF_SINGLESTEP))
+		debug_info->suspended_step = 1;
+	else
+		user_enable_single_step(current);
+}
diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
index e18c48c..6cb1e49 100644
--- a/arch/arm64/kernel/signal.c
+++ b/arch/arm64/kernel/signal.c
@@ -151,6 +151,7 @@ asmlinkage long sys_rt_sigreturn(struct pt_regs *regs)
 	if (restore_altstack(&frame->uc.uc_stack))
 		goto badframe;
 
+	signal_reinstall_single_step(regs->pstate);
 	return regs->regs[0];
 
 badframe:
@@ -292,6 +293,7 @@ static void handle_signal(struct ksignal *ksig, struct pt_regs *regs)
 	int usig = ksig->sig;
 	int ret;
 
+	regs->pstate |= signal_single_step_enable_bps();
 	/*
 	 * Set up the stack frame
 	 */
-- 
1.8.5.2

[toc] | [next] | [standalone]

#1361616 — [PATCH 2/2] arm64: Fix watchpoint recursion when single-step is wrongly triggered in irq

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.

Problem can be found at the following URL:
"http://thread.gmane.org/gmane.linux.kernel/2167918"

This patch pushes watchpoint status and disables single step if it is
triggered in irq handler and restores them back after irq is handled.

Signed-off-by: Wang Nan <wangnan0@huawei.com>
Signed-off-by: He Kuang <hekuang@huawei.com>
---
 arch/arm64/include/asm/debug-monitors.h |  9 +++++++
 arch/arm64/kernel/debug-monitors.c      | 13 ++++++++++
 arch/arm64/kernel/entry.S               |  6 +++++
 arch/arm64/kernel/hw_breakpoint.c       | 44 +++++++++++++++++++++++++++++++--
 4 files changed, 70 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
index b5902e8..fe6939e 100644
--- a/arch/arm64/include/asm/debug-monitors.h
+++ b/arch/arm64/include/asm/debug-monitors.h
@@ -133,7 +133,10 @@ int kernel_active_single_step(void);
 #ifdef CONFIG_HAVE_HW_BREAKPOINT
 int reinstall_suspended_bps(struct pt_regs *regs);
 u64 signal_single_step_enable_bps(void);
+u64 irq_single_step_enable_bps(void);
+
 void signal_reinstall_single_step(u64 pstate);
+void irq_reinstall_single_step(struct pt_regs *regs);
 #else
 static inline int reinstall_suspended_bps(struct pt_regs *regs)
 {
@@ -145,7 +148,13 @@ static inline u64 signal_single_step_enable_bps(void)
 	return 0;
 }
 
+static inline u64 irq_single_step_enable_bps(void)
+{
+	return 0;
+}
+
 static inline void signal_reinstall_single_step(u64 pstate) { }
+static inline void irq_reinstall_single_step(struct pt_regs *regs) { }
 #endif
 
 int aarch32_break_handler(struct pt_regs *regs);
diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
index c536c9e..fab1faa 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -245,9 +245,22 @@ static void send_user_sigtrap(int si_code)
 	force_sig_info(SIGTRAP, &info, current);
 }
 
+extern unsigned long el1_irq_ss_entry[];
+
 static int single_step_handler(unsigned long addr, unsigned int esr,
 			       struct pt_regs *regs)
 {
+	void *pc = (void *)instruction_pointer(regs);
+
+	if (pc == &el1_irq_ss_entry) {
+		struct pt_regs *irq_regs = (struct pt_regs *)(regs->sp);
+
+		irq_regs->pstate |= irq_single_step_enable_bps();
+		kernel_disable_single_step();
+
+		return 0;
+	}
+
 	/*
 	 * If we are stepping a pending breakpoint, call the hw_breakpoint
 	 * handler first.
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index 1f7f5a2..836d98e 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -402,12 +402,18 @@ ENDPROC(el1_sync)
 el1_irq:
 	kernel_entry 1
 	enable_dbg
+	.global el1_irq_ss_entry
+el1_irq_ss_entry:
 #ifdef CONFIG_TRACE_IRQFLAGS
 	bl	trace_hardirqs_off
 #endif
 
 	get_thread_info tsk
 	irq_handler
+#ifdef CONFIG_HAVE_HW_BREAKPOINT
+	mov	x0, sp
+	bl	irq_reinstall_single_step
+#endif
 
 #ifdef CONFIG_PREEMPT
 	ldr	w24, [tsk, #TI_PREEMPT]		// get preempt count
diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c
index 18fd3d3..0cf13ee 100644
--- a/arch/arm64/kernel/hw_breakpoint.c
+++ b/arch/arm64/kernel/hw_breakpoint.c
@@ -540,11 +540,12 @@ int arch_validate_hwbkpt_settings(struct perf_event *bp)
  * exception level at the register level.
  * This is used when single-stepping after a breakpoint exception.
  */
-static void toggle_bp_registers(int reg, enum dbg_active_el el, int enable)
+static bool toggle_bp_registers(int reg, enum dbg_active_el el, int enable)
 {
 	int i, max_slots, privilege;
 	u32 ctrl;
 	struct perf_event **slots;
+	bool origin_state = false;
 
 	switch (reg) {
 	case AARCH64_DBG_REG_BCR:
@@ -556,7 +557,7 @@ static void toggle_bp_registers(int reg, enum dbg_active_el el, int enable)
 		max_slots = core_num_wrps;
 		break;
 	default:
-		return;
+		return false;
 	}
 
 	for (i = 0; i < max_slots; ++i) {
@@ -568,12 +569,16 @@ static void toggle_bp_registers(int reg, enum dbg_active_el el, int enable)
 			continue;
 
 		ctrl = read_wb_reg(reg, i);
+		if (ctrl & 0x1)
+			origin_state = true;
 		if (enable)
 			ctrl |= 0x1;
 		else
 			ctrl &= ~0x1;
 		write_wb_reg(reg, i, ctrl);
 	}
+
+	return origin_state;
 }
 
 /*
@@ -982,6 +987,41 @@ u64 signal_single_step_enable_bps(void)
 	return retval;
 }
 
+u64 irq_single_step_enable_bps(void)
+{
+	u64 retval = 0;
+
+	if (!toggle_bp_registers(AARCH64_DBG_REG_WCR, DBG_ACTIVE_EL1, 1))
+		retval |= PSR_LINUX_HW_WP_SS;
+
+	if (!toggle_bp_registers(AARCH64_DBG_REG_BCR, DBG_ACTIVE_EL1, 1))
+		retval |= PSR_LINUX_HW_BP_SS;
+
+	return retval;
+}
+
+void irq_reinstall_single_step(struct pt_regs *regs)
+{
+	u64 pstate = regs->pstate;
+
+	if (likely(!(regs->pstate & PSR_LINUX_HW_SS)))
+		return;
+
+	if (!user_mode(regs)) {
+		if (pstate & PSR_LINUX_HW_BP_SS)
+			toggle_bp_registers(AARCH64_DBG_REG_BCR,
+					    DBG_ACTIVE_EL1, 0);
+		if (pstate & PSR_LINUX_HW_WP_SS)
+			toggle_bp_registers(AARCH64_DBG_REG_WCR,
+					    DBG_ACTIVE_EL1, 0);
+
+		if (!kernel_active_single_step()) {
+			asm volatile ("msr     daifset, #8\n");
+			kernel_enable_single_step(regs);
+		}
+	}
+}
+
 void signal_reinstall_single_step(u64 pstate)
 {
 	struct debug_info *debug_info = &current->thread.debug;
-- 
1.8.5.2

[toc] | [prev] | [next] | [standalone]

#1361682 — Re: [PATCH 2/2] arm64: Fix watchpoint recursion when single-step is wrongly triggered in irq

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

[toc] | [prev] | [next] | [standalone]

#1361692 — Re: [PATCH 2/2] arm64: Fix watchpoint recursion when single-step is wrongly triggered in irq

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

Could you please provide a test program for your case so we can test
it on our devices? I guess setting breakpoint on a "copy_from_user()"
accessing an invalid address can trigger this problem?

Thank you.

[toc] | [prev] | [next] | [standalone]

#1361714 — Re: [PATCH 2/2] arm64: Fix watchpoint recursion when single-step is wrongly triggered in irq

On 21/03/2016:06:38:31 PM, Wangnan (F) wrote:
> 
> 
> 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
> 
> Could you please provide a test program for your case so we can test
> it on our devices? I guess setting breakpoint on a "copy_from_user()"
> accessing an invalid address can trigger this problem?

My test case was to test kprobing of copy_from_user. I used kprobe64-v11.

I reverted "patch v11 3/9" and used following script  for __copy_to_user(),
which instruments kprobe at every instruction of a given function. I can easily
see "Unexpected kernel single-step exception at EL1".
-------------------------------------------------------------
#kprobe_at_function_all_inst.sh  
-------------------------------------------------------------
#! /bin/sh
#$1: function name
echo 0 > /sys/kernel/debug/tracing/events/kprobes/enable
echo >  /sys/kernel/debug/tracing/trace
echo > /sys/kernel/debug/tracing/kprobe_events
func=$(cat /proc/kallsyms | grep -A 1 -w $1 | cut -d ' ' -f 1)
func_start=$((0x$(echo $func | cut -d ' ' -f 1)))
func_end=$((0x$(echo $func | cut -d ' ' -f 2)))
offset=0
while [ $(($func_start + $offset)) -lt $func_end ]
  do
          printf -v cmd "p:probe_%x $1+0x%x" $offset $offset
          echo $cmd >> /sys/kernel/debug/tracing/kprobe_events
          offset=$((offset + 4))
  done
echo 1 > /sys/kernel/debug/tracing/events/kprobes/enable
-------------------------------------------------------------

# ./kprobe_at_function_all_inst.sh __copy_to_user

Now, if I apply the patch which I referred in [1], I can no longer see any
"Unexpected kernel single-step exception at EL1" with above test script.

If I understood correctly, then the problem you described in your patch is that
an irq (el1_irq) is raised when watchpoint was being handled by kernel(specially
before kernel could call reinstall_suspended_bps() to disable single stepping).
Since, I disable single stepping for all the el1 exception mode, if
kernel_enable_single_step() had been called but kernel_disable_single_step() had
n't been called. So, your test case could be another good test for my
patch.

~Pratyush

[toc] | [prev] | [next] | [standalone]

#1368261 — Re: [PATCH 2/2] arm64: Fix watchpoint recursion when single-step is wrongly triggered in irq

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?

Thanks,
Li Bin

> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
> .
> 

[toc] | [prev] | [next] | [standalone]

#1361961

On Mon, Mar 21, 2016 at 08:37:49AM +0000, He Kuang wrote:
> From: Wang Nan <wangnan0@huawei.com>
> 
> Store breakpoint single step state into pstate to fix the
> recursion issue on ARM64.
> 
> Signed-off-by: Kaixu Xia <xiakaixu@huawei.com>
> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> ---
>  arch/arm64/include/asm/debug-monitors.h |  9 ++++++
>  arch/arm64/include/uapi/asm/ptrace.h    | 10 +++++++
>  arch/arm64/kernel/hw_breakpoint.c       | 49 +++++++++++++++++++++++++++++++++
>  arch/arm64/kernel/signal.c              |  2 ++
>  4 files changed, 70 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
> index 279c85b5..b5902e8 100644
> --- a/arch/arm64/include/asm/debug-monitors.h
> +++ b/arch/arm64/include/asm/debug-monitors.h
> @@ -132,11 +132,20 @@ int kernel_active_single_step(void);
>  
>  #ifdef CONFIG_HAVE_HW_BREAKPOINT
>  int reinstall_suspended_bps(struct pt_regs *regs);
> +u64 signal_single_step_enable_bps(void);
> +void signal_reinstall_single_step(u64 pstate);
>  #else
>  static inline int reinstall_suspended_bps(struct pt_regs *regs)
>  {
>  	return -ENODEV;
>  }
> +
> +static inline u64 signal_single_step_enable_bps(void)
> +{
> +	return 0;
> +}
> +
> +static inline void signal_reinstall_single_step(u64 pstate) { }
>  #endif
>  
>  int aarch32_break_handler(struct pt_regs *regs);
> diff --git a/arch/arm64/include/uapi/asm/ptrace.h b/arch/arm64/include/uapi/asm/ptrace.h
> index 208db3d..8dbfdac 100644
> --- a/arch/arm64/include/uapi/asm/ptrace.h
> +++ b/arch/arm64/include/uapi/asm/ptrace.h
> @@ -52,6 +52,16 @@
>  #define PSR_N_BIT	0x80000000
>  
>  /*
> + * pstat in pt_regs and user_pt_regs are 64 bits. The highest 32 bits
> + * of it can be used by kernel. One user of them is signal handler.
> + */
> +#define PSR_LINUX_MASK        0xffffffff00000000UL
> +#define PSR_LINUX_HW_BP_SS    0x0000000100000000UL    /* Single step and disable breakpoints */
> +#define PSR_LINUX_HW_WP_SS    0x0000000200000000UL    /* Single step and disable watchpoints */
> +
> +#define PSR_LINUX_HW_SS    (PSR_LINUX_HW_BP_SS | PSR_LINUX_HW_WP_SS)

As I've said before, I'm not at all keen on this approach. We're changing
a UAPI header to include magic numbers that may or may not conflict with
the architecture in future in order to fix a problem that doesn't exist
outside of a contrived test case.

I'd much rather place a restriction on .wakeup_events of the hw_breakpoint
and simply refuse to initialise things in a way that leads to problems down
the line. Ptrace and GDB are the primary users of this interface and I'm not
willing to risk breaking them with these sorts of invasive changes.

Will

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web