Re: [PATCH v17 09/12] mm: enable device page migration from HMM pagewalk

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [Critical] mm: hmm_range_fault_locked() zeroes hmm_pfns on -EBUSY migration retry
- [Critical] mm/hmm: missing rollback of earlier entries after fault in hmm_range_fault()
- [High] mm: missing folio_test_anon() check in hmm_vma_handle_migrate_prepare_pmd()
- [High] mm: early return in hmm_vma_walk_pmd() on folio split failure skips PTEs
- [High] mm/hmm: folio_lock deadlock in migrate_vma_split_folio() and hmm_do_fault()
- [Medium] mm: lost MIGRATE_PFN_WRITE on writable protnone entries in HMM migration
- [Low] mm: build failure on incomplete struct migrate_vma when !CONFIG_MIGRATION
- [Low] mm/migrate_device: unused MIGRATE_VMA_FAULT and MIGRATE_VMA_WRITE flags in migrate_vma_setup()
--

commit 00843344cdcc1e298091d82f4ddd5b2d3478ddb1
Author: Mika Penttilä <mpenttil@redhat.com>

mm: enable device page migration from HMM pagewalk

This commit enables migration collection in hmm_range_fault() via
hmm_select_migrate(), switches migrate_vma_setup() to use the HMM pagewalk
path with new MIGRATE_VMA_FAULT and MIGRATE_VMA_WRITE flags, and removes
the old migrate_vma_collect() page table walk helpers.

> diff --git a/include/linux/migrate.h b/include/linux/migrate.h
> index dd7dd7ec6995e..3780c4c116b8a 100644
> --- a/include/linux/migrate.h
> +++ b/include/linux/migrate.h
> @@ -181,6 +181,8 @@ enum migrate_vma_info {
>  	MIGRATE_VMA_SELECT_DEVICE_PRIVATE = 1 << 1,
>  	MIGRATE_VMA_SELECT_DEVICE_COHERENT = 1 << 2,
>  	MIGRATE_VMA_SELECT_COMPOUND = 1 << 3,
> +	MIGRATE_VMA_FAULT = 1 << 4,
> +	MIGRATE_VMA_WRITE = 1 << 5,
>  };

[Severity: Low]
As reported by Christoph Hellwig on v15 [1], are MIGRATE_VMA_FAULT and
MIGRATE_VMA_WRITE in enum migrate_vma_info and their handling in
migrate_vma_setup() needed when there are no in-tree callers?

Later patches in the series (for test_hmm and drm/nouveau/svm) drive
migrate-on-fault directly through hmm_range_fault() with HMM_PFN_REQ_MIGRATE
and migrate_hmm_range_setup(), leaving MIGRATE_VMA_FAULT and MIGRATE_VMA_WRITE
unused across the tree.

> @@ -218,10 +220,14 @@ struct migrate_vma {
>  	struct page		*fault_page;
>  };
>  
> -// TODO: enable migration
>  static inline enum migrate_vma_info hmm_select_migrate(struct hmm_range *range)
>  {
> -	return 0;
> +	enum migrate_vma_info minfo;
> +
> +	minfo = (range->default_flags & HMM_PFN_REQ_MIGRATE) ?
> +		range->migrate->flags : 0;
> +
> +	return minfo;
>  }

[Severity: Low]
Will mm/hmm.c fail to build when CONFIG_HMM_MIRROR=y and CONFIG_MIGRATION=n?

Several drivers (such as INFINIBAND_ON_DEMAND_PAGING, DRM_AMDGPU_USERPTR, and
MSHV_ROOT) select CONFIG_HMM_MIRROR without selecting CONFIG_MIGRATION. When
CONFIG_MIGRATION is disabled, struct migrate_vma is only forward-declared in
include/linux/hmm.h, but hmm_range_fault_locked() in mm/hmm.c is compiled
outside #ifdef CONFIG_DEVICE_MIGRATION and dereferences
range->migrate->pgmap_owner:

mm/hmm.c:hmm_range_fault_locked() {
	if (hmm_select_migrate(range)) {
		WARN_ON_ONCE(range->pfn_flags_mask);
		WARN_ON_ONCE(range->dev_private_owner !=
			     range->migrate->pgmap_owner);
		...
	}
}

