[PATCH v3 0/5] ARM64: disable irq between breakpoint and step exception

[PATCH v3 0/5] ARM64: disable irq between breakpoint and step exception

[Pratyush Anand]

v2 -> v3
- Moved step_needed from uapi structure to kernel only structure
- Re-enable interrupt if stepped instruction faults
- Modified register_wide_hw_breakpoint() to accept step_needed arg
v2 was here: http://marc.info/?l=linux-arm-kernel&m=149942910730496&w=2

v1 -> v2:
- patch 1 of v1 has been modified to patch 1-3 of v2.
- Introduced a new event attribute step_needed and implemented
  hw_breakpoint_needs_single_step() (patch 1)
- Replaced usage of is_default_overflow_handler() with
  hw_breakpoint_needs_single_step(). (patch 2)
- Modified sample test to set set step_needed bit field (patch 3)
v1 was here: http://marc.info/?l=linux-arm-kernel&m=149910958418708&w=2

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.
Since, it also takes care of SW breakpoint, so I hope kgdb should also be
fine. However, I have not tested that.
@Takahiro: Will it be possible to test these patches for kgdb.

[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 (5):
  hw_breakpoint: Add step_needed event attribute
  arm64: use hw_breakpoint_needs_single_step() to decide if step is
    needed
  register_wide_hw_breakpoint(): modify to accept step_needed arg
  arm64: disable irq between breakpoint and step exception
  arm64: fault: re-enable irq if it was disabled for single stepping

 arch/arm64/kernel/debug-monitors.c      |  3 +++
 arch/arm64/kernel/hw_breakpoint.c       | 10 +++++-----
 arch/arm64/mm/fault.c                   | 28 ++++++++++++++++++++++++----
 arch/x86/kernel/kgdb.c                  |  2 +-
 include/linux/hw_breakpoint.h           | 10 ++++++++--
 include/linux/perf_event.h              |  6 ++++++
 kernel/events/core.c                    |  2 ++
 kernel/events/hw_breakpoint.c           |  4 +++-
 samples/hw_breakpoint/data_breakpoint.c |  3 ++-
 9 files changed, 54 insertions(+), 14 deletions(-)

-- 
2.9.4

[PATCH v3 1/5] hw_breakpoint: Add step_needed event attribute

[Pratyush Anand]

Architecture like ARM64 currently allows to use default hw breakpoint
single step handler only to perf. However, some other users like few
systemtap tests or kernel test in
samples/hw_breakpoint/data_breakpoint.c can also work with default step
handler implementation.

Therefore, this patch introduces a flag 'step_needed' in struct
hw_perf_event, so that arch specific code(specially on arm64) can make a
decision to enable single stepping.

Any architecture which is not using this field will not have any
side effect.

Signed-off-by: Pratyush Anand <panand@redhat.com>
---
 include/linux/hw_breakpoint.h | 6 ++++++
 include/linux/perf_event.h    | 6 ++++++
 kernel/events/core.c          | 2 ++
 3 files changed, 14 insertions(+)

diff --git a/include/linux/hw_breakpoint.h b/include/linux/hw_breakpoint.h
index 0464c85e63fd..b9ac9629bf74 100644
--- a/include/linux/hw_breakpoint.h
+++ b/include/linux/hw_breakpoint.h
@@ -38,6 +38,12 @@ static inline int hw_breakpoint_type(struct perf_event *bp)
 	return bp->attr.bp_type;
 }
 
