Date:	Wed, 13 May 2015 05:14:31 +0000 (GMT)
From:	Vaneet Narang <v.narang@...sung.com>
To:	Will Deacon <will.deacon@....com>
Cc:	Maninder Singh <maninder1.s@...sung.com>,
	"linux@....linux.org.uk" <linux@....linux.org.uk>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Amit Arora <amit.arora@...sung.com>,
	AJEET YADAV <ajeet.y@...sung.com>,
	AKHILESH KUMAR <akhilesh.k@...sung.com>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [EDT] [PATCH 1/1] Fix: hw watchpoint continually triggers callback

EP-2DAD0AFA905A4ACB804C4F82A001242F

>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:
>> >> 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@...sung.com>
>> >> Signed-off-by: Maninder Singh <maninder1.s@...sung.com>
>> >> Reviewed-by: Amit Arora <amit.arora@...sung.com>
>> >> Reviewed-by: Ajeet Yadav <ajeet.y@...sung.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.
>> 
>> 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.

>  - 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. 
and even with current implementation there is no such control available but still if debugger don't want to execute this instruction, 
it can just move PC to next instruction.

>> With this issue in place, it makes perf interface unusable. We didn't
>> faced this issue with x86. 

>This is a good point. If perf/hw_breakpoint are supposed to hide the
>Internal details of the debug architecture and make everything look and
>smell like x86, I'd like to see that documented somewhere. I don't think
>we'd generally be able to achieve that whilst solving the caveats I mention
>above, so we'd probably just end up removing this feature altogether, which
>would be a shame (and I don't think possible as it stands, since
>hw_breakpoint doesn't know about its caller).

>Will

The issue which I can see with this fix is kprobe doesn't follow step 1 in interrupt context but we are decoding instruction interrupt context. 
While decoding instruction kprobe allocates instruction slot with mutex protection which is not recommended for interrupt context. 
This can be fixed if we allocate instruction slot per CPU during initialization and will use same slot while execution.

Thanks 
Vaneet Narang

Powered by blists - more mailing lists