Re: [PATCH 3/5] drm/amdgpu/userq: bound the eviction fence rearm retry loop

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] The retry loop in amdgpu_userq_ensure_ev_fence() fails permanently with -ENOMEM when ev_fence_seq wraps to -1.

Pre-existing issues:
- [High] Wait dependencies are permanently lost if an error path unwinds the IOCTL or if the IOCTL restarts.
- [High] Missing cancellation of queue->hang_detect_work in the amdgpu_userq_create() error path leads to Use-After-Free.
- [High] Leaking fence_drv references stored in fence_drv_array when a fence is destroyed on error paths or queue teardown.
- [High] Missing rcu_assign_pointer and premature reference drop when updating evf_mgr->ev_fence causes deadlock and uninitialized memory access.
--

--- Patch [3]: [PATCH 3/5] drm/amdgpu/userq: bound the eviction fence rearm retry loop ---
commit ad19a8a5be75c8d4ceb626d159cd7abccb2976c4
Author: Junrui Luo <moonafterrain@outlook.com>

drm/amdgpu/userq: bound the eviction fence rearm retry loop

This patch bounds a previously infinite loop in the userq eviction fence
rearm logic by verifying that the sequence number has advanced. It returns
-ENOMEM if the rearm worker fails to make progress.

> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> @@ -448,12 +448,16 @@ static void amdgpu_userq_cleanup(struct amdgpu_usermode_queue *queue)
>  int
>  amdgpu_userq_ensure_ev_fence(struct amdgpu_userq_mgr *uq_mgr,
>  			     struct amdgpu_eviction_fence_mgr *evf_mgr)
>  {
>  	struct dma_fence *ev_fence;
> +	int seq, prev_seq = -1;
>  
>  retry:
[ ... ]
>  	if (dma_fence_is_signaled(ev_fence)) {
>  		dma_fence_put(ev_fence);
> +		seq = atomic_read(&evf_mgr->ev_fence_seq);
>  		mutex_unlock(&uq_mgr->userq_mutex);
> +		/*
> +		 * The sequence number is only bumped by a successful rearm, so
> +		 * if the flush above ran the worker without moving it then the
> +		 * restore failed and looping again would never terminate.
> +		 */
> +		if (seq == prev_seq)
> +			return -ENOMEM;

[Severity: Medium]
Could this condition match incorrectly if the sequence number wraps around?

Since ev_fence_seq is a 32-bit atomic counter incremented on successful
rearms, it will eventually wrap to -1 (0xFFFFFFFF).

If prev_seq is initialized to -1, seq == prev_seq would evaluate to true
on the first loop iteration when the counter wraps.

Does this cause amdgpu_userq_ensure_ev_fence() to immediately return
-ENOMEM without attempting to schedule the resume worker, permanently
wedging the user queue manager?

> +		prev_seq = seq;
[ ... ]
> @@ -747,7 +762,9 @@ amdgpu_userq_create(struct drm_file *filp, union drm_amdgpu_userq *args)
>  	if (r)
>  		goto clean_mqd;
>  
> -	amdgpu_userq_ensure_ev_fence(&fpriv->userq_mgr, &fpriv->evf_mgr);
> +	r = amdgpu_userq_ensure_ev_fence(&fpriv->userq_mgr, &fpriv->evf_mgr);
> +	if (r)
> +		goto erase_doorbell;

[Severity: High]
This is a pre-existing issue, but can the goto erase_doorbell error path
cause a use-after-free of the queue structure?

Looking at amdgpu_userq_create(), the queue is inserted into
adev->userq_doorbell_xa earlier in the function, making it globally
visible.

If a hardware fault occurs and schedules queue->hang_detect_work, and then
this error path frees the queue without calling
cancel_delayed_work_sync(&queue->hang_detect_work), will the deferred work
access the freed queue later?

>  	/* don't map the queue if scheduling is halted */
[ ... ]
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
> @@ -523,7 +523,15 @@ int amdgpu_userq_signal_ioctl(struct drm_device *dev, void *data,
>  		goto put_queue;
>  
>  	/* We are here means UQ is active, make sure the eviction fence is valid */
> -	amdgpu_userq_ensure_ev_fence(&fpriv->userq_mgr, &fpriv->evf_mgr);
> +	r = amdgpu_userq_ensure_ev_fence(&fpriv->userq_mgr, &fpriv->evf_mgr);
> +	if (r) {
> +		/* The fence is not initialized yet, so unwind it by hand */
> +		amdgpu_userq_fence_put_fence_drv_array(fence);
> +		amdgpu_userq_fence_driver_put(fence->fence_drv);
> +		kvfree(fence->fence_drv_array);
> +		kfree(fence);
> +		goto put_queue;
> +	}

[Severity: High]
This is a pre-existing issue, but does unwinding the fence here permanently
lose synchronization dependencies?

In amdgpu_userq_fence_alloc(), xa_extract() destructively removes fence
drivers from queue->fence_drv_xa and moves them into fence_drv_array.

If we abort here, the fence and its array are freed, but the dependencies
are never restored to the xarray. If the IOCTL restarts (for example, from
-ERESTARTSYS later), won't it create a new fence missing all previous wait
dependencies?

[Severity: High]
This is also a pre-existing issue, but does amdgpu_userq_fence_free()
leak these extracted dependencies on normal release paths?

While the hand-unwind path above correctly calls
amdgpu_userq_fence_put_fence_drv_array(fence), the standard cleanup
routine misses this:

drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c:amdgpu_userq_fence_free() {
    ...
    kvfree(userq_fence->fence_drv_array);
    kfree(userq_fence);
    ...
}

Since amdgpu_userq_fence_free() frees the array memory without dropping the
refcounts of the fence_drv objects inside it, do they leak permanently?

>  
>  	/* Create the new fence */
>  	amdgpu_userq_fence_init(queue, fence, wptr);
[ ... ]

While reviewing the eviction fence code related to this patch, I noticed
another pre-existing issue:

[Severity: High]
This is a pre-existing issue, but does this direct assignment without
rcu_assign_pointer() risk a deadlock and uninitialized memory access?

drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c:amdgpu_evf_mgr_rearm() {
    ...
    dma_fence_put(evf_mgr->ev_fence);
    evf_mgr->ev_fence = &ev_fence->base;
    ...
}

evf_mgr->ev_fence is an RCU annotated pointer. Dropping the reference
to the old fence before the assignment could leave the pointer referencing a
zero-refcount object if preempted here.

A concurrent reader using dma_fence_get_rcu_safe() might enter an infinite
loop if kref_get_unless_zero() fails. Furthermore, without a release
barrier, concurrent readers might see the new fence before it is fully
initialized.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260811-amdgpu-fixes-v1-0-4954a417b8ff@outlook.com?part=3