Re: [RFC PATCH] mm/mmap: Fix uprobe anon page be overwritten when expanding vma during mremap

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

On Fri, May 30, 2025 at 10:33:16AM +0200, David Hildenbrand wrote:
> On 29.05.25 18:07, Pu Lehui wrote:
> >
> > On 2025/5/28 17:03, David Hildenbrand wrote:
> > > On 27.05.25 15:38, Pu Lehui wrote:
> > > > Hi David,
> > > >
> > > > On 2025/5/27 2:46, David Hildenbrand wrote:
> > > > > On 26.05.25 17:48, Oleg Nesterov wrote:
> > > > > > Hi Lehui,
> > > > > >
> > > > > > As I said, I don't understand mm/, so can't comment, but...
> > > > > >
> > > > > > On 05/26, Pu Lehui wrote:
> > > > > > >
> > > > > > > To make things simpler, perhaps we could try post-processing, that is:
> > > > > > >
> > > > > > > diff --git a/mm/mremap.c b/mm/mremap.c
> > > > > > > index 83e359754961..46a757fd26dc 100644
> > > > > > > --- a/mm/mremap.c
> > > > > > > +++ b/mm/mremap.c
> > > > > > > @@ -240,6 +240,11 @@ static int move_ptes(struct
> > > > > > > pagetable_move_control
> > > > > > > *pmc,
> > > > > > >                    if (pte_none(ptep_get(old_pte)))
> > > > > > >                            continue;
> > > > > > >
> > > > > > > +               /* skip move pte when expanded range has uprobe */
> > > > > > > +               if (unlikely(pte_present(*new_pte) &&
> > > > > > > +                            vma_has_uprobes(pmc->new, new_addr,
> > > > > > > new_addr +
> > > > > > > PAGE_SIZE)))
> > > > > > > +                       continue;
> > > > > > > +
> > > > > >
> > > > > > I was thinking about
> > > > > >
> > > > > >       WARN_ON(!pte_none(*new_pte))
> > > > > >
> > > > > > at the start of the main loop.
> > > > > >
> > > > > > Obviously not to fix the problem, but rather to make it more explicit.
> > > > >
> > > > > Yeah, WARN_ON_ONCE().
> > > > >
> > > > > We really should fix the code to not install uprobes into the area we
> > > > > are moving.
> > > > Alright, so let's try this direction.
> > > >
> > > > >
> > > > > Likely, the correct fix will be to pass the range as well to
> > > > > uprobe_mmap(), and passing that range to build_probe_list().
> > > >
> > > > It will be great. But IIUC, the range we expand to is already included
> > > > when entering uprobe_mmap and also build_probe_list.
> > >
> > > Right, you'd have to communicate that information through all layers
> > > (expanded range).
> > >
> > > As an alternative, maybe we can really call handle_vma_uprobe() after
> > > moving the pages.
> >
> > Hi David,
> >
> > Not sure if this is possible, but I think it would be appropriate to not
> > handle this uprobe_mmap at the source, and maybe we should make it clear
> > that new_pte must be NULL when move_ptes, otherwise it should be an
> > exception?
>
> Yeah, we should ay least document that if we find any non-none pte in the
> range we are moving to, we have a big problem.
>
> I think the main issue is that vma_complete() calls uprobe_mmap() before
> moving the page tables over.

Well vma_complete() is not _normally_ invoked before moving page tables,
it's mremap that's making things strange :)

That's why I think my suggested approach of specifically indicating that we
want different behaviour for mremap is a reasonable one here, as it special
cases things for this case.

However...

>
> If we could defer the uprobe_mmap() call, we might be good.
>
> The entry point is copy_vma_and_data(), where we call copy_vma() before
> move_page_tables().
>
> copy_vma() should trigger the uprobe_mmap() through vma_merge_new_range().
>
> I wonder if there might be a clean way to move the uprobe_mmap() out of
> vma_complete(). (or at least specify to skip it because it will be done
> manually).

...I would also love to see some means of not having to invoke
uprobe_mmap() in the VMA code, but I mean _at all_.

But that leads into my desire to not do:

if (blah blah)
	some_specific_hardcoded_case();

I wish we had a better means of hooking stuff like this.

However I don't think currently we can reasonably do so, as in all other
merge cases we _do_ want to invoke it.

So I'm kinda not in favour of moving things around just to suit mremap
here.

>
> --
> Cheers,
>
> David / dhildenb
>

Overall I'd suggest the proposed approach is what we need to fix this _in
the short term_ but am obviously happy to see proposals to make uprobe
stuff less 'hacked in' :)