>> + if (new_handle == 0 || new_handle > INT_MAX || >> + args->handle == new_handle) >> + return -EINVAL; > >I'd like to open the same discussion point that I opened in the >corresponding IGT. Should we or should we not check for INT_MAX here. >There I wrote this: > >""" >About this one I am not sure. Is it not implementation details as long >as the uapi for handle is u32? Let say someone converts to XArray one >day and then we have to artificially limit that to s32 just so >gem_change_handle can keep working. >"""
uapi is u32 but idr only does handles from 0 to INT_MAX. I think it's correct for this to be an error now; if the interface one day changes such that INT_MAX < handle < UINT_MAX is now valid, I think we're okay to have some cases that were once errors now work (that's what happens whenever an ioctl adds new flags / replaces padding with new inputs). >> + ret = idr_alloc(&file_priv->object_idr, NULL, new_handle, >> + new_handle + 1, GFP_NOWAIT); > >And here I opened a point about what happens when target handle is taken >(again in IGT review): > >""" >Hmm.. this is actually leaking the wrong implementation detail of >idr_alloc to ABI. ENOSPC is "no free handles". It feels it would be >better if gem_change_handle returned EBUSY or EEXIST for this case so it >is not conflated. Thoughts? >""' On balance EBUSY is probably a more accurate error code for this than ENOSPC, I can make that change. Neither is perfect? "Busy" to me implies a temporary use of a resource, but this is about a handle which is persistently being used. EEXIST has the error message "File exists"; I don't know whether it's used for cases like this is practice. The definition of these error codes seems pretty loose, as long as it's distinct from the other error codes this can return David Francis ________________________________________ From: Tvrtko Ursulin <[email protected]> Sent: Thursday, November 5, 2026 6:12 AM To: Francis, David; [email protected] Cc: Koenig, Christian; Simona Vetter Subject: Re: [PATCH v3] drm/gem: Fix and enable CHANGE_HANDLE On 03/11/2026 15:54, 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 > and re-enable it. > > These changes are prerequisities for the enabling of this ioctl, which was > disabled over security concerns and the lack of IGT tests. > > For a summary of the saga so far, see > https://lists.freedesktop.org/archives/dri-devel/2026-June/571890.html > > I hope these changes leave the ioctl in a spot where it is safe to > re-enable. There are now igt tests covering all corner case behaviour > and race conditions between change_handle and itself, create, free, > import, and export. > > The new_handle = 0 and new_handle = old_handle corner cases > are now -EINVAL as recommended; these are covered in the igt tests. > > idr_preload is now correctly called before idr_alloc. > > With these changes the TODO can be removed and the ioctl can be > re-enabled. > > Accompanying IGT patch on igt-dev is > tests/intel: Add edge case and race condition tests for GEM_CHANGE_HANDLE > > v2: new_handle = old_handle is EINVAL > v3: More detailed commit message and some cleanup > > 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 | 30 ++++++++++-------------------- > drivers/gpu/drm/drm_ioctl.c | 3 +-- > 2 files changed, 11 insertions(+), 22 deletions(-) > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c > index 6a86bd2a0343e..bb55715e8d170 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) > { > @@ -1041,21 +1028,22 @@ int drm_gem_change_handle_ioctl(struct drm_device > *dev, void *data, > if (!drm_core_check_feature(dev, DRIVER_GEM)) > return -EOPNOTSUPP; > > - /* idr_alloc() limitation. */ > - if (args->new_handle > INT_MAX) > - return -EINVAL; > new_handle = args->new_handle; > > - if (args->handle == new_handle) > - return 0; > + /* idr_alloc() limitations. */ Nit- it's now more than idr_alloc limitations. > + if (new_handle == 0 || new_handle > INT_MAX || > + args->handle == new_handle) > + return -EINVAL; I'd like to open the same discussion point that I opened in the corresponding IGT. Should we or should we not check for INT_MAX here. There I wrote this: """ About this one I am not sure. Is it not implementation details as long as the uapi for handle is u32? Let say someone converts to XArray one day and then we have to artificially limit that to s32 just so gem_change_handle can keep working. """ > > 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, > - GFP_NOWAIT); > + ret = idr_alloc(&file_priv->object_idr, NULL, new_handle, > + new_handle + 1, GFP_NOWAIT); And here I opened a point about what happens when target handle is taken (again in IGT review): """ Hmm.. this is actually leaking the wrong implementation detail of idr_alloc to ABI. ENOSPC is "no free handles". It feels it would be better if gem_change_handle returned EBUSY or EEXIST for this case so it is not conflated. Thoughts? ""' > > if (ret < 0) { > spin_unlock(&file_priv->table_lock); > + idr_preload_end(); > goto out_unlock; > } > > @@ -1063,10 +1051,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, > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c > index e2df4becce629..ff193155129e7 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), > The rest looks good to me. Regards, Tvrtko
