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 */
