From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm/gem: fix race between change_handle and handle_delete
Date: Wed, 27 May 2026 14:13:42 +1000
Message-ID: <review-patch1-20260526085313.26791-1-kipreyyy@gmail.com>
In-Reply-To: <20260526085313.26791-1-kipreyyy@gmail.com>
References: <20260526085313.26791-1-kipreyyy@gmail.com> <20260526085313.26791-1-kipreyyy@gmail.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

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_unl=
ock` 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)` =E2=80=94 getting back `o=
bj`
2. Call `drm_gem_object_release_handle(args->handle, obj, filp)` =E2=80=94 =
decrementing `handle_count`
3. `idr_remove(args->handle)`

This leaves the new handle live in the IDR but with `handle_count` decremen=
ted, 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 =3D idr_replace(&filp->object_idr, NULL, handle);
...
if (IS_ERR_OR_NULL(obj))
    return -EINVAL;
```

**Issue 1 =E2=80=94 Patch does not apply to drm-next.** The patch context s=
hows `spin_unlock` coming *after* the `idr_alloc` error check (`goto out_un=
lock`), implying the lock is held continuously from `idr_alloc` through to =
the new `idr_replace`. But drm-next releases the lock immediately after `id=
r_alloc` at line 1047, before the error check at line 1049. The Fixes tag r=
eferences `5e28b7b94408 ("drm: Set old handle to NULL before prime swap in =
change_handle")` which presumably restructured this locking =E2=80=94 but t=
hat commit is not in the current drm-next tree. This patch needs to be reba=
sed against the correct base.

**Issue 2 =E2=80=94 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(arg=
s->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` wo=
uld return `ERR_PTR(-ENOENT)`. While this is silently ignored and doesn't c=
rash, it means the function proceeds to create the new handle even though t=
he old handle is already gone =E2=80=94 leaving `handle_count` in an incons=
istent state (new handle created without incrementing, old handle's decreme=
nt already happened via the concurrent delete). This is a pre-existing race=
 between `drm_gem_object_lookup` and `table_lock`, but worth noting since t=
he patch doesn't close it.

**Issue 3 =E2=80=94 Error-path restoration correctness.** The error-path ad=
dition:

```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 ow=
n `idr_replace` and return `-EINVAL` without removing the IDR entry. So the=
 entry for `args->handle` still exists (just NULLed) and can be restored. T=
his logic is correct.

**Minor =E2=80=94 commit message vs Fixes tag clarity.** The Fixes tag says=
 this patch fixes `5e28b7b94408`, but the commit message says the race exis=
ts 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 fi=
xing a regression introduced by `5e28b7b94408` or the underlying race that =
`5e28b7b94408` failed to fully fix.

---
Generated by Claude Code Patch Reviewer