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

[Claude Code Review Bot <claude-review@example.com>]

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