+static inline bool
+hw_breakpoint_needs_single_step(struct perf_event *bp)
+{
+	return bp->hw.step_needed;
+}
+
 static inline unsigned long hw_breakpoint_len(struct perf_event *bp)
 {
 	return bp->attr.bp_len;
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 24a635887f28..7da951f94b47 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -233,6 +233,12 @@ struct hw_perf_event {
 	 */
 	u64				freq_time_stamp;
 	u64				freq_count_stamp;
+	/*
+	 * A HW breakpoint user can either have it's own step handling
+	 * mechanism or it can use default step handling meachanism defined
+	 * by arch code. Set step_needed to use default mechanism.
+	 */
+	int				step_needed;
 #endif
 };
 
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 6c4e523dc1e2..66ce5574e778 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -9444,9 +9444,11 @@ perf_event_alloc(struct perf_event_attr *attr, int cpu,
 	} else if (is_write_backward(event)){
 		event->overflow_handler = perf_event_output_backward;
 		event->overflow_handler_context = NULL;
+		event->hw.step_needed = 1;
 	} else {
 		event->overflow_handler = perf_event_output_forward;
 		event->overflow_handler_context = NULL;
+		event->hw.step_needed = 1;
 	}
 
 	perf_event__state_init(event);
-- 
2.9.4

[PATCH v3 1/5] hw_breakpoint: Add step_needed event attribute

[Peter Zijlstra]

On Mon, Jul 31, 2017 at 04:10:29PM +0530, Pratyush Anand wrote:
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index 24a635887f28..7da951f94b47 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -233,6 +233,12 @@ struct hw_perf_event {
>  	 */
>  	u64				freq_time_stamp;
>  	u64				freq_count_stamp;
> +	/*
> +	 * A HW breakpoint user can either have it's own step handling
> +	 * mechanism or it can use default step handling meachanism defined
> +	 * by arch code. Set step_needed to use default mechanism.
> +	 */
> +	int				step_needed;

You'll note that there is an anonymous structure inside the anonymous
union specifically dedicated to hardware breakpoint state. Why not put
it there?

>  #endif
>  };
>  
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 6c4e523dc1e2..66ce5574e778 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -9444,9 +9444,11 @@ perf_event_alloc(struct perf_event_attr *attr, int cpu,
>  	} else if (is_write_backward(event)){
>  		event->overflow_handler = perf_event_output_backward;
>  		event->overflow_handler_context = NULL;
> +		event->hw.step_needed = 1;
>  	} else {
>  		event->overflow_handler = perf_event_output_forward;
>  		event->overflow_handler_context = NULL;
> +		event->hw.step_needed = 1;
>  	}

These here need a comment, because even if I'd know now why the heck we
need that = 1 here, I'd sure as hell won't know tomorrow.

[PATCH v3 2/5] arm64: use hw_breakpoint_needs_single_step() to decide if step is needed

[Pratyush Anand]

Currently we use is_default_overflow_handler() to decide whether a
"step" will be needed or not. However, is_default_overflow_handler() is
true only for perf implementation. There can be some custom kernel
module tests like samples/hw_breakpoint/data_breakpoint.c which can
rely on default step handler.

hw_breakpoint_needs_single_step() will be true if any hw_breakpoint user
wants to use default step handler and sets step_needed in hw_perf_event.

Signed-off-by: Pratyush Anand <panand@redhat.com>
---
 arch/arm64/kernel/hw_breakpoint.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c
index 749f81779420..9a73f85ab9ad 100644
--- a/arch/arm64/kernel/hw_breakpoint.c
+++ b/arch/arm64/kernel/hw_breakpoint.c
@@ -661,7 +661,7 @@ 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 (hw_breakpoint_needs_single_step(bp))
 			step = 1;
 unlock:
 		rcu_read_unlock();
@@ -789,7 +789,7 @@ 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 (hw_breakpoint_needs_single_step(wp))
 			step = 1;
 	}
 	if (min_dist > 0 && min_dist != -1) {
@@ -800,7 +800,7 @@ 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 (hw_breakpoint_needs_single_step(wp))
 			step = 1;
 	}
 	rcu_read_unlock();
-- 
2.9.4

[PATCH v3 3/5] register_wide_hw_breakpoint(): modify to accept step_needed arg

[Pratyush Anand]

arch like ARM64 expects 'step_needed = 1'  in order to use default
single step handler. Therefore, modify register_wide_hw_breakpoint()
implementation,so that we can set this field in struct hw_perf_event to
be used later by arch specific code.

Other arch will not have any affect as they do not use it so far.

Signed-off-by: Pratyush Anand <panand@redhat.com>
---
 arch/x86/kernel/kgdb.c                  | 2 +-
 include/linux/hw_breakpoint.h           | 4 ++--
 kernel/events/hw_breakpoint.c           | 4 +++-
 samples/hw_breakpoint/data_breakpoint.c | 3 ++-
 4 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/kgdb.c b/arch/x86/kernel/kgdb.c