In addition, hmm_select_migrate() in include/linux/migrate.h is guarded by
#ifdef CONFIG_MIGRATION rather than #ifdef CONFIG_DEVICE_MIGRATION, even
though the migration implementation in mm/hmm.c and mm/migrate_device.c
depends on CONFIG_DEVICE_MIGRATION.

Should hmm_select_migrate() be guarded by CONFIG_DEVICE_MIGRATION (returning 0
when !CONFIG_DEVICE_MIGRATION), and should the range->migrate dereference in
hmm_range_fault_locked() be guarded or moved into a helper?

[ ... ]

> diff --git a/mm/hmm.c b/mm/hmm.c
> index 1c8a3fa29c4e6..0b38bf84c3690 100644
> --- a/mm/hmm.c
> +++ b/mm/hmm.c
> @@ -1658,14 +1658,43 @@ static int hmm_range_fault_locked(struct hmm_range *range, bool *locked)
>   *		the invalidation to finish.
>   * -EFAULT:     A page was requested to be valid and could not be made valid
>   *              ie it has no backing VMA or it is illegal to access
> + * -ERANGE:     The range crosses multiple VMAs, or space for hmm_pfns array
> + *              is too low.
>   *
>   * This is similar to get_user_pages(), except that it can read the page tables
>   * without mutating them (ie causing faults).
>   *
>   * The mmap lock must be held by the caller and will remain held on return.
>   * New users should prefer hmm_range_fault_unlocked_timeout() unless they
> - * specifically need to keep the mmap lock held across the call. This helper
> - * cannot support VMAs whose fault handlers need to drop the mmap lock.
> + * specifically need to keep the mmap lock held across the call like while
> + * migrating. This helper cannot support VMAs whose fault handlers need to
> + * drop the mmap lock.
> + *
> + * If want to do migration after faulting, call hmm_range_fault() with
> + * range.default_flags of HMM_PFN_REQ_MIGRATE, and optionally
> + * HMM_PFN_REQ_FAULT|HMM_PFN_REQ_WRITE, and initialize range->migrate field.
> + * range->migrate->vma will be populated during the call,
> + * and must be stable across the whole migrate process, which is
> + * why mmap_lock must be held around this call.
> + *
> + * When HMM_PFN_REQ_MIGRATE is set, migration collection may be partial on
> + * return and the caller takes responsibility for completing or aborting it.
> + *
> + * On success, the caller must call migrate_hmm_range_setup() and may then
> + * proceed with the normal migrate_vma sequence: prepare destination pages,
> + * call migrate_vma_pages(), update device mappings as needed, and finally
> + * call migrate_vma_finalize().
> + *
> + * On -EBUSY, the caller may retry hmm_range_fault() using the same range and
> + * PFN array without undoing entries collected by the previous attempt. If
> + * the caller stops retrying, it must abort the partial migration.

[Severity: Critical]
When a caller retries hmm_range_fault() after -EBUSY with the same range and
PFN array as documented here, won't hmm_range_fault_locked() wipe out all
previously collected entries by unconditionally zeroing range->hmm_pfns?

At the start of hmm_range_fault_locked() in mm/hmm.c:

	if (hmm_select_migrate(range)) {
		WARN_ON_ONCE(range->pfn_flags_mask);
		WARN_ON_ONCE(range->dev_private_owner !=
			     range->migrate->pgmap_owner);
		memset(range->hmm_pfns, 0,
		       ((range->end - range->start) >> PAGE_SHIFT) *
		       sizeof(*range->hmm_pfns));
	}

During the first call, hmm_vma_handle_migrate_prepare() and
hmm_vma_handle_migrate_prepare_pmd() lock folios, take references, install
migration PTEs/PMDs, and record HMM_PFN_VALID | HMM_PFN_MIGRATE in
range->hmm_pfns[]. If a later page in the range triggers an mmu_notifier
invalidation (for example during hmm_do_fault() or folio splitting) so that
mmu_interval_check_retry() returns -EBUSY, retrying hmm_range_fault() zeroes
range->hmm_pfns[].

