[PATCH v3] drm/pagemap_util: Ensure proper cache lock management on free

[PATCH v3] drm/pagemap_util: Ensure proper cache lock management on free

[Jonathan Cavitt]

Static analysis issue:

Though probably unnecessary given the cache is being freed at this step,
for the sake of consistency, ensure that the cache lock is always
unlocked after drm_pagemap_cache_fini.

v2:
- Use requested code flow (Maarten)

v3:
- Clear cache->dpagemap (Matt Brost, Maarten)

Fixes: 77f14f2f2d73f ("drm/pagemap: Add a drm_pagemap cache and shrinker")
Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
Cc: Thomas Hellstrom <thomas.hellstrom@linux.intel.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_pagemap_util.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_pagemap_util.c b/drivers/gpu/drm/drm_pagemap_util.c
index 14ddb948a32e..6111d90a38e2 100644
--- a/drivers/gpu/drm/drm_pagemap_util.c
+++ b/drivers/gpu/drm/drm_pagemap_util.c
@@ -65,18 +65,14 @@ static void drm_pagemap_cache_fini(void *arg)
 	drm_dbg(cache->shrinker->drm, "Destroying dpagemap cache.\n");
 	spin_lock(&cache->lock);
 	dpagemap = cache->dpagemap;
-	if (!dpagemap) {
-		spin_unlock(&cache->lock);
-		goto out;
-	}
+	cache->dpagemap = NULL;
+	if (dpagemap && !drm_pagemap_shrinker_cancel(dpagemap))
+		dpagemap = NULL;
+	spin_unlock(&cache->lock);
 
-	if (drm_pagemap_shrinker_cancel(dpagemap)) {
-		cache->dpagemap = NULL;
-		spin_unlock(&cache->lock);
+	if (dpagemap)
 		drm_pagemap_destroy(dpagemap, false);
-	}
 
-out:
 	mutex_destroy(&cache->lookup_mutex);
 	kfree(cache);
 }
-- 
2.43.0

Claude review: drm/pagemap_util: Ensure proper cache lock management on free

[Claude Code Review Bot]

Overall Series Review

Subject: drm/pagemap_util: Ensure proper cache lock management on free
Author: Jonathan Cavitt <jonathan.cavitt@intel.com>
Patches: 1
Reviewed: 2026-03-09T08:43:45.286991

---

This is a single-patch fix for a real bug in `drm_pagemap_cache_fini` where the spinlock `cache->lock` is never released when `dpagemap` is non-NULL but `drm_pagemap_shrinker_cancel()` returns false. The patch simplifies the function into a clean linear flow with a single unlock point. The fix is correct, well-structured, and addresses the bug clearly.

**Verdict: Looks good.**

---
Generated by Claude Code Patch Reviewer

Claude review: drm/pagemap_util: Ensure proper cache lock management on free

[Claude Code Review Bot]

Patch Review

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

1. `dpagemap == NULL` → unlocks ✓
2. `dpagemap != NULL && drm_pagemap_shrinker_cancel() == true` → unlocks ✓
3. `dpagemap != NULL && drm_pagemap_shrinker_cancel() == false` → **never unlocks** ✗

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 analysis false positive — calling `mutex_destroy` under a spinlock is invalid, 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 = cache->dpagemap;
cache->dpagemap = NULL;
if (dpagemap && !drm_pagemap_shrinker_cancel(dpagemap))
    dpagemap = 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 improvement 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 double-destroy — 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 = NULL` before checking the shrinker is safe. The shrinker scan (`drm_pagemap_shrinker_scan`) takes `cache->lock` before clearing `cache->dpagemap`, so these two paths are properly serialized. `drm_pagemap_shrinker_cancel` accesses `dpagemap->cache` (the back-pointer from pagemap to cache), which is unaffected by clearing `cache->dpagemap`.

**Nit:** The commit message says "static analysis issue" and "probably unnecessary given the cache is being freed," which undersells the severity — this is a genuine spinlock leak bug that could cause lockups or undefined 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