Re: [PATCH v9 30/32] mm: handle PMD swap entry faults on swap-in

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Critical] mm: use-after-free in do_huge_pmd_swap_page() from
  unlocking PTL first
- [Critical] mm: unmapped PMD swap-cache folio in
  do_huge_pmd_swap_page() corrupts rmap
- [Medium] mm: missing PTE fault fallback after PMD split in
  do_huge_pmd_swap_page()
--

commit 651cc7048660407a18f57e406abe979d3a9ccbfe
Author: Usama Arif <usama.arif@linux.dev>

mm: handle PMD swap entry faults on swap-in

This commit adds do_huge_pmd_swap_page() and hooks it into
__handle_mm_fault() to fault in PMD swap entries at PMD granularity when a
PMD-sized folio is available, or split the PMD swap entry and fall back to
PTE swap-in otherwise.

> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index e02a885ea1d0b..b8d61cdf8cbfb 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
[ ... ]
> @@ -2425,6 +2426,266 @@ vm_fault_t do_huge_pmd_numa_page(struct vm_fault *vmf)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_THP_SWAP
> +/**
> + * do_huge_pmd_swap_page() - Handle a fault on a PMD-level swap entry.
> + * @vmf: Fault context. vmf->orig_pmd contains the swap PMD.
> + *
> + * A PMD swap entry is a compact encoding for HPAGE_PMD_NR consecutive swap
> + * slots. If the swap cache still has one PMD-sized folio covering the range,
> + * map it directly at PMD level. If the range has been split into per-page
> + * cache state, or zswap may have per-page state for it, split the PMD swap
> + * entry and retry at PTE granularity.
> + *
> + * Return: VM_FAULT_* flags.
> + */
> +vm_fault_t do_huge_pmd_swap_page(struct vm_fault *vmf)
> +{
[ ... ]
> +	if (!thp_vma_allowable_order(vma, vma->vm_flags, TVA_PAGEFAULT,
> +				     HPAGE_PMD_ORDER)) {
> +		__split_huge_pmd(vma, vmf->pmd, haddr);
> +		return 0;
> +	}
> +
> +	swp_entry = entry;
> +
> +	/* Prevent swapoff from happening to us. */
> +	si = get_swap_device(swp_entry);
> +	if (IS_ERR_OR_NULL(si)) {
> +		if (IS_ERR(si))
> +			return VM_FAULT_SIGBUS;
> +		return 0;
> +	}
> +
> +	cache_state = swap_pmd_cache_lookup(swp_entry, &folio);
> +	if (cache_state == SWAP_PMD_CACHE_SPLIT)
> +		goto split_fallback;
> +	if (!folio) {
> +		/*
> +		 * PMD swap entries encode ordinary per-page swap slots. If any
> +		 * slot is in zswap, split and let the PTE swap path load the
> +		 * range per page. Otherwise the range is all on disk and can be
> +		 * read back as one PMD-sized folio.
> +		 */
> +		if (zswap_is_present(swp_entry, HPAGE_PMD_NR))
> +			goto split_fallback;
> +
> +		folio = swapin_sync(swp_entry, GFP_HIGHUSER_MOVABLE,
> +				    BIT(HPAGE_PMD_ORDER), vmf, NULL, 0);
> +		if (IS_ERR_OR_NULL(folio))
> +			goto split_fallback;
> +
> +		/* Had to read from swap area: Major fault */
> +		ret = VM_FAULT_MAJOR;
> +		count_vm_event(PGMAJFAULT);
> +		count_memcg_event_mm(mm, PGMAJFAULT);
> +	}
> +
> +	ret |= folio_lock_or_retry(folio, vmf);
> +	if (ret & VM_FAULT_RETRY)
> +		goto out_release;
> +
> +	/* Verify the folio is still in swap cache and matches our entry */
> +	if (unlikely(!folio_matches_swap_entry(folio, swp_entry)))
> +		goto out_page;
[ ... ]
> +	/* Lock the PMD and verify it hasn't changed */
> +	vmf->ptl = pmd_lock(mm, vmf->pmd);
> +	if (unlikely(!pmd_same(vmf->orig_pmd, pmdp_get(vmf->pmd)))) {
> +		spin_unlock(vmf->ptl);
> +		goto out_page;
> +	}
[ ... ]
> +	set_pmd_at(mm, haddr, vmf->pmd, pmd);
> +	update_mmu_cache_pmd(vma, haddr, vmf->pmd);
> +
> +	/* Update orig_pmd for any follow-up wp_huge_pmd() below. */
> +	vmf->orig_pmd = pmd;
> +
> +	/*
> +	 * Conditionally try to free up the swap cache. Do it after mapping,
> +	 * so raced page faults will likely see the folio in swap cache and
> +	 * wait on the folio lock.
> +	 */
> +	if (should_try_to_free_swap(si, folio, vma, exclusive, vmf->flags))
> +		folio_free_swap(folio);
> +
> +	spin_unlock(vmf->ptl);
> +
> +	folio_unlock(folio);
> +	put_swap_device(si);

[Severity: Critical]
Can releasing vmf->ptl before calling folio_unlock(folio) in
do_huge_pmd_swap_page() lead to a use-after-free on folio?

do_huge_pmd_swap_page() holds a single reference to folio from
swap_pmd_cache_lookup() or swapin_sync(), and set_pmd_at() transfers that
reference to the PMD mapping. When should_try_to_free_swap() is true (such
as on SWP_SYNCHRONOUS_IO swap devices, exclusive write faults, or VM_LOCKED
VMAs), folio_free_swap(folio) drops the swap-cache references as well.

At that point the faulting thread no longer holds its own reference on
folio; only vmf->ptl prevents the PMD mapping reference from going away. In
mm/memory.c:do_swap_page(), folio_unlock(folio) is called before
pte_unmap_unlock(vmf->pte, vmf->ptl):

	if (should_try_to_free_swap(si, folio, vma, exclusive, vmf->flags))
		folio_free_swap(folio);

	folio_unlock(folio);
	...
	pte_unmap_unlock(vmf->pte, vmf->ptl);

With spin_unlock(vmf->ptl) happening first here, a racing MADV_DONTNEED or
munmap() in zap_huge_pmd() (or move_folios_to_lru() dropping a transient LRU
isolation reference) can unmap and free the folio while PG_locked is still
set before folio_unlock(folio) runs on this CPU. Should folio_unlock(folio)
be called before spin_unlock(vmf->ptl)?

> +
> +	/*
> +	 * If the write fault wasn't satisfied above (folio is shared without
> +	 * exclusivity), call wp_huge_pmd() to handle COW or
> +	 * userfaultfd-wp without forcing a second fault.
> +	 *
> +	 * wp_huge_pmd() may return VM_FAULT_FALLBACK if it had to split the
> +	 * PMD; that's a normal outcome, and the natural PTE-level refault will
> +	 * complete the COW. Mask it so callers (and the arch fault handler)
> +	 * don't see VM_FAULT_FALLBACK as a fatal VM_FAULT_ERROR.
> +	 */
> +	if (write && !pmd_write(pmd) && !rwp_restore) {
> +		vm_fault_t wp_ret = wp_huge_pmd(vmf);
> +
> +		wp_ret &= ~VM_FAULT_FALLBACK;
> +		ret |= wp_ret;
> +		if (ret & VM_FAULT_ERROR)
> +			ret &= VM_FAULT_ERROR;
> +	}
> +
> +	return ret;
> +
> +out_page:
> +	folio_unlock(folio);
> +out_release:
> +	folio_put(folio);
> +	put_swap_device(si);
> +	return ret;
> +
> +unlock_split_fallback:
> +	/*
> +	 * PTE fallback cannot add a single-page rmap to a PMD-sized folio that
> +	 * has never been mapped: do_swap_page() would hand the whole folio to
> +	 * folio_add_new_anon_rmap() while installing one PTE. Nor can it do
> +	 * anything useful with a folio that failed to read. Remove either from
> +	 * the swap cache so each slot is read back into its own order-0 folio.
> +	 * An uptodate anon swap-cache folio can be mapped one PTE at a time and
> +	 * must stay cached, so that any poisoned subpage stays visible to
> +	 * do_swap_page(). This mirrors unuse_pmd_entry().
> +	 */
> +	if (folio_matches_swap_entry(folio, swp_entry) &&
> +	    (!folio_test_uptodate(folio) || !folio_test_anon(folio)))
> +		swap_cache_del_folio(folio);
> +	folio_unlock(folio);
> +	folio_put(folio);

[Severity: Critical]
What happens if a newly allocated PMD-sized swap-cache folio
(!folio_test_anon(folio) or !folio_test_uptodate(folio)) exits via out_page
or out_release instead of unlock_split_fallback?

There appear to be a few paths where an unmapped PMD-sized folio remains in
the swap cache after the PMD swap entry is split or zapped:

1. If the PMD changed while swapin_sync() or folio_lock_or_retry() was
   in progress (for example, a racing thread split the PMD at
   split_fallback, or mprotect/madvise split the PMD), the !pmd_same()
   check jumps to out_page without deleting the !folio_test_anon(folio)
   (or !folio_test_uptodate(folio)) folio from the swap cache.

2. If the PMD changed because a racing zap_huge_pmd() or
   madvise_free_huge_pmd() zapped the swap entry while this thread held the
   folio lock, swap_put_entries_direct() -> __try_to_reclaim_swap() fails
   folio_trylock(folio). Unlike mm/memory.c:do_swap_page(), which calls
   folio_free_swap(folio) under out_page before unlocking the folio,
   out_page here does not call folio_free_swap(folio), leaving the folio
   and swap slots stranded in the swap cache.

3. On asynchronous swap devices, swapin_sync() inserts a locked
   !folio_test_anon(folio) PMD-sized folio into the swap cache, and
   folio_lock_or_retry() drops the fault lock and jumps to out_release with
   VM_FAULT_RETRY (and MADV_WILLNEED's swapin_pmd_swap_entry() similarly
   populates an unmapped PMD-sized folio). If the PMD is subsequently split
   before the folio is mapped at PMD level (including via the
   !thp_vma_allowable_order() check at the top of do_huge_pmd_swap_page(),
   which splits the PMD without checking the swap cache), the unmapped
   PMD-sized folio stays in the swap cache.

When a subsequent PTE fault enters mm/memory.c:do_swap_page() on the
HPAGE_PMD_NR contiguous split PTE swap entries, the large-folio PTE batching
logic batches all nr_pages = HPAGE_PMD_NR PTEs. Because nr_pages equals
folio_nr_pages(folio), the check in 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;
	}

