[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <b2dd29b0-aa12-4cb7-9c05-d3a998f7b0da@lucifer.local>
Date: Fri, 30 May 2025 09:41:28 +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: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' :)
Powered by blists - more mailing lists