Re: [PATCH 1/3] drm/amdgpu: Protect userq create and free fully with mutex

On 23/07/2026 19:50, David Francis wrote:
> amdgpu userq has a userq_mutex meant to protect the list of userqs
> and their state.
> 
> The CREATE option of the USERQ ioctl released the lock before
> performing a last xa_alloc, relying on the xa lock to avoid
> races.
> 
> The FREE option of the same ioctl didn't use the lock at all.
> 
> Acquire and hold the lock in both these cases, to ensure
> correct serialization of these ioctl options against each other
> and against future ioctl options.
> 
> Signed-off-by: David Francis <[email protected]>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 11 +++++------
>   1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> index 3fe10d6af757..cfee7466ef44 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> @@ -748,11 +748,11 @@ 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,
>   		     XA_LIMIT(1, AMDGPU_MAX_USERQ_COUNT),
>   		     GFP_KERNEL);
> +
> +	mutex_unlock(&uq_mgr->userq_mutex);

So the bug/race is that AMDGPU_USERQ_OP_FREE can drop the reference 
before amdgpu_userq_create() fully finishes, but then I think it is 
still there after this patch. Because a bit lower down, on the success 
path, there is this:

	amdgpu_debugfs_userq_init(filp, queue, qid);
	trace_amdgpu_userq_create_end(queue, 0);
	args->out.queue_id = qid;
	return 0;

So as soon as userq_mutex is unlocked, racing thread can do the last 
amdgpu_userq_put() making the first two of above use after free.

Possibly xa_alloc needs to be truly last with the corresponding debugfs 
unwind on the failure path. That would fix the existing race without the 
need to widen the mutex scope, but perhaps wouldn't help you for the 
LIST implementation in the following patch. Question there is could you 
get away with not locking the mutex.

>   	if (r) {
>   		/*
>   		 * This drops the last reference which should take care of
> @@ -890,18 +890,17 @@ int amdgpu_userq_ioctl(struct drm_device *dev, void *data,
>   		if (r)
>   			drm_file_err(filp, "Failed to create usermode queue\n");
>   		break;
> -
> -	case AMDGPU_USERQ_OP_FREE: {
> +	case AMDGPU_USERQ_OP_FREE:
> +		mutex_lock(&fpriv->userq_mgr.userq_mutex);
>   		xa_lock(&fpriv->userq_mgr.userq_xa);
>   		queue = __xa_erase(&fpriv->userq_mgr.userq_xa, args->in.queue_id);
>   		xa_unlock(&fpriv->userq_mgr.userq_xa);

xa_lock + __xa_erase + xa_unlock = xa_erase

Regards,

Tvrtko

> +		mutex_unlock(&fpriv->userq_mgr.userq_mutex);
>   		if (!queue)
>   			return -ENOENT;
>   
>   		amdgpu_userq_put(queue);
>   		break;
> -	}
> -
>   	default:
>   		drm_dbg_driver(dev, "Invalid user queue op specified: %d\n", args->in.op);
>   		return -EINVAL;