On 9/30/26 10:30, Jesse Zhang wrote:
> amdgpu_userq_create() inserts the queue into userq_xa and then sets up
> its debugfs entry without holding a reference. A concurrent
> AMDGPU_USERQ_OP_FREE on the (easily guessed) queue ID can free the queue
> in that window, so create writes to freed memory and leaves a queue_N
> debugfs directory whose mqd_info points to the freed queue:
>
> debugfs: 'queue_1' already exists in 'client-137'
I honestly don't see that as problematic.
debugfs has tons of such issues and that is generally ok.
Regards,
Christian.
>
> Only reserve the ID in xa_alloc() and store the queue once create is
> done with it. Don't let FREE release a reserved ID.
>
> Fixes: 28cacaace5cd ("drm/amdgpu/userq: defer queue publication until create
> completes")
> Signed-off-by: Jesse Zhang <[email protected]>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 14 ++++++++++++--
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> index 63875761dfac..89e03d3a930c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> @@ -775,7 +775,12 @@ amdgpu_userq_create(struct drm_file *filp, union
> drm_amdgpu_userq *args)
> atomic_inc(&uq_mgr->userq_count[queue->queue_type]);
> mutex_unlock(&uq_mgr->userq_mutex);
>
> - r = xa_alloc(&uq_mgr->userq_xa, &qid, queue,
> + /*
> + * Only reserve the ID here. Once the queue is visible in userq_xa,
> + * a concurrent AMDGPU_USERQ_OP_FREE can drop the last reference, so
> + * publish it only after we are done with it.
> + */
> + r = xa_alloc(&uq_mgr->userq_xa, &qid, NULL,
> XA_LIMIT(1, AMDGPU_MAX_USERQ_COUNT),
> GFP_KERNEL);
> if (r) {
> @@ -790,6 +795,8 @@ amdgpu_userq_create(struct drm_file *filp, union
> drm_amdgpu_userq *args)
>
> amdgpu_debugfs_userq_init(filp, queue, qid);
> trace_amdgpu_userq_create_end(queue, 0);
> + /* Storing to a reserved entry doesn't allocate memory and can't fail */
> + xa_store(&uq_mgr->userq_xa, qid, queue, GFP_KERNEL);
> args->out.queue_id = qid;
> return 0;
>
> @@ -924,7 +931,10 @@ int amdgpu_userq_ioctl(struct drm_device *dev, void
> *data,
>
> case AMDGPU_USERQ_OP_FREE: {
> xa_lock(&fpriv->userq_mgr.userq_xa);
> - queue = __xa_erase(&fpriv->userq_mgr.userq_xa,
> args->in.queue_id);
> + /* Don't release an ID reserved by an in-flight CREATE */
> + queue = xa_load(&fpriv->userq_mgr.userq_xa, args->in.queue_id);
> + if (queue)
> + __xa_erase(&fpriv->userq_mgr.userq_xa,
> args->in.queue_id);
> xa_unlock(&fpriv->userq_mgr.userq_xa);
> if (!queue)
> return -ENOENT;
