Re: [PATCHv6 perf/core 09/22] uprobes/x86: Add uprobe syscall to speed up uprobe

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Thu, Sep 4, 2025 at 7:03 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Thu, Sep 04, 2025 at 11:39:33AM +0200, Jann Horn wrote:
> > On Thu, Sep 4, 2025 at 9:56 AM Jiri Olsa <olsajiri@gmail.com> wrote:
> > > On Wed, Sep 03, 2025 at 04:12:37PM -0700, Andrii Nakryiko wrote:
> > > > On Wed, Sep 3, 2025 at 2:01 PM Peter Zijlstra <peterz@infradead.org> wrote:
> > > > >
> > > > > On Wed, Sep 03, 2025 at 10:56:10PM +0200, Jiri Olsa wrote:
> > > > >
> > > > > > > > +SYSCALL_DEFINE0(uprobe)
> > > > > > > > +{
> > > > > > > > +       struct pt_regs *regs = task_pt_regs(current);
> > > > > > > > +       struct uprobe_syscall_args args;
> > > > > > > > +       unsigned long ip, sp;
> > > > > > > > +       int err;
> > > > > > > > +
> > > > > > > > +       /* Allow execution only from uprobe trampolines. */
> > > > > > > > +       if (!in_uprobe_trampoline(regs->ip))
> > > > > > > > +               goto sigill;
> > > > > > >
> > > > > > > Hey Jiri,
> > > > > > >
> > > > > > > So I've been thinking what's the simplest and most reliable way to
> > > > > > > feature-detect support for this sys_uprobe (e.g., for libbpf to know
> > > > > > > whether we should attach at nop5 vs nop1), and clearly that would be
> > > > > > > to try to call uprobe() syscall not from trampoline, and expect some
> > > > > > > error code.
> > > > > > >
> > > > > > > How bad would it be to change this part to return some unique-enough
> > > > > > > error code (-ENXIO, -EDOM, whatever).
> > > > > > >
> > > > > > > Is there any reason not to do this? Security-wise it will be just fine, right?
> > > > > >
> > > > > > good question.. maybe :) the sys_uprobe sigill error path followed the
> > > > > > uprobe logic when things go bad, seem like good idea to be strict
> > > > > >
> > > > > > I understand it'd make the detection code simpler, but it could just
> > > > > > just fork and check for sigill, right?
> > > > >
> > > > > Can't you simply uprobe your own nop5 and read back the text to see what
> > > > > it turns into?
> > > >
> > > > Sure, but none of that is neither fast, nor cheap, nor that simple...
> > > > (and requires elevated permissions just to detect)
> > > >
> > > > Forking is also resource-intensive. (think from libbpf's perspective,
> > > > it's not cool for library to fork some application just to check such
> > > > a seemingly simple thing as whether to
> > > >
> > > > The question is why all that? That SIGILL when !in_uprobe_trampoline()
> > > > is just paranoid. I understand killing an application if it tries to
> > > > screw up "protocol" in all the subsequent checks. But here it's
> > > > equally secure to just fail that syscall with normal error, instead of
> > > > punishing by death.
> > >
> > > adding Jann to the loop, any thoughts on this ^^^ ?
> >
> > If I understand correctly, the main reason for the SIGILL is that if
> > you hit an error in here when coming from an actual uprobe, and if the
> > syscall were to just return an error, then you'd end up not restoring
> > registers as expected which would probably end up crashing the process
> > in a pretty ugly way?
>
> for some cases yes, for the initial checks I think we could just skip
> the uprobe and process would continue just fine
>

For non-buggy kernel implementation in_uprobe_trampoline(regs->ip)
will (should) always be true when triggered for kernel-installed
uprobe. So this check can fail only for cases when someone
intentionally called sys_uprobe not from kernel-generated and
kernel-controlled trampoline.

At which point it's totally fine to just return an error and do nothing.

> we use sigill because the trap code paths use it for errors and to be
> paranoid about the !in_uprobe_trampoline check

Yeah, and it should be totally fine to keep doing that.

It's just about that entry in_uprobe_trampoline() check. And that's
sufficient to make all this nicely integrated with USDT use cases.

(I'd say it would be nice to also amend this into original patch to
avoid someone cherry picking original commit and forgetting/missing
the follow up change. But that's up to Peter.)

Jiri, can you please send a quick patch and see how that goes? Thanks!

>
> jirka
>
> >
> > I guess as long as in_uprobe_trampoline() is reliable (which it should
> > be), it would be fine to return an error when in_uprobe_trampoline()
> > fails, though it would be nice to have a short comment describing that
> > calls from uprobe trampolines must never fail with an error.
>
>