On Wed, Jun 24, 2026 at 04:36:23PM +0200, Oleg Nesterov wrote:
> On 05/26, Jiri Olsa wrote:
> >
> > 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]>
>
> Reviewed-by: Oleg Nesterov <[email protected]>
>
> ---------------------------------------------------------------------
> Although I can't convince myself I fully understand this code with or
> without this patch ;)
>
> A couple of questions below...
>
> > -static struct uprobe_trampoline *create_uprobe_trampoline(unsigned long
> > vaddr)
> > +static struct vm_area_struct *get_uprobe_trampoline(struct mm_struct *mm,
> > unsigned long vaddr)
> > {
> > - struct pt_regs *regs = task_pt_regs(current);
> > - struct mm_struct *mm = current->mm;
> > - struct uprobe_trampoline *tramp;
> > + VMA_ITERATOR(vmi, mm, 0);
> > struct vm_area_struct *vma;
> >
> > - if (!user_64bit_mode(regs))
> > - return NULL;
> > + if (vaddr > TASK_SIZE || vaddr < PAGE_SIZE)
> > + return ERR_PTR(-EINVAL);
>
> Do we really need this check? It looks a bit confusing to me...
> vaddr is bp_vaddr from handle_swbp(), it should be valid?
true, will remove
>
> > +
> > + for_each_vma(vmi, vma) {
> > + if (!vma_is_special_mapping(vma, &tramp_mapping))
> > + continue;
> > + if (is_reachable_by_call(vma->vm_start, vaddr))
> > + return vma;
> > + }
>
> Perhaps we can later optimize this code a bit? I mean something like
>
> start_reachable = ...;
> end_reachable = ...;
>
> VMA_ITERATOR(vmi, mm, start_reachable);
>
> for_each_vma(vmi, vma) {
> if (!vma_is_special_mapping(...))
> continue;
> if (vma->vm_start > end_reachable)
> break;
> return vma;
> }
looks good, will try to use that
>
> > 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;
>
> I guess find_vma() can't fail, the caller arch_uprobe_optimize() has called
> copy_from_vaddr() under mmap_write_lock()... Nevermind.
hum, how's that.. I'll check, but where's the magic? :)
thanks,
jirka
