Hi Masami,
On Sun, Jul 12, 2020 at 9:37 PM Masami Hiramatsu <mhiramat@xxxxxxxxxx> wrote:
>
> On Sat, 11 Jul 2020 09:32:01 +0800
> Guo Ren <guoren@xxxxxxxxxx> wrote:
>
> > Thx Masami,
> >
> > On Fri, Jul 10, 2020 at 9:50 PM Masami Hiramatsu <mhiramat@xxxxxxxxxx> wrote:
> > >
> > > Hi Guo,
> > >
> > > On Thu, 9 Jul 2020 02:19:14 +0000
> > > guoren@xxxxxxxxxx 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.
>
> Yes, that is same on x86. But ftrace regs_caller on x86-64 modifies the
> return address on the stack by regs->ip.
>
> See arch/x86/kernel/ftrace_64.S:
>
> -----
> SYM_INNER_LABEL(ftrace_regs_call, SYM_L_GLOBAL)
> call ftrace_stub
>
> /* Copy flags back to SS, to restore them */
> movq EFLAGS(%rsp), %rax
> movq %rax, MCOUNT_REG_SIZE(%rsp)
>
> /* Handlers can change the RIP */
> movq RIP(%rsp), %rax
> movq %rax, MCOUNT_REG_SIZE+8(%rsp)
> -----
Yes, and in save_mcount_regs, it prepare the origin pt_regs RIP.
/* Move RIP to its proper location */
movq MCOUNT_REG_SIZE+\added(%rsp), %rdi
movq %rdi, RIP(%rsp)
I've fixed up riscv's: [1]
[1]: https://lore.kernel.org/linux-riscv/1594683562-68149-4-git-send-email-guoren@xxxxxxxxxx/T/#u
>
> The last part update the return address on the stack.
>
> > > 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.
>
> The "Changing Execution Path" section in the Documentation/kprobes.txt said
>
> -----
> Since kprobes can probe into a running kernel code, it can change the
> register set, including instruction pointer.
> -----
Got it, thx for correct me.
>
> As you can see, this is the expected behavior on kprobes.
> Since user will not know the kprobes is working on a breakpoint or
> ftrace, we have to support this behavior transparently. So, could you
> enable this feature at first on ftrace@RISCV?
Ditto
>
> >
> > >
> > > 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
>
> In kernel/fail_function.c
Nice tip, thx. And I've tested err-jnject: [2]
[2]: https://lore.kernel.org/linux-riscv/1594683562-68149-4-git-send-email-guoren@xxxxxxxxxx/T/#md66e4e58f3463c1369e11ab9e5d646343f7d250d
>
> -----
> static int fei_kprobe_handler(struct kprobe *kp, struct pt_regs *regs)
> {
> struct fei_attr *attr = container_of(kp, struct fei_attr, kp);
>
> if (should_fail(&fei_fault_attr, 1)) {
> regs_set_return_value(regs, attr->retval);
> override_function_with_return(regs);
> return 1;
> }
>
> return 0;
> }
> -----
>
> And follow the code related to CONFIG_BPF_KPROBE_OVERRIDE in
> kernel/trace/bpf_trace.c. (doing similar thing, but fail_function.c
> is simpler.)
Nice tip, thx
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
