On Tue, May 26, 2026 at 09:46:44PM +0000, [email protected] wrote:
> > diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> > index 2be6707e3320..733655bc610e 100644
> > --- a/arch/x86/kernel/uprobes.c
> > +++ b/arch/x86/kernel/uprobes.c
>
> [ ... ]
>
> > @@ -1111,21 +1051,18 @@ int set_orig_insn(struct arch_uprobe *auprobe,
> > struct vm_area_struct *vma,
> > static int __arch_uprobe_optimize(struct arch_uprobe *auprobe, struct
> > mm_struct *mm,
> > unsigned long vaddr)
> > {
> > - struct uprobe_trampoline *tramp;
> > - struct vm_area_struct *vma;
> > - bool new = false;
> > - int err = 0;
> > + struct pt_regs *regs = task_pt_regs(current);
> > + struct vm_area_struct *vma, *tramp;
> >
> > + if (!user_64bit_mode(regs))
> > + return -EINVAL;
> > vma = find_vma(mm, vaddr);
> > if (!vma)
> > return -EINVAL;
> > - tramp = get_uprobe_trampoline(vaddr, &new);
> > - if (!tramp)
> > - return -EINVAL;
> > - err = swbp_optimize(auprobe, vma, vaddr, tramp->vaddr);
> > - if (WARN_ON_ONCE(err) && new)
> > - destroy_uprobe_trampoline(tramp);
> > - return err;
> > + tramp = get_uprobe_trampoline(mm, vaddr);
> > + if (IS_ERR(tramp))
> > + return PTR_ERR(tramp);
> > + return WARN_ON_ONCE(swbp_optimize(auprobe, vma, vaddr,
> > tramp->vm_start));
> > }
>
> When swbp_optimize() fails in __arch_uprobe_optimize() and the preceding
> get_uprobe_trampoline() call created a new trampoline VMA via
> _install_special_mapping(), does the new VMA leak in the process address
> space?
>
> The pre-patch code tracked whether the trampoline was newly created and
> called destroy_uprobe_trampoline() on the failure path:
>
> err = swbp_optimize(auprobe, vma, vaddr, tramp->vaddr);
> if (WARN_ON_ONCE(err) && new)
> destroy_uprobe_trampoline(tramp);
> return err;
>
> The new code drops that cleanup entirely:
>
> return WARN_ON_ONCE(swbp_optimize(auprobe, vma, vaddr, tramp->vm_start));
>
> Although the old destroy_uprobe_trampoline only freed the struct (not the
> underlying VMA), the new code appears to introduce a VMA leak: the freshly
> mapped PAGE_SIZE special mapping in the user's address space stays mapped
> even though optimization failed. arch_uprobe_optimize() then sets
> ARCH_UPROBE_FLAG_OPTIMIZE_FAIL so subsequent calls won't retry, leaving the
> orphan trampoline mapping in the address space until exit_mmap() reaps it at
> process teardown.
>
> The commit message mentions: "Note the original code called
> destroy_uprobe_trampoline if the optimiation failed, but it only freed the
> struct uprobe_trampoline object, not the vma. The new vma leak is fixed in
> following change."
>
> Is the VMA leak addressed in the subsequent commit in this series?
yes, in:
[1] uprobes/x86: Unmap trampoline vma object in case it's unused
>
> A secondary behaviour change is that 'return WARN_ON_ONCE(swbp_optimize(...))'
> now returns the boolean truth value of the error (0 or 1) instead of the
> original errno. While the current caller (arch_uprobe_optimize) only treats
> the value as boolean, could this surprise a future caller that propagates the
> return code?
ah ok, this is actualy 'fixed' in [1] above, but yea we should
fix that directly in this change, will do
>
> Commit message:
>
> > uprobes/x86: Remove struct uprobe_trampoline object
> >
> > Removing struct uprobe_trampoline object and it's tracking code,
> > because it's not needed. We can do same thing directly on top of
> > struct vm_area_struct objects.
> >
> > This makes the code simpler and allows easy propagation of the
> > trampoline vma object into child process in following change.
> >
> > Note the original code called destroy_uprobe_trampoline if the
> > optimiation failed, but it only freed the struct uprobe_trampoline
> > object, not the vma. The new vma leak is fixed in following change.
> >
> > Acked-by: Andrii Nakryiko <[email protected]>
> > Signed-off-by: Jiri Olsa <[email protected]>
>
> CI bot suggested adding a Fixes tag in v3: 'Fixes: ba2bfc97b462
> ("uprobes/x86: Add support to optimize uprobes")' because the commit message
> notes a VMA resource leak in the original code.
>
> Reference:
> https://lore.kernel.org/bpf/8861cc7af8e0dfff2674ca4ae6381ea2d82c242dec115774e8e11048bde35...@mail.kernel.org/
>
> The author responded that 'the logic stays' and a separate fix is needed, but
> did not explain why the Fixes tag should not be included. If this patch is
> documenting a pre-existing bug (even if the full fix comes later), should it
> include a Fixes tag for backporting purposes?
there's not functional change in this patch, the vma leak fix is in [1]
jirka
