Good finds; will clean up >Finally, at the end of the function the final step is like this: > > spin_lock(&file_priv->table_lock); > idr_remove(&file_priv->object_idr, args->handle); > obj = idr_replace(&file_priv->object_idr, obj, new_handle); > spin_unlock(&file_priv->table_lock); > >Doesn't that leave a window where neither old or new handle can be >looked up by lockless readers? Does it matter? Would going the opposite >way and allowing to transiently see the same object twice be worse? > In fact both old and new handle are inaccessible for a much loner window than that - the idr_replace sets the old handle to NULL in the first lock section.
Having both handles be inaccessible for a period is acceptable; if you try to do something to either handle in the middle of a CHANGE_HANDLE it might fail to avoid a race condition. David ________________________________________ From: Tvrtko Ursulin <[email protected]> Sent: Tuesday, November 3, 2026 8:13 AM To: Francis, David; [email protected] Cc: Koenig, Christian; Simona Vetter Subject: Re: [PATCH v2] drm/gem: Fix and enable CHANGE_HANDLE On 07/10/2026 15:10, David Francis wrote: > Make the following changes to CHANGE_HANDLE > - reject new_handle = old_handle with EINVAL > - idr_preload before idr_alloc > - reject new_handle = 0 with EINVAL Please also mention why - like to address the TODO, ideally also putting a reference link to lore where the action plan was outlined. I'd at least like to read it to finish the review. ;) > See also the igt patch > tests/intel: Add edge case and race condition tests for GEM_CHANGE_HANDLE > on igt-dev > > v2: new_handle = old_handle is EINVAL > > cc: Tvrtko Ursulin <[email protected]> > cc: Christian Koenig <[email protected]> > cc: Simona Vetter <[email protected]> > Signed-off-by: David Francis <[email protected]> > --- > drivers/gpu/drm/drm_gem.c | 21 ++++++--------------- > drivers/gpu/drm/drm_ioctl.c | 3 +-- > 2 files changed, 7 insertions(+), 17 deletions(-) > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c > index 6a86bd2a0343..81aee038e3b2 100644 > --- a/drivers/gpu/drm/drm_gem.c > +++ b/drivers/gpu/drm/drm_gem.c > @@ -1018,19 +1018,6 @@ drm_gem_open_ioctl(struct drm_device *dev, void *data, > return ret; > } > > -/* > - * This ioctl is disabled for security reasons but also it failed > - * to follow process in terms of adding testing in igt and verifying > - * all the corner cases which made fixing security bugs in it even > - * harder than necessary. > - * > - * To re-enable this ioctl > - * 1. land working IGT tests in igt-gpu-tools that cover > - * all corner cases and race conditions. > - * 2. handle idr_preload > - * 3. handle == 0 > - * 4. handle == new_handle semantics definition. > - */ > int drm_gem_change_handle_ioctl(struct drm_device *dev, void *data, > struct drm_file *file_priv) > { > @@ -1042,20 +1029,22 @@ int drm_gem_change_handle_ioctl(struct drm_device > *dev, void *data, > return -EOPNOTSUPP; > > /* idr_alloc() limitation. */ > - if (args->new_handle > INT_MAX) > + if (args->new_handle == 0 || args->new_handle > INT_MAX) > return -EINVAL; > new_handle = args->new_handle; > > if (args->handle == new_handle) > - return 0; > + return -EINVAL; I would consider consolidating this a bit. Probably fetch new_handle into a local first and do both of the above checks with it. Because unless I am blind there is no reason it needs to be fetched in between the two conditions so that makes it a bit confusing. > > mutex_lock(&file_priv->prime.lock); > + idr_preload(GFP_KERNEL); > spin_lock(&file_priv->table_lock); > ret = idr_alloc(&file_priv->object_idr, NULL, new_handle, new_handle + > 1, Bit - the comma in the "+ 1," part is the only part of this function which straddles the 80th column so maybe use the opportunity to clean that up. > GFP_NOWAIT); > > if (ret < 0) { > spin_unlock(&file_priv->table_lock); > + idr_preload_end(); > goto out_unlock; > } > > @@ -1063,10 +1052,12 @@ int drm_gem_change_handle_ioctl(struct drm_device > *dev, void *data, > if (IS_ERR_OR_NULL(obj)) { > idr_remove(&file_priv->object_idr, new_handle); > spin_unlock(&file_priv->table_lock); > + idr_preload_end();> ret = -ENOENT; > goto out_unlock; > } > spin_unlock(&file_priv->table_lock); > + idr_preload_end(); > > if (obj->dma_buf) { > ret = drm_prime_add_buf_handle(&file_priv->prime, obj->dma_buf, There is a "ret = 0;" a bit lower down which I don't see is required so unless I am missing something maybe clean that up as well. Finally, at the end of the function the final step is like this: spin_lock(&file_priv->table_lock); idr_remove(&file_priv->object_idr, args->handle); obj = idr_replace(&file_priv->object_idr, obj, new_handle); spin_unlock(&file_priv->table_lock); Doesn't that leave a window where neither old or new handle can be looked up by lockless readers? Does it matter? Would going the opposite way and allowing to transiently see the same object twice be worse? Regards, Tvrtko > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c > index e2df4becce62..ff193155129e 100644 > --- a/drivers/gpu/drm/drm_ioctl.c > +++ b/drivers/gpu/drm/drm_ioctl.c > @@ -660,8 +660,7 @@ static const struct drm_ioctl_desc drm_ioctls[] = { > DRM_IOCTL_DEF(DRM_IOCTL_GEM_CLOSE, drm_gem_close_ioctl, > DRM_RENDER_ALLOW), > DRM_IOCTL_DEF(DRM_IOCTL_GEM_FLINK, drm_gem_flink_ioctl, DRM_AUTH), > DRM_IOCTL_DEF(DRM_IOCTL_GEM_OPEN, drm_gem_open_ioctl, DRM_AUTH), > - /* see drm_gem.c:drm_gem_change_handle_ioctl for why this is invalid */ > - DRM_IOCTL_DEF(DRM_IOCTL_GEM_CHANGE_HANDLE, drm_invalid_op, > DRM_RENDER_ALLOW), > + DRM_IOCTL_DEF(DRM_IOCTL_GEM_CHANGE_HANDLE, drm_gem_change_handle_ioctl, > DRM_RENDER_ALLOW), > > DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETRESOURCES, drm_mode_getresources, 0), >
