Re: [PATCH] arm64: kprobes: Use BRK instead of single-step when executing instructions out-of-line

[Jean-Philippe Brucker <jean-philippe@linaro.org>]

On Mon, Nov 02, 2020 at 10:52:55PM +0900, Masami Hiramatsu wrote:
> > > -	patch_text(p->addr, p->opcode);
> > > +	patch_text(p->addr, p->opcode, 0);
> > 
> > I don't think patch_text() is offering an awful lot to these callers. Why
> > don't we just call aarch64_insn_patch_text() directly, which I think will
> > make the code easier to read too?
> 
> Agreed. I guess it's just because histrically used.

Yes it's easy to remove, sending a v2

> 
> > >  void __kprobes arch_remove_kprobe(struct kprobe *p)
> > > @@ -163,20 +167,14 @@ static void __kprobes set_current_kprobe(struct kprobe *p)
> > >  }
> > >  
> > >  /*
> > > - * Interrupts need to be disabled before single-step mode is set, and not
> > > - * reenabled until after single-step mode ends.
> > > - * Without disabling interrupt on local CPU, there is a chance of
> > > - * interrupt occurrence in the period of exception return and  start of
> > > - * out-of-line single-step, that result in wrongly single stepping
> > > - * into the interrupt handler.
> > > + * Keep interrupts disabled while executing the instruction out-of-line. Since
> > > + * the kprobe state is per-CPU, we can't afford to be migrated.
> > >   */
> > >  static void __kprobes kprobes_save_local_irqflag(struct kprobe_ctlblk *kcb,
> > >  						struct pt_regs *regs)
> > >  {
> > >  	kcb->saved_irqflag = regs->pstate & DAIF_MASK;
> > >  	regs->pstate |= PSR_I_BIT;
> > > -	/* Unmask PSTATE.D for enabling software step exceptions. */
> > > -	regs->pstate &= ~PSR_D_BIT;
> > 
> > Can we set the D bit now? Would be one less thing to worry about.
> 
> Hmm, I think we just ignore D bit above. If someone (kgdb?) is set D flag
> then there may be no reason to prohibit it on the trampoline.
> (Of course they must handle this case)

Masking debug just for the trampoline would simplify reasoning about
nesting exceptions. Although it prevents from debugging kprobes using
KGDB, it seems reasonable to me because I don't think we can provide
guarantees that KGDB works reliably with kprobes. For example it doesn't
seem like a watchpoint would fire if the instruction performing the access
is simulated by kprobe.

Thanks,
Jean

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel