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

[PATCH] 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.

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 | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_pagemap_util.c b/drivers/gpu/drm/drm_pagemap_util.c
index c6ae3357c7fb..79e07b3658ed 100644
--- a/drivers/gpu/drm/drm_pagemap_util.c
+++ b/drivers/gpu/drm/drm_pagemap_util.c
@@ -65,18 +65,18 @@ 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);
+	if (!dpagemap)
 		goto out;
-	}
 
 	if (drm_pagemap_shrinker_cancel(dpagemap)) {
 		cache->dpagemap = NULL;
 		spin_unlock(&cache->lock);
 		drm_pagemap_destroy(dpagemap, false);
+	} else {
+out:
+		spin_unlock(&cache->lock);
 	}
 
-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]

Patch Review

The original code has three paths through `drm_pagemap_cache_fini`:

1. `dpagemap` is NULL → unlock, goto out
2. `drm_pagemap_shrinker_cancel()` returns true → set NULL, unlock, destroy
3. `drm_pagemap_shrinker_cancel()` returns false → **no unlock** (bug)

The fix correctly ensures all three paths call `spin_unlock`. However:

> +	if (!dpagemap)
> 		goto out;
>
> 	if (drm_pagemap_shrinker_cancel(dpagemap)) {
> 		cache->dpagemap = NULL;
> 		spin_unlock(&cache->lock);
> 		drm_pagemap_destroy(dpagemap, false);
> +	} else {
> +out:
> +		spin_unlock(&cache->lock);
> 	}

Placing a goto label inside an `else` block is valid C but unusual enough to make readers do a double-take. The `out:` label is jumped to when `dpagemap` is NULL (skipping the `if/else` condition entirely), while the `else` clause handles the cancel-failure case — these are logically different situations that happen to share the same cleanup action. A slightly cleaner alternative would be:

```c
if (!dpagemap)
    goto out;

if (drm_pagemap_shrinker_cancel(dpagemap)) {
    cache->dpagemap = NULL;
    spin_unlock(&cache->lock);
    drm_pagemap_destroy(dpagemap, false);
    goto done;
}
out:
spin_unlock(&cache->lock);
done:
mutex_destroy(&cache->lookup_mutex);
kfree(cache);
```

That said, the current approach does produce the correct behavior for all three paths, so this is a readability preference, not a correctness issue.

There is also a question about the cancel-failure path: when `drm_pagemap_shrinker_cancel()` returns false, the code unlocks but does not set `cache->dpagemap = NULL` and does not call `drm_pagemap_destroy()`. This means the dpagemap is left dangling as the cache is immediately freed afterward. Looking at `drm_pagemap_shrinker_scan()`, when the shrinker races and wins, it sets `cache->dpagemap = NULL` and calls `drm_pagemap_destroy()` itself, so the dpagemap is handled by the shrinker. This is fine — the race is benign because by the time `drm_pagemap_cache_fini` sees `shrink_link` as empty, the shrinker has already taken ownership. However, there's also a window where `drm_pagemap_shrinker_scan` has called `list_del_init` on `shrink_link` but has not yet reached `cache->dpagemap = NULL`. In that narrow window, `drm_pagemap_cache_fini` would see `dpagemap` as non-NULL and `shrink_link` as empty, take the else/out path, and then `kfree(cache)`. Then the shrinker would proceed to `spin_lock(&cache->lock)` on freed memory. This race existed before this patch and is not introduced by it, but it may be worth noting to the author since the Fixes tag suggests this is meant to be a complete fix for the function.

The commit message says "Static analysis issue" and "Though probably unnecessary given the cache is being freed at this step." This is inaccurate — the spinlock leak is a real bug, not a cosmetic static analysis finding. Even though the cache is about to be freed, calling `kfree()` on a locked spinlock is wrong and will trigger `CONFIG_DEBUG_LOCK_ALLOC` warnings. The commit message should state the actual consequence more clearly.

---
Generated by Claude Code Patch Reviewer

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-02-21T05:51:04.385592

---

This is a single-patch submission fixing a spinlock leak in `drm_pagemap_cache_fini()` in the new `drm_pagemap_util.c` infrastructure. The bug is real: when `dpagemap` is non-NULL but `drm_pagemap_shrinker_cancel()` returns false (meaning the shrinker scan already dequeued the dpagemap), the original code falls through the `if` block without ever calling `spin_unlock(&cache->lock)`. This would cause a spinlock deadlock on any subsequent lock attempt and trigger lockdep warnings.

The fix is correct but uses an unusual control flow pattern — placing the `out:` label inside an `else` block and jumping into it with `goto`. While this is legal C, it results in code that is harder to read than necessary for a two-path function. A straightforward alternative would be to simply add `spin_unlock(&cache->lock)` in the else branch without reusing the `out:` label, or to restructure the function to unlock unconditionally before the conditional destroy.

The commit message undersells the issue. It says the lock is "probably unnecessary given the cache is being freed" and frames it as a consistency fix for static analysis. But the spinlock leak is a genuine bug: if the path is reached, `spin_unlock` is never called, and the subsequent `kfree(cache)` frees a locked spinlock, which will trigger `CONFIG_DEBUG_LOCK_ALLOC` warnings. The path *is* reachable — the shrinker scan can race with cache teardown, removing the dpagemap from the shrink list and making `drm_pagemap_shrinker_cancel()` return false while `cache->dpagemap` is still non-NULL.

---
Generated by Claude Code Patch Reviewer