Re: [PATCH v17 10/12] lib/test_hmm: add a new testcase for the migrate on fault

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] mm: test_hmm: deadlock and folio leak on retry in dmirror_range_fault()
- [Medium] mm: test_hmm: unmapped device pages on retry in do_fault_and_migrate()
- [Low] lib/test_hmm: excessive stack frame in dmirror_fault_and_migrate_to_device()
--

commit 7670468c9cdf50d8e222db90061981491fa308c8
Author: Mika Penttilä <mpenttil@redhat.com>

lib/test_hmm: add a new testcase for the migrate on fault

This commit adds a migrate-on-fault testcase to the HMM test driver and
selftests using HMM_PFN_REQ_FAULT | HMM_PFN_REQ_MIGRATE with
hmm_range_fault() to fault in and migrate ranges to device-private memory.

> diff --git a/lib/test_hmm.c b/lib/test_hmm.c
> index f05f801b8404b..2045896493ae3 100644
> --- a/lib/test_hmm.c
> +++ b/lib/test_hmm.c
> @@ -36,6 +36,7 @@
>  #define DMIRROR_RANGE_FAULT_TIMEOUT	1000
>  #define DEVMEM_CHUNK_SIZE		(256 * 1024 * 1024U)
>  #define DEVMEM_CHUNKS_RESERVE		16
> +#define PFNS_ARRAY_SIZE			64
>  
>  /*
>   * For device_private pages, dpage is just a dummy struct page
> @@ -355,6 +356,7 @@ static int dmirror_range_fault(struct dmirror *dmirror,
>  	struct mm_struct *mm = dmirror->notifier.mm;
>  	unsigned long timeout =
>  		jiffies + msecs_to_jiffies(HMM_RANGE_DEFAULT_TIMEOUT);
> +	bool migrate = range->default_flags & HMM_PFN_REQ_MIGRATE;
>  	int ret;
>  
>  	while (true) {
> @@ -364,9 +366,15 @@ static int dmirror_range_fault(struct dmirror *dmirror,
>  		}
>  
>  		range->notifier_seq = mmu_interval_read_begin(range->notifier);
> -		mmap_read_lock(mm);
> -		ret = hmm_range_fault(range);
> -		mmap_read_unlock(mm);
> +
> +		/* mmap lock held for whole migrate on fault operation */
> +		if (!migrate) {
> +			mmap_read_lock(mm);
> +			ret = hmm_range_fault(range);
> +			mmap_read_unlock(mm);
> +		} else {
> +			ret = hmm_range_fault(range);
> +		}
>  		if (ret) {
>  			if (ret == -EBUSY)
>  				continue;
> @@ -382,7 +390,9 @@ static int dmirror_range_fault(struct dmirror *dmirror,
>  		break;
>  	}
>  
> -	ret = dmirror_do_fault(dmirror, range);
> +	/* update device page table after migration */
> +	if (!migrate)
> +		ret = dmirror_do_fault(dmirror, range);
>  
>  	mutex_unlock(&dmirror->mutex);
>  out:

[Severity: High]
Can retrying inside dmirror_range_fault() when migrate is true cause a
self-deadlock and leak folio locks and references?

During the first call to hmm_range_fault(),
hmm_vma_handle_migrate_prepare() and hmm_vma_handle_migrate_prepare_pmd()
collect present pages by locking the folio, taking a folio reference,
installing a migration PTE, and setting HMM_PFN_VALID | HMM_PFN_MIGRATE in
range->hmm_pfns.

If hmm_range_fault() returns -EBUSY, or if hmm_range_fault() returns 0 and
mmu_interval_read_retry() trips in dmirror_range_fault() (for example,
after migrate_vma_split_folio() splits a PTE-mapped large folio and fires
MMU_NOTIFY_CLEAR, or due to a concurrent invalidation),
dmirror_range_fault() loops back via continue without returning to
do_fault_and_migrate() to abort and finalize the already-collected
migration entries.

On the next iteration of the loop in dmirror_range_fault(), calling
hmm_range_fault() -> hmm_range_fault_locked() zeroes range->hmm_pfns:

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);
		memset(range->hmm_pfns, 0,
		       ((range->end - range->start) >> PAGE_SHIFT) *
		       sizeof(*range->hmm_pfns));
	}

When the retry walk visits the migration PTEs installed by the previous
attempt, hmm_pfn_collected(*hmm_pfn) evaluates to false in
hmm_vma_handle_pte() because *hmm_pfn was zeroed:

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;

It then treats its own migration entry as an external migration entry and
calls hmm_do_fault() -> handle_mm_fault() -> migration_entry_wait(),
waiting forever on the folio lock already held by current. In addition, if
any collected PTEs were unmapped before the retry walk (or if the retry
walk fails or times out before revisiting them), zeroing range->hmm_pfns
prevents hmm_vma_handle_migrate_prepare_rollback() and
migrate_hmm_range_setup() / migrate_vma_finalize() from unlocking those
folios and dropping their references.

Furthermore, if dmirror_range_fault() times out at time_after(jiffies,
timeout) before calling hmm_range_fault() on a subsequent subrange chunk,
do_fault_and_migrate() still runs migrate_hmm_range_setup(),
migrate_vma_pages(), and migrate_vma_finalize() on the stale src_pfns and
migrate->start/end state from the previous chunk.

