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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [Critical] mm: hmm_vma_handle_pte() corrupts migration PTE state during migration
- [High] mm: migrate_vma_setup() skips non-VM_READ VMAs via hmm_vma_walk_test()
- [High] mm: hmm_vma_handle_pte() faults entire PMD range [addr, end) on fault
- [High] mm: hmm_range_fault_locked() calls handle_mm_fault() under folio_lock()
- [High] mm: hmm_vma_walk_pmd() fails to roll back collected entries on retry
- [High] mm: uninitialized migrate bounds and pfn offset in hmm_range_fault()
- [High] mm: self-invalidation of range->notifier during hmm_range_fault()
- [High] mm/hmm: refcount-before-lock and blocking folio_lock() in migrate_vma_split_folio()
- [Medium] mm: migrate_vma_setup() skips protnone pages when fault flag is unset
--

commit 6029fe80390f5816cec1bea4014e1df1b5d1785d
Author: Mika Penttilä <mpenttil@redhat.com>

mm: enable device page migration from HMM pagewalk

This commit enables device page migration through the HMM page table walk in
hmm_range_fault(), switches migrate_vma_setup() to use hmm_range_fault() and
migrate_hmm_range_setup(), and removes the legacy migrate_vma_collect() walk.

> 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,
>  };
>  
>  struct migrate_vma {
> @@ -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: Critical]
When hmm_select_migrate() is enabled, can hmm_vma_handle_pte() in mm/hmm.c
roll back an already-installed migration PTE without restoring the page table
entry, and also skip waiting on foreign migration entries?

In mm/hmm.c:hmm_vma_handle_pte():

	if (!pte_present(pte)) {
		const softleaf_t entry = softleaf_from_pte(pte);
		...
		required_fault =
			hmm_pte_need_fault(hmm_vma_walk, pfn_req_flags, 0);
		if (!required_fault)
			goto out;
		...
		if (softleaf_is_migration(entry)) {
			if (!hmm_select_migrate(range)) {
				HMM_ASSERT_UNLOCKED(hmm_vma_walk);
				pte_unmap(ptep);
				hmm_vma_walk->last = addr;
				migration_entry_wait(walk->mm, pmdp, addr);
				return -EBUSY;
			}
			return 0;
		}
	...
out:
	hmm_vma_handle_migrate_prepare_rollback(hmm_vma_walk, addr,
						addr + PAGE_SIZE,
						hmm_pfn);
	*hmm_pfn = (*hmm_pfn & HMM_PFN_INOUT_FLAGS) | new_pfn_flags;
	return 0;

First, when HMM_PFN_REQ_FAULT is not set (the default in migrate_vma_setup()),
required_fault is 0. If hmm_vma_handle_migrate_prepare() collects a page at
addr (locking the folio, taking a reference, installing a migration PTE, and
setting HMM_PFN_VALID | HMM_PFN_MIGRATE in *hmm_pfn) and that PTE is visited
again (for example when hmm_vma_walk_pmd() restarts from start after !ptep,
when hmm_range_fault_locked() loops on -EBUSY after a THP split, or when a
caller retries hmm_range_fault() on -EBUSY), hmm_vma_handle_pte() sees
!pte_present(pte) and jumps to out: before checking softleaf_is_migration().
At out:, hmm_vma_handle_migrate_prepare_rollback() unlocks the folio, drops
its reference, and clears *hmm_pfn to 0, leaving the migration PTE dangling
in the page table. A similar issue happens if migrate_vma_split_folio() fails
in hmm_vma_walk_pmd() and calls hmm_pfns_fill(addr, end, hmm_vma_walk,
HMM_PFN_ERROR) over entries that were already collected in a prior pass.

Second, when HMM_PFN_REQ_FAULT is set and hmm_vma_handle_pte() or
hmm_vma_walk_pmd() encounters a migration PTE or PMD installed by another
thread (!hmm_pfn_collected(*hmm_pfn)), checking if (!hmm_select_migrate(range))
or if (!minfo) causes the walk to return 0 without waiting via
migration_entry_wait() / pmd_migration_entry_wait() and without populating
*hmm_pfn.

Should hmm_vma_handle_pte() check softleaf_is_migration(entry) before the
!required_fault branch and distinguish entries already collected by this walk
(hmm_pfn_collected(*hmm_pfn)) from foreign migration entries?

[Severity: High]
Also in mm/hmm.c:hmm_vma_walk_pmd(), when minfo is non-zero and the PTE lock
is dropped mid-PMD (such as in migrate_vma_split_folio() returning -EAGAIN at
addr > start), could already-collected entries in [start, addr) fail to be
rolled back if the PTE page table disappears or is replaced?

In mm/hmm.c:hmm_vma_walk_pmd():

	if (minfo) {
		ptep = pte_offset_map_lock(mm, pmdp, addr, &hmm_vma_walk->ptl);
		if (ptep) {
			lazy_mmu_mode_enable();
			hmm_vma_walk->ptelocked = true;
		} else {
			/* The pte table is gone */
			hmm_vma_handle_migrate_prepare_rollback(walk->private, addr, end, hmm_pfns);
		}
	} else {
		ptep = pte_offset_map(pmdp, addr);
	}
	if (!ptep) {
		addr = start;
		goto again;
	}

When addr > start, the entries collected earlier in the same PMD are in
[start, addr) starting at hmm_pfns_start, whereas [addr, end) starting at
hmm_pfns is the unwalked remainder of the PMD. Passing addr, end, hmm_pfns to
hmm_vma_handle_migrate_prepare_rollback() leaves [start, addr) locked and
referenced with HMM_PFN_VALID | HMM_PFN_MIGRATE set.

Moreover, if addr is reset to start and goto again finds that *pmdp is now a
PMD migration entry (pmd_is_migration_entry(pmd)), or if hmm_vma_walk_hole(),
hmm_vma_handle_pmd(), or hmm_vma_handle_absent_pmd() returns HMM_FAULT_PENDING
or -EFAULT before reaching their rollback calls, the previously collected
folios in hmm_pfns_start are never rolled back either.

Should the rollback when !ptep (and before early returns after goto again)
cover start to end using hmm_pfns_start?

[Severity: High]
Enabling hmm_select_migrate() also activates migrate_vma_split_folio() in
mm/hmm.c when hmm_vma_handle_migrate_prepare() encounters a PTE-mapped large
folio:

static int migrate_vma_split_folio(struct folio *folio,
				   struct page *fault_page,
				   struct hmm_vma_walk *hmm_vma_walk,
				   pte_t *ptep)
{
	struct folio *fault_folio = fault_page ? page_folio(fault_page) : NULL;

	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);
}

