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 <[email protected]> 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/[email protected]?part=3
