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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAEf4BzZOCW6oFrNDfKMWfNJAuA1-8nnaCUyfNT3-BAUynZQ5HQ@mail.gmail.com>
Date: Tue, 25 Feb 2025 09:10:51 -0800
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Jiri Olsa <olsajiri@...il.com>
Cc: Alexei Starovoitov <alexei.starovoitov@...il.com>, Oleg Nesterov <oleg@...hat.com>, 
	Peter Zijlstra <peterz@...radead.org>, Andrii Nakryiko <andrii@...nel.org>, bpf <bpf@...r.kernel.org>, 
	LKML <linux-kernel@...r.kernel.org>, 
	linux-trace-kernel <linux-trace-kernel@...r.kernel.org>, X86 ML <x86@...nel.org>, 
	Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>, 
	John Fastabend <john.fastabend@...il.com>, Hao Luo <haoluo@...gle.com>, 
	Steven Rostedt <rostedt@...dmis.org>, Masami Hiramatsu <mhiramat@...nel.org>, 
	Alan Maguire <alan.maguire@...cle.com>, David Laight <David.Laight@...lab.com>, 
	Thomas Weißschuh <thomas@...ch.de>
Subject: Re: [PATCH RFCv2 08/18] uprobes/x86: Add uprobe syscall to speed up uprobe

On Tue, Feb 25, 2025 at 5:35 AM Jiri Olsa <olsajiri@...il.com> wrote:
>
> 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@...nel.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

It's an "internal" syscall, why can't we rename it, if we want to?

Though I'm not sure I see the problem having both sys_uprobe and
sys_uretprobe, tbh. We just add sys_uprobe to the same list(s) that
sys_uretprobe is in for seccomp.

>
> >
> > > +       "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@...92(%rbp) -8@...00(%rbp) 8@...x
>
> 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