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

["Lorenzo Stoakes (ARM)" <ljs@kernel.org>]

On Mon, Oct 12, 2026 at 10:33:34AM -0400, Zi Yan wrote:
> > 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 ]
>
> The PMD becomes pmd_none().

Yup.

>
> > 	...
>
> There are two readers making decisions based on “pmd_none()” locklessly:
>
> 1. munmap case is via zap_pmd_range() in mm/memory.c: it skips pmd_none(),
> so the above folio is not zapped;

Yup I get at that below.

It also doesn't serialise on the PMD lock as a result.

That's really key isn't it?

		if (pmd_is_huge(*pmd)) { 				<- returns false
			if (next - addr != HPAGE_PMD_SIZE)
				__split_huge_pmd(vma, pmd, addr, false);
			else if (zap_huge_pmd(tlb, vma, pmd, addr)) {	<- doesn't lock the PMD
				addr = next;
				continue;
			}

So it skips it and nothing happens to the PMD nor is there any serialisation
between unmap_huge_pmd_locked() and this code.

try_to_unmap_one() will be called under the PMD lock because of this In
page_vma_mapped_walk(), I believe:

bool page_vma_mapped_walk(struct page_vma_mapped_walk *pvmw)
{
	...
		if (IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE) &&
		    (pmd_trans_huge(pmde) || pmd_is_migration_entry(pmde) ||
		    pmd_is_device_private_entry(pmde))) {
			pvmw->ptl = pmd_lock(mm, pvmw->pmd);
			...
			if (likely(pmd_trans_huge(pmde))) {
				...
				return true;
			}
			...
		}
	...
}

>
> 2. MREMAP_DONTUNMAP case is via move_page_tables() in mm/mremap.c calls
> get_old_pmd(): get_old_pmd() returns NULL for pmd_none(),
> so move_page_tables() skips the above folio.

move_page_tables() is called for _all_ mremap() operations. So the same issue
would occur across mremap().

That bit isn't special to MREMAP_DONTUNMAP.

>
> > 	[ reinstates PMD entry later ]
>
>
> 1. for munmap, unmap_region() calls free_pgtables(). In free_pmd_range(),
> the restored PMD is treated as pmd_bad() and cleared. And the folio’s
> rmap is not removed.

Yup I got to that point below :)

>
> 2. for meremap, dontunmap_complete() unlinks anon_vma but the source VMA
> still maps the folio.

Well, only if the set_pmd_at() managed to reinstall it AND the PUD page table
that mapped it still exists.

That set pmd could also UAF on the PMD page table in theory couldn't it?

It's all really horrible.

>
> >
> > 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()?
>
> No. zap and rmap walkers are serialized by PTL. The reason is described above.
>

Well, no, that's not quite right AFAICT - as above the PMD PTL _is not taken_
because the PMD entry is cleared.

So the serialisation that would otherwise exist between the try_to_unmap_one()
and zap_pmd_range() doesn't happen.

> >
> > 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?
>
> Yes to the underlying page isn’t freed.

The refcount and mapcount etc. would be correct? I think the refcount would be
such that it permanently leaks and the mapcount would be incorrect or only
accidentally correct?

>
> >
> > 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.
>
> 1. SPARC has thp_pte_count-- in its pmdp_invalidate(), but set_pmd_at()
> does not add it back, since the code assumes pmdp_invalidate() is only
> used in THP splitting code.
>
> 2. GUP-fast accepts an invalidate PMD on s390, risk, loongarch. They
> need to fix it to prevent GUP-fast from succeeding for this new
> pmdp_invalidate().

Thanks that's useful to know! And ugh.

They seem like they are existing bugs effectively in pmdp_invalidate()? Or stuff
that we would want to fix anyway I guess?

In general the point of all the above discussion is to encourage a commit
message that conveys this information clearly.

When I read the patch first there was a lot of 'huh' and that's a sign that the
commit msg doesn't say enough.

> >> 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.
>
> 1) it keeps the backport small, 2) I am going to restore the function and
> this reduce the code churn.
>
> I can remove the dead code if you think that is better.

No, if you plan to reinstate it (soon!) then that's fine, but please
explain that you're intentionally doing that in the commit message please!

--
Cheers, Lorenzo