Re: [PATCH v16 04/12] mm/hmm: do the plumbing for HMM to participate in migration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] mm/hmm: broken migration entries in hmm_vma_handle_pte() and hmm_vma_walk_pmd()
- [High] mm/hmm: broken state and rollback in hmm_vma_walk_pmd() on retry after -EAGAIN
- [Medium] mm/hmm: hmm_vma_walk_pmd() aborts PMD walk and clobbers PFNs on split failure
--

commit 25901a209351c6985ba22f8dfc40a2db03ec4bdf
Author: Mika Penttilä <mpenttil@redhat.com>

mm/hmm: do the plumbing for HMM to participate in migration

This commit adds page table locking state and migration preparation plumbing
to hmm_range_fault() and its pagewalk callbacks in mm/hmm.c so that PTE and
PMD locks are held across entry handling and migration collection and dropped
before faulting.

> diff --git a/mm/hmm.c b/mm/hmm.c
> index 4e9542aedc253..119e70dbca56f 100644
> --- a/mm/hmm.c
> +++ b/mm/hmm.c

[ ... ]

> @@ -305,14 +360,25 @@ static int hmm_vma_handle_pte(struct mm_walk *walk, unsigned long addr,
>  			goto fault;
>  
>  		if (softleaf_is_migration(entry)) {
> -			pte_unmap(ptep);
> -			hmm_vma_walk->last = addr;
> -			migration_entry_wait(walk->mm, pmdp, addr);
> -			return -EBUSY;
> +			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;
>  		}

[Severity: High]
In include/linux/hmm.h, the input request flags share the same bit positions
as the output flags (HMM_PFN_REQ_FAULT == HMM_PFN_VALID,
HMM_PFN_REQ_WRITE == HMM_PFN_WRITE, HMM_PFN_REQ_MIGRATE == HMM_PFN_MIGRATE),
and callers using range->default_flags with range->pfn_flags_mask == 0 are
not required to initialize range->hmm_pfns[] on input.

In hmm_vma_handle_pte(), the !pte_present(pte) path evaluates
required_fault = hmm_pte_need_fault(hmm_vma_walk, pfn_req_flags, 0) first:

	required_fault =
		hmm_pte_need_fault(hmm_vma_walk, pfn_req_flags, 0);
	if (!required_fault)
		goto out;

Because of that check, if (softleaf_is_migration(entry)) is only reached when
required_fault != 0 (HMM_PFN_REQ_FAULT was requested). When
hmm_select_migrate(range) is non-zero, returning 0 directly here skips both
migration_entry_wait() (which would require unlocking hmm_vma_walk->ptl first)
and the out: label that masks *hmm_pfn to HMM_PFN_INOUT_FLAGS:

out:
	*hmm_pfn = (*hmm_pfn & HMM_PFN_INOUT_FLAGS) | new_pfn_flags;
	return 0;

Won't returning 0 directly on a concurrent migration entry leave stale input
flags or uninitialized bits (such as HMM_PFN_REQ_FAULT / HMM_PFN_VALID and
HMM_PFN_REQ_MIGRATE / HMM_PFN_MIGRATE) in *hmm_pfn, pass a migration PTE into
hmm_vma_handle_migrate_prepare(), and allow hmm_range_fault() to succeed
without waiting for the migration entry to resolve despite HMM_PFN_REQ_FAULT?

Conversely, when HMM_PFN_REQ_FAULT is not set (such as the default in
migrate_vma_setup()), if (!required_fault) goto out runs before checking
softleaf_is_migration(entry). If a retry re-walks a migration PTE that was
already installed by this walk, won't jumping to out: clear *hmm_pfn (and
trigger rollback in the later patch "mm/hmm: implement rollback for device
page migration in HMM pagewalk") while leaving the migration PTE in the page
table?

[ ... ]

> @@ -468,46 +593,116 @@ static int hmm_vma_walk_pmd(pmd_t *pmdp,
>  {
>  	struct hmm_vma_walk *hmm_vma_walk = walk->private;
>  	struct hmm_range *range = hmm_vma_walk->range;
> -	unsigned long *hmm_pfns =
> -		&range->hmm_pfns[(start - range->start) >> PAGE_SHIFT];
>  	unsigned long npages = (end - start) >> PAGE_SHIFT;
> +	struct mm_struct *mm = walk->vma->vm_mm;
> +	unsigned long *hmm_pfns, *hmm_pfns_start;
> +	enum migrate_vma_info minfo;
>  	unsigned long addr = start;
> +	bool unmapped = false;
> +	unsigned long i;
>  	pte_t *ptep;
>  	pmd_t pmd;
> +	int r = 0;
>  
> +	minfo = hmm_select_migrate(range);
> +	hmm_pfns_start = &range->hmm_pfns[(start - range->start) >> PAGE_SHIFT];
>  again:
> -	pmd = pmdp_get_lockless(pmdp);
> -	if (pmd_none(pmd))
> -		return hmm_vma_walk_hole(start, end, -1, walk);
> +	hmm_pfns = &range->hmm_pfns[(addr - range->start) >> PAGE_SHIFT];
> +	hmm_vma_walk->ptelocked = false;
> +	hmm_vma_walk->pmdlocked = false;
> +
> +	if (minfo) {
> +		hmm_vma_walk->ptl = pmd_lock(mm, pmdp);
> +		hmm_vma_walk->pmdlocked = true;
> +		pmd = pmdp_get(pmdp);
> +	} else
> +		pmd = pmdp_get_lockless(pmdp);
> +
> +	if (pmd_none(pmd)) {
> +		r = hmm_vma_walk_hole(start, end, -1, walk);
> +
> +		if (hmm_vma_walk->pmdlocked) {
> +			spin_unlock(hmm_vma_walk->ptl);
> +			hmm_vma_walk->pmdlocked = false;
> +		}
> +		return r;
> +	}
>  
>  	if (thp_migration_supported() && pmd_is_migration_entry(pmd)) {
> -		if (hmm_range_need_fault(hmm_vma_walk, hmm_pfns, npages, 0)) {
> -			hmm_vma_walk->last = addr;
> -			pmd_migration_entry_wait(walk->mm, pmdp);
> -			return -EBUSY;
> +		if (!minfo) {
> +			if (hmm_range_need_fault(hmm_vma_walk, hmm_pfns_start, npages, 0)) {
> +				hmm_vma_walk->last = addr;
> +				pmd_migration_entry_wait(walk->mm, pmdp);
> +				return -EBUSY;
> +			}
> +			for (i = 0; start < end; start += PAGE_SIZE, i++)
> +				hmm_pfns_start[i] &= HMM_PFN_INOUT_FLAGS;
> +		}
> +
> +		if (hmm_vma_walk->pmdlocked) {
> +			spin_unlock(hmm_vma_walk->ptl);
> +			hmm_vma_walk->pmdlocked = false;
>  		}
> -		return hmm_pfns_fill(start, end, range, 0);
> +
> +		return 0;
>  	}

[Severity: High]
Similarly, in hmm_vma_walk_pmd(), when thp_migration_supported() &&
pmd_is_migration_entry(pmd) is true and minfo != 0, both the
hmm_range_need_fault() check and the loop masking hmm_pfns_start[] to
HMM_PFN_INOUT_FLAGS are skipped by if (!minfo).

When a concurrent PMD migration entry is encountered during a migration walk,
doesn't skipping that block leave hmm_pfns_start[0..npages - 1] unmasked and
uninitialized, and also fail to unlock hmm_vma_walk->ptl and wait in
pmd_migration_entry_wait() when a fault was requested? Neither this path nor
the PTE migration entry path in hmm_vma_handle_pte() appears to be updated in
the subsequent patches in the series.

> -	if (!pmd_present(pmd))
> -		return hmm_vma_handle_absent_pmd(walk, start, end, hmm_pfns,
> -						 pmd);
> +	if (pmd_trans_huge(pmd) || !pmd_present(pmd)) {
> +		if (!pmd_present(pmd)) {
> +			r = hmm_vma_handle_absent_pmd(walk, start, end, hmm_pfns_start,
> +						      pmd);
> +			// If not migrating we are done
> +			if (r || !minfo) {
> +				if (hmm_vma_walk->pmdlocked) {
> +					spin_unlock(hmm_vma_walk->ptl);
> +					hmm_vma_walk->pmdlocked = false;
> +				}
> +				return r;
> +			}
> +		}
>  
> -	if (pmd_trans_huge(pmd)) {
> -		/*
> -		 * No need to take pmd_lock here, even if some other thread
> -		 * is splitting the huge pmd we will get that event through
> -		 * mmu_notifier callback.
> -		 *
> -		 * So just read pmd value and check again it's a transparent
> -		 * huge or device mapping one and compute corresponding pfn
> -		 * values.
> -		 */
> -		pmd = pmdp_get_lockless(pmdp);
> -		if (!pmd_trans_huge(pmd))
> -			goto again;
> +		if (pmd_trans_huge(pmd)) {
> +			/*
> +			 * No need to take pmd_lock here if not migrating,
> +			 * even if some other thread is splitting the huge
> +			 * pmd we will get that event through mmu_notifier callback.
> +			 *
> +			 * So just read pmd value and check again it's a transparent
> +			 * huge or device mapping one and compute corresponding pfn
> +			 * values.
> +			 */
> +
> +			if (!minfo) {
> +				pmd = pmdp_get_lockless(pmdp);
> +				if (!pmd_trans_huge(pmd))
> +					goto again;
> +			}
>  
> -		return hmm_vma_handle_pmd(walk, addr, end, hmm_pfns, pmd);
> +			r = hmm_vma_handle_pmd(walk, start, end, hmm_pfns_start, pmd);
> +
> +			// If not migrating we are done
> +			if (r || !minfo) {
> +				if (hmm_vma_walk->pmdlocked) {
> +					spin_unlock(hmm_vma_walk->ptl);
> +					hmm_vma_walk->pmdlocked = false;
> +				}
> +				return r;
> +			}
> +		}
> +
> +		r = hmm_vma_handle_migrate_prepare_pmd(walk, pmdp, start, end, hmm_pfns_start);
> +
> +		if (hmm_vma_walk->pmdlocked) {
> +			spin_unlock(hmm_vma_walk->ptl);
> +			hmm_vma_walk->pmdlocked = false;
> +		}
> +
> +		return r;
> +	}
> +
> +	if (hmm_vma_walk->pmdlocked) {
> +		spin_unlock(hmm_vma_walk->ptl);
> +		hmm_vma_walk->pmdlocked = false;
>  	}
>  
>  	/*
> @@ -517,24 +712,64 @@ static int hmm_vma_walk_pmd(pmd_t *pmdp,
>  	 * recover.
>  	 */
>  	if (pmd_bad(pmd)) {
> -		if (hmm_range_need_fault(hmm_vma_walk, hmm_pfns, npages, 0))
> +		if (hmm_range_need_fault(hmm_vma_walk, hmm_pfns_start, npages, 0))
>  			return -EFAULT;
> -		return hmm_pfns_fill(start, end, range, HMM_PFN_ERROR);
> +		return hmm_pfns_fill(start, end, hmm_vma_walk, HMM_PFN_ERROR);
>  	}
>  
> -	ptep = pte_offset_map(pmdp, addr);
> -	if (!ptep)
> +	if (minfo) {
> +		ptep = pte_offset_map_lock(mm, pmdp, addr, &hmm_vma_walk->ptl);
> +		if (ptep)
> +			hmm_vma_walk->ptelocked = true;
> +	} else {
> +		ptep = pte_offset_map(pmdp, addr);
> +	}
> +	if (!ptep) {
> +		addr = start;
>  		goto again;
> +	}
> +
>  	for (; addr < end; addr += PAGE_SIZE, ptep++, hmm_pfns++) {
> -		int r;
>  
>  		r = hmm_vma_handle_pte(walk, addr, end, pmdp, ptep, hmm_pfns);
>  		if (r) {
> -			/* hmm_vma_handle_pte() did pte_unmap() */
> +			/* hmm_vma_handle_pte() did pte_unmap() / pte_unmap_unlock */
> +			if (unmapped)
> +				flush_tlb_range(walk->vma, start, addr);
>  			return r;
>  		}
> +
> +		r = hmm_vma_handle_migrate_prepare(walk, pmdp, ptep, addr, hmm_pfns, &unmapped);
> +		if (r == -EAGAIN) {
> +			HMM_ASSERT_UNLOCKED(hmm_vma_walk);
> +			if (unmapped) {
> +				flush_tlb_range(walk->vma, start, addr);
> +				unmapped = false;
> +			}
> +			goto again;
> +		}

[Severity: High]
When hmm_vma_handle_migrate_prepare() (populated in the later patch "mm/hmm:
migrate collection in HMM pagewalk - pte level") encounters a PTE-mapped large
folio at addr > start after order-0 pages in [start, addr) have already been
collected, migrate_vma_split_folio() drops the PTE lock and returns -EAGAIN,
jumping to again: with addr > start.

Even with the later patch "mm/hmm: implement rollback for device page
migration in HMM pagewalk", can jumping back to again: with addr > start leave
stale collected state in [start, addr) or leak folio locks and references?

1. If pte_offset_map_lock() returns null (!ptep) at addr > start because the
   PTE table disappeared while the PTE lock was dropped, the rollback patch
   calls hmm_vma_handle_migrate_prepare_rollback(walk->private, addr, end,
   hmm_pfns) on the unwalked tail [addr, end) instead of rolling back the
   already-collected prefix [start, addr) at hmm_pfns_start before resetting
   addr = start.

2. If the PTE page table was removed or replaced while the PTE lock was
   dropped in migrate_vma_split_folio() (or after !ptep jumps to again:), the
   PMD-level paths at again: do not roll back [start, addr) when
   pmd_is_migration_entry(pmd) is true (which returns 0 with [start, addr)
   still marked HMM_PFN_VALID | HMM_PFN_MIGRATE and addr still marked
   HMM_PFN_VALID from hmm_vma_handle_pte()), or when hmm_vma_walk_hole(),
   hmm_vma_handle_absent_pmd(), hmm_vma_handle_pmd(), or pmd_bad() returns
   HMM_FAULT_PENDING or -EFAULT. Likewise, if the PTE table was replaced by a
   new PTE table while the lock was dropped, again: resumes at addr > start
   without rolling back or re-walking [start, addr).

> +		if (r) {
> +			/* A non -EAGAIN error here means migrate_vma_split_folio()
> +			 * already dropped the PTE lock and cleared ptelocked.
> +			 */
> +			HMM_ASSERT_UNLOCKED(hmm_vma_walk);
> +			if (unmapped)
> +				flush_tlb_range(walk->vma, start, addr);
> +			hmm_pfns_fill(addr, end, hmm_vma_walk, HMM_PFN_ERROR);
> +			return 0;
> +		}

[Severity: Medium]
When hmm_vma_handle_migrate_prepare() returns a non-EAGAIN error in the later
patch "mm/hmm: migrate collection in HMM pagewalk - pte level" because
migrate_vma_split_folio() failed to split a PTE-mapped large folio, why does
hmm_vma_walk_pmd() fill [addr, end) with HMM_PFN_ERROR and return 0?

First, hmm_vma_handle_pte() has already recorded the valid PFN and
HMM_PFN_VALID in *hmm_pfns for addr. Overwriting addr with HMM_PFN_ERROR means
callers such as nouveau_dmem_migrate_fault() (in "drm/nouveau/svm: add support
for migrate on fault") that fall back to mapping present CPU pages in place
when migration does not happen cannot use the PFN, unlike folio_trylock() or
folio_try_share_anon_rmap_pte() failures which preserve the valid PFN without
HMM_PFN_MIGRATE.

Second, returning 0 immediately aborts the loop for the rest of the PMD range
[addr + PAGE_SIZE, end), skipping both faulting (hmm_vma_handle_pte()) and
migration collection (hmm_vma_handle_migrate_prepare()) for all remaining PTEs
in that PMD and returning 0 from hmm_range_fault() with HMM_PFN_ERROR entries
even when HMM_PFN_REQ_FAULT was requested.

>  	}
> -	pte_unmap(ptep - 1);
> +
> +	if (unmapped)
> +		flush_tlb_range(walk->vma, start, addr);
> +
> +	if (hmm_vma_walk->ptelocked) {
> +		pte_unmap_unlock(ptep - 1, hmm_vma_walk->ptl);
> +		hmm_vma_walk->ptelocked = false;
> +	} else {
> +		pte_unmap(ptep - 1);
> +	}
> +
>  	return 0;
>  }

[ ... ]

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