lkml.org 
[lkml]   [2022]   [Dec]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [PATCH v7 2/4] LoongArch: Add kprobe support
From
Date


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

\
 
 \ /
  Last update: 2022-12-07 10:18    [W:0.051 / U:0.136 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site