Should dmirror_range_fault() return -EBUSY to do_fault_and_migrate() when
migrate is true so the caller can run the migrate_hmm_range_setup(),
migrate_vma_pages(), and migrate_vma_finalize() cleanup before retrying?

[ ... ]

> @@ -1377,6 +1398,127 @@ static int dmirror_migrate_to_device(struct dmirror *dmirror,
>  	return ret;
>  }
>  
> +static int do_fault_and_migrate(struct dmirror *dmirror, struct hmm_range *range)
> +{
> +	struct migrate_vma *migrate = range->migrate;
> +	unsigned long timeout =
> +		jiffies + msecs_to_jiffies(HMM_RANGE_DEFAULT_TIMEOUT);
> +	int ret;
> +
> +	do {
> +		mmap_read_lock(dmirror->notifier.mm);
> +
> +		/* Fault-in pages for migration */
> +		ret = dmirror_range_fault(dmirror, range);
> +		/*
> +		 * Set this up even on error: hmm_range_fault() may have collected
> +		 * part of the range before failing.
> +		 */
> +		migrate_hmm_range_setup(range);
> +		if (ret) {
> +			/*
> +			 * dst[] entries are empty, so this marks every collected
> +			 * migration as failed. finalize then restores the source
> +			 * mappings and drops the associated locks/references.
> +			 */
> +			migrate_vma_pages(migrate);
> +			migrate_vma_finalize(migrate);
> +			goto unlock;
> +		}
> +
> +		pr_debug("Migrating from sys mem to device mem\n");
> +
> +		dmirror_migrate_alloc_and_copy(migrate, dmirror);
> +		migrate_vma_pages(migrate);
> +
> +		mutex_lock(&dmirror->mutex);
> +		if (mmu_interval_read_retry(&dmirror->notifier,
> +					    range->notifier_seq))
> +			ret = -EBUSY;
> +		else
> +			ret = __dmirror_migrate_map_locked(migrate, dmirror);
> +		mutex_unlock(&dmirror->mutex);
> +
> +		migrate_vma_finalize(migrate);
> +unlock:
> +		mmap_read_unlock(dmirror->notifier.mm);
> +	} while (ret == -EBUSY && !time_after(jiffies, timeout));
> +
> +	return ret;
> +}

[Severity: Medium]
As reported by Sashiko on v16 [1], when mmu_interval_read_retry() returns
true in do_fault_and_migrate() after migrate_vma_pages() has executed, can
this leave migrated device-private pages unmapped in dmirror->pt and cause
dmirror_fault_and_migrate_to_device() to fail with -ENOENT?

When mmu_interval_read_retry() trips here (for example, due to an
invalidation anywhere in the process mm since dmirror->notifier covers the
entire address space, or a partial zap, or split_huge_pmd() in
migrate_vma_insert_page()), __dmirror_migrate_map_locked() is skipped,
while migrate_vma_finalize() still commits any non-zapped migration PTEs to
device-private memory (and migrate_vma_insert_page() has already installed
device-private PTEs for pte_none or zero-page entries).

On the subsequent -EBUSY retry iteration of do_fault_and_migrate(), those
already-committed device-private pages are reported by hmm_vma_handle_pte()
as HMM_PFN_VALID without HMM_PFN_MIGRATE because migrate.flags only sets
MIGRATE_VMA_SELECT_SYSTEM, and dmirror_range_fault() skips
dmirror_do_fault() because HMM_PFN_REQ_MIGRATE is set.

Although the commit message notes that a real device would map committed
pages on its next fault, dmirror_fault_and_migrate_to_device() immediately
verifies the range by calling dmirror_do_read() directly without a fault
retry loop:

	mutex_lock(&dmirror->mutex);
	ret = dmirror_do_read(dmirror, start, end, &bounce);
	mutex_unlock(&dmirror->mutex);

Wouldn't dmirror_do_read() find no entry in dmirror->pt for those
already-migrated pages and fail the ioctl with -ENOENT?

> +
> +static int dmirror_fault_and_migrate_to_device(struct dmirror *dmirror,
> +					       struct hmm_dmirror_cmd *cmd)
> +{
> +	unsigned long start, size, end, next;
> +	unsigned long src_pfns[PFNS_ARRAY_SIZE] = { 0 };
> +	unsigned long dst_pfns[PFNS_ARRAY_SIZE] = { 0 };
> +	struct migrate_vma migrate = { 0 };
> +	struct hmm_range range = { 0 };
> +	struct dmirror_bounce bounce;

[Severity: Low]
Does allocating src_pfns[64] and dst_pfns[64] on the stack in
dmirror_fault_and_migrate_to_device() risk triggering -Wframe-larger-than
warnings?

On 64-bit architectures, src_pfns and dst_pfns alone consume 1024 bytes of
stack space alongside struct migrate_vma, struct hmm_range, and struct
dmirror_bounce (exceeding 1230 bytes of local variables before saved
registers and inlining do_fault_and_migrate()).

Commit 6046a3bed1c2 ("lib/test_hmm: reduce stack usage") previously had to
reduce the 64-element stack arrays in dmirror_migrate_to_device() after
hitting a 1408-byte stack frame build failure, and commit 775465fd26a3
("lib/test_hmm: use kvcalloc() for src/dst pfns allocation") moved
dmirror_migrate_to_device() and dmirror_migrate_to_system() to dynamic
allocation with kvcalloc(). Should dmirror_fault_and_migrate_to_device()
also use kvcalloc() or a smaller PFNS_ARRAY_SIZE?

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

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