Re: [PATCH v17 05/12] mm/hmm: migrate collection in HMM pagewalk - pte level

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] mm/hmm: split of non-anonymous large folios in hmm_vma_handle_migrate_prepare()
- [High] mm/hmm: early PMD walk termination on folio split failure in hmm_vma_walk_pmd()
- [High] mm/hmm: reject KSM folios in hmm_vma_handle_migrate_prepare()
--

commit 9b85d918ada9ba7990835bcd807b9973c5976885
Author: Mika Penttilä <mpenttil@redhat.com>

mm/hmm: migrate collection in HMM pagewalk - pte level

This commit implements PTE-level migration preparation and collection in
hmm_vma_handle_migrate_prepare(), along with migrate_vma_split_folio() to
split PTE-mapped large folios during an HMM page table walk.

> diff --git a/mm/hmm.c b/mm/hmm.c
> index 854d2c090b20c..ec9280ef26822 100644
> --- a/mm/hmm.c
> +++ b/mm/hmm.c
> @@ -495,6 +495,71 @@ static int hmm_vma_handle_absent_pmd(struct mm_walk *walk, unsigned long start,
>  #endif  /* CONFIG_ARCH_HAS_PMD_SOFTLEAVES */
>  
>  #ifdef CONFIG_DEVICE_MIGRATION

[ ... ]

> @@ -512,8 +582,230 @@ static int hmm_vma_handle_migrate_prepare(const struct mm_walk *walk,
>  					  unsigned long *hmm_pfn,
>  					  bool *unmapped)
>  {
> -	// TODO: implement migration entry insertion
> +	struct hmm_vma_walk *hmm_vma_walk = walk->private;
> +	struct hmm_range *range = hmm_vma_walk->range;
> +	struct migrate_vma *migrate = range->migrate;

[ ... ]

> +		if (folio_test_large(folio)) {
> +			int ret;
> +
> +			ret = migrate_vma_split_folio(folio,
> +						      migrate->fault_page,
> +						      hmm_vma_walk,
> +						      ptep);
> +			if (ret)
> +				goto out_error;
> +			return -EAGAIN;
> +		}
> +
> +		pfn = page_to_pfn(page);
> +		if (softleaf_is_device_private_write(entry))
> +			writable = true;
> +	} else {
> +		pfn = pte_pfn(pte);
> +		if (is_zero_pfn(pfn) &&
> +		    (minfo & MIGRATE_VMA_SELECT_SYSTEM)) {
> +			/* Like pte_none(): only anonymous memory can be populated */
> +			if (!vma_is_anonymous(walk->vma))
> +				goto out;
> +			*hmm_pfn &= HMM_PFN_INOUT_FLAGS;
> +			*hmm_pfn |= HMM_PFN_MIGRATE;
> +			goto out;
> +		}
> +		page = vm_normal_page(walk->vma, addr, pte);
> +		if (page && !is_zone_device_page(page) &&
> +		    !(minfo & MIGRATE_VMA_SELECT_SYSTEM)) {
> +			goto out;
> +		} else if (page && is_device_coherent_page(page)) {
> +			pgmap = page_pgmap(page);
> +
> +			if (!(minfo &
> +			      MIGRATE_VMA_SELECT_DEVICE_COHERENT) ||
> +			    pgmap->owner != migrate->pgmap_owner)
> +				goto out;
> +		}
> +
> +		folio = page ? page_folio(page) : NULL;
> +		if (folio && folio_test_large(folio)) {
> +			int ret;
> +
> +			ret = migrate_vma_split_folio(folio,
> +						      migrate->fault_page,
> +						      hmm_vma_walk,
> +						      ptep);
> +			if (ret)
> +				goto out_error;
> +			return -EAGAIN;
> +		}
> +
> +		writable = pte_write(pte);
> +	}

[Severity: High]
When walking a private file-backed VMA (which can contain a mix of
copy-on-write anonymous pages and read-only pagecache pages), will this
attempt to split non-anonymous large pagecache folios before checking
folio_test_anon()?

In the pte_present(pte) branch of hmm_vma_handle_migrate_prepare(),
migrate_vma_split_folio() is called on any large folio before verifying that
the folio is anonymous. If a PTE maps a read-only large file-backed folio in
the page cache, migrate_vma_split_folio() drops the PTE lock, takes the
sleeping folio_lock(folio) on the pagecache folio while already holding locks
on previously collected anonymous folios, and calls split_folio().

For file-backed folios, split_folio() -> __folio_freeze_split_file() takes
i_mmap_lock_read() and unmaps the folio without remapping it, leaving the
PTEs as pte_none() on the -EAGAIN retry, only for the resulting order-0
pagecache folio to be skipped by the !folio_test_anon() check. Furthermore,
if the filesystem sets mapping_min_folio_order() > 0, split_folio() fails
with -EINVAL (triggering VM_WARN_ONCE() in __folio_split()) and jumps to
out_error.

Should the !page || !folio_test_anon(folio) check happen before attempting to
split the folio in the present PTE path?

> +
> +	/* Only anonymous memory can be migrated to device memory */
> +	if (!page || !folio_test_anon(page_folio(page)))
> +		goto out;

[Severity: High]
Can KSM folios pass this check and be collected for device migration?

In include/linux/page-flags.h, FOLIO_MAPPING_KSM is defined as:

	FOLIO_MAPPING_ANON | FOLIO_MAPPING_ANON_KSM

Because FOLIO_MAPPING_ANON is set on KSM folios, folio_test_anon() returns
true for them. When hmm_range_fault() runs without HMM_PFN_REQ_WRITE (for
example during migrate_vma_setup() or a read fault with migration),
hmm_vma_handle_pte() does not break CoW on a read-only KSM PTE, and
hmm_vma_handle_migrate_prepare() locks and collects the KSM folio.

If the same KSM folio is mapped a second time later in the walked range,
hmm_vma_handle_pte() marks the second entry HMM_PFN_VALID before
folio_trylock() fails in hmm_vma_handle_migrate_prepare(), and
try_to_migrate() later unmaps that second mapping via rmap_walk_ksm(),
leaving a stale PFN in range->hmm_pfns. More importantly, migrating a KSM
folio to MEMORY_DEVICE_PRIVATE memory updates stable_node->kpfn via
folio_migrate_ksm() to a non-CPU-accessible ZONE_DEVICE PFN and installs
device-private swap entries into other processes sharing the KSM page,
causing ksmd to fault when it later accesses the page in memcmp_pages().

Should hmm_vma_handle_migrate_prepare() also reject folio_test_ksm(folio)?

[ ... ]

> +		set_pte_at(mm, addr, ptep, swp_pte);
> +		folio_remove_rmap_pte(folio, page, walk->vma);
> +		folio_put(folio);
> +		*hmm_pfn |= HMM_PFN_VALID | HMM_PFN_MIGRATE;
> +		if (pte_present(pte))
> +			*unmapped = true;
> +	} else {
> +		folio_put(folio);
> +	}
> +out:
>  	return 0;
> +out_error:
> +	return -EFAULT;
>  }

