Re: [PATCH v2 2/2] drm/amdgpu: Add CHANGE_ID option for USERQ ioctl

Thanks for the comments. Most of these are just mistakes on my part and will be fixed.

Regarding the locking,

on CHANGE,
> Same as for the list ioctl, GFP_KERNEL under the xa_lock will not work.
>
> You can have a look on how I've done it in "drm/amdgpu: Add context
> handle renaming operation" and see if you can punch some holes in my
> logic there?

I think the solution here is just to use idr_preload.
I'd rather hold the lock the whole time to avoid having to do the
dance from GEM_CHANGE_HANDLE.

in LIST
> GFP_KERNEL under xa_lock will not work.

This one is harder. I can shift around the allocations but the point
remains that if I don't hold the xa_lock the whole time
there's a chance that create / free / other modifications
of a queue's data will happen in the meantime.

I can avoid over-writing the end of the arrays / structs just by
re-checking that the array index never gets past the size of
the array, but that wouldn't protect about returning
corrupted data if LIST races another operation
(such as CHANGE_HANDLE).

I guess in that case it isn't a security risk and you could
say it's the user's fault for creating this race condition, but
I'd prefer that wasn't part of the interface.

Thanks,
David

________________________________________
From: Tvrtko Ursulin <[email protected]>
Sent: Thursday, August 13, 2026 4:48 AM
To: Francis, David; [email protected]
Subject: Re: [PATCH v2 2/2] drm/amdgpu: Add CHANGE_ID option for USERQ ioctl


On 11/08/2026 15:12, David Francis wrote:
> Add a new option to the USERQ ioctl, which is called with
> the queue_id of an existing user queue and an unused queue_id,
> and changes that queue's id to the new value.
>
> Calling with an invalid new handle will fail. Calling with new_handle
> =handle will succeed if that queue exists but not do anything.
>
> This operation holds userq_mutex and the userq_xa xa_lock for its
> entire duration.
>
> Performing this operation on a queue with signals or waits
> outstanding is fine, as those hold not the queue_id but a
> direct reference to the queue object.
>
> Signed-off-by: David Francis <[email protected]>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c | 43 +++++++++++++++++++++++
>   include/uapi/drm/amdgpu_drm.h             | 17 +++++++--
>   2 files changed, 57 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> index 3c930425c1bb..b532ba0f4cef 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq.c
> @@ -852,6 +852,11 @@ static int amdgpu_userq_input_args_validate(struct drm_device *dev,
>               break;
>       case AMDGPU_USERQ_OP_LIST:
>               break;
> +     case AMDGPU_USERQ_OP_CHANGE_ID:
> +             if (!args->change_in.new_queue_id ||
> +                 args->change_in.new_queue_id > AMDGPU_MAX_USERQ_COUNT)
> +                     return -EINVAL;
> +             break;
>       default:
>               return -EINVAL;
>       }
> @@ -1012,6 +1017,41 @@ amdgpu_userq_list(struct drm_file *filp, union drm_amdgpu_userq *args)
>       return ret;
>   }
>
> +static int amdgpu_userq_change_id(struct drm_file *filp, union drm_amdgpu_userq *args)
> +{
> +     struct amdgpu_fpriv *fpriv = filp->driver_priv;
> +     struct amdgpu_userq_mgr *uq_mgr = &fpriv->userq_mgr;
> +     struct amdgpu_usermode_queue *queue;
> +     int ret = 0;
> +
> +     mutex_lock(&uq_mgr->userq_mutex);
> +     xa_lock(&uq_mgr->userq_xa);
> +
> +     queue = xa_load(&uq_mgr->userq_xa, args->change_in.queue_id);
> +     if (!queue) {
> +             ret = -ENOENT;
> +             goto unlock;
> +     }
> +
> +     if (args->change_in.new_queue_id == args->change_in.queue_id) {

I would move this outside the lock or even consider returning -EINVAL.
Or you have a reason why returning success is handy? Probing what
exists? Why?

> +             ret = 0;
> +             goto unlock;
> +     }
> +
> +     ret = __xa_insert(&uq_mgr->userq_xa, args->change_in.new_queue_id, queue, GFP_KERNEL);

Same as for the list ioctl, GFP_KERNEL under the xa_lock will not work.

You can have a look on how I've done it in "drm/amdgpu: Add context
handle renaming operation" and see if you can punch some holes in my
logic there?

If that works question will be do you really need both the userq_mutext
and xa_lock or perhaps xa_lock would be enough throughout the series.

Regards,

Tvrtko

> +     if (ret) {
> +             ret = -EINVAL;
> +             goto unlock;
> +     }
> +
> +     __xa_erase(&uq_mgr->userq_xa, args->change_in.queue_id);
> +
> +unlock:
> +     xa_unlock(&uq_mgr->userq_xa);
> +     mutex_unlock(&uq_mgr->userq_mutex);
> +     return ret;
> +}
> +
>   bool amdgpu_userq_enabled(struct drm_device *dev)
>   {
>       struct amdgpu_device *adev = drm_to_adev(dev);
> @@ -1058,6 +1098,9 @@ int amdgpu_userq_ioctl(struct drm_device *dev, void *data,
>       case AMDGPU_USERQ_OP_LIST:
>               r = amdgpu_userq_list(filp, args);
>               break;
> +     case AMDGPU_USERQ_OP_CHANGE_ID:
> +             r = amdgpu_userq_change_id(filp, args);
> +             break;
>       default:
>               drm_dbg_driver(dev, "Invalid user queue op specified: %d\n", args->in.op);
>               return -EINVAL;
> diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h
> index 678f3d531df7..de9ae1296819 100644
> --- a/include/uapi/drm/amdgpu_drm.h
> +++ b/include/uapi/drm/amdgpu_drm.h
> @@ -330,9 +330,10 @@ union drm_amdgpu_ctx {
>   };
>
>   /* user queue IOCTL operations */
> -#define AMDGPU_USERQ_OP_CREATE       1
> -#define AMDGPU_USERQ_OP_FREE 2
> -#define AMDGPU_USERQ_OP_LIST 3
> +#define AMDGPU_USERQ_OP_CREATE               1
> +#define AMDGPU_USERQ_OP_FREE         2
> +#define AMDGPU_USERQ_OP_LIST         3
> +#define AMDGPU_USERQ_OP_CHANGE_ID    4
>
>   /* queue priority levels */
>   /* low < normal low < normal high < high */
> @@ -463,10 +464,20 @@ struct drm_amdgpu_userq_list_in_out {
>       __u64   entries;
>   };
>
> +struct drm_amdgpu_userq_change_id_in {
> +     /** AMDGPU_USERQ_OP_CHANGE_ID */
> +     __u32   op;
> +     /** Queue id of some queue */
> +     __u32   queue_id;
> +     /** Queue id to change that queue to */
> +     __u32   new_queue_id;
> +};
> +
>   union drm_amdgpu_userq {
>       struct drm_amdgpu_userq_in in;
>       struct drm_amdgpu_userq_out out;
>       struct drm_amdgpu_userq_list_in_out list_in_out;
> +     struct drm_amdgpu_userq_change_id_in change_in;
>   };
>
>   /* GFX V11 IP specific MQD parameters */