Re: [PATCH] mm/huge_memory: fix dereferencing invalid pmd migration entry
From: Gavin Guo
Date: Thu Apr 17 2025 - 08:04:31 EST
On 4/17/25 19:32, Zi Yan wrote:
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@xxxxxxxxxx>
@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.
Sure, will send out another patch for your review. Thank you for the review.
“
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@xxxxxxxxxx>
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