Re: [PATCH v16 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:
- [Critical] mm/hmm: premature rollback of migration PTEs in hmm_vma_handle_pte()
- [High] mm/hmm: incomplete rollback on mid-PMD lock drop in hmm_vma_walk_pmd()
- [High] mm/hmm: missing rollback on fault and PMD migration in hmm_vma_walk_pmd()
--

commit 5c75620371e34a43110051163d33f29ab4f700b4
Author: Mika Penttilä <mpenttil@redhat.com>

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

This commit adds rollback handling during HMM migration page walks to unlock
and drop references on previously collected folios if page tables change while
locks are dropped.

> diff --git a/mm/hmm.c b/mm/hmm.c
> index 36a4edde9823c..5a1471fd5d288 100644
> --- a/mm/hmm.c
> +++ b/mm/hmm.c

[ ... ]

> @@ -282,6 +289,8 @@ static int hmm_vma_handle_pmd(struct mm_walk *walk, unsigned long addr,
>  		return hmm_record_fault(addr, end, required_fault, walk);
>  	}
>  
> +	hmm_vma_handle_migrate_prepare_rollback(hmm_vma_walk, addr,
> +						end, hmm_pfns);
>  	pfn = pmd_pfn(pmd) + ((addr & ~PMD_MASK) >> PAGE_SHIFT);
>  	for (i = 0; addr < end; addr += PAGE_SIZE, i++, pfn++) {
>  		hmm_pfns[i] &= HMM_PFN_INOUT_FLAGS;

[Severity: High]
What happens if entries in hmm_pfns were previously collected, locks were
dropped, and on re-walk the page table entry now requires a fault, fails with
-EFAULT, or is a PMD migration entry?

In hmm_vma_handle_pmd(), if required_fault is non-zero, it returns
hmm_record_fault() before calling hmm_vma_handle_migrate_prepare_rollback().
Several other paths similarly return before rolling back previously collected
entries in range->hmm_pfns:

  - hmm_vma_walk_hole() returns -EFAULT (when !walk->vma) or
    hmm_record_fault() when required_fault is non-zero before reaching
    hmm_pfns_fill().
  - hmm_vma_handle_absent_pmd() returns hmm_record_fault() or -EFAULT when
    required_fault is non-zero without calling
    hmm_vma_handle_migrate_prepare_rollback().
  - hmm_vma_handle_pte() jumps to the fault: label or returns -EFAULT without
    rolling back *hmm_pfn.
  - hmm_vma_walk_pmd() returns -EFAULT on pmd_bad(pmd) when
    hmm_range_need_fault() is non-zero, and returns 0 when
    thp_migration_supported() && pmd_is_migration_entry(pmd) is true with
    minfo non-zero, without rolling back collected PTE entries in
    hmm_pfns_start.

If hmm_record_fault() returns HMM_FAULT_PENDING while the current task still
holds folio_lock() on previously collected folios in that range, can
hmm_do_fault() -> handle_mm_fault() deadlock trying to lock the same folio, or
can the early returns leave stale collected folios locked and referenced in
range->hmm_pfns?

> @@ -412,6 +421,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;
>  

[Severity: Critical]
Could reaching the out: label in hmm_vma_handle_pte() prematurely roll back a
migration PTE that this walk already installed?

In hmm_vma_handle_pte(), when !pte_present(pte) is true, the required_fault
check runs before softleaf_is_migration(entry):

	if (!pte_present(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)) {
				...
				return -EBUSY;
			}
			return 0;
		}

During a migration walk without HMM_PFN_REQ_FAULT (such as migrate_vma_setup()
where range.default_flags is HMM_PFN_REQ_MIGRATE),
hmm_vma_handle_migrate_prepare() locks the folio, takes a reference, installs
a migration PTE, and marks *hmm_pfn with HMM_PFN_VALID | HMM_PFN_MIGRATE.

If the walk restarts over that range (for example, after dropping locks in
migrate_vma_split_folio() or on an -EBUSY retry), hmm_vma_handle_pte() sees
!pte_present(pte) for its own migration PTE. Because HMM_PFN_REQ_FAULT is not
set, hmm_pte_need_fault() returns 0 and jumps to out: with new_pfn_flags = 0.
At out:, hmm_vma_handle_migrate_prepare_rollback() unlocks the folio, calls
folio_put(), and clears HMM_PFN_VALID | HMM_PFN_MIGRATE from *hmm_pfn while
the migration PTE remains installed in the page table, so
remove_migration_ptes() will never be called for it.

Conversely, when HMM_PFN_REQ_FAULT is set (required_fault != 0), the
softleaf_is_migration(entry) branch returns 0 whenever
hmm_select_migrate(range) is non-zero without checking that
hmm_pfn_collected(*hmm_pfn) is true and that softleaf_to_pfn(entry) matches
the PFN in *hmm_pfn. If the migration PTE belongs to a concurrent migration
(at an uncollected slot or in a replacement PTE table), shouldn't
hmm_vma_handle_pte() roll back any mismatched *hmm_pfn and wait via
migration_entry_wait()?

[ ... ]

> @@ -1117,6 +1193,9 @@ static int hmm_vma_walk_pmd(pmd_t *pmdp,
>  		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);

[Severity: High]
Does this rollback miss entries collected in [start, addr) when the PTE lock
is dropped partway through a PMD?

In hmm_vma_walk_pmd(), the PTE loop advances addr and hmm_pfns from start
toward end while collecting folios into hmm_pfns_start. If
hmm_vma_handle_migrate_prepare() hits a large folio at addr > start,
migrate_vma_split_folio() drops the PTE lock and returns -EAGAIN, and
hmm_vma_walk_pmd() jumps to again: without resetting addr = start:

again:
	hmm_pfns = &range->hmm_pfns[(addr - range->start) >> PAGE_SHIFT];
	...
	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);
		}
	}
	if (!ptep) {
		addr = start;
		goto again;
	}

When ptep is NULL at addr > start, hmm_vma_handle_migrate_prepare_rollback()
is called with addr and hmm_pfns before addr is reset to start, so it only
scans the uncollected tail [addr, end) and leaves folios collected in
[start, addr) (at hmm_pfns_start) locked and referenced.

In addition, if the PTE table was cleared and replaced by another PTE table
while the lock was dropped in migrate_vma_split_folio() (or during a mid-PMD
fault where hmm_record_fault() sets hmm_vma_walk->last = addr),
pte_offset_map_lock() succeeds at addr > start on the new PTE table and resumes
from addr without re-walking or rolling back [start, addr). Should mid-PMD lock
drops restart from start (with hmm_pfns_start) so that [start, addr) is
re-validated?

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