does not trigger, and do_swap_page() passes the PMD-mappable folio to
folio_add_new_anon_rmap() while installing PTEs. Since
folio_add_new_anon_rmap() initializes any folio_test_pmd_mappable(folio)
for a PMD mapping (_entire_mapcount = 0, _large_mapcount = 1,
_nr_pages_mapped = ENTIRELY_MAPPED, and PageAnonExclusive only on subpage
0), wouldn't PTE-mapping it corrupt the rmap/mapcounts and PageAnonExclusive
state across subpages?

> diff --git a/mm/memory.c b/mm/memory.c
> index 47bba4695d855..e71c590113e07 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
[ ... ]
> @@ -6778,6 +6747,9 @@ static vm_fault_t __handle_mm_fault(struct vm_area_struct *vma,
>  
>  		if (pmd_is_migration_entry(vmf.orig_pmd))
>  			pmd_migration_entry_wait(mm, vmf.pmd);
> +		else if (IS_ENABLED(CONFIG_THP_SWAP) &&
> +			 pmd_is_swap_entry(vmf.orig_pmd))
> +			return do_huge_pmd_swap_page(&vmf);
>  		return 0;
>  	}
>  	if (pmd_trans_huge(vmf.orig_pmd)) {

[Severity: Medium]
When do_huge_pmd_swap_page() splits the PMD (either at the
!thp_vma_allowable_order() check, at split_fallback, or inside wp_huge_pmd()
when a non-exclusive swapped-in THP must be split into PTEs for COW), should
__handle_mm_fault() fall through to the fallback label and call
handle_pte_fault(&vmf) rather than returning 0 (or masking out
VM_FAULT_FALLBACK)?

Other PMD handlers in __handle_mm_fault() such as create_huge_pmd() and
wp_huge_pmd() return VM_FAULT_FALLBACK so __handle_mm_fault() jumps to
fallback: and resolves the PTE fault in the same handle_mm_fault() call.

By returning 0 after splitting the PMD swap entry (or clearing
VM_FAULT_FALLBACK when wp_huge_pmd() splits the read-only PMD into read-only
PTEs without copying), callers of handle_mm_fault() that do not loop on 0 -
such as fixup_user_fault(), fault_in_safe_writeable(), and
user_event_mm_fault_in() - see a successful return while the target PTE is
still a non-present swap entry or read-only PTE, causing subsequent
pagefault-disabled user accesses to fail with -EFAULT.

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