Re: [PATCH] drm/gem: Fix and enable CHANGE_HANDLE

Whoops, good catch. Fix is simple; add an idr_preload_end() on that path. Will be in v2.

David Francis

________________________________________
From: [email protected] <[email protected]>
Sent: Friday, August 7, 2026 2:46 PM
To: Francis, David
Cc: [email protected]
Subject: Re: [PATCH] drm/gem: Fix and enable CHANGE_HANDLE

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Missing idr_preload_end() in the idr_alloc error path leaves preemption permanently disabled.
--

--- Patch [1]: [PATCH] drm/gem: Fix and enable CHANGE_HANDLE ---
commit 4926fe37208872dd7c9115003f10226cea7a200a
Author: David Francis <[email protected]>
drm/gem: Fix and enable CHANGE_HANDLE

This patch enables the CHANGE_HANDLE ioctl for GEM and adds validation
for the handle inputs. It also adds idr_preload before idr_alloc to
ensure memory allocation can be fulfilled without sleeping while holding
a spinlock.

> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 018df97d590dc..00693626d7683 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
[ ... ]
> @@ -1038,14 +1025,22 @@ int drm_gem_change_handle_ioctl(struct drm_device *dev, void *data,
[ ... ]
>       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);
> @@ -1059,10 +1054,12 @@ int drm_gem_change_handle_ioctl(struct drm_device *dev, void *data,

[Severity: Critical]
Does this error path leave preemption permanently disabled?

Looking at the code just after idr_alloc() in drm_gem_change_handle_ioctl(),
there is an error path that skips idr_preload_end():

        ret = idr_alloc(&file_priv->object_idr, NULL, new_handle, new_handle + 1,
                        GFP_NOWAIT);

        if (ret < 0) {
                spin_unlock(&file_priv->table_lock);
                goto out_unlock;
        }

If idr_alloc() fails (for example, returning -ENOSPC when new_handle is
already in use), the code jumps to out_unlock which only releases prime.lock
and returns. Since idr_preload() disables preemption, wouldn't we need an
idr_preload_end() call before returning to avoid a scheduling while atomic
system panic?

>       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,

--
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1