Messages in this thread Patch in this message | | | Subject | Re: [PATCH v7 2/4] LoongArch: Add kprobe support | From | Tiezhu Yang <> | Date | Wed, 7 Dec 2022 17:17:45 +0800 |
| |
On 12/07/2022 11:08 AM, Huacai Chen wrote: > Hi, Tiezhu, > > On Fri, Dec 2, 2022 at 9:08 PM Tiezhu Yang <yangtiezhu@loongson.cn> 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
| |