Re: [PATCH RFC bpf-next 1/3] uprobe: Add uretprobe syscall to speed up return probe

[Jiri Olsa <olsajiri@gmail.com>]

On Mon, Mar 18, 2024 at 06:11:06PM -0700, Andrii Nakryiko wrote:

SNIP

> > diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
> > index 7e8d46f4147f..af0a33ab06ee 100644
> > --- a/arch/x86/entry/syscalls/syscall_64.tbl
> > +++ b/arch/x86/entry/syscalls/syscall_64.tbl
> > @@ -383,6 +383,7 @@
> >  459    common  lsm_get_self_attr       sys_lsm_get_self_attr
> >  460    common  lsm_set_self_attr       sys_lsm_set_self_attr
> >  461    common  lsm_list_modules        sys_lsm_list_modules
> > +462    64      uretprobe               sys_uretprobe
> >
> >  #
> >  # Due to a historical design error, certain syscalls are numbered differently
> > diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> > index 6c07f6daaa22..069371e86180 100644
> > --- a/arch/x86/kernel/uprobes.c
> > +++ b/arch/x86/kernel/uprobes.c
> > @@ -12,6 +12,7 @@
> >  #include <linux/ptrace.h>
> >  #include <linux/uprobes.h>
> >  #include <linux/uaccess.h>
> > +#include <linux/syscalls.h>
> >
> >  #include <linux/kdebug.h>
> >  #include <asm/processor.h>
> > @@ -308,6 +309,53 @@ static int uprobe_init_insn(struct arch_uprobe *auprobe, struct insn *insn, bool
> >  }
> >
> >  #ifdef CONFIG_X86_64
> > +
> > +asm (
> > +       ".pushsection .rodata\n"
> > +       ".global uretprobe_syscall_entry\n"
> > +       "uretprobe_syscall_entry:\n"
> > +       "pushq %rax\n"
> > +       "pushq %rcx\n"
> > +       "pushq %r11\n"
> > +       "movq $462, %rax\n"
> 
> nit: is it possible to avoid hardcoding 462 here? Can we use
> __NR_uretprobe instead?

yep, will do that

> 
> > +       "syscall\n"
> 
> oh, btw, do we need to save flags register as well or it's handled
> somehow? I think according to manual syscall instruction does
> something to rflags register. So do we need pushfq before syscall?

it's saved and restored by syscall instruction.. but apart from RF
flag as Oleg mentioned, it looks like we don't need to care about
that one

> 
> > +       ".global uretprobe_syscall_end\n"
> > +       "uretprobe_syscall_end:\n"
> > +       ".popsection\n"
> > +);
> > +
> > +extern u8 uretprobe_syscall_entry[];
> > +extern u8 uretprobe_syscall_end[];
> > +
> > +void *arch_uprobe_trampoline(unsigned long *psize)
> > +{
> > +       *psize = uretprobe_syscall_end - uretprobe_syscall_entry;
> > +       return uretprobe_syscall_entry;
> > +}
> > +
> > +SYSCALL_DEFINE0(uretprobe)
> > +{
> > +       struct pt_regs *regs = task_pt_regs(current);
> > +       unsigned long sregs[3], err;
> > +
> > +       /*
> > +        * We set rax and syscall itself changes rcx and r11, so the syscall
> > +        * trampoline saves their original values on stack. We need to read
> > +        * them and set original register values and fix the rsp pointer back.
> > +        */
> > +       err = copy_from_user((void *) &sregs, (void *) regs->sp, sizeof(sregs));
> > +       WARN_ON_ONCE(err);
> > +
> > +       regs->r11 = sregs[0];
> > +       regs->cx = sregs[1];
> > +       regs->ax = sregs[2];
> > +       regs->orig_ax = -1;
> > +       regs->sp += sizeof(sregs);
> > +
> > +       uprobe_handle_trampoline(regs);
> 
> probably worth leaving a comment that uprobe_handle_trampoline() is
> rewriting userspace RIP and so syscall "returns" to the original
> caller.
> 
> > +       return regs->ax;
> 
> and this is making sure that caller function gets the correct function
> return value, right? It's all a bit magical, so worth leaving a
> comment here, IMO.

ok will add more comments about that

thanks,
jirka