Re: [PATCH v10 8/8] drm/gpusvm: Use hmm_range_fault_unlocked_timeout() for range faults

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] TOCTOU race on volatile `jiffies` can result in a 0 timeout, causing an infinite loop.
- [High] The absolute outer deadline in `drm_gpusvm_get_pages()` is defeated by the relative timeout extension semantics of `hmm_range_fault_unlocked_timeout()`.

Pre-existing issues:
- [High] Failure to allocate `pfns` in `drm_gpusvm_range_evict()` leaks the `mm_struct` reference.
--

--- Patch [8]: [PATCH v10 8/8] drm/gpusvm: Use hmm_range_fault_unlocked_timeout() for range faults ---
commit f0577dfbb2f6c27cfd07d3be42e1db31600b9e4d
Author: Stanislav Kinsburskii <[email protected]>
Subject: drm/gpusvm: Use hmm_range_fault_unlocked_timeout() for range faults

This commit replaces open-coded HMM fault retry loops in the GPU SVM paths
with the new hmm_range_fault_unlocked_timeout() helper. It updates
drm_gpusvm_scan_mm(), drm_gpusvm_range_evict(), and drm_gpusvm_get_pages()
to delegate mmap_lock acquisition and retry logic to the helper.

> diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c
> index 958cb605aedd5..b946f920b7a00 100644
> --- a/drivers/gpu/drm/drm_gpusvm.c
> +++ b/drivers/gpu/drm/drm_gpusvm.c
[ ... ]
> @@ -1408,6 +1392,7 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm,
>  	void *zdd;
>  	unsigned long timeout =
>  		jiffies + msecs_to_jiffies(HMM_RANGE_DEFAULT_TIMEOUT);
> +	unsigned long remaining;
>  	unsigned long i, j;
>  	unsigned long npages = npages_in_range(pages_start, pages_end);
>  	unsigned long num_dma_mapped;
> @@ -1422,9 +1407,11 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm,
>  	struct dma_iova_state *state = &svm_pages->state;
>  
>  retry:
> -	if (time_after(jiffies, timeout))
> +	if (time_after_eq(jiffies, timeout))
>  		return -EBUSY;
>  
> +	remaining = timeout - jiffies;

[Severity: High]
Does this double read of jiffies introduce a race condition?

Because jiffies is volatile, it is fetched from memory for the
time_after_eq() check, and then fetched again for the subtraction. If
jiffies increments between these reads, remaining can evaluate to 0
(if jiffies equals timeout) or ULONG_MAX (if jiffies ticks past timeout).

When remaining is 0, hmm_range_fault_unlocked_timeout() disables the
deadline entirely. When it is ULONG_MAX, the timeout is effectively
infinite, which could hang the page-fault worker.

>  	hmm_range.notifier_seq = mmu_interval_read_begin(notifier);
>  	if (drm_gpusvm_pages_valid_unlocked(gpusvm, svm_pages))
>  		goto set_seqno;
> @@ -1439,21 +1426,7 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm,
>  	}
>  
>  	hmm_range.hmm_pfns = pfns;
> -	while (true) {
> -		mmap_read_lock(mm);
> -		err = hmm_range_fault(&hmm_range);
> -		mmap_read_unlock(mm);
> -
> -		if (err == -EBUSY) {
> -			if (time_after(jiffies, timeout))
> -				break;
> -
> -			hmm_range.notifier_seq =
> -				mmu_interval_read_begin(notifier);
> -			continue;
> -		}
> -		break;
> -	}
> +	err = hmm_range_fault_unlocked_timeout(&hmm_range, remaining);

[Severity: High]
Does passing remaining defeat the absolute deadline intended for this path?

The commit message notes that drm_gpusvm_get_pages() keeps an absolute outer
deadline to prevent endless invalidation retry loops from hanging GPU
page-fault workers.

However, hmm_range_fault_unlocked_timeout() treats the passed value as a
relative timeout and recalculates its internal deadline every time the lock
is dropped:

mm/hmm.c:hmm_range_fault_unlocked_timeout() {
    ...
    if (timeout && !locked)
        deadline = jiffies + timeout;
    ...
}

Since remaining is passed as the full timeout budget, the helper will extend
its deadline by the full amount of remaining into the future every time it
drops the lock, which can allow an infinite loop.

>  	mmput(mm);
>  	if (err)
>  		goto err_free;
[ ... ]
> @@ -1736,24 +1708,12 @@ int drm_gpusvm_range_evict(struct drm_gpusvm *gpusvm,
>  		return -ENOMEM;

[Severity: High]
This is a pre-existing issue, but does this error path leak the mm_struct
reference?

Earlier in drm_gpusvm_range_evict(), the code acquires an mm reference:

    if (!mmget_not_zero(mm))
        return -EFAULT;

If kvmalloc_array() fails, the function returns -ENOMEM directly, bypassing
the mmput(mm) call at the end of the function. This permanently leaks the mm
reference, preventing the target process's memory structures from being freed
upon exit.

>  
>  	hmm_range.hmm_pfns = pfns;
> -	while (!time_after(jiffies, timeout)) {
> -		hmm_range.notifier_seq = mmu_interval_read_begin(notifier);
> -		if (time_after(jiffies, timeout)) {
> -			err = -ETIME;
> -			break;
> -		}
> -
> -		mmap_read_lock(mm);
> -		err = hmm_range_fault(&hmm_range);
> -		mmap_read_unlock(mm);
> -		if (err != -EBUSY)
> -			break;
> -	}
> +	err = hmm_range_fault_unlocked_timeout(&hmm_range, timeout);
>  
>  	kvfree(pfns);
>  	mmput(mm);
>  
> -	return err;
> +	return err == -EBUSY ? -ETIME : err;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8