Skip to contentSkip to conversation

ARM64: Fix irq generation between breakpoint and step exception

PATCH Superseded Pratyush Anand 2 participants 5 messages Jul 3, 2017 – Jul 4, 2017

Subsystems (2)
arm64 Arm PMU profiling and debugging

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Pratyush Anand

author
#0 lore raw

cover ARM64: Fix irq generation between breakpoint and step exception

samples/hw_breakpoint/data_breakpoint.c passes with x86_64 but fails with ARM64. Even though it has been NAKed previously on upstream [1, 2], I have tried to come up with patches which can resolve it for ARM64 as well.

I noticed that even perf step exception can go into an infinite loop if CPU receives an interrupt while executing breakpoint/watchpoint handler. So, event though we are not concerned about above test, we will have to find a solution for the perf issue.

This patchset attempts to resolve both the issue. Please review.

[1] http://marc.info/?l=linux-arm-kernel&m=149580777524910&w=2
[2] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-April/425266.html

Pratyush Anand (2):
arm64: hw_breakpoint: Allow stepping if a kernel mode overflow handler
exists
arm64: disable irq between breakpoint and step exception

 arch/arm64/kernel/debug-monitors.c |  3 +++
 arch/arm64/kernel/hw_breakpoint.c  | 13 ++++++++-----
 arch/arm64/mm/fault.c              | 22 ++++++++++++++++++----
 3 files changed, 29 insertions(+), 9 deletions(-)
Signature · 1 line
2.9.3
author
#1 lore raw

PATCH 1/2 arm64: hw_breakpoint: Allow stepping if a kernel mode overflow handler exists

Currently we allow to single step only for the perf user. However, we have a kernel sample test (samples/hw_breakpoint/data_breakpoint.c) which implements its own overflow handler. Therefore, additionally allow single stepping if there exists a overflow handler in kernel mode.

We still have issues with test, which causes kernel to go into an infinite loop of overflow_handler being called, and that reveals a corner case bug with perf breakpoint implementation as well. See the next patch, which talks more about it and attempts to resolve it.

Signed-off-by Pratyush Anand <[email protected]>
---
 arch/arm64/kernel/hw_breakpoint.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
arch/arm64/kernel/hw_breakpoint.c +6 -3
index 749f81779420..46dbbf94f72d 100644
@@ -661,7 +661,8 @@ static int breakpoint_handler(unsigned long unused, unsigned int esr,
 		perf_bp_event(bp, regs);
 
 		/* Do we need to handle the stepping? */
-		if (is_default_overflow_handler(bp))
+		if (is_default_overflow_handler(bp) ||
+				(!user_mode(regs) && bp->overflow_handler))
 			step = 1;
 unlock:
 		rcu_read_unlock();

@@ -789,7 +790,8 @@ static int watchpoint_handler(unsigned long addr, unsigned int esr,
 		perf_bp_event(wp, regs);
 
 		/* Do we need to handle the stepping? */
-		if (is_default_overflow_handler(wp))
+		if (is_default_overflow_handler(wp) ||
+				(!user_mode(regs) && wp->overflow_handler))
 			step = 1;
 	}
 	if (min_dist > 0 && min_dist != -1) {

@@ -800,7 +802,8 @@ static int watchpoint_handler(unsigned long addr, unsigned int esr,
 		perf_bp_event(wp, regs);
 
 		/* Do we need to handle the stepping? */
-		if (is_default_overflow_handler(wp))
+		if (is_default_overflow_handler(wp) ||
+				(!user_mode(regs) && wp->overflow_handler))
 			step = 1;
 	}
 	rcu_read_unlock();
Signature · 1 line
2.9.3
author
#4 lore raw

PATCH 2/2 arm64: disable irq between breakpoint and step exception

In reply to Pratyush Anand

If an interrupt is generated between breakpoint and step handler then step handler can not get correct step address. This situation can easily be invoked by samples/hw_breakpoint/data_breakpoint.c. It can also be reproduced if we insert any printk() statement or dump_stack() in perf overflow_handler. So, it seems that perf is working fine just luckily. If the CPU which is handling perf breakpoint handler receives any interrupt then, perf step handler will not execute sanely.

This patch improves do_debug_exception() handling, which enforces now, that exception handler function:
- should return 0 for any software breakpoint and hw breakpoint/watchpoint handler if it does not expect a single step stage
- should return 1 if it expects single step.
- A single step handler should always return 0.
- All handler should return a -ve error in any other case.

Now, we can know in do_debug_exception() that whether a step exception will be followed or not. If there will a step exception then disable irq. Re-enable it after single step handling.

Signed-off-by Pratyush Anand <[email protected]>
---
 arch/arm64/kernel/debug-monitors.c |  3 +++
 arch/arm64/kernel/hw_breakpoint.c  |  4 ++--
 arch/arm64/mm/fault.c              | 22 ++++++++++++++++++----
 3 files changed, 23 insertions(+), 6 deletions(-)
arch/arm64/kernel/debug-monitors.c +3 -0
index d618e25c3de1..16f29f853b54 100644
@@ -325,6 +325,9 @@ static int brk_handler(unsigned long addr, unsigned int esr,
 		return -EFAULT;
 	}
 
+	if (kernel_active_single_step() || test_thread_flag(TIF_SINGLESTEP))
+		return 1;
+
 	return 0;
 }
 NOKPROBE_SYMBOL(brk_handler);
arch/arm64/kernel/hw_breakpoint.c +2 -2
index 46dbbf94f72d..cb0d6cbdb767 100644
@@ -698,7 +698,7 @@ static int breakpoint_handler(unsigned long unused, unsigned int esr,
 		}
 	}
 
