[EDT] [PATCH 1/1] Fix: hw watchpoint continually triggers callback

[EDT] [PATCH 1/1] Fix: hw watchpoint continually triggers callback

[Maninder Singh]

EP-2DAD0AFA905A4ACB804C4F82A001242F

On ARM, when a watchpoint is registered using register_wide_hw_breakpoint, 
the callback handler endlessly runs until the watchpoint is unregistered.
The reason for this issue is debug interrupts gets raised before executing the instruction,
and after interrupt handling ARM tries to execute the same instruction again , which results
in interrupt getting raised again.

This patch fixes this issue by using KPROBES (getting the instruction executed and incrementing PC
to next instruction).

Signed-off-by: Vaneet Narang <v.narang@samsung.com>
Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
Reviewed-by: Amit Arora <amit.arora@samsung.com>
Reviewed-by: Ajeet Yadav <ajeet.y@samsung.com>
---
 arch/arm/kernel/hw_breakpoint.c |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/arch/arm/kernel/hw_breakpoint.c b/arch/arm/kernel/hw_breakpoint.c
index dc7d0a9..ec72f86 100644
--- a/arch/arm/kernel/hw_breakpoint.c
+++ b/arch/arm/kernel/hw_breakpoint.c
@@ -37,6 +37,9 @@
 #include <asm/hw_breakpoint.h>
 #include <asm/kdebug.h>
 #include <asm/traps.h>
+#ifdef CONFIG_KPROBES
+#include <linux/kprobes.h>
+#endif
 
 /* Breakpoint currently in use for each BRP. */
 static DEFINE_PER_CPU(struct perf_event *, bp_on_reg[ARM_MAX_BRP]);