Unlike hmm_vma_handle_migrate_prepare_pmd() and the order-0 path in
hmm_vma_handle_migrate_prepare(), which use folio_trylock() because current
may already hold folio_lock on fault_folio and on folios collected at earlier
addresses in the walk, migrate_vma_split_folio() calls blocking folio_lock().
Can this deadlock if two concurrent migrations lock folios in opposite order,
or if another task holds folio_lock(folio) while waiting on a migration entry
installed earlier in this walk?

In addition, calling folio_get(folio) before blocking on folio_lock(folio)
means that if two tasks concurrently enter migrate_vma_split_folio() on the
same large folio, the waiting task's extra reference will cause the lock
holder's split_folio() call to fail with -EAGAIN. Would it be safer to use
folio_trylock() here as well?

> diff --git a/mm/hmm.c b/mm/hmm.c
> index 5a1471fd5d288..4d853ee8e8b8c 100644
> --- a/mm/hmm.c
> +++ b/mm/hmm.c
> @@ -1542,14 +1542,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.
> + *
> + * On any other error, or when abandoning a retry, the caller must call
> + * migrate_hmm_range_setup(), migrate_vma_pages() with no valid destination
> + * entries, and migrate_vma_finalize() to abort the partial migration.

[Severity: High]
What happens in migrate_hmm_range_setup() if hmm_range_fault() fails before
hmm_vma_capture_migrate_range() arms hmm_vma_walk->mmu_armed, or if
range->start begins in an unmapped hole before walk->vma->vm_start?

In mm/hmm.c:hmm_vma_post_range_fault():

	if (hmm_select_migrate(range) &&
	    hmm_vma_walk->mmu_armed) {
		if (range->notifier) {
			hmm_fill_migrate_vma(range,
					     hmm_vma_walk->vma,
					     hmm_vma_walk->start,
					     hmm_vma_walk->end);
		}
		mmu_notifier_invalidate_range_end(&hmm_vma_walk->mmu_range);
	}

When a caller drives migration directly through hmm_range_fault() with
range->notifier set, range->migrate->vma, range->migrate->start, and
range->migrate->end are only populated if hmm_vma_walk->mmu_armed is true. If
hmm_range_fault_locked() fails before mmu_armed is set (for instance, if
mmu_interval_check_retry() returns -EBUSY on entry, hmm_vma_walk_hole()
returns -EFAULT when !walk->vma, or hmm_vma_capture_migrate_range() returns
-ERANGE), calling migrate_hmm_range_setup() as documented above will compute
migrate->npages = (migrate->end - migrate->start) >> PAGE_SHIFT from
uninitialized or stale fields in range->migrate.

