Re: [PATCH] drm/amdgpu/userq: clean up VA state on create failure

[Christian König]

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 <lgs201920130244@xxxxxxxxx>
> ---
>  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
>