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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <412E70A4-4775-4AF7-A878-7FEBF9A122D8@nvidia.com>
Date: Thu, 17 Apr 2025 07:32:28 -0400
From: Zi Yan <ziy@...dia.com>
To: Gavin Guo <gavinguo@...lia.com>
Cc: David Hildenbrand <david@...hat.com>, Hugh Dickins <hughd@...gle.com>,
 linux-mm@...ck.org, akpm@...ux-foundation.org, willy@...radead.org,
 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 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@...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.

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