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

[David Hildenbrand <david@redhat.com>]

On 30.05.25 11:03, Lorenzo Stoakes wrote:
> 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.

Let me try to summarize it:

Essentially, what it does is go over the VMA to find where to install 
breakpoints. A breakpoint is essentially faulting in the file page, to 
then trigger a COW fault to get an anonymous page instead that we can 
modify to ... install the breakpoint.

So wherever we have a breakpoint, we want to have an anonymous page 
mapped later.

That is only required when we map a new file range. When ordinarily 
merging/splitting/moving, we already called uprobe before and installed 
the breakpoints.

Calling uprobe_mmap() when we already installed breakpoints is not 
really problematic, only suboptimal.

Calling uprobe_mmap() before moving page tables is bad.

-- 
Cheers,

David / dhildenb