Re: [PATCH v16 06/12] mm/hmm: migrate collection in HMM pagewalk - pmd level

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] mm/hmm: stale hmm_pfns left across PMD split re-walk in hmm_vma_walk_pmd()
- [Medium] mm/hmm: hmm_vma_handle_migrate_prepare_pmd() clobbers valid PFNs on skip
--

commit 19d71de5ba9a8df49f896d9047f6e5016503f7dc
Author: Mika Penttilä <mpenttil@redhat.com>

mm/hmm: migrate collection in HMM pagewalk - pmd level

This commit implements hmm_vma_handle_migrate_prepare_pmd() and integrates it
into hmm_vma_walk_pmd() to prepare PMD-level THP and device-private entries
for migration or split them for PTE re-walking.

> diff --git a/mm/hmm.c b/mm/hmm.c
> index 0ce947d0e5727..5891efee3e182 100644
> --- a/mm/hmm.c
> +++ b/mm/hmm.c

[ ... ]

> @@ -548,14 +536,145 @@ static int migrate_vma_split_folio(struct folio *folio,
>  	return 0;
>  }
>  
> +/**
> + * migrate_vma_split_folio() - drop the pte lock and split a THP folio
> + * @folio: the folio to split
> + * @fault_page: struct page associated with the fault if any
> + * @hmm_vma_walk: walk in progress
> + * @ptep: pte_t * for unmap and unlock ptl
> + *
> + * Returns 0 on success
> + */
> +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);
> +
> +	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);
> +}
> +
>  static int hmm_vma_handle_migrate_prepare_pmd(const struct mm_walk *walk,
>  					      pmd_t *pmdp,
>  					      unsigned long start,
>  					      unsigned long end,
>  					      unsigned long *hmm_pfn)
>  {

[ ... ]

> +	folio_get(folio);
> +
> +	if (folio != fault_folio && unlikely(!folio_trylock(folio))) {
> +		folio_put(folio);
> +		hmm_pfns_fill(start, end, hmm_vma_walk, HMM_PFN_ERROR);
> +		return 0;
> +	}
> +
> +	if (thp_migration_supported() &&
> +	    (migrate->flags & MIGRATE_VMA_SELECT_COMPOUND) &&
> +	    (IS_ALIGNED(start, HPAGE_PMD_SIZE) &&
> +	     IS_ALIGNED(end, HPAGE_PMD_SIZE))) {
> +		struct page_vma_mapped_walk pvmw = {
> +			.ptl = hmm_vma_walk->ptl,
> +			.address = start,
> +			.pmd = pmdp,
> +			.vma = walk->vma,
> +		};
> +
> +		hmm_pfn[0] |= HMM_PFN_MIGRATE | HMM_PFN_COMPOUND;
> +
> +		r = set_pmd_migration_entry(&pvmw, folio_page(folio, 0));
> +		if (r) {
> +			hmm_pfn[0] &= ~(HMM_PFN_MIGRATE | HMM_PFN_COMPOUND);
> +			goto split;	/* fall back to splitting the pmd */
> +		}
> +		for (i = 1, start += PAGE_SIZE; start < end; start += PAGE_SIZE, i++)
> +			hmm_pfn[i] &= HMM_PFN_INOUT_FLAGS;
> +
> +	} else {
> +		goto split;		/* fall back to splitting the pmd */
> +	}
> +
> +out:
> +	return r;
> +
> +split:
> +	spin_unlock(hmm_vma_walk->ptl);
> +	hmm_vma_walk->pmdlocked = false;
> +
> +	/*
> +	 * folio_get() above took an extra reference. For the fault folio the
> +	 * caller still holds a reference and the lock, which is the
> +	 * precondition of __migrate_vma_split_folio(), so drop the extra one.
> +	 */
> +	if (folio == fault_folio)
> +		folio_put(folio);
> +
> +	r = __migrate_vma_split_folio(folio, migrate->fault_page);
> +	if (r)
> +		return -EFAULT;
> +
> +	hmm_vma_walk->last = start;
> +	return -EBUSY;
>  }

[Severity: High]
Can returning -EBUSY from the split path in
hmm_vma_handle_migrate_prepare_pmd() leave stale pre-split PMD entries in
range->hmm_pfns[] across the re-walk?

