Re: [PATCH] riscv/kprobe: Optimize the performance of patching instruction slot

[Google]

On Thu, 8 Sep 2022 09:43:45 +0800
"liaochang (A)" <liaochang1@xxxxxxxxxx> wrote:

> Thanks for comment.
> 
> 在 2022/9/8 1:21, Jisheng Zhang 写道:
> > On Wed, Sep 07, 2022 at 10:33:27AM +0800, Liao Chang wrote:
> >> Since no race condition occurs on each instruction slot, hence it is
> >> safe to patch instruction slot without stopping machine.
> > 
> > hmm, IMHO there's race when arming kprobe under SMP, so stopping
> > machine is necessary here. Maybe I misundertand something.
> > 
> 
> It is indeed necessary to stop machine when arm kprobe under SMP,
> but i don't think it need to stop machine when prepare instruction slot,
> two reasons:
> 
> 1. Instruction slot is dynamically allocated data.
> 2. Kernel would not execute instruction slot until original instruction
>    is replaced by breakpoint.

Ah, this is for ss (single step out of line) slot. So until
kprobe is enabled, this should not be used from other cores.
OK, then it should be safe.


> >>
> >> Signed-off-by: Liao Chang <liaochang1@xxxxxxxxxx>
> >> ---
> >>  arch/riscv/kernel/probes/kprobes.c | 8 +++++---
> >>  1 file changed, 5 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/arch/riscv/kernel/probes/kprobes.c b/arch/riscv/kernel/probes/kprobes.c
> >> index e6e950b7cf32..eff7d7fab535 100644
> >> --- a/arch/riscv/kernel/probes/kprobes.c
> >> +++ b/arch/riscv/kernel/probes/kprobes.c
> >> @@ -24,12 +24,14 @@ post_kprobe_handler(struct kprobe *, struct kprobe_ctlblk *, struct pt_regs *);
> >>  static void __kprobes arch_prepare_ss_slot(struct kprobe *p)
> >>  {
> >>  	unsigned long offset = GET_INSN_LENGTH(p->opcode);
> >> +	const kprobe_opcode_t brk_insn = __BUG_INSN_32;
> >> +	kprobe_opcode_t slot[MAX_INSN_SIZE];
> >>  
> >>  	p->ainsn.api.restore = (unsigned long)p->addr + offset;
> >>  
> >> -	patch_text(p->ainsn.api.insn, p->opcode);
> >> -	patch_text((void *)((unsigned long)(p->ainsn.api.insn) + offset),
> >> -		   __BUG_INSN_32);
> >> +	memcpy(slot, &p->opcode, offset);
> >> +	memcpy((void *)((unsigned long)slot + offset), &brk_insn, 4);
> >> +	patch_text_nosync(p->ainsn.api.insn, slot, offset + 4);

BTW, didn't you have a macro for the size of __BUG_INSN_32?

Thank you,


> >>  }
> >>  
> >>  static void __kprobes arch_prepare_simulate(struct kprobe *p)
> >> -- 
> >> 2.17.1
> >>
> >>
> >> _______________________________________________
> >> linux-riscv mailing list
> >> linux-riscv@xxxxxxxxxxxxxxxxxxx
> >> http://lists.infradead.org/mailman/listinfo/linux-riscv
> > .
> 
> -- 
> BR,
> Liao, Chang


-- 
Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>