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

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

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

[PATCH v2] 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)

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

diff --git a/drivers/gpu/drm/drm_pagemap_util.c b/drivers/gpu/drm/drm_pagemap_util.c
index 14ddb948a32e..66203a19f8f6 100644
--- a/drivers/gpu/drm/drm_pagemap_util.c
+++ b/drivers/gpu/drm/drm_pagemap_util.c
@@ -65,18 +65,13 @@ 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;
-	}
+	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]

Patch Review

**The Bug — missing `cache->dpagemap = NULL`:**

The original code sets `cache->dpagemap = NULL` inside the `drm_pagemap_shrinker_cancel` success path (line 74):

```c
if (drm_pagemap_shrinker_cancel(dpagemap)) {
    cache->dpagemap = NULL;   // <-- this is present in the original
    spin_unlock(&cache->lock);
    drm_pagemap_destroy(dpagemap, false);
}
```

The new code drops this assignment entirely:

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

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

The local variable `dpagemap` is set to NULL, but `cache->dpagemap` is never cleared. While the cache is about to be freed (the function proceeds to `kfree(cache)`), the original code was explicitly NULLing the field while still holding the spinlock, which prevents any concurrent accessor from seeing a stale pointer between the `spin_unlock` and the `kfree`. Since `drm_pagemap_get_from_cache_if_active()` does `spin_lock(&cache->lock); dpagemap = drm_pagemap_get_unless_zero(cache->dpagemap); spin_unlock(&cache->lock);`, there is a small window where a concurrent reader could grab `cache->dpagemap` after the lock is dropped but before `kfree(cache)`. The original code avoided this by setting `cache->dpagemap = NULL` under the lock. The new code should do:

```c
if (dpagemap && !drm_pagemap_shrinker_cancel(dpagemap))
    dpagemap = NULL;
cache->dpagemap = NULL;  /* clear before unlock */
spin_unlock(&cache->lock);
```

That said, this is a `devm` fini callback — at device teardown time, concurrent cache accesses would themselves be a bug. So in practice this may be benign, but for correctness under the stated goal of "consistency," the `cache->dpagemap = NULL` assignment should be preserved.

**The original bug being fixed:**

In the existing code, when `drm_pagemap_shrinker_cancel()` returns `false`, the lock is **never released** — control falls through without calling `spin_unlock`, then jumps to `out:` where `mutex_destroy` and `kfree` happen with the spinlock still held. This is a genuine bug (lock leak), so the patch is fixing a real issue, not just a static analysis aesthetic.

```c
// Original: if shrinker_cancel returns false, no unlock!
if (drm_pagemap_shrinker_cancel(dpagemap)) {
    cache->dpagemap = NULL;
    spin_unlock(&cache->lock);     // only unlocked here
    drm_pagemap_destroy(dpagemap, false);
}
// falls through to out: without unlock
```

The new code correctly ensures the lock is always released on a single path, which is good.

**Minor style note:** The negated condition `!drm_pagemap_shrinker_cancel(dpagemap)` makes the logic slightly harder to follow — "if dpagemap exists and cancel *failed*, then null out dpagemap" — but this matches the flow requested by Maarten per the v2 changelog, so it's a reasonable style trade-off.

**Summary:** The patch fixes a real spin_unlock omission bug but introduces a minor regression by dropping the `cache->dpagemap = NULL` assignment. That assignment should be added back (unconditionally, before the unlock) to maintain the original defensive clearing behavior.

---
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: 3
Reviewed: 2026-02-27T15:07:14.058937

---

This is a single patch (v2) that refactors the `drm_pagemap_cache_fini()` function to address a static analysis warning about the cache spinlock not being consistently unlocked on all paths. The change is a code cleanup / correctness fix that restructures the function to ensure `spin_unlock(&cache->lock)` is always called on a single path rather than on two separate branches.

The patch is generally correct but has one real bug.

---
Generated by Claude Code Patch Reviewer

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

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

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