index 8e36f249646e..19b24c50a952 100644
--- a/arch/x86/kernel/kgdb.c
+++ b/arch/x86/kernel/kgdb.c
@@ -674,7 +674,7 @@ void kgdb_arch_late(void)
 	for (i = 0; i < HBP_NUM; i++) {
 		if (breakinfo[i].pev)
 			continue;
-		breakinfo[i].pev = register_wide_hw_breakpoint(&attr, NULL, NULL);
+		breakinfo[i].pev = register_wide_hw_breakpoint(&attr, NULL, NULL, 0);
 		if (IS_ERR((void * __force)breakinfo[i].pev)) {
 			printk(KERN_ERR "kgdb: Could not allocate hw"
 			       "breakpoints\nDisabling the kernel debugger\n");
diff --git a/include/linux/hw_breakpoint.h b/include/linux/hw_breakpoint.h
index b9ac9629bf74..8cbc8ded6d50 100644
--- a/include/linux/hw_breakpoint.h
+++ b/include/linux/hw_breakpoint.h
@@ -71,7 +71,7 @@ register_wide_hw_breakpoint_cpu(struct perf_event_attr *attr,
 extern struct perf_event * __percpu *
 register_wide_hw_breakpoint(struct perf_event_attr *attr,
 			    perf_overflow_handler_t triggered,
-			    void *context);
+			    void *context, int step);
 
 extern int register_perf_hw_breakpoint(struct perf_event *bp);
 extern int __register_perf_hw_breakpoint(struct perf_event *bp);
@@ -110,7 +110,7 @@ register_wide_hw_breakpoint_cpu(struct perf_event_attr *attr,
 static inline struct perf_event * __percpu *
 register_wide_hw_breakpoint(struct perf_event_attr *attr,
 			    perf_overflow_handler_t triggered,
-			    void *context)		{ return NULL; }
+			    void *context, int step)	{ return NULL; }
 static inline int
 register_perf_hw_breakpoint(struct perf_event *bp)	{ return -ENOSYS; }
 static inline int
diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
index 3f8cb1e14588..0dcb175276b2 100644
--- a/kernel/events/hw_breakpoint.c
+++ b/kernel/events/hw_breakpoint.c
@@ -492,13 +492,14 @@ EXPORT_SYMBOL_GPL(unregister_hw_breakpoint);
  * register_wide_hw_breakpoint - register a wide breakpoint in the kernel
  * @attr: breakpoint attributes
  * @triggered: callback to trigger when we hit the breakpoint
+ * @step: tells if framework can use default arch step handler
  *
  * @return a set of per_cpu pointers to perf events
  */
 struct perf_event * __percpu *
 register_wide_hw_breakpoint(struct perf_event_attr *attr,
 			    perf_overflow_handler_t triggered,
-			    void *context)
+			    void *context, int step)
 {
 	struct perf_event * __percpu *cpu_events, *bp;
 	long err = 0;
@@ -512,6 +513,7 @@ register_wide_hw_breakpoint(struct perf_event_attr *attr,
 	for_each_online_cpu(cpu) {
 		bp = perf_event_create_kernel_counter(attr, cpu, NULL,
 						      triggered, context);
+		bp->hw.step_needed = step;
 		if (IS_ERR(bp)) {
 			err = PTR_ERR(bp);
 			break;
diff --git a/samples/hw_breakpoint/data_breakpoint.c b/samples/hw_breakpoint/data_breakpoint.c
index ef7f32291852..f64e59f9fbc6 100644
--- a/samples/hw_breakpoint/data_breakpoint.c
+++ b/samples/hw_breakpoint/data_breakpoint.c
@@ -60,7 +60,8 @@ static int __init hw_break_module_init(void)
 	attr.bp_len = HW_BREAKPOINT_LEN_4;
 	attr.bp_type = HW_BREAKPOINT_W | HW_BREAKPOINT_R;
 
-	sample_hbp = register_wide_hw_breakpoint(&attr, sample_hbp_handler, NULL);
+	sample_hbp = register_wide_hw_breakpoint(&attr, sample_hbp_handler,
+						 NULL, 1);
 	if (IS_ERR((void __force *)sample_hbp)) {
 		ret = PTR_ERR((void __force *)sample_hbp);
 		goto fail;
-- 
2.9.4

[PATCH v3 4/5] arm64: disable irq between breakpoint and step exception

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

Since we also fix brk_handler() for the above rule, so all SW kernel
breakpoint handler like kgdb and kprobe should behave similar to perf HW
breakpoint. Interrupt will be disabled if kgdb or kprobe breakpoint
handler requires a single stepping.

Signed-off-by: Pratyush Anand <panand@redhat.com>
---
 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(-)

diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
index d618e25c3de1..16f29f853b54 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -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);
diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c
index 9a73f85ab9ad..d39b8039c70e 100644
--- a/arch/arm64/kernel/hw_breakpoint.c
+++ b/arch/arm64/kernel/hw_breakpoint.c
@@ -697,7 +697,7 @@ static int breakpoint_handler(unsigned long unused, unsigned int esr,
 		}
 	}
 
-	return 0;
+	return 1;
 }
 NOKPROBE_SYMBOL(breakpoint_handler);
 
@@ -840,7 +840,7 @@ static int watchpoint_handler(unsigned long addr, unsigned int esr,
 		}
 	}
 
-	return 0;
+	return 1;
 }
 NOKPROBE_SYMBOL(watchpoint_handler);
 
diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
index 37b95dff0b07..ce5290dacba3 100644
--- a/arch/arm64/mm/fault.c
+++ b/arch/arm64/mm/fault.c
@@ -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))
-- 
2.9.4

