Re: [PATCH v1 1/4] mm: Fix uprobe pte be overwritten when expanding vma

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

On Mon, Jun 02, 2025 at 06:28:58PM +0200, David Hildenbrand wrote:
> On 02.06.25 15:26, Lorenzo Stoakes wrote:
> > On Mon, Jun 02, 2025 at 02:26:21PM +0200, David Hildenbrand wrote:
> > > On 02.06.25 13:55, Lorenzo Stoakes wrote:
> > > > On Fri, May 30, 2025 at 08:51:14PM +0200, David Hildenbrand wrote:
> > > > > >     	if (vp->remove) {
> > > > > > @@ -1823,6 +1829,14 @@ struct vm_area_struct *copy_vma(struct vm_area_struct **vmap,
> > > > > >     		faulted_in_anon_vma = false;
> > > > > >     	}
> > > > > > +	/*
> > > > > > +	 * If the VMA we are copying might contain a uprobe PTE, ensure
> > > > > > +	 * that we do not establish one upon merge. Otherwise, when mremap()
> > > > > > +	 * moves page tables, it will orphan the newly created PTE.
> > > > > > +	 */
> > > > > > +	if (vma->vm_file)
> > > > > > +		vmg.skip_vma_uprobe = true;
> > > > > > +
> > > > >
> > > > > Assuming we extend the VMA on the way (not merge), would we handle that
> > > > > properly?
> > > > >
> > > > > Or is that not possible on this code path or already broken either way?
> > > >
> > > > I'm not sure in what context you mean expand, vma_merge_new_range() calls
> > > > vma_expand() so we call an expand a merge here, and this flag will be
> > > > obeyed.
> > >
> > > Essentially, an mremap() that grows an existing mapping while moving it.
> > >
> > > Assume we have
> > >
> > > [ VMA 0 ] [ VMA X]
> > >
> > > And want to grow VMA 0 by 1 page.
> > >
> > > We cannot grow in-place, so we'll have to copy VMA 0 to another VMA, and
> > > while at it, expand it by 1 page.
> > >
> > > expand_vma()->move_vma()->copy_vma_and_data()->copy_vma()
> >
> > OK so in that case you'd not have a merge at all, you'd have a new VMA and all
> > would be well and beautiful :) or I mean hopefully. Maybe?
>
> I'm really not sure. :)
>
> Could there be some very odd cases like
>
> [VMA 0 ][ VMA 1 ][ VMA X]
>
> and when we mremap() [ VMA 1 ] to grow, we would place it before [VMA 0 ],
> and just by pure lick end up merging with that if the ranges match?

When we invoke copy_vma() we pass vrm->new_addr and vrm->new_len so this would
trigger a merge and the correct uprobe handling.

Since we just don't trigger the breakpoint install in this situation, we'd
correctly move over the breakpoint to the right position, and overwrite anything
we expanded into.

I do want to do a mremap doc actually to cover all the weird cases, because
there's some weird stuff in there and it's worth covering off stuff for users
and stuff for kernel people :)

>
> We're in the corner cases now, ... so this might not be relevant. But I hope
> we can clean up that uprobe mmap call later ...

Yeah with this initial fix in we can obviously revisit as needed!

>
> >
> > >
> > >
> > > But maybe I'm getting lost in the code. (e.g., expand_vma() vs. vma_expand()
> > > ... confusing :) )
> >
> > Yeah I think Liam or somebody else called me out for this :P I mean it's
> > accurate naming in mremap.c but that's kinda in the context of the mremap.
> >
> > For VMA merging vma_expand() is used generally for a new VMA, since you're
> > always expanding into the gap, but because we all did terrible things in past
> > lives also called by relocate_vma_down() which is a kinda-hack for initial stack
> > relocation on initial process setup.
> >
> > It maybe needs renaming... But expand kinda accurately describes what's going on
> > just semi-overloaded vs. mremap() now :>)
> >
> > VMA merge code now at least readable enough that you can pick up on the various
> > oddnesses clearly :P
>
> :)
>
> --
> Cheers,
>
> David / dhildenb
>

Cheers, Lorenzo