Re: [PATCH RFCv2 08/18] uprobes/x86: Add uprobe syscall to speed up uprobe

[Jiri Olsa <olsajiri@gmail.com>]

On Mon, Feb 24, 2025 at 11:22:42AM -0800, Alexei Starovoitov wrote:
> On Mon, Feb 24, 2025 at 6:08 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > +SYSCALL_DEFINE0(uprobe)
> > +{
> > +       struct pt_regs *regs = task_pt_regs(current);
> > +       unsigned long bp_vaddr;
> > +       int err;
> > +
> > +       err = copy_from_user(&bp_vaddr, (void __user *)regs->sp + 3*8, sizeof(bp_vaddr));
> > +       if (err) {
> > +               force_sig(SIGILL);
> > +               return -1;
> > +       }
> > +
> > +       /* Allow execution only from uprobe trampolines. */
> > +       if (!in_uprobe_trampoline(regs->ip)) {
> > +               force_sig(SIGILL);
> > +               return -1;
> > +       }
> > +
> > +       handle_syscall_uprobe(regs, bp_vaddr - 5);
> > +       return 0;
> > +}
> > +
> > +asm (
> > +       ".pushsection .rodata\n"
> > +       ".balign " __stringify(PAGE_SIZE) "\n"
> > +       "uprobe_trampoline_entry:\n"
> > +       "endbr64\n"
> 
> why endbr is there?
> The trampoline is called with a direct call.

ok, that's wrong, will remove that

> 
> > +       "push %rcx\n"
> > +       "push %r11\n"
> > +       "push %rax\n"
> > +       "movq $" __stringify(__NR_uprobe) ", %rax\n"
> 
> To avoid introducing a new syscall for a very similar operation
> can we disambiguate uprobe vs uretprobe via %rdi or
> some other way?
> imo not too late to change uretprobe api.
> Maybe it was discussed already.

yes, I recall discussing that early during uretprobe work with the decision to
have separate syscalls for each uprobe and uretprobe.. however wrt recent seccomp
changes, it might be easier just to add argument to uretprobe syscall to handle
uprobe

too bad it's not the other way around.. uprobe syscall with argument to do uretprobe
would sound better

> 
> > +       "syscall\n"
> > +       "pop %rax\n"
> > +       "pop %r11\n"
> > +       "pop %rcx\n"
> > +       "ret\n"
> 
> In later patches I see nop5 is replaced with a call to
> uprobe_trampoline_entry, but which part saves
> rdi and other regs?
> Compiler doesn't automatically spill/fill around USDT's nop/nop5.
> Selftest is doing:
> +__naked noinline void uprobe_test(void)
> so just lucky ?

if you mean registers that would carry usdt arguments, ebpf programs
access those based on assembler operand string stored in usdt record:

  stapsdt              0x00000048       NT_STAPSDT (SystemTap probe descriptors)
    Provider: test
    Name: usdt3
    Location: 0x0000000000712f2f, Base: 0x0000000002f516b0, Semaphore: 0x0000000003348ec2
->  Arguments: -4@-1192(%rbp) -8@-1200(%rbp) 8@%rax

it's up to bpf program to know which register(+offset) to access, libbpf have all
this logic hidden behind usdt_manager_attach_usdt and bpf_usdt_arg bpf call

the trampoline only saves rcx/r11/rax, because they are changed by syscall instruction

but actually I forgot to load these saved values (of rcx/r11/rax) and rsp into regs that
are passed to ebpf program, (like we do in uretprobe syscall) will fix that in next version

I'll add tests for optimized usdt with more arguments

thanks,
jirka