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/pagemap_util: Ensure proper cache lock management on free
Date: Mon, 09 Mar 2026 08:43:45 +1000
Message-ID: <review-patch1-20260306154322.60120-2-jonathan.cavitt@intel.com>
In-Reply-To: <20260306154322.60120-2-jonathan.cavitt@intel.com>
References: <20260306154322.60120-2-jonathan.cavitt@intel.com> <20260306154322.60120-2-jonathan.cavitt@intel.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

**Bug analysis:** The original code has three paths through the function af=
ter acquiring `cache->lock`:

1. `dpagemap =3D=3D NULL` =E2=86=92 unlocks =E2=9C=93
2. `dpagemap !=3D NULL && drm_pagemap_shrinker_cancel() =3D=3D true` =E2=86=
=92 unlocks =E2=9C=93
3. `dpagemap !=3D NULL && drm_pagemap_shrinker_cancel() =3D=3D false` =E2=
=86=92 **never unlocks** =E2=9C=97

In case 3, execution falls through to `mutex_destroy()` and `kfree(cache)` =
with the spinlock still held. This is a real bug, not just a static analysi=
s false positive =E2=80=94 calling `mutex_destroy` under a spinlock is inva=
lid, and `kfree` frees the lock memory while it's held.

**Review of the fix:**

The new code consolidates to a single unlock point:

```c
spin_lock(&cache->lock);
dpagemap =3D cache->dpagemap;
cache->dpagemap =3D NULL;
if (dpagemap && !drm_pagemap_shrinker_cancel(dpagemap))
    dpagemap =3D NULL;
spin_unlock(&cache->lock);

if (dpagemap)
    drm_pagemap_destroy(dpagemap, false);
```

This is clean and correct:
- `cache->dpagemap` is unconditionally cleared under the lock (v3 improveme=
nt per reviewer feedback).
- If `drm_pagemap_shrinker_cancel()` returns false (the shrinker is already=
 processing this dpagemap), the local `dpagemap` is NULLed so we don't doub=
le-destroy =E2=80=94 the shrinker scan path (`drm_pagemap_shrinker_scan` at=
 line 382-388) will handle destruction.
- If cancel succeeds, we own the dpagemap and destroy it outside the lock.
- The lock is always released exactly once.

**Race condition check:** Setting `cache->dpagemap =3D NULL` before checkin=
g the shrinker is safe. The shrinker scan (`drm_pagemap_shrinker_scan`) tak=
es `cache->lock` before clearing `cache->dpagemap`, so these two paths are =
properly serialized. `drm_pagemap_shrinker_cancel` accesses `dpagemap->cach=
e` (the back-pointer from pagemap to cache), which is unaffected by clearin=
g `cache->dpagemap`.

**Nit:** The commit message says "static analysis issue" and "probably unne=
cessary given the cache is being freed," which undersells the severity =E2=
=80=94 this is a genuine spinlock leak bug that could cause lockups or unde=
fined behavior. But that doesn't affect the code itself.

**Reviewed-by worthy: Yes.** The patch is correct and well-structured.

---
Generated by Claude Code Patch Reviewer