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

[David Hildenbrand <david@redhat.com>]

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?

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

-- 
Cheers,

David / dhildenb