Re: [PATCHv4 02/13] uprobes/x86: Remove struct uprobe_trampoline object

[Jiri Olsa <olsajiri@gmail.com>]

On Tue, May 26, 2026 at 09:46:44PM +0000, bot+bpf-ci@kernel.org 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 <andrii@kernel.org>
> >     Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> 
> 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/8861cc7af8e0dfff2674ca4ae6381ea2d82c242dec115774e8e11048bde358c4@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