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
>