[PATCH v3 5/5] arm64: fault: re-enable irq if it was disabled for single stepping

[Pratyush Anand]

We disable irq before single stepping and re-enable it after it.
However, if stepped instruction will cause a fault then we will enter
into fault handler with interrupt disabled, which is not desired.
But, we should be safe if we re-enable interrupt in fault handler if
it was disabled for single stepping.

Signed-off-by: Pratyush Anand <panand@redhat.com>
---
 arch/arm64/mm/fault.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
index ce5290dacba3..2b88807eb964 100644
--- a/arch/arm64/mm/fault.c
+++ b/arch/arm64/mm/fault.c
@@ -589,6 +589,8 @@ static const struct fault_info fault_info[] = {
 	{ do_bad,		SIGBUS,  0,		"unknown 63"			},
 };
 
+static DEFINE_PER_CPU(bool, irq_enable_needed);
+
 /*
  * Dispatch a data abort to the relevant handler.
  */
@@ -597,6 +599,12 @@ asmlinkage void __exception do_mem_abort(unsigned long addr, unsigned int esr,
 {
 	const struct fault_info *inf = esr_to_fault_info(esr);
 	struct siginfo info;
+	bool *irq_en_needed = this_cpu_ptr(&irq_enable_needed);
+
+	if (*irq_en_needed) {
+		regs->pstate &= ~PSR_I_BIT;
+		*irq_en_needed = false;
+	}
 
 	if (!inf->fn(addr, esr, regs))
 		return;
@@ -672,8 +680,6 @@ 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)
-- 
2.9.4

[PATCH v3 0/5] ARM64: disable irq between breakpoint and step exception

[James Morse]

Hi Pratyush,

On 31/07/17 11:40, Pratyush Anand wrote:
> 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 caught my eye as I've been reworking the order the DAIF flags get
set/cleared[0].

What causes your infinite loop? Is it single-stepping kernel_exit? If so patch 4
"arm64: entry.S mask all exceptions during kernel_exit" [1] may help.

If its more like "single stepping something we didn't expect" you will get the
same problem if we take an SError. (which with that series is unmasked ~all the
time).
Either way this looks like a new and exciting way of hitting the 'known issue'
described in patch 12 [3].


Would disabling MDSCR_EL1.SS if we took an exception solve your problem?

If so, I think we should add a new flag, 'TIF_KSINGLESTEP', causing us to
save/restore MDSCR_EL1.SS into pt_regs on el1 exceptions. This would let us
single-step without modifying the DAIF flags for the location we are stepping,
and allow taking any kind of exception from that location.

We should disable nested users of single-step, we can do that by testing the
flag, print a warning then pretend we missed the breakpoint. (hence it needs to
be separate from the user single-step flag).


Thanks,

James

[0] https://www.spinics.net/lists/arm-kernel/msg596684.html
[1] https://www.spinics.net/lists/arm-kernel/msg596686.html
[2] https://www.spinics.net/lists/arm-kernel/msg596689.html

[PATCH v3 0/5] ARM64: disable irq between breakpoint and step exception

[Pratyush Anand]

Hi James,

On Monday 31 July 2017 10:45 PM, James Morse wrote:
> Hi Pratyush,
> 
> On 31/07/17 11:40, Pratyush Anand wrote:
>> 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 caught my eye as I've been reworking the order the DAIF flags get
> set/cleared[0].

Thanks for pointing to your series.
> 
> What causes your infinite loop? Is it single-stepping kernel_exit? If so patch 4
> "arm64: entry.S mask all exceptions during kernel_exit" [1] may help.

Flow is like this:
- A SW or HW breakpoint exception is being generated on a cpu lets say CPU5
- Breakpoint handler does something which causes an interrupt to be active on 
the same CPU. In fact there might be many other reasons for an interrupt to be 
active on a CPU while breakpoint handler was being executed.
- So, as soon as we return from breakpoint exception, we go to the IRQ 
exception handler, while we were expecting a single step exception.

I do not think that your patch 4 will help here. That patch disables interrupt 
while kernel_exit will execute.So,until we enable PSR I bit, we can not stop 
an interrupt to be generated before step exception.

You can easily reproduce the issue with following:
# insmod data_breakpoint.ko ksym=__sysrq_enabled
# cat /proc/sys/kernel/sysrq

Where data_breakpoint.ko is module from samples/hw_breakpoint/data_breakpoint.c.

> 
> If its more like "single stepping something we didn't expect" you will get the
> same problem if we take an SError. (which with that series is unmasked ~all the
> time).
> Either way this looks like a new and exciting way of hitting the 'known issue'
> described in patch 12 [3].
> 
> 
> Would disabling MDSCR_EL1.SS if we took an exception solve your problem?
> 
> If so, I think we should add a new flag, 'TIF_KSINGLESTEP', causing us to
> save/restore MDSCR_EL1.SS into pt_regs on el1 exceptions. This would let us
> single-step without modifying the DAIF flags for the location we are stepping,
> and allow taking any kind of exception from that location.
> 
> We should disable nested users of single-step, we can do that by testing the
> flag, print a warning then pretend we missed the breakpoint. (hence it needs to
> be separate from the user single-step flag).
> 
> 
> Thanks,
> 
> James
> 
> [0] https://www.spinics.net/lists/arm-kernel/msg596684.html
> [1] https://www.spinics.net/lists/arm-kernel/msg596686.html
> [2] https://www.spinics.net/lists/arm-kernel/msg596689.html
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

-- 
Regards
Pratyush

[PATCH v3 0/5] ARM64: disable irq between breakpoint and step exception

[James Morse]

Hi Pratyush,

On 01/08/17 05:18, Pratyush Anand wrote:
> On Monday 31 July 2017 10:45 PM, James Morse wrote:
>> On 31/07/17 11:40, Pratyush Anand wrote:
>>> 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.

> You can easily reproduce the issue with following:
> # insmod data_breakpoint.ko ksym=__sysrq_enabled
> # cat /proc/sys/kernel/sysrq

Thanks, that happily dump-stacks forever. Your first three patches fix the
stepping over the watchpoint, I've had a go at fixing the interrupt interaction,
(instead of just masking interrupts).

gdb single-step works, as does kprobes, FWIW for those three:
Tested-by: James Morse <james.morse@arm.com>


>> What causes your infinite loop? 

> Flow is like this:
> - A SW or HW breakpoint exception is being generated on a cpu lets say CPU5
> - Breakpoint handler does something which causes an interrupt to be active on
> the same CPU. In fact there might be many other reasons for an interrupt to be
> active on a CPU while breakpoint handler was being executed.
> - So, as soon as we return from breakpoint exception, we go to the IRQ exception
> handler, while we were expecting a single step exception.

What breaks when this happens?

With your reproducer and the first three patches I see it hitting the watchpoint
multiple times and stepping the irq handler.

I think we have two or three interacting bugs here. I'm not convinced masking
interrupts is the best fix as the data abort handler inherits this value. We
might mask interrupts for a fault that can't be handled with interrupts masked.

I will post some RFC/fixes, but need to get my head round the debug/exception
interaction in the ARM-ARM first!


Thanks,

James

[PATCH v3 0/5] ARM64: disable irq between breakpoint and step exception

[Pratyush Anand]

Hi James,

Thanks for your analysis.

On Wednesday 02 August 2017 10:43 PM, James Morse wrote:
> Hi Pratyush,
> 
> On 01/08/17 05:18, Pratyush Anand wrote:
>> On Monday 31 July 2017 10:45 PM, James Morse wrote:
>>> On 31/07/17 11:40, Pratyush Anand wrote:
>>>> 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.
> 
>> You can easily reproduce the issue with following:
>> # insmod data_breakpoint.ko ksym=__sysrq_enabled
>> # cat /proc/sys/kernel/sysrq
> 
> Thanks, that happily dump-stacks forever. Your first three patches fix the
> stepping over the watchpoint, I've had a go at fixing the interrupt interaction,
> (instead of just masking interrupts).
> 
> gdb single-step works, as does kprobes, FWIW for those three:
> Tested-by: James Morse <james.morse@arm.com>
> 
> 
>>> What causes your infinite loop?
> 
>> Flow is like this:
>> - A SW or HW breakpoint exception is being generated on a cpu lets say CPU5
>> - Breakpoint handler does something which causes an interrupt to be active on
>> the same CPU. In fact there might be many other reasons for an interrupt to be
>> active on a CPU while breakpoint handler was being executed.
>> - So, as soon as we return from breakpoint exception, we go to the IRQ exception
>> handler, while we were expecting a single step exception.
> 
> What breaks when this happens?

I think, as soon as we call enable_dbg from el1_irq, step exception will be 
generated, and we will be stepping very first instruction after enable_dbg 
which is not expected by single step handler.

> 
> With your reproducer and the first three patches I see it hitting the watchpoint
> multiple times and stepping the irq handle
Lets say we were executing instruction from address 0x2000 when watchpoint 
exception occurred. We programmed, ELR with 0x2000 for single stepping, 
however we received an interrupt before instruction at 0x2000 could have been 
single stepped.

Now if 0x3010 is the address next to enable_dbg from el1_irq, then the 
instruction from address 0x3010 will be single stepped. We will jump back to 
0x2000 again after addressing el1_irq, but that is no more available for 
single-stepping and while executing instruction at that address we again land 
into watchpoint exception handler.

I do not have a HW debugger, but this is what looked like while going through 
code.

> 
> I think we have two or three interacting bugs here. I'm not convinced masking
> interrupts is the best fix as the data abort handler inherits this value. We
> might mask interrupts for a fault that can't be handled with interrupts masked.
> 

In my understanding problems are:
(1) Single stepping of unwanted instruction (ie. instruction  next to 
enable_dbg from el1_irq)
(2) We do not have memory at the end of el1_irq, so that we can set watchpoint 
exception generating instruction for single stepping.

I think, we can find a way to take care for (2), but not sure how (1) can be 
taken care, without the approach I am taking.

> I will post some RFC/fixes, but need to get my head round the debug/exception
> interaction in the ARM-ARM first!
> 
> 
> Thanks,
> 
> James
> 

-- 
Regards
Pratyush

[PATCH v3 0/5] ARM64: disable irq between breakpoint and step exception

[James Morse]

Hi Pratyush,

On 02/08/17 19:46, Pratyush Anand wrote:
> In my understanding problems are:
> (1) Single stepping of unwanted instruction (ie. instruction  next to enable_dbg
> from el1_irq)
> (2) We do not have memory at the end of el1_irq, so that we can set watchpoint
> exception generating instruction for single stepping.

Yes, for (2) the PSTATE.SS bit is saved in SPSR.SS when we take the irq, but it
isn't restored because we ERET from the irq handler with PSTATE.D clear.


> I think, we can find a way to take care for (2), but not sure how (1) can be
> taken care, without the approach I am taking.

We can fix (1) by making 'enable_dbg' inherit the debug state of the interrupted
EL1, unless the SPSR.SS bit is set, in which case we interrupted a
single-stepped instruction and shouldn't re-enable debug because we know must
MDSCR_EL1.SS be set.

This way a synchronous data-abort from a single-stepped instruction with
interrupts unmasked can unmask interrupts in el1_sync but keep debug disabled.
This should give us the least surprises if we call core-code.

I've posted a series that (in addition to your perf/watchpoint patches) fixes
all the issues I saw with your example. Can you test it fixes the
single-step:interrupt interaction on your platform?


Thanks,

James

[PATCH v3 0/5] ARM64: disable irq between breakpoint and step exception

[AKASHI Takahiro]

Hi Pratyush,

On Mon, Jul 31, 2017 at 04:10:28PM +0530, Pratyush Anand wrote:
> v2 -> v3
> - Moved step_needed from uapi structure to kernel only structure
> - Re-enable interrupt if stepped instruction faults
> - Modified register_wide_hw_breakpoint() to accept step_needed arg
> v2 was here: http://marc.info/?l=linux-arm-kernel&m=149942910730496&w=2
> 
> v1 -> v2:
> - patch 1 of v1 has been modified to patch 1-3 of v2.
> - Introduced a new event attribute step_needed and implemented
>   hw_breakpoint_needs_single_step() (patch 1)
> - Replaced usage of is_default_overflow_handler() with
>   hw_breakpoint_needs_single_step(). (patch 2)
> - Modified sample test to set set step_needed bit field (patch 3)
> v1 was here: http://marc.info/?l=linux-arm-kernel&m=149910958418708&w=2
> 
> 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.
> Since, it also takes care of SW breakpoint, so I hope kgdb should also be
> fine. However, I have not tested that.
> @Takahiro: Will it be possible to test these patches for kgdb.

I have not yet understood the details of your patch, but
I gave it a try and didn't see any difference around the behavior
of kgdb's single stepping.

I also gave a try to James' patch, but again nothing different
as long as kgdb is concerned.
(I'm tackling some issue in single stepping at irq's kernel_exit,
in particular, 'eret'.)

-Takahiro AKASHI


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

[PATCH v3 0/5] ARM64: disable irq between breakpoint and step exception

[Pratyush Anand]

Hi Takahiro,

On Tuesday 01 August 2017 01:44 PM, AKASHI Takahiro wrote:
> Hi Pratyush,
> 
> On Mon, Jul 31, 2017 at 04:10:28PM +0530, Pratyush Anand wrote:
>> v2 -> v3
>> - Moved step_needed from uapi structure to kernel only structure
>> - Re-enable interrupt if stepped instruction faults
>> - Modified register_wide_hw_breakpoint() to accept step_needed arg
>> v2 was here: http://marc.info/?l=linux-arm-kernel&m=149942910730496&w=2
>>
>> v1 -> v2:
>> - patch 1 of v1 has been modified to patch 1-3 of v2.
>> - Introduced a new event attribute step_needed and implemented
>>    hw_breakpoint_needs_single_step() (patch 1)
>> - Replaced usage of is_default_overflow_handler() with
>>    hw_breakpoint_needs_single_step(). (patch 2)
>> - Modified sample test to set set step_needed bit field (patch 3)
>> v1 was here: http://marc.info/?l=linux-arm-kernel&m=149910958418708&w=2
>>
>> 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.
>> Since, it also takes care of SW breakpoint, so I hope kgdb should also be
>> fine. However, I have not tested that.
>> @Takahiro: Will it be possible to test these patches for kgdb.
> 
> I have not yet understood the details of your patch, but
> I gave it a try and didn't see any difference around the behavior
> of kgdb's single stepping.
> 
> I also gave a try to James' patch, but again nothing different
> as long as kgdb is concerned.
> (I'm tackling some issue in single stepping at irq's kernel_exit,
> in particular, 'eret'.)

You mean that you were expecting an step exception after eret (and this eret 
was being called from kgdb breakpoint exception handler), but you got irq 
exception? This is what I understood from your previous patch [0].

If that was the case, then I was expecting that this patch series should help.
See, patch 4/5:
- kgdb breakpoint handler kgdb_brk_fn() will be called from 
arch/arm64/kernel/debug-monitors.c: brk_handler().
- If we are expecting a step exception after servicing this breakpoint 
handler, then kgdb code would have called kernel_enable_single_step(). So, we 
should see kernel_active_single_step() true in brk_handler().
- If above happens then do_debug_exception() will make sure that PSR I bit is 
set before eret is called and we should not see an IRQ exception after eret.

Can you please help me with your reproducer test case?

[0]  http://lists.infradead.org/pipermail/linux-arm-kernel/2017-May/508066.html

-- 
Regards
Pratyush

[PATCH v3 0/5] ARM64: disable irq between breakpoint and step exception

[Pratyush Anand]

Hi Mark, James and Will,

What is the take on this series?

Just to summarize:

- James took first three patches of the series and replaced 4th and 5th patch 
with his 3 patch series [3].
- My patchset disables interrupt while exiting from HW breakpoint/watchpoint 
and SW breakpoint handlers, and re-enables after single step handling..but it 
will work reliably for single stepping even if an interrupt is generated 
during  breakpoint/watchpoint handler execution.
- James's approach keeps interrupt enabled, but it might fail if a hardware 
breakpoint is instrumented on an ISR called between breakpoint and step exception.

@James, I understand there would be other things on priority..do you have a 
plan to fix the pitfall and send a new series. If not, can I address Peter's 
concern on patch 1/5 and send v4?

[3] https://www.spinics.net/lists/arm-kernel/msg598214.html

On Monday 31 July 2017 04:10 PM, Pratyush Anand wrote:
> v2 -> v3
> - Moved step_needed from uapi structure to kernel only structure
> - Re-enable interrupt if stepped instruction faults
> - Modified register_wide_hw_breakpoint() to accept step_needed arg
> v2 was here: http://marc.info/?l=linux-arm-kernel&m=149942910730496&w=2
> 
> v1 -> v2:
> - patch 1 of v1 has been modified to patch 1-3 of v2.
> - Introduced a new event attribute step_needed and implemented
>    hw_breakpoint_needs_single_step() (patch 1)
> - Replaced usage of is_default_overflow_handler() with
>    hw_breakpoint_needs_single_step(). (patch 2)
> - Modified sample test to set set step_needed bit field (patch 3)
> v1 was here: http://marc.info/?l=linux-arm-kernel&m=149910958418708&w=2
> 
> 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.
> Since, it also takes care of SW breakpoint, so I hope kgdb should also be
> fine. However, I have not tested that.
> @Takahiro: Will it be possible to test these patches for kgdb.
> 
> [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 (5):
>    hw_breakpoint: Add step_needed event attribute
>    arm64: use hw_breakpoint_needs_single_step() to decide if step is
>      needed
>    register_wide_hw_breakpoint(): modify to accept step_needed arg
>    arm64: disable irq between breakpoint and step exception
>    arm64: fault: re-enable irq if it was disabled for single stepping
> 
>   arch/arm64/kernel/debug-monitors.c      |  3 +++
>   arch/arm64/kernel/hw_breakpoint.c       | 10 +++++-----
>   arch/arm64/mm/fault.c                   | 28 ++++++++++++++++++++++++----
>   arch/x86/kernel/kgdb.c                  |  2 +-
>   include/linux/hw_breakpoint.h           | 10 ++++++++--
>   include/linux/perf_event.h              |  6 ++++++
>   kernel/events/core.c                    |  2 ++
>   kernel/events/hw_breakpoint.c           |  4 +++-
>   samples/hw_breakpoint/data_breakpoint.c |  3 ++-
>   9 files changed, 54 insertions(+), 14 deletions(-)
> 

-- 
Regards
Pratyush