Re: [PATCH v17 03/12] mm/hmm: preparations for HMM to participate in migration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] mm/hmm: memset() in hmm_range_fault_locked() wipes collected pfns on retry
- [Low] mm/hmm: build failure in hmm_range_fault_locked() when !CONFIG_MIGRATION
--

commit 9e255c525419b859ce3c5e39d7de2c6153874013
Author: Mika Penttilä <mpenttil@redhat.com>

mm/hmm: preparations for HMM to participate in migration

This commit prepares hmm_range_fault() to participate in page migration by
capturing the VMA during page table walks, managing MMU_NOTIFY_MIGRATE
invalidations, and clearing range->hmm_pfns at the start of migrating
faults.

> diff --git a/mm/hmm.c b/mm/hmm.c
> index 2f1e98c6b6440..9220e6648636d 100644
> --- a/mm/hmm.c
> +++ b/mm/hmm.c

[ ... ]

> @@ -694,19 +774,45 @@ static int hmm_range_fault_locked(struct hmm_range *range, bool *locked)
>  {
>  	struct hmm_vma_walk hmm_vma_walk = {
>  		.range = range,
> -		.locked = locked,
> +		.mmlocked = locked,
>  		.last = range->start,
>  	};
> -	struct mm_struct *mm = range->notifier->mm;
> +	/*
> +	 *  Could be serving a device fault or come from migrate
> +	 *  entry point. For the former we have not resolved the vma
> +	 *  yet, and the latter we don't have a notifier (but have a vma).
> +	 *
> +	 */
> +	struct mm_struct *mm = hmm_range_fault_mm(range);
>  	int ret;
>  
> +	/* Migration is incompatible with mmap lock drop */
> +	if (locked && hmm_select_migrate(range))
> +		return -EINVAL;
> +
> +	/*
> +	 * Migration takes its requests only from default_flags, so the input
> +	 * pfns carry nothing. Clear them so that collected entries can be
> +	 * told apart reliably during the walk and on rollback.
> +	 */
> +	if (hmm_select_migrate(range)) {
> +		WARN_ON_ONCE(range->pfn_flags_mask);
> +		WARN_ON_ONCE(range->dev_private_owner !=
> +			     range->migrate->pgmap_owner);

[Severity: Low]
Will this break the build when CONFIG_HMM_MIRROR=y and CONFIG_MIGRATION=n?

Several Kconfig options (such as CONFIG_INFINIBAND_ON_DEMAND_PAGING and
CONFIG_DRM_AMDGPU_USERPTR) select CONFIG_HMM_MIRROR without depending on
CONFIG_MIGRATION. In include/linux/hmm.h, struct migrate_vma is only
forward-declared, and in include/linux/migrate.h, its definition is guarded
by #ifdef CONFIG_MIGRATION while a stub is provided for
hmm_select_migrate() when !CONFIG_MIGRATION.

Even though hmm_select_migrate(range) returns 0 when CONFIG_MIGRATION is
disabled, the compiler still type-checks range->migrate->pgmap_owner inside
hmm_range_fault_locked() in mm/hmm.c and fails due to dereferencing a
pointer to an incomplete type.

Should this check be moved into a helper in include/linux/migrate.h with a
matching !CONFIG_MIGRATION stub?

> +		memset(range->hmm_pfns, 0,
> +		       ((range->end - range->start) >> PAGE_SHIFT) *
> +		       sizeof(*range->hmm_pfns));
> +	}
> +
>  	mmap_assert_locked(mm);
>  
>  	do {
>  		/* If range is no longer valid force retry. */
> -		if (mmu_interval_check_retry(range->notifier,
> -					     range->notifier_seq))
> -			return -EBUSY;
> +		if (range->notifier && mmu_interval_check_retry(range->notifier,
> +								range->notifier_seq)) {
> +			ret =  -EBUSY;
> +			break;
> +		}

[Severity: High]
Can unconditionally calling memset() on range->hmm_pfns in
hmm_range_fault_locked() cause a self-deadlock on the folio lock or leak
locked folios when a migrating hmm_range_fault() call is retried after
-EBUSY?

When migration is enabled in this series with range->notifier set (such as
in dmirror_range_fault() in lib/test_hmm.c), walk_page_range() collects
present pages by locking each folio, elevating its reference count,
installing a migration PTE, and marking range->hmm_pfns[i] with
HMM_PFN_VALID | HMM_PFN_MIGRATE.

If a later page in the range needs a fault, walk_page_range() returns
HMM_FAULT_PENDING, hmm_vma_migrate_disarm() closes the MMU_NOTIFY_MIGRATE
invalidation, and hmm_do_fault() calls handle_mm_fault() while the
previously collected folios stay locked by current and mapped as migration
PTEs. If handle_mm_fault() or a concurrent operation triggers an MMU
notifier invalidation, mmu_interval_check_retry() on the next loop
iteration returns true and hmm_range_fault_locked() exits with -EBUSY (or
the caller retries after mmu_interval_read_retry() fails).

When the caller retries hmm_range_fault(range) with the same
range->hmm_pfns array, the unconditional memset(range->hmm_pfns, 0, ...) at
the top of hmm_range_fault_locked() wipes the HMM_PFN_VALID |
HMM_PFN_MIGRATE markers for the folios that are still locked and installed
as migration PTEs. On the retry walk, hmm_pfn_collected() will return false
for those entries, causing hmm_vma_handle_pte() or hmm_vma_walk_pmd() to
treat current's own migration entries as another task's migration entries
and wait in migration_entry_wait() on a folio already locked by current
(or leak the locked folios and migration PTEs if the caller aborts).

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