On the retry walk, hmm_pfn_collected() in hmm_vma_handle_pte() and
hmm_vma_walk_pmd() evaluates to false for the migration entries installed by
the first attempt:

mm/hmm.c:hmm_vma_handle_pte() {
	if (softleaf_is_migration(entry) && hmm_select_migrate(range) &&
	    hmm_pfn_collected(*hmm_pfn) &&
	    hmm_pfn_to_page(*hmm_pfn) == softleaf_to_page(entry))
		return 0;
}

With HMM_PFN_REQ_FAULT set, HMM treats its own migration PTE/PMD as a foreign
migration entry and calls hmm_do_fault() -> handle_mm_fault() ->
migration_entry_wait(), deadlocking on the folio lock already held by current.
Without HMM_PFN_REQ_FAULT (or if the retry aborts before reaching those
entries), range->hmm_pfns[] remains zeroed, so migrate_hmm_range_setup() never
populates migrate->src[] for those pages and leaks the folio locks, folio
references, and migration PTEs.

[ ... ]

> diff --git a/mm/migrate_device.c b/mm/migrate_device.c
> index ba6775a0d37e6..266f9c6d7b607 100644
> --- a/mm/migrate_device.c
> +++ b/mm/migrate_device.c

[ ... ]

> @@ -741,9 +243,20 @@ int migrate_vma_setup(struct migrate_vma *args)
>  {
>  	const struct vm_area_struct *vma = args->vma;
>  	long nr_pages = (args->end - args->start) >> PAGE_SHIFT;
> +	struct hmm_range range = {
> +		.notifier = NULL,
> +		.hmm_pfns = args->src,
> +		.dev_private_owner = args->pgmap_owner,
> +		.migrate = args,
> +		.default_flags = HMM_PFN_REQ_MIGRATE
> +	};
> +	int ret;
>  
>  	args->start &= PAGE_MASK;
>  	args->end &= PAGE_MASK;
> +	range.start = args->start;
> +	range.end = args->end;
> +
>  	if (!vma)
>  		return -EINVAL;
>  	/* The migrate_vma page walk has no hugetlb support. */
> @@ -766,10 +279,24 @@ int migrate_vma_setup(struct migrate_vma *args)
>  	args->cpages = 0;
>  	args->npages = 0;
>  
> -	migrate_vma_collect(args);
> +	if (args->flags & MIGRATE_VMA_FAULT)
> +		range.default_flags |= HMM_PFN_REQ_FAULT;
>  
> -	if (args->cpages)
> -		migrate_vma_unmap(args);
> +	if (args->flags & MIGRATE_VMA_WRITE)
> +		range.default_flags |= HMM_PFN_REQ_FAULT | HMM_PFN_REQ_WRITE;
> +
> +	ret = hmm_range_fault(&range);
> +
> +	migrate_hmm_range_setup(&range);

[Severity: Critical]
As reported by Sashiko on v16 [2], when hmm_range_fault() faults at an address
after already collecting earlier PTEs in the range, can concurrent page table
zapping cause a use-after-free of destination pages because the walk resumes
at hmm_vma_walk.last without re-validating [range->start, hmm_vma_walk.last)?

When HMM_PFN_REQ_MIGRATE | HMM_PFN_REQ_FAULT is used, walk_page_range()
collects present pages in [range->start, addr) via
hmm_vma_handle_migrate_prepare(), installing migration PTEs and setting
HMM_PFN_VALID | HMM_PFN_MIGRATE in range->hmm_pfns[]. When a later address
addr requires a fault, hmm_vma_handle_pte() drops the PTE lock, sets
hmm_vma_walk.last = addr via hmm_record_fault(), and returns
HMM_FAULT_PENDING:

mm/hmm.c:hmm_range_fault_locked() {
	ret = walk_page_range(mm, hmm_vma_walk.last, range->end,
			      &hmm_walk_ops, &hmm_vma_walk);
	if (ret == HMM_FAULT_PENDING) {
		hmm_vma_migrate_disarm(&hmm_vma_walk);
		ret = hmm_do_fault(mm, &hmm_vma_walk);
		...
	}
}

While the PTE spinlock and migration mmu_notifier invalidation are dropped
during hmm_do_fault(), a concurrent madvise(MADV_DONTNEED) only requires
mmap_read_lock and can clear the migration PTEs in [range->start, addr) via
zap_pte_range() -> zap_nonpresent_ptes() without taking the folio lock. When
the loop resumes walk_page_range() at hmm_vma_walk.last = addr,
hmm_vma_handle_migrate_prepare_rollback() only inspects [addr, end) and never
re-checks [range->start, addr).

Those zapped entries keep HMM_PFN_VALID | HMM_PFN_MIGRATE in range->hmm_pfns[]
and pass migrate_vma_check_page() because folio_mapped(src) is false. Later,
in __migrate_device_finalize(), remove_migration_ptes(src, dst, 0) finds no
migration PTEs to replace (so dst is never mapped into the CPU page tables or
given a page table refcount), and folio_put(dst) frees the destination folio
while the device driver still has it mapped.

[Severity: High]
As reported by Sashiko on v16 [3], can switching migrate_vma_setup() and
hmm_range_fault() to the HMM migration path deadlock on folio locks in
migrate_vma_split_folio() and hmm_do_fault(), or cause spurious split_folio()
-EAGAIN failures?

First, during migration collection, hmm_vma_handle_migrate_prepare() keeps all
previously collected folios in the range locked (using folio_trylock() to
avoid deadlocks). However, when it encounters a PTE-mapped large folio, it
calls migrate_vma_split_folio(), which takes a reference and does a blocking
folio_lock() while still holding the locks and migration PTEs on fault_folio
and all earlier collected folios:

mm/hmm.c:migrate_vma_split_folio() {
	if (folio != fault_folio)
		folio_get(folio);

	lazy_mmu_mode_disable();
	pte_unmap_unlock(ptep, hmm_vma_walk->ptl);
	hmm_vma_walk->ptelocked = false;

	if (folio != fault_folio)
		folio_lock(folio);

	return __migrate_vma_split_folio(folio, fault_page);
}

If another task holds folio_lock(folio) and waits on one of the earlier
migration PTEs via migration_entry_wait() (or tries to lock one of the
already-collected folios), both tasks deadlock. Taking folio_get(folio) before
folio_lock(folio) also means a concurrent caller blocking on folio_lock(folio)
elevates the refcount and causes the lock holder's split_folio() to fail with
-EAGAIN.

Second, when HMM_PFN_REQ_FAULT is used with HMM_PFN_REQ_MIGRATE, encountering
another thread's migration entry in hmm_vma_handle_pte() or hmm_vma_walk_pmd()
(or a swap or device-private entry) invokes hmm_do_fault() ->
handle_mm_fault(), which sleeps in migration_entry_wait() or folio_lock()
while still holding the locks and migration PTEs on all folios collected
earlier in the walk.

[Severity: High]
Does hmm_vma_handle_migrate_prepare_pmd() in mm/hmm.c miss a
folio_test_anon(folio) check when collecting or splitting a PMD-mapped THP?

At the PTE level, hmm_vma_handle_migrate_prepare() rejects non-anonymous
folios:

mm/hmm.c:hmm_vma_handle_migrate_prepare() {
	/* Only anonymous memory can be migrated to device memory */
	if (!page || !folio_test_anon(page_folio(page)))
		goto out;
}

However, hmm_vma_handle_migrate_prepare_pmd() selects any non-zero
pmd_trans_huge(*pmdp) folio when MIGRATE_VMA_SELECT_SYSTEM is set:

mm/hmm.c:hmm_vma_handle_migrate_prepare_pmd() {
	if (pmd_trans_huge(*pmdp)) {
		if (!(minfo & MIGRATE_VMA_SELECT_SYSTEM))
			goto out;

		folio = pmd_folio(*pmdp);
		if (is_huge_zero_folio(folio))
			return hmm_pfns_fill(start, end, hmm_vma_walk, 0);

	}
}

If HMM migration runs on a private file-backed or shmem VMA containing a
PMD-mapped page-cache/shmem THP, hmm_vma_handle_migrate_prepare_pmd() locks
the non-anonymous folio and either installs a PMD migration entry (holding the
file/shmem folio lock across subsequent faults in hmm_do_fault(), which take
mapping->invalidate_lock and invert the mapping->invalidate_lock -> folio_lock
ordering) or splits the THP even though hmm_vma_handle_migrate_prepare() will
reject all of its PTEs.

[Severity: High]
In hmm_vma_walk_pmd() in mm/hmm.c, when splitting a PTE-mapped large folio
fails in hmm_vma_handle_migrate_prepare(), doesn't returning 0 immediately
skip the remaining PTEs in [addr + PAGE_SIZE, end)?

In the PTE loop of hmm_vma_walk_pmd():

mm/hmm.c:hmm_vma_walk_pmd() {
	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, addr, ptep,
						   hmm_pfns, &unmapped);
		if (r == -EAGAIN) {
			addr = start;
			goto again;
		}
		if (r) {
			HMM_ASSERT_UNLOCKED(hmm_vma_walk);
			if (unmapped)
				flush_tlb_range(walk->vma, start, addr);
			return 0;
		}
	}
}