When migration is enabled (in the later commit "mm: enable device page
migration from HMM pagewalk"), hmm_vma_walk_pmd() first calls
hmm_vma_handle_pmd() or hmm_vma_handle_absent_pmd() before calling
hmm_vma_handle_migrate_prepare_pmd():

mm/hmm.c:hmm_vma_walk_pmd()
  hmm_vma_handle_pmd() / hmm_vma_handle_absent_pmd()
    for (i = 0; addr < end; addr += PAGE_SIZE, i++, pfn++) {
        hmm_pfns[i] &= HMM_PFN_INOUT_FLAGS;
        hmm_pfns[i] |= pfn | cpu_flags;
    }
  hmm_vma_handle_migrate_prepare_pmd()

This populates hmm_pfns_start[0..npages-1] with the pre-split PMD PFNs,
HMM_PFN_VALID, HMM_PFN_WRITE, and hmm_pfn_flags_order(PMD_SHIFT - PAGE_SHIFT).

When hmm_vma_handle_migrate_prepare_pmd() then takes the split path (for
example, when MIGRATE_VMA_SELECT_COMPOUND is not set, [start, end) is smaller
than HPAGE_PMD_SIZE, or set_pmd_migration_entry() fails), it drops
hmm_vma_walk->ptl, splits the folio via __migrate_vma_split_folio() (which
also unlocks the folio when folio != fault_folio), sets
hmm_vma_walk->last = start, and returns -EBUSY without clearing
hmm_pfns_start[0..npages-1].

This breaks the hmm_range_fault_locked() expectation that range->hmm_pfns[]
entries at or after hmm_vma_walk.last have not been overwritten with output
flags when -EBUSY is returned.

On the subsequent PTE re-walk of [start, end), if any split page is
concurrently turned into a migration entry while the page table and folio
locks were dropped (with HMM_PFN_REQ_FAULT set), hmm_vma_handle_pte() returns
0 without updating *hmm_pfn:

mm/hmm.c:hmm_vma_handle_pte()
  if (softleaf_is_migration(entry)) {
      if (!hmm_select_migrate(range)) {
          ...
          return -EBUSY;
      }
      return 0;
  }

and hmm_vma_handle_migrate_prepare() sees !pte_present(pte) and
!softleaf_is_device_private(entry) and also returns 0 without updating
*hmm_pfn (similarly, if the PMD became a PMD migration entry,
hmm_vma_walk_pmd() returns 0 without clearing hmm_pfns_start). The later
commit "mm/hmm: implement rollback for device page migration in HMM pagewalk"
only rolls back entries that have both HMM_PFN_VALID and HMM_PFN_MIGRATE set,
so these pre-split entries (which lack HMM_PFN_MIGRATE) remain in
range->hmm_pfns[], allowing callers that map non-migrated HMM PFNs in place
(such as nouveau_dmem_migrate_fault()) to map a stale 2MB host range into the
device MMU.

Furthermore, if a caller sets range->pfn_flags_mask, overwriting
hmm_pfns_start[0..npages-1] with HMM_PFN_VALID (which shares its bit position
with HMM_PFN_REQ_FAULT) and HMM_PFN_WRITE (which shares its bit position with
HMM_PFN_REQ_WRITE) before the split clobbers the per-page request flags for
the PTE re-walk.

> @@ -958,6 +1077,17 @@ static int hmm_vma_walk_pmd(pmd_t *pmdp,
>  			hmm_vma_walk->pmdlocked = false;
>  		}
>  
> +		/*
> +		 * hmm_vma_handle_migrate_prepare_pmd() splits the huge pmd in
> +		 * place when needed and returns -EBUSY to re-walk the range as
> +		 * PTEs; any other error means the split failed.
> +		 */
> +		if (r == -EBUSY)
> +			return -EBUSY;
> +		if (r) {
> +			/* Split not successful, skip */
> +			return hmm_pfns_fill(start, end, hmm_vma_walk, HMM_PFN_ERROR);
> +		}
>  		return r;
>  	}

[Severity: Medium]
Should the folio_trylock() failure path in
hmm_vma_handle_migrate_prepare_pmd() and the split failure path here in
hmm_vma_walk_pmd() preserve the valid PFNs in range->hmm_pfns[] instead of
overwriting them with HMM_PFN_ERROR?

Before hmm_vma_handle_migrate_prepare_pmd() is called, hmm_vma_handle_pmd()
(or hmm_vma_handle_absent_pmd()) has already populated
hmm_pfns_start[0..npages-1] with the valid PFNs (HMM_PFN_VALID without
HMM_PFN_MIGRATE).

However, in two best-effort migration skip cases, those valid PFNs are
clobbered with HMM_PFN_ERROR:

1. In hmm_vma_handle_migrate_prepare_pmd(), when folio_trylock(folio) fails
   while holding hmm_vma_walk->ptl, it calls
   hmm_pfns_fill(start, end, hmm_vma_walk, HMM_PFN_ERROR) even though the PMD
   lock was never dropped and the mapping is still valid.

2. When compound migration is not selected or [start, end) is smaller than
   HPAGE_PMD_SIZE (such as single-page faults in nouveau_dmem_migrate_fault()
   in the later commit "drm/nouveau/svm: add support for migrate on fault"),
   hmm_vma_handle_migrate_prepare_pmd() jumps to split and calls
   __migrate_vma_split_folio(). If split_folio() fails (for example, when the
   THP is pinned or has extra references), it returns -EFAULT and
   hmm_vma_walk_pmd() overwrites the range with HMM_PFN_ERROR.

By contrast, the other non-migrating exits in
hmm_vma_handle_migrate_prepare_pmd() (goto out) and folio_trylock() failure
at the PTE level in hmm_vma_handle_migrate_prepare():

mm/hmm.c:hmm_vma_handle_migrate_prepare()
	if (fault_folio == folio || folio_trylock(folio)) {
		...
	} else {
		folio_put(folio);
	}
out:
	return 0;

leave range->hmm_pfns[] with the valid PFNs and without HMM_PFN_MIGRATE.
Because migrate_hmm_range_setup() only collects entries with HMM_PFN_MIGRATE
set, leaving the valid PFNs intact still skips migration while allowing
callers such as nouveau_dmem_migrate_fault() to map the present THP in place
instead of treating it as an error and falling back to a redundant page table
walk and GPU PFNMAP ioctl.

Could both skip paths return 0 without calling hmm_pfns_fill(...,
HMM_PFN_ERROR)?

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