Message-ID: <f344d741-962c-48d3-84b7-ce3de5619122@igalia.com>
Date: Thu, 17 Apr 2025 19:21:48 +0800
From: Gavin Guo <gavinguo@...lia.com>
To: David Hildenbrand <david@...hat.com>, Hugh Dickins <hughd@...gle.com>
Cc: linux-mm@...ck.org, akpm@...ux-foundation.org, willy@...radead.org,
 ziy@...dia.com, linmiaohe@...wei.com, revest@...gle.com,
 kernel-dev@...lia.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mm/huge_memory: fix dereferencing invalid pmd migration
 entry

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@...hat.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.

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

Powered by blists - more mailing lists