Re: [PATCH bpf 1/2] uprobes/x86: Fix red zone clobbering in nop5 optimization

On Fri, May 8, 2026 at 7:02 PM <[email protected]> wrote:
>
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Critical] `resolve_uprobe_addr()` unconditionally uses `current->mm`, causing `is_optimized()` to fail when called from tracer context, which leads to target instruction stream corruption and potential kernel panics.
> - [High] Exhausting a process's trampoline page slots permanently disables nop5 optimization for that uprobe across the entire system.
> --
>
> commit 2c63c19710be8b050bc0e126d84e52c24209e062
> Author: Andrii Nakryiko <[email protected]>
> Date:   Sat May 9 00:30:56 2026 +0000
>
> uprobes/x86: Fix red zone clobbering in nop5 optimization
>
> The x86 uprobe nop5 optimization currently replaces a 5-byte NOP at the
> probe site with a CALL into a uprobe trampoline. This commit changes the
> optimization to use JMP instead, introducing a trampoline page with slots
> to handle register state saving and jumping to the syscall.
>
> > diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> > index ebb1baf1eb1d0..7e1f14200bbbf 100644
> > --- a/arch/x86/kernel/uprobes.c
> > +++ b/arch/x86/kernel/uprobes.c
>
> [ ... ]
>
> > @@ -761,147 +785,153 @@ void arch_uprobe_clear_state(struct mm_struct *mm)
> [ ... ]
> > +static bool resolve_uprobe_addr(unsigned long ip, unsigned long *probe_addr)
> >  {
> > -     struct vm_area_struct *vma = vma_lookup(current->mm, ip);
> > +     struct uprobes_state *state = &current->mm->uprobes_state;
>
> Can this cause a NULL pointer dereference or instruction corruption when
> executed outside the target task's context?
>

...

>
> Could we pass the correct mm struct down from is_optimized() to
> resolve_uprobe_addr() instead of relying on current->mm?

yes, good observation, I'll change resolve_uprobe_addr() to accept
mm_struct directly

>
> [ ... ]
>
> > @@ -1113,8 +1149,9 @@ static int __arch_uprobe_optimize(struct arch_uprobe *auprobe, struct mm_struct
> >  {
> >       struct uprobe_trampoline *tramp;
> >       struct vm_area_struct *vma;
> > +     unsigned long slot_vaddr;
> >       bool new = false;
> > -     int err = 0;
> > +     int slot, err;
> >
> >       vma = find_vma(mm, vaddr);
> >       if (!vma)
> > @@ -1122,8 +1159,17 @@ static int __arch_uprobe_optimize(struct arch_uprobe *auprobe, struct mm_struct
> >       tramp = get_uprobe_trampoline(vaddr, &new);
> >       if (!tramp)
> >               return -EINVAL;
> > -     err = swbp_optimize(auprobe, vma, vaddr, tramp->vaddr);
> > -     if (WARN_ON_ONCE(err) && new)
> > +
> > +     slot = tramp_alloc_slot(tramp, vaddr);
> > +     if (slot < 0) {
> > +             if (new)
> > +                     destroy_uprobe_trampoline(tramp);
> > +             return slot;
> > +     }
>
> If a process exhausts its trampoline page slots, will this permanently
> disable the nop5 optimization for this uprobe across the entire system?
>
> If get_uprobe_trampoline() returns a reachable trampoline that is completely
> full (all 256 slots used), tramp_alloc_slot() returns -ENOSPC. This error
> propagates up to arch_uprobe_optimize(), which handles failures by setting
> the ARCH_UPROBE_FLAG_OPTIMIZE_FAIL bit on auprobe->flags.
>
> Since struct arch_uprobe is shared across all processes tracing the same
> inode and offset, hitting the 256-slot limit in any single process seems
> to permanently blacklist the probe from being optimized system-wide for
> all other processes, even if those processes have completely empty
> trampolines.
>
> Should get_uprobe_trampoline() also check if the trampoline has available
> slots before returning it, or can the failure be handled without setting
> the global failure flag?

It's a trade off. If we don't set ARCH_UPROBE_FLAG_OPTIMIZE_FAIL,
we'll keep wasting time trying to find and use uprobe trampoline for
each uprobe execution in that process, which is wasteful. So, given
this is a performance optimization, I think we'll just live with this.
The alternative would be a per-mm+per-uprobe extra information, which
seems like an overkill, so probably a no-go.

>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1