[PATCH drm] drm/gem: fix race between change_handle and handle_delete

[PATCH drm] drm/gem: fix race between change_handle and handle_delete

[Zhenghang Xiao]

drm_gem_change_handle_ioctl leaves the old handle live in the IDR
during the window between spin_unlock(table_lock) and the final
spin_lock(table_lock). A concurrent drm_gem_handle_delete on the old
handle succeeds in this window, decrements handle_count to 0, and frees
the GEM object while the new handle's IDR entry still references it.

NULL the old handle's IDR entry before dropping table_lock so that any
concurrent GEM_CLOSE on the old handle sees NULL and returns -EINVAL.
Restore the old entry on the prime-bookkeeping error path.

Fixes: 5e28b7b94408 ("drm: Set old handle to NULL before prime swap in change_handle")
Signed-off-by: Zhenghang Xiao <kipreyyy@gmail.com>
---
 drivers/gpu/drm/drm_gem.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 8afab57fc055..189b990cc78e 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -1065,6 +1065,7 @@ int drm_gem_change_handle_ioctl(struct drm_device *dev, void *data,
 	       goto out_unlock;
        }
 
+	idr_replace(&file_priv->object_idr, NULL, args->handle);
 	spin_unlock(&file_priv->table_lock);
 
 	if (obj->dma_buf) {
@@ -1073,6 +1074,7 @@ int drm_gem_change_handle_ioctl(struct drm_device *dev, void *data,
 		if (ret < 0) {
 			spin_lock(&file_priv->table_lock);
 			idr_remove(&file_priv->object_idr, handle);
+			idr_replace(&file_priv->object_idr, obj, args->handle);
 			spin_unlock(&file_priv->table_lock);
 			goto out_unlock;
 		}
-- 
2.50.1 (Apple Git-155)

Claude review: drm/gem: fix race between change_handle and handle_delete

[Claude Code Review Bot]

Overall Series Review

Subject: drm/gem: fix race between change_handle and handle_delete
Author: Zhenghang Xiao <kipreyyy@gmail.com>
Patches: 1
Reviewed: 2026-05-27T14:13:41.988454

---

This is a single patch fixing a race condition in `drm_gem_change_handle_ioctl` where a concurrent `drm_gem_handle_delete` on the old handle can succeed during the window between allocating the new handle and removing the old one, leading to incorrect `handle_count` and potential use-after-free via `drm_gem_object_release_handle`.

The race analysis in the commit message is correct and the fix approach — NULLing the old handle's IDR entry before dropping `table_lock` — is sound. However, the patch is based on a tree that includes commit `5e28b7b94408` which is not present in the current drm-next tree. The code structure in drm-next is different (the `spin_unlock` after `idr_alloc` happens immediately at line 1047, not after the error check as the patch context implies). The patch does not apply cleanly and would need to be rebased.

There are also a few correctness concerns with the error-path restoration and a remaining race window that the patch doesn't address.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/gem: fix race between change_handle and handle_delete

[Claude Code Review Bot]

Patch Review

**Race analysis: correct.** In the current drm-next code at `drm_gem.c:1044-1069`, after `idr_alloc` at line 1045 creates the new handle and `spin_unlock` at line 1047 drops the lock, both the old handle (`args->handle`) and the new handle (`handle`) point to the same `obj` in the IDR. A concurrent `drm_gem_handle_delete(args->handle)` during this window would:

1. Successfully `idr_replace(NULL, args->handle)` — getting back `obj`
2. Call `drm_gem_object_release_handle(args->handle, obj, filp)` — decrementing `handle_count`
3. `idr_remove(args->handle)`

This leaves the new handle live in the IDR but with `handle_count` decremented, which is wrong.

**Fix approach: correct.** NULLing the old handle via `idr_replace` before dropping the spinlock ensures concurrent `drm_gem_handle_delete` sees NULL and returns `-EINVAL` (see `drm_gem.c:407-410`):

```c
obj = idr_replace(&filp->object_idr, NULL, handle);
...
if (IS_ERR_OR_NULL(obj))
    return -EINVAL;
```

**Issue 1 — Patch does not apply to drm-next.** The patch context shows `spin_unlock` coming *after* the `idr_alloc` error check (`goto out_unlock`), implying the lock is held continuously from `idr_alloc` through to the new `idr_replace`. But drm-next releases the lock immediately after `idr_alloc` at line 1047, before the error check at line 1049. The Fixes tag references `5e28b7b94408 ("drm: Set old handle to NULL before prime swap in change_handle")` which presumably restructured this locking — but that commit is not in the current drm-next tree. This patch needs to be rebased against the correct base.

**Issue 2 — Unchecked `idr_replace` return value.** The added line:

```c
idr_replace(&file_priv->object_idr, NULL, args->handle);
```

does not check the return value. If a concurrent `drm_gem_handle_delete(args->handle)` ran between `drm_gem_object_lookup` (line 1033, which releases `table_lock` internally) and the later `spin_lock(table_lock)` (line 1044), the old handle could already be removed from the IDR, and `idr_replace` would return `ERR_PTR(-ENOENT)`. While this is silently ignored and doesn't crash, it means the function proceeds to create the new handle even though the old handle is already gone — leaving `handle_count` in an inconsistent state (new handle created without incrementing, old handle's decrement already happened via the concurrent delete). This is a pre-existing race between `drm_gem_object_lookup` and `table_lock`, but worth noting since the patch doesn't close it.

**Issue 3 — Error-path restoration correctness.** The error-path addition:

```c
spin_lock(&file_priv->table_lock);
idr_remove(&file_priv->object_idr, handle);
idr_replace(&file_priv->object_idr, obj, args->handle);
spin_unlock(&file_priv->table_lock);
```

is safe: because the old handle was NULLed while holding `table_lock`, any concurrent `drm_gem_handle_delete(args->handle)` would see NULL from its own `idr_replace` and return `-EINVAL` without removing the IDR entry. So the entry for `args->handle` still exists (just NULLed) and can be restored. This logic is correct.

**Minor — commit message vs Fixes tag clarity.** The Fixes tag says this patch fixes `5e28b7b94408`, but the commit message says the race exists in `drm_gem_change_handle_ioctl` generally. The race window between `idr_alloc` and `idr_remove` of the old handle exists in the drm-next code too (without `5e28b7b94408`). It would be helpful to clarify whether this is fixing a regression introduced by `5e28b7b94408` or the underlying race that `5e28b7b94408` failed to fully fix.

---
Generated by Claude Code Patch Reviewer