-	return 0;
+	return 1;
 }
 NOKPROBE_SYMBOL(breakpoint_handler);
 

@@ -843,7 +843,7 @@ static int watchpoint_handler(unsigned long addr, unsigned int esr,
 		}
 	}
 
-	return 0;
+	return 1;
 }
 NOKPROBE_SYMBOL(watchpoint_handler);
arch/arm64/mm/fault.c +18 -4
index 37b95dff0b07..ce5290dacba3 100644
@@ -653,6 +653,13 @@ static struct fault_info __refdata debug_fault_info[] = {
 	{ do_bad,	SIGBUS,		0,		"unknown 7"		},
 };
 
+/*
+ * fn should return 0 from any software breakpoint and hw
+ * breakpoint/watchpoint handler if it does not expect a single step stage
+ * and 1 if it expects single step followed by its execution. A single step
+ * handler should always return 0. All handler should return a -ve error in
+ * any other case.
+ */
 void __init hook_debug_fault_code(int nr,
 				  int (*fn)(unsigned long, unsigned int, struct pt_regs *),
 				  int sig, int code, const char *name)

@@ -665,6 +672,8 @@ void __init hook_debug_fault_code(int nr,
 	debug_fault_info[nr].name	= name;
 }
 
+static DEFINE_PER_CPU(bool, irq_enable_needed);
+
 asmlinkage int __exception do_debug_exception(unsigned long addr,
 					      unsigned int esr,
 					      struct pt_regs *regs)