@@ -757,6 +760,21 @@ static void watchpoint_handler(unsigned long addr, unsigned int fsr,
 		 */
 		if (!wp->overflow_handler)
 			enable_single_step(wp, instruction_pointer(regs));
+#ifdef CONFIG_KPROBES
+		else {
+			struct kprobe kp;
+			unsigned long flags;
+
+			arch_uninstall_hw_breakpoint(wp);
+			kp.addr = (kprobe_opcode_t *)instruction_pointer(regs);
+			if (!arch_prepare_kprobe(&kp)) {
+				local_irq_save(flags);
+				kp.ainsn.insn_singlestep(&kp, regs);
+				local_irq_restore(flags);
+			}
+			arch_install_hw_breakpoint(wp);
+		}
+#endif
 
 unlock:
 		rcu_read_unlock();
-- 
1.7.1


Thanks ,
Maninder Singh

[EDT] [PATCH 1/1] Fix: hw watchpoint continually triggers callback

[Will Deacon]

On Tue, May 12, 2015 at 12:48:13PM +0100, Maninder Singh wrote:
> EP-2DAD0AFA905A4ACB804C4F82A001242F
> 
> On ARM, when a watchpoint is registered using register_wide_hw_breakpoint, 
> the callback handler endlessly runs until the watchpoint is unregistered.
> The reason for this issue is debug interrupts gets raised before executing the instruction,
> and after interrupt handling ARM tries to execute the same instruction again , which results
> in interrupt getting raised again.
> 
> This patch fixes this issue by using KPROBES (getting the instruction executed and incrementing PC
> to next instruction).
> 
> Signed-off-by: Vaneet Narang <v.narang@samsung.com>
> Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
> Reviewed-by: Amit Arora <amit.arora@samsung.com>
> Reviewed-by: Ajeet Yadav <ajeet.y@samsung.com>
> ---
>  arch/arm/kernel/hw_breakpoint.c |   18 ++++++++++++++++++
>  1 files changed, 18 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/kernel/hw_breakpoint.c b/arch/arm/kernel/hw_breakpoint.c
> index dc7d0a9..ec72f86 100644
> --- a/arch/arm/kernel/hw_breakpoint.c
> +++ b/arch/arm/kernel/hw_breakpoint.c
> @@ -37,6 +37,9 @@
>  #include <asm/hw_breakpoint.h>
>  #include <asm/kdebug.h>
>  #include <asm/traps.h>
> +#ifdef CONFIG_KPROBES
> +#include <linux/kprobes.h>
> +#endif
>  
>  /* Breakpoint currently in use for each BRP. */
>  static DEFINE_PER_CPU(struct perf_event *, bp_on_reg[ARM_MAX_BRP]);
> @@ -757,6 +760,21 @@ static void watchpoint_handler(unsigned long addr, unsigned int fsr,
>  		 */
>  		if (!wp->overflow_handler)
>  			enable_single_step(wp, instruction_pointer(regs));
> +#ifdef CONFIG_KPROBES
> +		else {
> +			struct kprobe kp;
> +			unsigned long flags;
> +
> +			arch_uninstall_hw_breakpoint(wp);
> +			kp.addr = (kprobe_opcode_t *)instruction_pointer(regs);
> +			if (!arch_prepare_kprobe(&kp)) {
> +				local_irq_save(flags);
> +				kp.ainsn.insn_singlestep(&kp, regs);
> +				local_irq_restore(flags);
> +			}
> +			arch_install_hw_breakpoint(wp);
> +		}
> +#endif

I don't think this is the right thing to do at all; the kernel already
handles step exceptions using mismatched breakpoints when there is no
overflow handler specified (e.g. using perf mem events). If you register a
handler (e.g. gdb via ptrace) then you have to handle the step yourself.

Will

[EDT] [PATCH 1/1] Fix: hw watchpoint continually triggers callback

[Will Deacon]

On Wed, May 13, 2015 at 06:14:30AM +0100, Vaneet Narang wrote:
> EP-2DAD0AFA905A4ACB804C4F82A001242F

Ok, I have to ask: what on Earth is this number and what does [EDT] mean?

> >On Tue, May 12, 2015 at 02:12:54PM +0100, Vaneet Narang wrote:
> >> On Tue, May 12, 2015 at 12:48:13PM +0100, Maninder Singh wrote:
> >> This fix is given for kernel developers who wants to use perf interface by
> >> registering callback using register_wide_hw_breakpoint API.  On every
> >> callback trigger they have to unregister watchpoints otherwise callback
> >> gets called in a loop and now issue is "when to register watch point back
> >> ?". 
> 
> >If you want to solve this, I think we need a better way to expose software
> >single-step/emulation to the overflow handler. If we try to do this in
> >the hw_breakpoint code itself, we run into problems:
> 
> >  - What if another thread hits the same instruction whilst we are trying
> >   to step it?
> 
> >  - What if there are two breakpoints or a breakpoint + watchpoint
> >   triggered by the same instruction?
> 
> Thanks for you input. I am not sure, issues which you have mentioned with
> this implementation will actually come.
> To address the issues you have raised, I need to brief about kprobe.
> Kprobe follows 3 steps for breakpoint (BP) handling.
> 1. Decode BP instruction.
> 2. Replace BP instruction with new instruction that will generate SWI.
> 3. Execute instruction & move PC to next instruction.
> Kprobe follows step 1 & step 2 on addition of BP and 3rd step is followed
> when SWI gets triggered.
> 
> For this fix we have used step 1 & step 3, we have skipped step 2. As we
> don't know the caller of watch point & also HW generates interrupt so step
> 2 is not required. The only difference is since we don't know the caller
> we can't decode instruction in advance. We have to follow step 1 and step
> 3 when HWI gets triggered.  Since we are not replacing instruction from
> memory and I assume kprobe implementation for execution of instruction in
> interrupt context is tested and stable, so it shouldn't  produce any of
> the above race condition issues.

Ok, so my first point shouldn't be a problem if we're just emulating the
instruction. However, I still think there are corner cases. For example,
imagine hitting a breakpoint on a ldr pc, [&foo] instruction where we've
also got a watchpoint on foo. Even with emulation, it's going to be
difficult to ensure that watchpoint is safely delivered.

As I say, I'd really rather have a kprobes-agnostic way of stepping an
instruction and let the debugger decide whether it wants to use that or
not.

> >  - What if the debugger didn't want to execute the instruction at all?
> 
> if debugger doesn't want to execute instruction then debugger should use
> single step implementation without overflow handler. 

But the debugger might want to use the overflow handler to regain control
on the exception (like ptrace does for userspace).

Will

[EDT] [PATCH 1/1] Fix: hw watchpoint continually triggers callback

[Will Deacon]

On Mon, May 18, 2015 at 02:17:05PM +0100, Vaneet Narang wrote:
> >Ok, so my first point shouldn't be a problem if we're just emulating the
> >instruction. However, I still think there are corner cases. For example,
> >imagine hitting a breakpoint on a ldr pc, [&foo] instruction where we've
> >also got a watchpoint on foo. Even with emulation, it's going to be
> >difficult to ensure that watchpoint is safely delivered.
> >
> >As I say, I'd really rather have a kprobes-agnostic way of stepping an
> >instruction and let the debugger decide whether it wants to use that or
> >not.
> >
> 
> 2 breakpoints will not be any issue but watchpoint + breakpoint is
> interesting scenario with ldr pc, [&foo] instruction in place. 
> How would ARM will behave in this case without kprobe ? I think It will
> keep on generating Watch point interrupt only.

It should work ok, because the mismatch breakpoint won't fire until we've
actually stepped off the faulting instruction.

> With kprobe watchpoint interrupt gets triggered first and as soon as we
> execute ldr pc, [&foo] using kprobe it will trigger Breakpoint interrupt.  
> This can be taken care with special handling for such instruction where PC
> gets changed. Can you please suggest what should be correct behavior in
> this case ?

Ideally, kprobes wouldn't interfere with the any concurrent debugging, but
I suspect that's not actually the reality on any architectures, particularly
if we end up executing copies of instructions out of a kprobes buffer.

> Is this scenario possible with any other instruction. ? I am not able to
> think other instructions. Is it possisble with push or pop ?

Not sure, you'd need to check for anything that can access memory and
write the PC in one instruction.

> >> >  - What if the debugger didn't want to execute the instruction at all?
> >> 
> >> if debugger doesn't want to execute instruction then debugger should use
> >> single step implementation without overflow handler. 
> >
> >But the debugger might want to use the overflow handler to regain control
> >on the exception (like ptrace does for userspace).
> >
> I have tried same kernel module on x86, Linux 3.5. Behavior on x86 is to
> execute instruction.  I am not sure how ptrace works on x86, if
> instruction gets executed without any control from overflow handler.
> Behavior on ARM should be same as x86. Since perf API interface is same on
> ARM as well as x86.

Unfortunately, I don't think we can easily provide that illusion without
breaking ptrace. The proper fix would be to divorce hw_breakpoint from
perf, allowing ptrace to hook directly into the backend, but that's not
a simple task...

Will