When migrate_vma_split_folio() fails at addr, it drops the PTE lock and
hmm_vma_handle_migrate_prepare() returns -EFAULT. Returning 0 from
hmm_vma_walk_pmd() tells walk_page_range() that the entire PMD range
[start, end) is complete, leaving [addr + PAGE_SIZE, end) unvisited and
unpopulated in range->hmm_pfns[]. Moreover, if an earlier pass in
hmm_vma_walk_pmd() had already collected PTEs at addresses above addr before
restarting via addr = start; goto again, dropping the PTE lock at addr and
returning 0 skips re-verifying and rolling back those collected entries in
[addr + PAGE_SIZE, end).

[Severity: Medium]
Can writable protnone PTEs and PMDs lose MIGRATE_PFN_WRITE in migrate->src[]
when migrated via migrate_vma_setup() or hmm_range_fault() without
HMM_PFN_REQ_FAULT?

For a present protnone PTE or PMD, pte_to_hmm_pfn_flags() and
pmd_to_hmm_pfn_flags() return 0, so HMM_PFN_WRITE is not set in *hmm_pfn.
Then hmm_vma_handle_migrate_prepare() and hmm_vma_handle_migrate_prepare_pmd()
collect the protnone entry, install a writable migration entry if
pte_write(pte) or pmd_write(*pmdp) is true, and set HMM_PFN_VALID |
HMM_PFN_MIGRATE in *hmm_pfn, but never set HMM_PFN_WRITE:

mm/hmm.c:hmm_vma_handle_migrate_prepare() {
		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;
}

When migrate_hmm_range_setup() builds migrate->src[i] from range->hmm_pfns[i]:

mm/migrate_device.c:migrate_hmm_range_setup() {
		migrate->src[i] |= MIGRATE_PFN_MIGRATE;
		migrate->src[i] |= (pfn & HMM_PFN_WRITE) ? MIGRATE_PFN_WRITE : 0;
		migrate->src[i] |= (pfn & HMM_PFN_COMPOUND) ? MIGRATE_PFN_COMPOUND : 0;
}

MIGRATE_PFN_WRITE is left unset even though the deleted
migrate_vma_collect_pmd() and migrate_vma_collect_huge_pmd() helpers preserved
MIGRATE_PFN_WRITE whenever pte_write(pte) or pmd_write(*pmdp) was set. Should
hmm_vma_handle_migrate_prepare() and hmm_vma_handle_migrate_prepare_pmd() also
set HMM_PFN_WRITE on *hmm_pfn when writable is true?

[1] https://lore.kernel.org/r/arY5zKsLuf7f_EYw@infradead.org
[2] https://lore.kernel.org/r/20261021060506.2377545-10-mpenttil@redhat.com
[3] https://lore.kernel.org/r/20261021060506.2377545-10-mpenttil@redhat.com

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