Re: [PATCH v9 28/32] mm: handle PMD swap entries in UFFDIO_MOVE

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] mm: move_swap_pmd() sets anon rmap on unmapped swap-cache
  folios
- [Medium] mm: move_pages_huge_pmd() drops src_ptl before swap lookup
  and split
- [Medium] mm: move_pages() splits PMD swap entries before checking
  movability
--

commit 0e469173a6b536c3bd6b664124f92ddaf042afb0
Author: Usama Arif <usama.arif@linux.dev>

mm: handle PMD swap entries in UFFDIO_MOVE

This commit adds support for moving PMD-level swap entries in UFFDIO_MOVE via
a new move_swap_pmd() helper in move_pages_huge_pmd(), while rejecting PMD
swap entries at the destination in move_pages() with -EEXIST.

> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 65922d9e4bec1..e02a885ea1d0b 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -2938,6 +2938,79 @@ int change_huge_pud(struct mmu_gather *tlb, struct vm_area_struct *vma,
>  #endif
>  
>  #ifdef CONFIG_USERFAULTFD
> +#ifdef CONFIG_THP_SWAP
> +/*
> + * Move a PMD-level swap entry from src_pmd to dst_pmd. Both PMD locks are
> + * acquired here; src_folio (if present) must already be locked. The deposited
> + * page table backing the source THP is moved across with the entry.
> + */
> +static int move_swap_pmd(struct mm_struct *mm, struct vm_area_struct *dst_vma,
> +			 unsigned long dst_addr, unsigned long src_addr,
> +			 pmd_t *dst_pmd, pmd_t *src_pmd,
> +			 pmd_t orig_dst_pmd, pmd_t orig_src_pmd,
> +			 spinlock_t *dst_ptl, spinlock_t *src_ptl,
> +			 struct folio *src_folio, swp_entry_t entry)
> +{

[ ... ]

> +	if (src_folio) {
> +		folio_move_anon_rmap(src_folio, dst_vma);
> +		src_folio->index = linear_anon_page_index(dst_vma, dst_addr);

[Severity: Medium]
Can src_folio be a newly allocated swap-cache folio that has not yet been
marked anonymous with folio_add_new_anon_rmap()?

If a swapped-out THP had its folio evicted from the swap cache and a fresh
PMD-order folio was subsequently allocated into the swap cache (for example,
via MADV_WILLNEED in swapin_walk_pmd_entry() -> swapin_pmd_swap_entry() ->
swapin_sync() -> __swap_cache_alloc_folio(), or a racing swap-in fault before
the folio is mapped), src_folio->mapping is still NULL
(!folio_test_anon(src_folio)).

Calling folio_move_anon_rmap(src_folio, dst_vma) unconditionally in
move_swap_pmd() writes dst_vma->anon_vma + FOLIO_MAPPING_ANON into
src_folio->mapping without going through folio_add_new_anon_rmap():

mm/rmap.c:folio_add_new_anon_rmap() {
	...
	__folio_mod_stat(folio, nr, nr_pmdmapped);
	mod_mthp_stat(folio_order(folio), MTHP_STAT_NR_ANON, 1);
}

When the folio is later freed, free_pages_prepare() sees
folio_test_anon(folio) and decrements MTHP_STAT_NR_ANON without a matching
increment:

mm/page_alloc.c:free_pages_prepare() {
	...
	if (folio_test_anon(folio)) {
		mod_mthp_stat(order, MTHP_STAT_NR_ANON, -1);
		folio->mapping = NULL;
	}
}

Stamping FOLIO_MAPPING_ANON early also defeats the !folio_test_anon(folio)
checks in unuse_pmd_entry(), do_huge_pmd_swap_page(), and do_swap_page() that
delete newly allocated swap-cache folios on PTE fallback or read failure:

mm/swapfile.c:unuse_pmd_entry() {
	...
	if (ret && folio_matches_swap_entry(folio, entry) &&
	    (!folio_test_uptodate(folio) || !folio_test_anon(folio)))
		swap_cache_del_folio(folio);
}

Should move_swap_pmd() check folio_test_anon(src_folio) before calling
folio_move_anon_rmap() and updating src_folio->index?

[ ... ]

> @@ -2972,11 +3045,95 @@ int move_pages_huge_pmd(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd, pm
>  	}
>  
>  	if (!pmd_trans_huge(src_pmdval)) {
> -		spin_unlock(src_ptl);
>  		if (pmd_is_migration_entry(src_pmdval)) {
> +			spin_unlock(src_ptl);
>  			pmd_migration_entry_wait(mm, src_pmd);
>  			return -EAGAIN;
>  		}
> +#ifdef CONFIG_THP_SWAP
> +		if (pmd_is_swap_entry(src_pmdval)) {
> +			swp_entry_t entry;
> +			struct swap_info_struct *si;
> +			enum swap_pmd_cache cache_state;
> +
> +			/*
> +			 * UFFDIO_MOVE on anon mappings requires single-owner
> +			 * semantics; refuse to move a shared swap entry.
> +			 */
> +			if (!pmd_swp_exclusive(src_pmdval)) {
> +				spin_unlock(src_ptl);
> +				return -EBUSY;
> +			}
> +
> +			entry = softleaf_from_pmd(src_pmdval);
> +			spin_unlock(src_ptl);
> +
> +			/*
> +			 * Pin the swap device against a racing swapoff. NULL
> +			 * means swapoff is in progress, which resolves on its
> +			 * own, so ask the caller to retry. An error pointer
> +			 * means the entry names no swap device at all: that
> +			 * never resolves, so report it instead of spinning in
> +			 * the caller's -EAGAIN loop.
> +			 */
> +			si = get_swap_device(entry);
> +			if (!si)
> +				return -EAGAIN;
> +			if (IS_ERR(si))
> +				return PTR_ERR(si);
> +
> +			src_folio = NULL;
> +			cache_state = swap_pmd_cache_lookup(entry, &src_folio);
> +			if (cache_state == SWAP_PMD_CACHE_SPLIT) {
> +				put_swap_device(si);
> +				__split_huge_pmd(src_vma, src_pmd, src_addr);
> +				return -EAGAIN;
> +			}

[Severity: Medium]
What happens if src_pmd changes after spin_unlock(src_ptl) before
get_swap_device(), swap_pmd_cache_lookup(), and __split_huge_pmd() run in
move_pages_huge_pmd()?

Because src_ptl is released before calling get_swap_device(entry) and
swap_pmd_cache_lookup(entry, &src_folio), and __split_huge_pmd() is called
without verifying that src_pmd still matches src_pmdval:

1. A racing swap-in fault, MADV_WILLNEED, or swapoff can swap the THP back in
   (making src_pmd a present THP) or populate a PMD-sized swap-cache folio
   while those swap slots are reused or partially populated as order-0 folios.
   If swap_pmd_cache_lookup() then sees SWAP_PMD_CACHE_SPLIT,
   move_pages_huge_pmd() unconditionally calls __split_huge_pmd(src_vma,
   src_pmd, src_addr), shattering a present THP (whose folio will then be
   split in move_pages_ptes() on retry) or splitting a PMD swap entry backed
   by a PMD-sized swap-cache folio so the retry in move_pages_ptes() fails
   with -EBUSY.

2. A racing swapoff followed by swapon of a smaller swap device on the same
   swap type can complete after spin_unlock(src_ptl) and before
   get_swap_device(entry), causing get_swap_device() to log a false "Bad swap
   offset" error and return ERR_PTR(-EIO), failing UFFDIO_MOVE with -EIO
   instead of retrying.

By comparison, swapin_pmd_swap_entry() in mm/madvise.c performs
swap_pmd_cache_lookup() and get_swap_device() while still holding the PMD
lock, and swapin_walk_pmd_entry() rechecks pmd_same() before calling
__split_huge_pmd():

mm/madvise.c:swapin_pmd_swap_entry() {
	...
	cache_state = swap_pmd_cache_lookup(entry, &folio);
	...
	si = get_swap_device(entry);
	spin_unlock(ptl);
}

Could move_pages_huge_pmd() keep src_ptl held across get_swap_device() and
swap_pmd_cache_lookup(), and split under src_ptl via split_huge_pmd_locked()
(or recheck pmd_same() before splitting)?

[ ... ]

> diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> index 8cff0f820ca51..38b63e9cff62d 100644
> --- a/mm/userfaultfd.c
> +++ b/mm/userfaultfd.c
> @@ -2072,6 +2072,20 @@ static ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
>  			break;
>  		}
>  
> +		/*
> +		 * A PMD swap entry at dst is a swapped-out THP, not a hole,
> +		 * and unlike a PMD migration entry it will not resolve on its
> +		 * own. Nothing below faults it back in: pte_alloc() skips a
> +		 * !pmd_none PMD, pte_offset_map_rw_nolock() then fails on the
> +		 * non-present PMD, and the -EAGAIN that produces would be
> +		 * retried forever by the loop below. Be strict, exactly as for
> +		 * a present THP.
> +		 */
> +		if (unlikely(pmd_is_swap_entry(dst_pmdval))) {
> +			err = -EEXIST;
> +			break;
> +		}
> +
>  		ptl = pmd_trans_huge_lock(src_pmd, src_vma);
>  		if (ptl) {
>  			/* Check if we can move the pmd without splitting it. */

[Severity: Medium]
When move_splits_huge_pmd() or !pmd_none(dst_pmdval) is true in move_pages(),
doesn't the fallback path unconditionally call split_huge_pmd() on a source
PMD swap entry before checking whether the resulting PTE swap entries can be
moved?

In mm/userfaultfd.c:move_pages(), the split branch only checks exclusivity
when pmd_present(*src_pmd) is true:

mm/userfaultfd.c:move_pages() {
	...
	if (move_splits_huge_pmd(dst_addr, src_addr, src_start + len) ||
	    !pmd_none(dst_pmdval)) {
		/* Can be a migration entry */
		if (pmd_present(*src_pmd)) {
			struct folio *folio = pmd_folio(*src_pmd);

			if (!is_huge_zero_folio(folio) &&
			    !PageAnonExclusive(&folio->page)) {
				spin_unlock(ptl);
				err = -EBUSY;
				break;
			}
		}

		spin_unlock(ptl);
		split_huge_pmd(src_vma, src_pmd, src_addr);
		/* The folio will be split by move_pages_pte() */
		continue;
	}
}

If *src_pmd is a PMD swap entry and UFFDIO_MOVE is called with a sub-PMD
length, an unaligned address, or a destination PMD that already has an empty
PTE page table (!pmd_none(dst_pmdval)):

1. If !pmd_swp_exclusive(*src_pmd), split_huge_pmd() shatters the shared PMD
   swap entry into PTE swap entries before move_pages_ptes() rejects them
   with -EBUSY.

2. If *src_pmd is exclusive and backed by a PMD-sized folio in the swap cache
   (SWAP_PMD_CACHE_HUGE), split_huge_pmd() shatters the PMD swap entry into
   PTE swap entries that still point to the large swap-cache folio. On the
   next iteration, move_pages_ptes() immediately fails with -EBUSY because
   folio_test_large(folio) is true:

mm/userfaultfd.c:move_pages_ptes() {
	...
	if (folio) {
		if (folio_test_large(folio)) {
			ret = -EBUSY;
			folio_put(folio);
			goto out;
		}
}

That leaves src_pmd permanently split into PTE swap entries that even a
subsequent PMD-aligned UFFDIO_MOVE cannot move while the large folio remains
in the swap cache.

Should move_pages() check pmd_is_swap_entry(*src_pmd) before splitting, and
refuse with -EBUSY if !pmd_swp_exclusive(*src_pmd) or if the swap cache holds
a large folio?

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