@@ -672,6 +681,7 @@ asmlinkage int __exception do_debug_exception(unsigned long addr,
 	const struct fault_info *inf = debug_fault_info + DBG_ESR_EVT(esr);
 	struct siginfo info;
 	int rv;
+	bool *irq_en_needed = this_cpu_ptr(&irq_enable_needed);
 
 	/*
 	 * Tell lockdep we disabled irqs in entry.S. Do nothing if they were

@@ -680,9 +690,8 @@ asmlinkage int __exception do_debug_exception(unsigned long addr,
 	if (interrupts_enabled(regs))
 		trace_hardirqs_off();
 
-	if (!inf->fn(addr, esr, regs)) {
-		rv = 1;
-	} else {
+	rv = inf->fn(addr, esr, regs);
+	if (rv < 0) {
 		pr_alert("Unhandled debug exception: %s (0x%08x)@0x%016lx\n",
 			 inf->name, esr, addr);
 

@@ -691,7 +700,12 @@ asmlinkage int __exception do_debug_exception(unsigned long addr,
 		info.si_code  = inf->code;
 		info.si_addr  = (void __user *)addr;
 		arm64_notify_die("", regs, &info, 0);
-		rv = 0;
+	} else if (rv == 1 && interrupts_enabled(regs)) {
+		regs->pstate |= PSR_I_BIT;
+		*irq_en_needed = true;
+	} else if (rv == 0 && *irq_en_needed) {
+		regs->pstate &= ~PSR_I_BIT;
+		*irq_en_needed = false;
 	}
 
 	if (interrupts_enabled(regs))
Signature · 1 line
2.9.3
maintainer
#2 lore raw

PATCH 1/2 arm64: hw_breakpoint: Allow stepping if a kernel mode overflow handler exists

In reply to Pratyush Anand
Quoted 20 lines · Pratyush Anand

Currently we allow to single step only for the perf user. However, we have a kernel sample test (samples/hw_breakpoint/data_breakpoint.c) which implements its own overflow handler. Therefore, additionally allow single stepping if there exists a overflow handler in kernel mode.

We still have issues with test, which causes kernel to go into an infinite loop of overflow_handler being called, and that reveals a corner case bug with perf breakpoint implementation as well. See the next patch, which talks more about it and attempts to resolve it.

Signed-off-by Pratyush Anand <[email protected]>
---
 arch/arm64/kernel/hw_breakpoint.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
arch/arm64/kernel/hw_breakpoint.c +0 -0
index 749f81779420..46dbbf94f72d 100644

arch/arm64/kernel/hw_breakpoint.c static int breakpoint_handler(unsigned long unused, unsigned int esr, · line 665

Quoted 6 lines
arch/arm64/kernel/hw_breakpoint.c +2 -1
@@ -661,7 +661,8 @@ static int breakpoint_handler(unsigned long unused, unsigned int esr,
 		perf_bp_event(bp, regs);
 
 		/* Do we need to handle the stepping? */
-		if (is_default_overflow_handler(bp))
+		if (is_default_overflow_handler(bp) ||
+				(!user_mode(regs) && bp->overflow_handler))

I don't think it makes sense to do this differently dependent on the regs.

If common code needs a particular single-stepping behaviour that we can provide, the best thing would be to have a flag on the event, so that we can do something like:

if (event_needs_single_step(bp))

Then we can ensure that the events used by GDB *don't* have that flag set, so we don't step unexpectedly.

Thanks,
Mark.

Quoted 26 lines
arch/arm64/kernel/hw_breakpoint.c +4 -2
 			step = 1;
 unlock:
 		rcu_read_unlock();

@@ -789,7 +790,8 @@ static int watchpoint_handler(unsigned long addr, unsigned int esr,
 		perf_bp_event(wp, regs);
 
 		/* Do we need to handle the stepping? */
-		if (is_default_overflow_handler(wp))
+		if (is_default_overflow_handler(wp) ||
+				(!user_mode(regs) && wp->overflow_handler))
 			step = 1;
 	}
 	if (min_dist > 0 && min_dist != -1) {

@@ -800,7 +802,8 @@ static int watchpoint_handler(unsigned long addr, unsigned int esr,
 		perf_bp_event(wp, regs);
 
 		/* Do we need to handle the stepping? */
-		if (is_default_overflow_handler(wp))
+		if (is_default_overflow_handler(wp) ||
+				(!user_mode(regs) && wp->overflow_handler))
 			step = 1;
 	}
 	rcu_read_unlock();
Signature · 1 line
2.9.3
author
#3 lore raw

PATCH 1/2 arm64: hw_breakpoint: Allow stepping if a kernel mode overflow handler exists

Hi Mark,

Quoted 31 lines · Mark Rutland
Quoted 20 lines · Pratyush Anand

Currently we allow to single step only for the perf user. However, we have a kernel sample test (samples/hw_breakpoint/data_breakpoint.c) which implements its own overflow handler. Therefore, additionally allow single stepping if there exists a overflow handler in kernel mode.

We still have issues with test, which causes kernel to go into an infinite loop of overflow_handler being called, and that reveals a corner case bug with perf breakpoint implementation as well. See the next patch, which talks more about it and attempts to resolve it.

Signed-off-by Pratyush Anand <[email protected]>
---
 arch/arm64/kernel/hw_breakpoint.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
arch/arm64/kernel/hw_breakpoint.c +0 -0
index 749f81779420..46dbbf94f72d 100644

arch/arm64/kernel/hw_breakpoint.c static int breakpoint_handler(unsigned long unused, unsigned int esr, · line 665

Quoted 6 lines
arch/arm64/kernel/hw_breakpoint.c +2 -1
@@ -661,7 +661,8 @@ static int breakpoint_handler(unsigned long unused, unsigned int esr,
 		perf_bp_event(bp, regs);
 
 		/* Do we need to handle the stepping? */
-		if (is_default_overflow_handler(bp))
+		if (is_default_overflow_handler(bp) ||
+				(!user_mode(regs) && bp->overflow_handler))

I don't think it makes sense to do this differently dependent on the regs.

Quoted 9 lines

If common code needs a particular single-stepping behaviour that we can provide, the best thing would be to have a flag on the event, so that we can do something like:

if (event_needs_single_step(bp))

Then we can ensure that the events used by GDB *don't* have that flag set, so we don't step unexpectedly.

I think, that would be doable. I can send another version with these modification. I will wait for some more time for other review comments for 2/2 (if any).

Thanks for your feedback.

Pratyush

Quoted 29 lines

Thanks,
Mark.

Quoted 26 lines
arch/arm64/kernel/hw_breakpoint.c +4 -3
 			step = 1;
 unlock:
 		rcu_read_unlock();

@@ -789,7 +790,8 @@ static int watchpoint_handler(unsigned long addr, unsigned int esr,
 		perf_bp_event(wp, regs);
 
 		/* Do we need to handle the stepping? */
-		if (is_default_overflow_handler(wp))
+		if (is_default_overflow_handler(wp) ||
+				(!user_mode(regs) && wp->overflow_handler))
 			step = 1;
 	}
 	if (min_dist > 0 && min_dist != -1) {

@@ -800,7 +802,8 @@ static int watchpoint_handler(unsigned long addr, unsigned int esr,
 		perf_bp_event(wp, regs);
 
 		/* Do we need to handle the stepping? */
-		if (is_default_overflow_handler(wp))
+		if (is_default_overflow_handler(wp) ||
+				(!user_mode(regs) && wp->overflow_handler))
 			step = 1;
 	}
 	rcu_read_unlock();
--

2.9.3