Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: Re: [PATCH v7 2/4] LoongArch: Add kprobe support
To:     Huacai Chen <chenhuacai@kernel.org>
References: <1669986505-11322-1-git-send-email-yangtiezhu@loongson.cn>
 <1669986505-11322-3-git-send-email-yangtiezhu@loongson.cn>
 <CAAhV-H7nVtZRMpqpK6bi9-OqH+_pHxB=qPb5fTisb+EnaKKPAw@mail.gmail.com>
Cc:     WANG Xuerui <kernel@xen0n.name>,
        Masami Hiramatsu <mhiramat@kernel.org>,
        loongarch@lists.linux.dev, linux-kernel@vger.kernel.org
From:   Tiezhu Yang <yangtiezhu@loongson.cn>
Message-ID: <c76b6b50-246e-f325-5f4b-b67327e06c10@loongson.cn>
Date:   Wed, 7 Dec 2022 17:17:45 +0800
User-Agent: Mozilla/5.0 (X11; Linux mips64; rv:45.0) Gecko/20100101
 Thunderbird/45.4.0
MIME-Version: 1.0
In-Reply-To: <CAAhV-H7nVtZRMpqpK6bi9-OqH+_pHxB=qPb5fTisb+EnaKKPAw@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


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