Hi Christian, Thanks for pointing this out.
On Fri, 5 Jun 2026 at 17:32, Christian König <[email protected]> wrote: > > On 6/4/26 08:39, Guangshuo Li wrote: > > amdgpu_userq_input_va_validate() is not a side-effect-free validator. > > When it succeeds, it allocates a VA cursor, links it on > > queue->userq_va_list and marks the corresponding bo_va as userq mapped. > > That was already removed, you are looking at outdated code. > > Regards, > Christian. > > > > > The user queue create path validates queue_va, rptr_va and wptr_va with a > > short-circuit OR expression. If an earlier validation succeeds and a > > later validation fails, the error path frees the queue directly. The VA > > cursor added by the successful validation is leaked and > > bo_va->userq_va_mapped remains set even though no user queue was created. > > > > The same stale VA tracking state can also survive later create failures > > after all VA validations have succeeded, because those paths also free > > the queue without unwinding queue->userq_va_list. > > > > Route the create error paths through common unwind labels and call > > amdgpu_userq_buffer_vas_list_cleanup() before freeing the queue. This > > releases any VA cursors added during validation and clears the stale > > userq VA mapping state. > > > > Fixes: 9e46b8bb0539 ("drm/amdgpu: validate userq buffer virtual address and > > size") > > Signed-off-by: Guangshuo Li <[email protected]> > > --- > > drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 32 +++++++++++------------ > > 1 file changed, 15 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c > > index 0a1b93259887..dba0f786ae4a 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c > > @@ -826,17 +826,15 @@ amdgpu_userq_create(struct drm_file *filp, union > > drm_amdgpu_userq *args) > > amdgpu_userq_input_va_validate(adev, queue, args->in.rptr_va, > > AMDGPU_GPU_PAGE_SIZE) || > > amdgpu_userq_input_va_validate(adev, queue, args->in.wptr_va, > > AMDGPU_GPU_PAGE_SIZE)) { > > r = -EINVAL; > > - kfree(queue); > > - goto unlock; > > + goto free_queue; > > } > > > > /* Convert relative doorbell offset into absolute doorbell index */ > > index = amdgpu_userq_get_doorbell_index(uq_mgr, &db_info, filp); > > if (index == (uint64_t)-EINVAL) { > > drm_file_err(uq_mgr->file, "Failed to get doorbell for > > queue\n"); > > - kfree(queue); > > r = -EINVAL; > > - goto unlock; > > + goto free_queue; > > } > > > > queue->doorbell_index = index; > > @@ -844,15 +842,14 @@ amdgpu_userq_create(struct drm_file *filp, union > > drm_amdgpu_userq *args) > > r = amdgpu_userq_fence_driver_alloc(adev, queue); > > if (r) { > > drm_file_err(uq_mgr->file, "Failed to alloc fence > > driver\n"); > > - goto unlock; > > + goto free_queue; > > } > > > > r = uq_funcs->mqd_create(queue, &args->in); > > if (r) { > > drm_file_err(uq_mgr->file, "Failed to create Queue\n"); > > amdgpu_userq_fence_driver_free(queue); > > - kfree(queue); > > - goto unlock; > > + goto free_queue; > > } > > > > /* drop this refcount during queue destroy */ > > @@ -862,21 +859,17 @@ amdgpu_userq_create(struct drm_file *filp, union > > drm_amdgpu_userq *args) > > down_read(&adev->reset_domain->sem); > > r = xa_err(xa_store_irq(&adev->userq_doorbell_xa, index, queue, > > GFP_KERNEL)); > > if (r) { > > - kfree(queue); > > up_read(&adev->reset_domain->sem); > > - goto unlock; > > + goto free_queue; > > } > > > > r = xa_alloc(&uq_mgr->userq_xa, &qid, queue, > > XA_LIMIT(1, AMDGPU_MAX_USERQ_COUNT), GFP_KERNEL); > > if (r) { > > drm_file_err(uq_mgr->file, "Failed to allocate a queue > > id\n"); > > - amdgpu_userq_fence_driver_free(queue); > > - uq_funcs->mqd_destroy(queue); > > - kfree(queue); > > r = -ENOMEM; > > up_read(&adev->reset_domain->sem); > > - goto unlock; > > + goto free_queue; > > } > > up_read(&adev->reset_domain->sem); > > > > @@ -892,10 +885,7 @@ amdgpu_userq_create(struct drm_file *filp, union > > drm_amdgpu_userq *args) > > if (r) { > > drm_file_err(uq_mgr->file, "Failed to map Queue\n"); > > xa_erase(&uq_mgr->userq_xa, qid); > > - amdgpu_userq_fence_driver_free(queue); > > - uq_funcs->mqd_destroy(queue); > > - kfree(queue); > > - goto unlock; > > + goto free_queue; > > } > > } > > > > @@ -915,7 +905,15 @@ amdgpu_userq_create(struct drm_file *filp, union > > drm_amdgpu_userq *args) > > > > args->out.queue_id = qid; > > atomic_inc(&uq_mgr->userq_count[queue->queue_type]); > > + goto unlock; > > > > +free_mqd: > > + uq_funcs->mqd_destroy(queue); > > +free_fence_driver: > > + amdgpu_userq_fence_driver_free(queue); > > +free_queue: > > + amdgpu_userq_buffer_vas_list_cleanup(adev, queue); > > + kfree(queue); > > unlock: > > mutex_unlock(&uq_mgr->userq_mutex); > > > > -- > > 2.43.0 > > > I was looking at an outdated codebase. Sorry for the noise. Regards, Guangshuo
