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

[Gavin Guo <gavinguo@igalia.com>]

On 4/17/25 19:36, David Hildenbrand wrote:
> 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.

Ah, I understand. I'll clean up the current one and propose another 
patch based on your RFC patch. Thanks for your review!

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