Re: [PATCH v16 03/12] mm/hmm: preparations 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: dead range check in hmm_vma_capture_migrate_range() allows partial VMA
- [High] mm/hmm: mmu_notifier_invalidate_range_start() held across hmm_do_fault()
- [Medium] mm/hmm: hmm_vma_walk_test() captures VMA for migration before checking vm_flags
--

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

mm/hmm: preparations for HMM to participate in migration

This commit prepares hmm_range_fault() to support page migration by saving
the resolved VMA and range during the page walk, rejecting lock-drop mode
when migrating, and invoking MMU notifier invalidations around the walk.

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

[ ... ]

> @@ -400,6 +409,58 @@ static int hmm_vma_handle_absent_pmd(struct mm_walk *walk, unsigned long start,
>  }
>  #endif  /* CONFIG_ARCH_HAS_PMD_SOFTLEAVES */
>  
> +static int hmm_vma_capture_migrate_range(unsigned long start,
> +					 unsigned long end,
> +					 struct mm_walk *walk)
> +{
> +	struct hmm_vma_walk *hmm_vma_walk = walk->private;
> +	struct hmm_range *range = hmm_vma_walk->range;
> +
> +	if (!hmm_select_migrate(range))
> +		return 0;
> +
> +	if (hmm_vma_walk->vma && (hmm_vma_walk->vma != walk->vma))
> +		return -ERANGE;
> +
> +	hmm_vma_walk->vma = walk->vma;
> +
> +	if (end - start > range->end - range->start)
> +		return -ERANGE;

[Severity: High]
Can this condition in mm/hmm.c:hmm_vma_capture_migrate_range() ever be true?

In hmm_range_fault_locked() -> walk_page_range() ->
walk_page_range_mm_unsafe() -> walk_page_test() -> hmm_vma_walk_test(), the
walker clamps the VMA subrange to the requested walk bounds:

    start >= hmm_vma_walk.last >= range->start
    end = min(range->end, vma->vm_end) <= range->end

Because end - start is always less than or equal to range->end - range->start,
this check never triggers.

If [range->start, range->end) starts with an unmapped hole (without
HMM_PFN_REQ_FAULT set on the hole entries), walk_page_range_mm_unsafe() calls
hmm_vma_walk_hole() for [range->start, vma->vm_start) and then invokes
hmm_vma_walk_test() with start == vma->vm_start > range->start.

hmm_vma_capture_migrate_range() accepts this partial VMA and records
hmm_vma_walk->start = vma->vm_start, which hmm_vma_post_range_fault() writes
to range->migrate->start via hmm_fill_migrate_vma(). However, the page walk
callbacks index range->hmm_pfns[] relative to range->start:

    &range->hmm_pfns[(start - range->start) >> PAGE_SHIFT]

When migrate_hmm_range_setup() later reads range->hmm_pfns[0] for
migrate->start, the PFN array is misaligned with the virtual address range.
Additionally, if the entire [range->start, range->end) falls in an unmapped
hole without fault requests, hmm_vma_walk_test() is never called and
range->migrate->vma remains uninitialized.

Should this check instead verify that the single VMA covers the entire
requested range (for example, vma->vm_start > range->start ||
vma->vm_end < range->end), and should hmm_vma_walk_hole() reject holes when
hmm_select_migrate(range) is enabled?

> +
> +	if (!hmm_vma_walk->mmu_armed) {
> +		hmm_vma_walk->start = start;
> +		hmm_vma_walk->end = end;
> +		mmu_notifier_range_init_owner(&hmm_vma_walk->mmu_range, MMU_NOTIFY_MIGRATE, 0,
> +					      walk->vma->vm_mm, start, end,
> +					      range->dev_private_owner);
> +		mmu_notifier_invalidate_range_start(&hmm_vma_walk->mmu_range);
> +		hmm_vma_walk->mmu_armed = true;
> +	}
> +
> +	return 0;
> +}
> +
> +static void hmm_vma_post_range_fault(struct hmm_vma_walk *hmm_vma_walk)
> +{
> +
> +	struct hmm_range *range = hmm_vma_walk->range;
> +
> +	if (hmm_select_migrate(range) &&
> +	    hmm_vma_walk->mmu_armed) {
> +		/*
> +		 *  The migrate_vma path has the following initialized,
> +		 *  so take care of fault path below.
> +		 */
> +		if (range->notifier) {
> +			hmm_fill_migrate_vma(range,
> +					     hmm_vma_walk->vma,
> +					     hmm_vma_walk->start,
> +					     hmm_vma_walk->end);
> +		}
> +		mmu_notifier_invalidate_range_end(&hmm_vma_walk->mmu_range);
> +	}
> +}
> +
>  static int hmm_vma_walk_pmd(pmd_t *pmdp,
>  			    unsigned long start,
>  			    unsigned long end,
> @@ -594,6 +655,11 @@ static int hmm_vma_walk_test(unsigned long start, unsigned long end,
>  	struct hmm_vma_walk *hmm_vma_walk = walk->private;
>  	struct hmm_range *range = hmm_vma_walk->range;
>  	struct vm_area_struct *vma = walk->vma;
> +	int r;
> +
> +	r = hmm_vma_capture_migrate_range(start, end, walk);
> +	if (r)
> +		return r;
>  
>  	if (!(vma->vm_flags & (VM_IO | VM_PFNMAP)) &&
>  	    vma->vm_flags & VM_READ)

[Severity: Medium]
In mm/hmm.c:hmm_vma_walk_test(), is it intentional that
hmm_vma_capture_migrate_range() runs before validating vma->vm_flags?

When walk->vma has VM_IO or VM_PFNMAP set, or lacks VM_READ, calling
hmm_vma_capture_migrate_range() first records hmm_vma_walk->vma = walk->vma
and invokes mmu_notifier_invalidate_range_start() on the unsupported VMA. If
hmm_range_need_fault() then returns 0, hmm_vma_walk_test() fills
range->hmm_pfns with HMM_PFN_ERROR and returns 1 (skipping the VMA), after
which hmm_vma_post_range_fault() populates range->migrate->vma with the
unsupported VMA.

Also, in the fault migration path where range->notifier is set and
migrate_vma_setup() is not used, nothing in hmm_vma_walk_test() or
hmm_vma_capture_migrate_range() rejects is_vm_hugetlb_page(vma),
(vma->vm_flags & VM_SPECIAL), or vma_is_dax(vma), which migrate_vma_setup()
rejects with -EINVAL. Should VMA eligibility for both HMM and migration be
verified before capturing the VMA and arming the MMU notifier?

[ ... ]

> @@ -694,19 +760,31 @@ 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;
> +
>  	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;
> +		}
>  		ret = walk_page_range(mm, hmm_vma_walk.last, range->end,
>  				      &hmm_walk_ops, &hmm_vma_walk);
>  		/*
> @@ -732,6 +810,8 @@ static int hmm_range_fault_locked(struct hmm_range *range, bool *locked)
>  		 * output, and all >= are still at their input values.
>  		 */
>  	} while (ret == -EBUSY);
> +
> +	hmm_vma_post_range_fault(&hmm_vma_walk);
>  	return ret;
>  }

