Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <1594261154-69745-1-git-send-email-guoren@kernel.org>
 <1594261154-69745-7-git-send-email-guoren@kernel.org> <20200710225017.5ce329485e911f99e17cd483@kernel.org>
In-Reply-To: <20200710225017.5ce329485e911f99e17cd483@kernel.org>
From:   Guo Ren <guoren@kernel.org>
Date:   Sat, 11 Jul 2020 09:32:01 +0800
Message-ID: <CAJF2gTSfFY6qf7gZ9t80P-3cACWi3oEe4X8ek+_1nQZZT3Uk5w@mail.gmail.com>
Subject: Re: [PATCH v2 6/6] riscv: Add KPROBES_ON_FTRACE supported
To:     Masami Hiramatsu <mhiramat@kernel.org>
Cc:     Palmer Dabbelt <palmerdabbelt@google.com>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        Anup Patel <anup@brainfault.org>,
        Greentime Hu <greentime.hu@sifive.com>,
        Zong Li <zong.li@sifive.com>,
        =?UTF-8?Q?Patrick_St=C3=A4hlin?= <me@packi.ch>,
        =?UTF-8?B?QmrDtnJuIFTDtnBlbA==?= <bjorn.topel@gmail.com>,
        Atish Patra <atish.patra@wdc.com>, penberg@kernel.org,
        linux-riscv <linux-riscv@lists.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        linux-csky@vger.kernel.org, Guo Ren <guoren@linux.alibaba.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Thx Masami,

On Fri, Jul 10, 2020 at 9:50 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
>
> Hi Guo,
>
> On Thu,  9 Jul 2020 02:19:14 +0000
> guoren@kernel.org wrote:
>
> > +/* Ftrace callback handler for kprobes -- called under preepmt disabed */
> > +void kprobe_ftrace_handler(unsigned long ip, unsigned long parent_ip,
> > +                        struct ftrace_ops *ops, struct pt_regs *regs)
> > +{
> > +     struct kprobe *p;
> > +     struct kprobe_ctlblk *kcb;
> > +
> > +     p = get_kprobe((kprobe_opcode_t *)ip);
> > +     if (unlikely(!p) || kprobe_disabled(p))
> > +             return;
> > +
> > +     kcb = get_kprobe_ctlblk();
> > +     if (kprobe_running()) {
> > +             kprobes_inc_nmissed_count(p);
> > +     } else {
> > +             /*
> > +              * The regs->epc hasn't been saved by SAVE_ALL in mcount-dyn.S
> > +              * So no need to resume it, just for kprobe handler.
> > +              */
> > +             instruction_pointer_set(regs, ip);
> > +             __this_cpu_write(current_kprobe, p);
> > +             kcb->kprobe_status = KPROBE_HIT_ACTIVE;
> > +             if (!p->pre_handler || !p->pre_handler(p, regs)) {
> > +                     /*
> > +                      * Emulate singlestep (and also recover regs->pc)
> > +                      * as if there is a nop
> > +                      */
> > +                     instruction_pointer_set(regs,
> > +                             (unsigned long)p->addr + MCOUNT_INSN_SIZE);
> > +                     if (unlikely(p->post_handler)) {
> > +                             kcb->kprobe_status = KPROBE_HIT_SSDONE;
> > +                             p->post_handler(p, regs, 0);
> > +                     }
>
> Hmm, don't you need restoring the previous instruction pointer here?
look at  riscv mcount-dyn.S SAVE_ALL function, sp frame lay out like this:
-----------------------
| return address |
-----------------------
| frame pointer   |
-----------------------
| pt_regs x1-x31|
-----------------------
It's not a complete pt_regs for the handler, so modifing regs->ip is no use.

> If you don't support modifying the instruction pointer in the handler,
We can modify ip like this if necessary:
*(unsigned long *)((unsigned long)regs + sizeof(struct pt_regs) + 8) = xxx;

> it must not be compatible with kprobes.
Why, can you show related codes? thank you very much.

>
> Now BPF function override and function error injection depends on
> this behevior, so could you consider to support it in the "ftrace"
> implementation at first? (And if it is enabled, you can enable the
> livepatch on RISCV too)
Great message!

But can you show me codes that bpf and err-jnject using the behavior? Thx

I'll try to fix up it :)

-- 
Best Regards
 Guo Ren

ML: https://lore.kernel.org/linux-csky/