Re: [PATCH v14 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: migration range start address truncated on page walk retry
- [High] mm: hmm: unbalanced MMU notifier invalidation when dev_private_owner is NULL
- [High] mm: hmm: NULL pointer dereference in hmm_range_fault_unlocked_timeout
--

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

    mm/hmm: preparations for HMM to participate in migration
    
    For migration to happen after hmm_range_fault(), the vma has to
    retrieved. Luckily, pagewalk already resolves that for us, so
    just have to save it for further use. For migration, mmap_lock
    has to be hold for the whole operation, so vma stays stable.

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

[ ... ]

> @@ -400,6 +407,57 @@ 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;
> +	hmm_vma_walk->start = start;

[Severity: High]
Is the tracked start address unconditionally overwritten on every walk restart?

If the walk requires a retry, returning -EBUSY, the outer loop restarts
walk_page_range() from the intermediate faulting address. On retry, could this
clobber the tracking start with the intermediate address, leading to a
truncated start address?

> +	hmm_vma_walk->end = end;
> +
> +	if (end - start > range->end - range->start)
> +		return -ERANGE;
> +
> +	if (!hmm_vma_walk->mmu_range.owner) {

[Severity: High]
Could using the owner field as an initialization flag be problematic here?

If range->dev_private_owner is legitimately NULL, mmu_range.owner becomes
NULL. If a page fault retry occurs, the check passes again, causing a
duplicate mmu_notifier_invalidate_range_start(). Can this cause unbalanced
MMU notifier invalidation?

> +		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);
> +	}
> +
> +	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_range.owner) {

[Severity: High]
Are we missing a call to mmu_notifier_invalidate_range_end() if the owner is
NULL?

Assuming a NULL owner means the invalidation was never started could
permanently leak the invalidation if dev_private_owner is legitimately NULL.

> +		/*
> +		 *  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,

[Severity: High]
Will this propagate the truncated start address back to the caller's migration
struct?

If the walk was retried and the start address was clobbered in
hmm_vma_capture_migrate_range(), it could cause the migration setup to
miscalculate the page count and read from the wrong array offset.

> +					     hmm_vma_walk->end);
> +		}
> +		mmu_notifier_invalidate_range_end(&hmm_vma_walk->mmu_range);
> +	}
> +}

[ ... ]

> @@ -694,19 +757,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);

[Severity: High]
How does this API change affect hmm_range_fault_unlocked_timeout()?

The patch modifies the HMM API to allow range->notifier to be NULL for
migration entry points, handling it gracefully here with
hmm_range_fault_mm(). However, looking at hmm_range_fault_unlocked_timeout()
in mm/hmm.c:

int hmm_range_fault_unlocked_timeout(struct hmm_range *range,
                                     unsigned long timeout)
{
        struct mm_struct *mm = range->notifier->mm;

It unconditionally dereferences range->notifier->mm at its very first line. Can
this trigger a NULL pointer dereference if driver code passes a migration range
with a NULL notifier to the unlocked variant?

>  	int ret;
>  
> +	/* Migration is incompatible with mmap lock drop */
> +	if (locked && hmm_select_migrate(range))
> +		return -EINVAL;
> +

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