[Severity: High]
What happens in mm/hmm.c:hmm_range_fault_locked() when migration is enabled
(as activated in the subsequent commit "mm: enable device page migration from
HMM pagewalk") and a page in the range needs to be faulted in via
hmm_do_fault()?

On the first iteration of walk_page_range(), hmm_vma_walk_test() ->
hmm_vma_capture_migrate_range() calls mmu_notifier_invalidate_range_start()
and sets hmm_vma_walk->mmu_armed = true before any PTEs are inspected, while
mmu_notifier_invalidate_range_end() is deferred until
hmm_vma_post_range_fault() after the entire do { ... } while (ret == -EBUSY)
loop completes.

When walk_page_range() returns HMM_FAULT_PENDING, hmm_do_fault() ->
handle_mm_fault() runs inside the active mmu_notifier_invalidate_range_start()
and mmu_notifier_invalidate_range_end() section:

1. Across sleeping page faults (such as swap/file I/O, reclaim, userfaultfd,
   or migrate_to_ram()), mm->notifier_subscriptions->active_invalidate_ranges
   remains elevated and invalidate_seq remains odd, preventing
   mn_itree_inv_end() from completing nested invalidations and blocking
   concurrent mmu_interval_read_begin() and mmu_interval_notifier_remove()
   callers on subscriptions->wq.

2. When range->notifier is non-NULL, any nested invalidation inside
   handle_mm_fault() (for example, wp_page_copy() on a write fault or
   remove_device_exclusive_entry()) or any notifier not filtered by
   range->dev_private_owner updates range->notifier->invalidate_seq via
   mmu_interval_set_seq(). When hmm_do_fault() returns -EBUSY to re-walk the
   newly faulted page, the next iteration of the loop hits
   mmu_interval_check_retry(range->notifier, range->notifier_seq) and aborts
   with -EBUSY.

3. Because mmu_notifier_invalidate_range_start() is only called once before
   faulting when !hmm_vma_walk->mmu_armed, any PTE faulted into a previously
   empty entry by hmm_do_fault() is subsequently unmapped and replaced with a
   migration entry on the retry walk without a post-fault
   mmu_notifier_invalidate_range_start() notification.

Should the mmu_notifier_invalidate_range_start() and
mmu_notifier_invalidate_range_end() pair be scoped around the migration
unmapping step rather than held across the hmm_do_fault() loop?

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