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

[David Hildenbrand <david@redhat.com>]

On 17.04.25 13: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.

Yes, that makes sense.

> 
> 2. Simplify the nested if-statements into a single if-statement to
>      reduce indentation.
> 
> 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?

Yes, the current patch is okay for upstream+stable, but we should look 
into cleaning that up.

See the cleanup RFC patch I posted where we remove the folio check 
completely. Please let me know if you need more information.

-- 
Cheers,

David / dhildenb