Additionally, in hmm_vma_capture_migrate_range():

	if (end - start > range->end - range->start)
		return -ERANGE;

	if (!hmm_vma_walk->mmu_armed) {
		hmm_vma_walk->start = start;
		hmm_vma_walk->end = end;

Because walk_page_range() clamps start and end to the VMA boundaries within
[range->start, range->end), end - start is never greater than
range->end - range->start. And if range->start < vma->vm_start (with
HMM_PFN_REQ_FAULT unset across the hole), hmm_vma_walk->start is set to
vma->vm_start while hmm_vma_walk_pmd() indexes range->hmm_pfns by
(addr - range->start) >> PAGE_SHIFT. When migrate_hmm_range_setup() then
reads range->hmm_pfns[0 .. migrate->npages - 1], won't it read from the
beginning of range->hmm_pfns instead of the VMA offset, misaligning
migrate->src[] and leaking any collected folios and migration PTEs at the end
of the range?

[Severity: High]
When hmm_range_fault() is called with both range->notifier and
HMM_PFN_REQ_MIGRATE set, won't internal MMU notifier invalidations during the
walk trip mmu_interval_check_retry() and cause premature termination of the
internal -EBUSY loop?

In mm/hmm.c:hmm_range_fault_locked():

	do {
		/* If range is no longer valid force retry. */
		if (range->notifier && mmu_interval_check_retry(range->notifier,
								range->notifier_seq)) {
			ret =  -EBUSY;
			break;
		}
		ret = walk_page_range(mm, hmm_vma_walk.last, range->end,
				      &hmm_walk_ops, &hmm_vma_walk);
		if (ret == HMM_FAULT_PENDING) {
			ret = hmm_do_fault(mm, &hmm_vma_walk);
			...
		}
	} while (ret == -EBUSY);

	hmm_vma_post_range_fault(&hmm_vma_walk);
	return ret;

During the walk, hmm_vma_capture_migrate_range() calls
mmu_notifier_invalidate_range_start() with MMU_NOTIFY_MIGRATE, and any page
fault in hmm_do_fault() (such as a CoW or swap fault) or THP split in
hmm_vma_handle_migrate_prepare_pmd() triggers an MMU notifier invalidation
with owner = NULL. If range->notifier covers the migrated range, those
invalidations increment range->notifier's sequence count. When hmm_do_fault()
or hmm_vma_handle_migrate_prepare_pmd() returns -EBUSY to re-walk the faulted
or split pages, mmu_interval_check_retry() will immediately break out of the
loop with -EBUSY, and hmm_vma_post_range_fault() will call
mmu_notifier_invalidate_range_end() while partial migration PTEs from earlier
addresses are still installed.

> + *
> + * The mmap read lock must remain held until the migration has either been
> + * completed or aborted.
>   */
>  int hmm_range_fault(struct hmm_range *range)
>  {
> diff --git a/mm/migrate_device.c b/mm/migrate_device.c
> index 3bcb398c4e02c..c61a9e54e06bb 100644
> --- a/mm/migrate_device.c
> +++ b/mm/migrate_device.c
[ ... ]
> @@ -764,10 +276,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;

[Severity: High]
When MIGRATE_VMA_FAULT or MIGRATE_VMA_WRITE is used (or HMM_PFN_REQ_MIGRATE
is combined with HMM_PFN_REQ_FAULT), can a fault on one PTE cause
hmm_do_fault() to fault across the rest of the PMD range, deadlocking on
already-locked folios or faulting caller-owned device-private pages back to
RAM?

In mm/hmm.c:hmm_vma_handle_pte():

fault:
	if (hmm_vma_walk->ptelocked) {
		lazy_mmu_mode_disable();
		pte_unmap_unlock(ptep, hmm_vma_walk->ptl);
		hmm_vma_walk->ptelocked = false;
	} else {
		pte_unmap(ptep);
	}
	/* Fault any virtual address we were asked to fault */
	return hmm_record_fault(addr, end, required_fault, walk);

Here end is the end of the PMD walk range passed from hmm_vma_walk_pmd(), and
hmm_do_fault() loops from addr to end calling handle_mm_fault() on every page
in [addr, end):

	for (; addr < end; addr += PAGE_SIZE) {
		vm_fault_t ret;

		ret = handle_mm_fault(vma, addr, fault_flags, NULL);

If a subsequent PTE in [addr + PAGE_SIZE, end) is a device-private entry
owned by args->pgmap_owner, handle_mm_fault() will invoke do_swap_page() ->
pgmap->ops->migrate_to_ram() even though hmm_vma_handle_pte() normally avoids
faulting caller-owned device-private pages (and if that entry is
args->fault_page, which is already locked, do_swap_page() will wait forever
in softleaf_entry_wait_on_locked()). Similarly, if a subsequent PTE in
[addr + PAGE_SIZE, end) already has a migration entry installed by the
current thread during an earlier pass, handle_mm_fault() will call
migration_entry_wait() and deadlock on the folio lock held by current.

Should hmm_vma_handle_pte() pass addr + PAGE_SIZE instead of end to
hmm_record_fault()?

[Severity: High]
Relatedly, when hmm_range_fault_locked() calls hmm_do_fault() ->
handle_mm_fault() after a walk callback returns HMM_FAULT_PENDING, current
still holds folio_lock on all folios collected at earlier addresses in the
walk (via hmm_vma_handle_migrate_prepare() or
hmm_vma_handle_migrate_prepare_pmd()).

Does calling handle_mm_fault() while holding folio_lock on previously
collected folios violate mm lock ordering (where mapping->invalidate_lock in
filemap_fault() is acquired outside folio_lock) and risk a self-deadlock if
the faulted address resolves to the same folio that was already locked at an
earlier address in the range (such as a shared, KSM, or swap-cached folio
mapped at two addresses)?

> +
> +	ret = hmm_range_fault(&range);
> +
> +	migrate_hmm_range_setup(&range);

[Severity: High]
By switching migrate_vma_setup() from migrate_vma_collect() to
hmm_range_fault(), the walk now runs hmm_vma_walk_test() in mm/hmm.c:

	if (!(vma->vm_flags & (VM_IO | VM_PFNMAP)) &&
	    vma->vm_flags & VM_READ)
		return 0;
	...
	hmm_pfns_fill(start, end, hmm_vma_walk, HMM_PFN_ERROR);

	/* Skip this vma and continue processing the next vma. */
	return 1;

Previously, migrate_vma_collect() did not have a .test_walk callback requiring
VM_READ. With this change, if a VMA containing device-private or
device-coherent pages does not have VM_READ set (for example, an execute-only
VMA that faults on instruction fetch, or a VMA changed to PROT_NONE via
mprotect() before driver eviction or a FOLL_FORCE access), hmm_vma_walk_test()
skips the VMA, migrate_hmm_range_setup() sets args->cpages = 0, and
migrate_vma_setup() returns 0.

In pgmap->ops->migrate_to_ram() handlers (such as nouveau_dmem_migrate_to_ram(),
__drm_pagemap_migrate_to_ram(), svm_migrate_to_ram(), and
dmirror_devmem_fault()), a 0 return from migrate_vma_setup() with cpages == 0
is treated as a race where another thread already migrated the page, so the
handler returns 0 without resolving the device-private swap PTE and the CPU
page fault retries indefinitely. Driver eviction paths also fail to migrate
device pages back to RAM when VM_READ is clear.

Should hmm_vma_walk_test() allow non-VM_READ VMAs when migrating (or when
faulting is not requested during migration)?

[Severity: Medium]
Does switching migrate_vma_setup() to hmm_range_fault() also cause present
pte_protnone() and pmd_protnone() pages (such as pages marked for NUMA
hinting faults by automatic NUMA balancing) to be skipped when
MIGRATE_VMA_FAULT is not set?

In mm/hmm.c, pte_to_hmm_pfn_flags() and pmd_to_hmm_pfn_flags() return 0
(without HMM_PFN_VALID) when pte_protnone(pte) or pmd_protnone(pmd) is true:

static inline unsigned long pte_to_hmm_pfn_flags(struct hmm_range *range,
						 pte_t pte)
{
	if (pte_none(pte) || !pte_present(pte) || pte_protnone(pte))
		return 0;
	return pte_write(pte) ? (HMM_PFN_VALID | HMM_PFN_WRITE) : HMM_PFN_VALID;
}

When callers invoke migrate_vma_setup() without MIGRATE_VMA_FAULT, no fault is
triggered for protnone entries, and both hmm_vma_handle_migrate_prepare() and
hmm_vma_handle_migrate_prepare_pmd() bail out at:

	if (!(hmm_pfn[0] & HMM_PFN_VALID))
		goto out;

Previously, migrate_vma_collect_pmd() and migrate_vma_collect_huge_pmd()
collected and migrated present protnone PTEs and THP PMDs without requiring a
fault.

[ ... ]

>
>  	/*
>  	 * At this point pages are locked and unmapped, and thus they have

титься

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