Re: [PATCH v3 04/13] mm/huge_memory: handle buggy PMD entry in zap_huge_pmd()

[Lorenzo Stoakes (Oracle)]

On Sat, Mar 28, 2026 at 12:05:08PM -0700, Suren Baghdasaryan wrote:
> On Fri, Mar 20, 2026 at 11:07 AM Lorenzo Stoakes (Oracle)
> <ljs@xxxxxxxxxx> wrote:
> >
> > A recent bug I analysed managed to, through a bug in the userfaultfd
> > implementation, reach an invalid point in the zap_huge_pmd() code where
> > the PMD was none of:
> >
> > - A non-DAX, PFN or mixed map.
> > - The huge zero folio
> > - A present PMD entry
> > - A softleaf entry
> >
> > The code at this point calls folio_test_anon() on a known-NULL folio.
> > Having logic like this explicitly NULL dereference in the code is hard to
> > understand, and makes debugging potentially more difficult.
> >
> > Add an else branch to handle this case and WARN().
> >
> > No functional change intended.
> >
> > Link: https://lore.kernel.org/all/6b3d7ad7-49e1-407a-903d-3103704160d8@lucifer.local/
> > Reviewed-by: Baolin Wang <baolin.wang@xxxxxxxxxxxxxxxxx>
> > Signed-off-by: Lorenzo Stoakes (Oracle) <ljs@xxxxxxxxxx>
>
> Overall LGTM, just a question below.
>
> Reviewed-by: Suren Baghdasaryan <surenb@xxxxxxxxxx>

Thanks!

>
> > ---
> >  mm/huge_memory.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > index 3c9e2ebaacfa..0056ac27ec9a 100644
> > --- a/mm/huge_memory.c
> > +++ b/mm/huge_memory.c
> > @@ -2385,6 +2385,10 @@ bool zap_huge_pmd(struct mmu_gather *tlb, struct vm_area_struct *vma,
> >
> >                 if (!thp_migration_supported())
> >                         WARN_ONCE(1, "Non present huge pmd without pmd migration enabled!");
> > +       } else {
> > +               WARN_ON_ONCE(true);
> > +               spin_unlock(ptl);
> > +               return true;
>
> Apologies if this was already discussed in earlier versions but why do
> we return "true" for this case which would be interpreted as
> "success"? Perhaps because we still managed to do
> tlb_remove_pmd_tlb_entry()?

If we return false it can result in a potential loop in the caller I
believe. Basically the caller won't handle this case properly. It was raised on
review, previously I was returning false :)

>
> >         }
> >
> >         if (folio_test_anon(folio)) {
> > --
> > 2.53.0
> >

Cheers, Lorenzo