Re: [PATCH] mm/huge_memory: fix dereferencing invalid pmd migration entry

[Zi Yan <ziy@nvidia.com>]

On 17 Apr 2025, at 7:21, Gavin Guo wrote:

> On 4/17/25 17:04, David Hildenbrand wrote:
>> On 17.04.25 10:55, Hugh Dickins wrote:
>>> On Thu, 17 Apr 2025, David Hildenbrand wrote:
>>>> On 17.04.25 09:18, David Hildenbrand wrote:
>>>>> On 17.04.25 07:36, Hugh Dickins wrote:
>>>>>> On Wed, 16 Apr 2025, David Hildenbrand wrote:
>>>>>>>
>>>>>>> Why not something like
>>>>>>>
>>>>>>> struct folio *entry_folio;
>>>>>>>
>>>>>>> if (folio) {
>>>>>>>   if (is_pmd_migration_entry(*pmd))
>>>>>>>       entry_folio = pfn_swap_entry_folio(pmd_to_swp_entry(*pmd)));
>>>>>>>   else
>>>>>>>    entry_folio = pmd_folio(*pmd));
>>>>>>>
>>>>>>>   if (folio != entry_folio)
>>>>>>>         return;
>>>>>>> }
>>>>>>
>>>>>> My own preference is to not add unnecessary code:
>>>>>> if folio and pmd_migration entry, we're not interested in entry_folio.
>>>>>> But yes it could be written in lots of other ways.
>>>>>
>>>>> While I don't disagree about "not adding unnecessary code" in general,
>>>>> in this particular case just looking the folio up properly might be the
>>>>> better alternative to reasoning about locking rules with conditional
>>>>> input parameters :)
>>>>>
>>>>
>>>> FWIW, I was wondering if we can rework that code, letting the caller to the
>>>> checking and getting rid of the folio parameter. Something like this
>>>> (incomplete, just to
>>>> discuss if we could move the TTU_SPLIT_HUGE_PMD handling).
>>>
>>> Yes, I too dislike the folio parameter used for a single case, and agree
>>> it's better for the caller who chose pmd to check that *pmd fits the folio.
>>>
>>> I haven't checked your code below, but it looks like a much better way
>>> to proceed, using the page_vma_mapped_walk() to get pmd lock and check;
>>> and cutting out two or more layers of split_huge_pmd obscurity.
>>>
>>> Way to go.  However... what we want right now is a fix that can easily
>>> go to stable: the rearrangements here in 6.15-rc mean, I think, that
>>> whatever goes into the current tree will have to be placed differently
>>> for stable, no seamless backports; but Gavin's patch (reworked if you
>>> insist) can be adapted to stable (differently for different releases)
>>> more more easily than the future direction you're proposing here.
>>
>> I'm fine with going with the current patch and looking into cleaning it up properly (if possible).
>>
>> So for this patch
>>
>> Acked-by: David Hildenbrand <david@redhat.com>
>>
>> @Gavin, can you look into cleaning that up?
>
> Thank you for your review. Before I begin the cleanup, could you please
> confirm the following action items:
>
> Zi Yan's suggestions for the patch are:
> 1. Replace the page fault with an invalid address access in the commit
>    description.
>
> 2. Simplify the nested if-statements into a single if-statement to
>    reduce indentation.

3. Can you please add Huge’s explanation below in the commit log?
That clarifies the issue. Thank you for the fix.

“
an anon_vma lookup points to a
location which may contain the folio of interest, but might instead
contain another folio: and weeding out those other folios is precisely
what the "folio != pmd_folio((*pmd)" check (and the "risk of replacing
the wrong folio" comment a few lines above it) is for.
”

With that, Acked-by: Zi Yan <ziy@nvidia.com>

>
> David, based on your comment, I understand that you are recommending the
> entry_folio implementation. Also, from your discussion with Hugh, it
> appears you agreed with my original approach of returning early when
> encountering a PMD migration entry, thereby avoiding unnecessary checks.
> Is that correct? If so, I will keep the current logic. Do you have any
> additional cleanup suggestions?
>
> I will start the cleanup work after confirmation.
>
>>
>>>
>>> (Hmm, that may be another reason for preferring the reasoning by
>>> folio lock: forgive me if I'm misremembering, but didn't those
>>> page migration swapops get renamed, some time around 5.11?)
>>
>> I remember that we did something to PTE handling stuff in the context of PTE markers. But things keep changing all of the time .. :)
>>


Best Regards,
Yan, Zi