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

[David Hildenbrand <david@redhat.com>]

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.

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).

-- 
Cheers,

David / dhildenb