Re: [PATCH v9 27/32] mm: handle PMD swap entries in MADV_WILLNEED

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Critical] mm: rmap corruption from unmapped PMD folio cached by
  swapin_pmd_swap_entry()
--

commit 14ed08337de0c0909e93ae1d8b999993fb793692
Author: Usama Arif <usama.arif@linux.dev>

mm: handle PMD swap entries in MADV_WILLNEED

This commit teaches MADV_WILLNEED to prefetch PMD swap entries as a single
PMD-order folio when THP policy allows it, falling back to splitting the
PMD into PTEs when PMD-order swapin is disallowed or fails.

> diff --git a/mm/madvise.c b/mm/madvise.c
> index f7f17efdd926d..576be4b8744c2 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
[ ... ]
> @@ -193,6 +194,108 @@ static int madvise_update_vma(vm_flags_t new_flags,
>  }
>  
>  #ifdef CONFIG_SWAP
> +/*
> + * Prefetch a whole PMD swap entry as one PMD-order folio.
> + *
> + * Called with the PMD lock held; always drops it. Returns true when the
> + * caller should ask the walker to retry so the PTE path can handle the
> + * covered slots individually.
> + */
> +static bool swapin_pmd_swap_entry(struct vm_area_struct *vma, pmd_t *pmd,
> +				  unsigned long addr, softleaf_t entry,
> +				  spinlock_t *ptl)
> +{
[ ... ]
> +	/*
> +	 * The range was PMD-mapped when it was swapped out, but the policy may
> +	 * have changed since: MADV_NOHUGEPAGE, or the sysfs knob. Reading it
> +	 * back at PMD order would then hand do_swap_page() a PMD-sized folio to
> +	 * map with PTEs, which is exactly what the policy forbids. Split now
> +	 * and let the PTE path prefetch at order 0 - the next fault would split
> +	 * the entry anyway, so nothing is lost that the VMA still permits.
> +	 */
> +	if (!thp_vma_allowable_order(vma, vma->vm_flags, TVA_PAGEFAULT,
> +				     HPAGE_PMD_ORDER)) {
> +		spin_unlock(ptl);
> +		return true;
> +	}
> +
> +	cache_state = swap_pmd_cache_lookup(entry, &folio);
> +	if (cache_state == SWAP_PMD_CACHE_HUGE) {
> +		/* Already cached as one PMD-sized folio, nothing to do. */
> +		folio_put(folio);
> +		spin_unlock(ptl);
> +		return false;
> +	}
> +	if (cache_state == SWAP_PMD_CACHE_SPLIT ||
> +	    zswap_is_present(entry, HPAGE_PMD_NR)) {
> +		spin_unlock(ptl);
> +		return true;
> +	}
> +
> +	/*
> +	 * Pin the swap device under the PMD lock so the PMD-swap-entry
> +	 * observation keeps the entry valid for swapin_sync().
> +	 */
> +	si = get_swap_device(entry);
> +	spin_unlock(ptl);
> +	if (IS_ERR_OR_NULL(si))
> +		return false;
> +
> +	folio = swapin_sync(entry, GFP_HIGHUSER_MOVABLE, BIT(HPAGE_PMD_ORDER),
> +			    &vmf, NULL, 0);

[Severity: Critical]
Can leaving a newly allocated PMD-order folio unmapped in the swap cache
corrupt rmap and mapcount state if the PMD swap entry is split before a
PMD fault maps it?

When madvise_willneed() -> swapin_walk_pmd_entry() ->
swapin_pmd_swap_entry() calls swapin_sync() with BIT(HPAGE_PMD_ORDER), it
inserts a fresh HPAGE_PMD_ORDER folio into the swap cache and leaves it
unmapped (!folio_test_anon(folio)).

Before a PMD page fault maps that folio via do_huge_pmd_swap_page(), the
PMD swap entry can be split into 512 contiguous PTE swap entries without
removing the cached folio. For example, if userspace applies
MADV_NOHUGEPAGE (or the sysfs THP policy changes) and then either calls
MADV_WILLNEED again (hitting the !thp_vma_allowable_order() check above
and splitting via __split_huge_pmd()) or faults on the range (causing
do_huge_pmd_swap_page() to split the PMD for PTE fallback), or performs a
partial munmap(), mprotect(), MADV_DONTNEED, or MADV_FREE, the unmapped
PMD-order folio remains in the swap cache while the page table now holds
PTE swap entries.

When do_swap_page() then faults on one of those 512 contiguous PTE swap
entries, swap_cache_get_folio() returns the HPAGE_PMD_ORDER folio and
swap_pte_batch() batches all 512 PTEs so nr_pages == folio_nr_pages(folio),
bypassing the partial mapping check:

mm/memory.c:do_swap_page() {
	...
	if (!folio_test_anon(folio) && folio_test_large(folio) &&
	    nr_pages != folio_nr_pages(folio)) {
		if (!WARN_ON_ONCE(folio_test_dirty(folio)))
			swap_cache_del_folio(folio);
		goto out_nomap;
	}
	...
	} else if (!folio_test_anon(folio)) {
		VM_WARN_ON_ONCE_FOLIO(folio_nr_pages(folio) != nr_pages, folio);
		VM_WARN_ON_ONCE_FOLIO(folio_mapped(folio), folio);
		folio_add_new_anon_rmap(folio, vma, address, rmap_flags);
		folio_put_swap(folio, NULL);
	}
	...
	set_ptes(vma->vm_mm, addr, ptep, pte, nr_pages);
}

Because folio_test_pmd_mappable(folio) is true for an HPAGE_PMD_ORDER
folio, folio_add_new_anon_rmap() initializes the folio as PMD-mapped
rather than PTE-mapped:

mm/rmap.c:folio_add_new_anon_rmap() {
	...
	} else if (!folio_test_pmd_mappable(folio)) {
		...
	} else {
		nr = folio_large_nr_pages(folio);
		/* increment count (starts at -1) */
		atomic_set(&folio->_entire_mapcount, 0);
		folio_set_large_mapcount(folio, 1, vma);
		if (IS_ENABLED(CONFIG_PAGE_MAPCOUNT))
			atomic_set(&folio->_nr_pages_mapped, ENTIRELY_MAPPED);
		if (exclusive)
			SetPageAnonExclusive(&folio->page);
		nr_pmdmapped = nr;
	}
}

This maps the folio with 512 PTEs while setting _entire_mapcount = 0,
_large_mapcount = 1, nr_pmdmapped = 512, leaving per-page _mapcount at -1,
and only setting PageAnonExclusive on the head page even when tail pages
are mapped by writable PTEs.

Similarly, if swapoff runs on the split PTE swap entries, unuse_pte() will
find the unmapped HPAGE_PMD_ORDER folio in the swap cache:

mm/swapfile.c:unuse_pte() {
	...
	if (!folio_test_anon(folio)) {
		VM_WARN_ON_ONCE(folio_test_large(folio));
		VM_WARN_ON_FOLIO(!folio_test_locked(folio), folio);
		folio_add_new_anon_rmap(folio, vma, addr, rmap_flags);
	}
}

Would it make sense to ensure that unmapped PMD-order folios in the swap
cache are either removed when a PMD swap entry is split or rejected by PTE
swapin paths such as do_swap_page() and unuse_pte()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105183940.2236717-1-usama.arif@linux.dev?part=27