Hi Sandeepa, Thanks for the review.
On 23/08/2016:04:33:08 PM, Sandeepa Prabhu wrote: > Thanks for the fix, feel free to add my ACK as well. Has it been tested on > guest kernel? have not tested on guest kernel.Will do. ~Pratyush > > Acked-by: Sandeepa Prabhu <sandeepa.s.pra...@gmail.com> > > > > On Mon, Aug 22, 2016 at 8:35 PM, Masami Hiramatsu <mhira...@kernel.org> > wrote: > > > On Mon, 22 Aug 2016 12:16:00 +0530 > > Pratyush Anand <pan...@redhat.com> wrote: > > > > > Whenever we are hitting a kprobe from a none-kprobe debug exception > > > handler, we hit an infinite occurrences of "Unexpected kernel single-step > > > exception at EL1" > > > > > > PSTATE.D is debug exception mask bit. It is set whenever we enter into an > > > exception mode. When it is set then Watchpoint, Breakpoint, and Software > > > Step exceptions are masked. However, software Breakpoint Instruction > > > exceptions can never be masked. Therefore, if we ever execute a BRK > > > instruction, irrespective of D-bit setting, we will be receiving a > > > corresponding breakpoint exception. > > > For example: > > > - We are executing kprobe pre/post handler, and kprobe has been inserted > > in > > > one of the instruction of a function called by handler. So, it executes > > > BRK instruction and we land into the case of KPROBE_REENTER. (This > > case is > > > already handled by current code) > > > - We are executing uprobe handler or any other BRK handler such as in > > > WARN_ON (BRK BUG_BRK_IMM), and we trace that path using kprobe.So, we > > > enter into kprobe breakpoint handler,from another BRK handler.(This > > case > > > is not being handled currently) > > > In all such cases kprobe breakpoint exception will be raised when we were > > > already in debug exception mode. > > > SPSR's D bit (bit 9) shows the value of PSTATE.D immediately before the > > > exception was taken.So, in above example cases we would find it set in > > > kprobe breakpoint handler. > > > Single step exception will always be followed by a kprobe breakpoint > > > exception.However, it will only be raised gracefully if we clear D bit > > > while returning from breakpoint exception. If D bit is set then, it > > results > > > into undefined exception and when it's handler enables dbg then single > > step > > > exception is generated, however it will never be handled(because address > > > does not match and therefore treated as unexpected). > > > This patch clears D-flag unconditionally in setup_singlestep, so that we > > > can always get single step exception correctly after returning from > > > breakpoint exception. > > > Additionally, it also removes D-flag set statement for KPROBE_REENTER > > > return path, because debug exception for KPROBE_REENTER will always take > > > place in a debug exception state. So, D-flag will already be set in this > > > case. > > > > OK, It explains this issue clearly. > > > > > > > > Signed-off-by: Pratyush Anand <pan...@redhat.com> > > > > Acked-by: Masami Hiramatsu <mhira...@kernel.org> > > > > Thanks! > > > > > --- > > > arch/arm64/kernel/probes/kprobes.c | 29 +++++++++++++---------------- > > > 1 file changed, 13 insertions(+), 16 deletions(-) > > > > > > diff --git a/arch/arm64/kernel/probes/kprobes.c > > b/arch/arm64/kernel/probes/kprobes.c > > > index e51f4ad97525..6e13361b9c01 100644 > > > --- a/arch/arm64/kernel/probes/kprobes.c > > > +++ b/arch/arm64/kernel/probes/kprobes.c > > > @@ -166,13 +166,18 @@ static void __kprobes set_current_kprobe(struct > > kprobe *p) > > > } > > > > > > /* > > > - * The D-flag (Debug mask) is set (masked) upon debug exception entry. > > > - * Kprobes needs to clear (unmask) D-flag -ONLY- in case of recursive > > > - * probe i.e. when probe hit from kprobe handler context upon > > > - * executing the pre/post handlers. In this case we return with > > > - * D-flag clear so that single-stepping can be carried-out. > > > - * > > > - * Leave D-flag set in all other cases. > > > + * When PSTATE.D is set (masked), then software step exceptions can not > > be > > > + * generated. > > > + * SPSR's D bit shows the value of PSTATE.D immediately before the > > > + * exception was taken. PSTATE.D is set while entering into any > > exception > > > + * mode, however software clears it for any normal > > (none-debug-exception) > > > + * mode in the exception entry. Therefore, when we are entering into > > kprobe > > > + * breakpoint handler from any normal mode then SPSR.D bit is already > > > + * cleared, however it is set when we are entering from any debug > > exception > > > + * mode. > > > + * Since we always need to generate single step exception after a kprobe > > > + * breakpoint exception therefore we need to clear it unconditionally, > > when > > > + * we become sure that the current breakpoint exception is for kprobe. > > > */ > > > static void __kprobes > > > spsr_set_debug_flag(struct pt_regs *regs, int mask) > > > @@ -245,10 +250,7 @@ static void __kprobes setup_singlestep(struct > > kprobe *p, > > > > > > set_ss_context(kcb, slot); /* mark pending ss */ > > > > > > - if (kcb->kprobe_status == KPROBE_REENTER) > > > - spsr_set_debug_flag(regs, 0); > > > - else > > > - WARN_ON(regs->pstate & PSR_D_BIT); > > > + spsr_set_debug_flag(regs, 0); > > > > > > /* IRQs and single stepping do not mix well. */ > > > kprobes_save_local_irqflag(kcb, regs); > > > @@ -333,8 +335,6 @@ int __kprobes kprobe_fault_handler(struct pt_regs > > *regs, unsigned int fsr) > > > BUG(); > > > > > > kernel_disable_single_step(); > > > - if (kcb->kprobe_status == KPROBE_REENTER) > > > - spsr_set_debug_flag(regs, 1); > > > > > > if (kcb->kprobe_status == KPROBE_REENTER) > > > restore_previous_kprobe(kcb); > > > @@ -457,9 +457,6 @@ kprobe_single_step_handler(struct pt_regs *regs, > > unsigned int esr) > > > kprobes_restore_local_irqflag(kcb, regs); > > > kernel_disable_single_step(); > > > > > > - if (kcb->kprobe_status == KPROBE_REENTER) > > > - spsr_set_debug_flag(regs, 1); > > > - > > > post_kprobe_handler(kcb, regs); > > > } > > > > > > -- > > > 2.5.5 > > > > > > > > > -- > > Masami Hiramatsu <mhira...@kernel.org> > >