[Severity: High]
As reported by Sashiko on v16 [1], when migrate_vma_split_folio() fails in
hmm_vma_handle_migrate_prepare() and jumps to out_error (-EFAULT), does the
caller in hmm_vma_walk_pmd() abort the PMD walk too early and leave the
remaining pages in the PMD unprocessed?

In hmm_vma_walk_pmd(), the PTE loop handles non-zero returns from
hmm_vma_handle_migrate_prepare() by immediately returning 0:

	for (; addr < end; addr += PAGE_SIZE, ptep++, hmm_pfns++) {

		r = hmm_vma_handle_pte(walk, addr, end, pmdp, ptep, hmm_pfns);
		...
		r = hmm_vma_handle_migrate_prepare(walk, pmdp, ptep, addr, hmm_pfns, &unmapped);
		if (r == -EAGAIN) {
			...
			goto again;
		}
		if (r) {
			HMM_ASSERT_UNLOCKED(hmm_vma_walk);
			if (unmapped)
				flush_tlb_range(walk->vma, start, addr);
			return 0;
		}
	}

Because hmm_vma_handle_pte() is called inside the same per-page loop,
returning 0 from hmm_vma_walk_pmd() when a split fails at addr skips both
hmm_vma_handle_pte() and hmm_vma_handle_migrate_prepare() for all remaining
addresses in [addr + PAGE_SIZE, end) of the PMD. As a result,
hmm_range_fault() can return 0 (success) while range->hmm_pfns[] entries for
[addr + PAGE_SIZE, end) remain zeroed without HMM_PFN_VALID, and pages in
that tail of the PMD are neither faulted in (when HMM_PFN_REQ_FAULT is
requested) nor migrated.

[1] https://lore.kernel.org/r/20261021060506.2377545-6-mpenttil@redhat.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103061629.972669-1-mpenttil@redhat.com?part=5