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

[Gavin Guo]

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