> 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?
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?
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?
---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/26475828601
