Message-ID: <205f8165-449c-441f-8ee9-58f69d23dbeb@lucifer.local>
Date: Fri, 30 May 2025 10:03:44 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: David Hildenbrand <david@...hat.com>
Cc: Pu Lehui <pulehui@...weicloud.com>, Oleg Nesterov <oleg@...hat.com>,
        mhiramat@...nel.org, peterz@...radead.org, Liam.Howlett@...cle.com,
        akpm@...ux-foundation.org, vbabka@...e.cz, jannh@...gle.com,
        pfalcato@...e.de, linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        pulehui@...wei.com
Subject: Re: [RFC PATCH] mm/mmap: Fix uprobe anon page be overwritten when
 expanding vma during mremap

On Fri, May 30, 2025 at 10:50:25AM +0200, David Hildenbrand wrote:
> On 30.05.25 10:41, Lorenzo Stoakes wrote:
> > 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.

By the way I agree with this.

> > >
> > > 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.
>
> "all other" -- not so sure.
>
> Why would we invoke uprobe when merging VMAs after mprotect, mremap,
> madvise, ordinary mremap where we are not mapping anything new but just ...
> merging VMAs?
>
> Really, we need to invoke uprobe only when adding new VMAs or extending
> existing VMAs -- mapping new file ranges some way.
>
> Or am I missing something important?

Well, this is where my limited knowledge of uprobe comes in.

I'm _assuming_ we must invoke it for merge. I'm happy to consider a refactoring
that applies generally, I'm not happy to see something that changes what merge
code does in non-mremap cases just for mremap.

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

Powered by blists - more mailing lists