Re: [PATCH v7 2/4] LoongArch: Add kprobe support

[Tiezhu Yang]

On 12/07/2022 11:08 AM, Huacai Chen wrote:
> Hi, Tiezhu,
>
> On Fri, Dec 2, 2022 at 9:08 PM Tiezhu Yang <yangtiezhu@xxxxxxxxxxx> wrote:
> > Kprobes allows you to trap at almost any kernel address and
> > execute a callback function, this commit adds kprobe support
> > for LoongArch.

...

> > +
> > +       if (p->ainsn.insn->word == breakpoint_insn.word) {
> > +               regs->csr_prmd &= ~CSR_PRMD_PIE;
> > +               regs->csr_prmd |= kcb->kprobe_saved_irq;
> > +               preempt_enable_no_resched();
> > +               return;
> > +       }
> > +
> > +       regs->csr_prmd &= ~CSR_PRMD_PIE;
> You can put this line before the previous if, then the line in the if
> can be removed.

OK, will modify it.

> > +
> > +       if (insns_are_not_simulated(p, regs)) {
> > +               kcb->kprobe_status = KPROBE_HIT_SS;
> > +               regs->csr_era = (unsigned long)&p->ainsn.insn[0];
> > +       } else {
> > +               kcb->kprobe_status = KPROBE_HIT_SSDONE;
> > +               if (p->post_handler)
> > +                       p->post_handler(p, regs, 0);
> > +               reset_current_kprobe();
> > +               preempt_enable_no_resched();
> > +       }
> > +}
> > +NOKPROBE_SYMBOL(setup_singlestep);

...

> > +bool kprobe_singlestep_handler(struct pt_regs *regs)
> > +{
> > +       struct kprobe *cur = kprobe_running();
> > +       struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
> > +
> > +       if (!cur)
> > +               return false;
> > +
> > +       /* Restore back the original saved kprobes variables and continue */
> > +       if (kcb->kprobe_status == KPROBE_REENTER) {
> > +               restore_previous_kprobe(kcb);
> > +               goto out;
> > +       }
> > +
> > +       /* Call post handler */
> > +       kcb->kprobe_status = KPROBE_HIT_SSDONE;
> > +       if (cur->post_handler)
> > +               cur->post_handler(cur, regs, 0);
> > +
> > +       regs->csr_era = kcb->kprobe_saved_era + LOONGARCH_INSN_SIZE;
> > +       regs->csr_prmd |= kcb->kprobe_saved_irq;
> > +
> > +       reset_current_kprobe();
> > +out:
> > +       preempt_enable_no_resched();
> You didn't disable preemption, why enable preemption here? I think you
> should do similar things like kprobe_breakpoint_handler().
>
> > +       return true;
> > +}
> > +NOKPROBE_SYMBOL(kprobe_singlestep_handler);
> > +
> > +bool kprobe_fault_handler(struct pt_regs *regs, int trapnr)
> > +{
> > +       struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
> > +
> > +       if (kcb->kprobe_status & KPROBE_HIT_SS) {
> > +               regs->csr_era = kcb->kprobe_saved_era + LOONGARCH_INSN_SIZE;
> > +               regs->csr_prmd |= kcb->kprobe_saved_irq;
> > +
> > +               reset_current_kprobe();
> > +               preempt_enable_no_resched();
> Here has the same problem as before, I doubt you haven't tested your
> code (at least insufficient), and this is completely unacceptable for
> me.

preempt_disable() in kprobe_breakpoint_handler() is valid for
the entire duration of kprobe processing, so call the function
preempt_enable_no_resched() in kprobe_singlestep_handler() and
kprobe_fault_handler() has no problem, let me add a code comment
before preempt_disable(), like this:

+       /*
+        * We don't want to be preempted for the entire
+        * duration of kprobe processing.
+        */
        preempt_disable();

> > +       }
> > +
> > +       return false;
> > +}
> > +NOKPROBE_SYMBOL(kprobe_fault_handler);

...

> > diff --git a/arch/loongarch/mm/fault.c b/arch/loongarch/mm/fault.c
> > index 1ccd536..fc9225a 100644
> > --- a/arch/loongarch/mm/fault.c
> > +++ b/arch/loongarch/mm/fault.c
> > @@ -253,12 +253,16 @@ asmlinkage void __kprobes do_page_fault(struct pt_regs *regs,
> >  {
> >         irqentry_state_t state = irqentry_enter(regs);
> >
> > +       if (kprobe_page_fault(regs, current->thread.trap_nr))
> > +               goto out;
> > +
> This should be after the conditional local_irq_enable(), I think.

OK, will modify it.

Thanks,
Tiezhu