Re: [PATCH] mm/rmap: don't discard lazyfree THPs at PMD level

[Lorenzo Stoakes (ARM)]

On Fri, Oct 09, 2026 at 10:08:18PM -0400, Zi Yan wrote:
> __discard_anon_folio_pmd_locked() clears a lazyfree THP PMD before it knows
> whether the folio can be discarded, and restores it if the folio was
> redirtied or has extra references. A concurrent munmap() or
> MREMAP_DONTUNMAP skips the temporary none PMD and unlinks the VMA from its
> anon_vma, so the folio stays mapped after the anon_vma is freed and a later

Yeah this seems pretty horrible, instating a page table entry under the rmap
lock is pretty gross in general, is there anywhere else where we actually
_install_ (even if 'reinstate') page table entries under the rmap lock?

I think this commit message needs to be clearer/more detailed though, I've been
sat here working this through in my head for a while.

I'm confused about the locking, which is really the key point about this change
isn't it?

unlink_anon_vmas() takes the anon rmap lock so that can't happen concurrently,
so is it _just_ the case that:

- The folio is not properly freed/unmapped and ends up with a dangling anon_vma
  pointer

In which case, nobody can actually access that folio since it's not mapped
anywhere, or is it the far more terrible case of the above and:

- set_pmd_at() in __discard_anon_folio_pmd_locked() reinstates the pmd entry
  regardless of the fact that the VMA and surrounding page tables could go away
  concurrently

?

But I'm confused here because unlink_anon_vmas() takes the anon_vma lock?

EDIT: See below, I figured out what it was.

Looking at the code:

<anon_vma lock>
[reclaim]
< PMD lock >
-> try_to_unmap_one()
    -> unmap_huge_pmd_locked()
      -> __discard_anon_folio_pmd_locked()
        [ clears PMD entry ]
	...
	[ reinstates PMD entry later ]

Concurrent munmap():

<mmap write lock, downgraded to mmap read lock later>
do_vmi_align_munmap()
  -> vms_complete_munmap_vmas()
    -> vms_clear_ptes()
      -> unmap_region()
        -> unmap_vmas() < zaps > <----- EDIT: the real issue, see below
	-> free_pgtables()
	   -> unlink_anon_vmas()
	     -> < anon_vma lock taken! >
	   ... clears down page tables (no rmap lock) ...

Is the issue actually the zapping in unmap_vmas(), which doesn't seem to be
protected by rmap lock at all, meaning that _that_ can happen with
concurrent munmap()?

So unmap_vmas() doesn't correctly populate the mmu_gather struct for the
range, meaning that the underlying page isn't freed and the refcount,
mapcount etc. are all left as they were?

MREMAP_DONTUNMAP seems a bit of a bizarre callout, but I suppose it is the
other case in which (under certain, likely, circumstances
e.g. non-partial/self-merging remap) an unlink_anon_vmas()'s call is done.

With normal mremap() that'd happen via an unmap of the original VMA.

> rmap walk uses the freed anon_vma.
>
> Using an invalidated PMD instead of a cleared one requires additional arch
> code fixes. Instead, disable the PMD level discard of lazyfree THPs, as
> before commit 735ecdfaf4e8 ("mm/vmscan: avoid split lazyfree THP during
> shrink_folio_list()").

Again it feels like this commit message assumes a bunch of information that
isn't presented here.

Why would you need to invalidate the PMD? Is the issue actually TLB flush?
How does that get around the fact that a PMD entry can be 'reinstated'
concurrent with a zap?

Or is it that you put something not-pmd_none() there? Hmm yeah I think so.

Doesn't all this cause pmd_none_or_clear_bad() to fire too?

All-round terrible.

I've not dug into the reports in detail, but I think the commit message
should also talk about what exactly is missing from pmdp_invalidate() that
is broken here? Are some arches not doing things quite right?

OK reading the code more and it seems the _real_ issue is actually
pmd_is_huge() in zap_pmd_range() right?

It's the -> pmd_none() in general. By doing that, you are breaking a
contract with PMD leaf entries.

So you get:

zap_pmd_range()
  -> pmd_is_huge() ? [ no ]
     -> zap_huge_pmd() [ WOULD HAVE taken PMD lock that blocks against reclaim ]

So using something like pmdp_invalidate() avoids this, because it keeps the
entry huge but invalid.

But I guess not every arch does this properly somehow?

>
> Fixes: 735ecdfaf4e8 ("mm/vmscan: avoid split lazyfree THP during shrink_folio_list()")
> Reported-by: Kyle Zeng <kylebot@xxxxxxxxxx>
> Closes: https://lore.kernel.org/r/20261009165214.40212-2-kylebot@xxxxxxxxxx
> Cc: stable@xxxxxxxxxxxxxxx
> Assisted-by: LLM
> Signed-off-by: Zi Yan <ziy@xxxxxxxxxx>

With the commit message made a lot clearer and calling out the specific
locking scenarios that allow this + getting rid of the dead code:

Reviewed-by: Lorenzo Stoakes (ARM) <ljs@xxxxxxxxxx>

> ---
> The proposed fix[1] requires additional arch code fixes[2]. Disable it
> first for stable fix. The function will be re-enabled along with arch code
> fixes.
>
> Link: https://lore.kernel.org/all/20261009165214.40212-2-kylebot@xxxxxxxxxx/ [1]
> Link: https://lore.kernel.org/all/DM0II2QHWWS1.2RXEJXC8S0101@xxxxxxxxxx/ [2]
> ---
>  mm/rmap.c | 11 -----------
>  1 file changed, 11 deletions(-)
>
> diff --git a/mm/rmap.c b/mm/rmap.c
> index 805db93fe0428..1131b76bbbc28 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -2275,17 +2275,6 @@ static bool try_to_unmap_one(struct folio *folio, struct vm_area_struct *vma,
>  		}
>
>  		if (!pvmw.pte) {
> -			if (folio_test_lazyfree(folio)) {
> -				if (unmap_huge_pmd_locked(vma, pvmw.address, pvmw.pmd, folio))

This is the only caller of unmap_huge_pmd_locked(), making that and
__discard_anon_folio_pmd_locked() dead code, shouldn't we remove all of it?

Otherwise we're leaving dead code around.


> -					goto walk_done;
> -				/*
> -				 * unmap_huge_pmd_locked has either already marked
> -				 * the folio as swap-backed or decided to retain it
> -				 * due to GUP or speculative references.
> -				 */
> -				goto walk_abort;
> -			}
> -
>  			if (flags & TTU_SPLIT_HUGE_PMD) {
>  				/*
>  				 * We temporarily have to drop the PTL and
>
> ---
> base-commit: 8b38ed9ab5b09c8ba168cbcc49524e9b380ee5c4
> change-id: 20261009-lazyfree-stable-fix-67712b2049dd
>
> Best regards,
> --
> Yan, Zi
>

--
Cheers, Lorenzo