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

On Mon, May 11, 2026 at 9:38 AM Andrii Nakryiko
<[email protected]> wrote:
>
> 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.

We can have per-mm flag inside uprobes_state common to all uprobes,
but that will make reattachment to previously attached uprobe with
allocated slot not work, if I understand correctly.

Jiri, any thoughts or ideas?

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