Re: [PATCH v17 08/12] mm/hmm: implement rollback for device page migration in HMM pagewalk

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] mm/hmm: missing rollback of prior entries when restarting after fault
- [High] mm/hmm: missing rollback and incomplete walk on folio split failure
- [High] mm/hmm: self-deadlock on -EBUSY retry due to memset() of hmm_pfns
--

commit 57b87c4290e06aea8db775c69fa86c746acfe43d
Author: Mika Penttilä <mpenttil@redhat.com>

mm/hmm: implement rollback for device page migration in HMM pagewalk

This commit adds rollback handling to unlock and unreference already
collected folios if page tables vanish or change while page table locks are
dropped during a migration pagewalk. It also skips migration PTEs installed
earlier in the same walk when restarting a PMD walk.

> diff --git a/mm/hmm.c b/mm/hmm.c
> index 690b73908f34e..1c8a3fa29c4e6 100644
> --- a/mm/hmm.c
> +++ b/mm/hmm.c

[ ... ]

> @@ -331,6 +344,15 @@ static int hmm_vma_handle_pte(struct mm_walk *walk, unsigned long addr,
>  	if (!pte_present(pte)) {
>  		const softleaf_t entry = softleaf_from_pte(pte);
>  
> +		/*
> +		 * Our own migration entry, collected earlier in this walk:
> +		 * keep the collected pfn, don't fault or roll it back.
> +		 */
> +		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;

[Severity: High]
When hmm_range_fault_locked() aborts with -EBUSY (or a caller retries
hmm_range_fault() after mmu_interval_read_retry() trips) after collecting
earlier pages in the range, those folios remain locked, referenced, and
mapped as migration PTEs.

When the caller retries hmm_range_fault() with the same range and hmm_pfns
array, 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));
	}
    ...
}

On the retry walk, hmm_pfn_collected(*hmm_pfn) evaluates to false for the
migration PTEs installed by the current thread on the previous attempt. Does
this cause hmm_vma_handle_pte() and hmm_vma_walk_pmd() to treat our own
migration entries as belonging to another thread and call
hmm_do_fault()->handle_mm_fault()->migration_entry_wait() on folios already
locked by current, resulting in a self-deadlock?

> +
>  		/*
>  		 * Don't fault in device private pages owned by the caller,
>  		 * just report the PFN.
> @@ -416,6 +438,9 @@ static int hmm_vma_handle_pte(struct mm_walk *walk, unsigned long addr,
>  
>  	new_pfn_flags = pte_pfn(pte) | cpu_flags;
>  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;
>  
> @@ -427,7 +452,19 @@ static int hmm_vma_handle_pte(struct mm_walk *walk, unsigned long addr,
>  	} else {
>  		pte_unmap(ptep);
>  	}
> -	/* Fault any virtual address we were asked to fault */
> +	/*
> +	 * This pte needs a fault, so it is not our migration entry: a pfn
> +	 * collected here earlier is gone, roll it back before faulting.
> +	 */
> +	hmm_vma_handle_migrate_prepare_rollback(hmm_vma_walk, addr,
> +						addr + PAGE_SIZE, hmm_pfn);
> +	/*
> +	 * Fault any virtual address we were asked to fault. When migrating,
> +	 * only this page: later ptes may be our own migration entries or
> +	 * caller owned device private pages, which must not be faulted.
> +	 */
> +	if (hmm_select_migrate(range))
> +		end = addr + PAGE_SIZE;
>  	return hmm_record_fault(addr, end, required_fault, walk);
>  }

[Severity: High]
As reported by Sashiko on v16 [1], when a PTE fault occurs at addr after
earlier PTEs at addresses below addr have already been collected by
hmm_vma_handle_migrate_prepare(), the fault path drops hmm_vma_walk->ptl,
rolls back only [addr, addr + PAGE_SIZE), and calls hmm_record_fault(),
which sets hmm_vma_walk->last = addr and returns HMM_FAULT_PENDING.

In hmm_range_fault_locked(), HMM_FAULT_PENDING disarms the migration
invalidation range via hmm_vma_migrate_disarm() and runs hmm_do_fault()
without holding any page table spinlock, then resumes walk_page_range() at
hmm_vma_walk.last:

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

Because walk_page_range() restarts at addr rather than range->start,
hmm_vma_walk_pmd() is invoked with start = addr (and any earlier PMDs in
[range->start, addr) are skipped completely).

If the migration PTEs or PTE page table covering [range->start, addr) were
zapped or replaced while locks were dropped during hmm_do_fault(), can those
earlier collected entries in range->hmm_pfns ever be revisited or rolled
back, or will they leak folio locks and references?

[ ... ]

> @@ -1192,6 +1311,12 @@ static int hmm_vma_walk_pmd(pmd_t *pmdp,
>  				flush_tlb_range(walk->vma, start, addr);
>  				unmapped = false;
>  			}
> +			/*
> +			 * The pte lock was dropped and the pte table may have
> +			 * changed: re-walk from start. Our own migration entries
> +			 * are kept, anything else collected is rolled back.
> +			 */
> +			addr = start;
>  			goto again;
>  		}
>  		if (r) {

[Severity: High]
When hmm_vma_handle_migrate_prepare() attempts to split a large folio via
migrate_vma_split_folio(), it unconditionally drops hmm_vma_walk->ptl and
sleeps on folio_lock() before calling __migrate_vma_split_folio():

mm/hmm.c:migrate_vma_split_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 __migrate_vma_split_folio() fails, hmm_vma_handle_migrate_prepare()
returns -EFAULT, and the if (r) branch in hmm_vma_walk_pmd() flushes the TLB
and returns 0 immediately:

mm/hmm.c:hmm_vma_walk_pmd() {
    ...
		if (r) {
			/*
			 * Splitting the folio failed; migrate_vma_split_folio()
			 * already dropped the PTE lock. Don't migrate the rest
			 * of the pmd range; addr keeps its valid pfn.
			 */
			HMM_ASSERT_UNLOCKED(hmm_vma_walk);
			if (unmapped)
				flush_tlb_range(walk->vma, start, addr);
			return 0;
		}
    ...
}

Since the PTE lock was dropped while waiting on folio_lock(), if migration
PTEs or the PTE table in [start, end) vanished during that window, won't
returning 0 without re-walking [start, end) skip rolling back those
collected folios, and also leave range->hmm_pfns unpopulated for the
remaining addresses in [addr + PAGE_SIZE, end)?

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

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