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

[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

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

[Matthew Brost]

On Tue, Feb 24, 2026 at 03:48:33PM +0000, Jonathan Cavitt wrote:
> 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);

The logic is different here as 'cache->dpagemap' never gets set to NULL
under cache->lock when drm_pagemap_shrinker_cancel returns true.

Also be sure to include intel-xe list for CI too.

Matt

> -	}
>  
> -out:
>  	mutex_destroy(&cache->lookup_mutex);
>  	kfree(cache);
>  }
> -- 
> 2.43.0
> 

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

[Cavitt, Jonathan]

-----Original Message-----
From: Brost, Matthew <matthew.brost@intel.com> 
Sent: Tuesday, February 24, 2026 3:12 PM
To: Cavitt, Jonathan <jonathan.cavitt@intel.com>
Cc: dri-devel@lists.freedesktop.org; Gupta, Saurabhg <saurabhg.gupta@intel.com>; Zuo, Alex <alex.zuo@intel.com>; thomas.hellstrom@linux.intel.com; maarten.lankhorst@linux.intel.com
Subject: Re: [PATCH v2] drm/pagemap_util: Ensure proper cache lock management on free
> 
> On Tue, Feb 24, 2026 at 03:48:33PM +0000, Jonathan Cavitt wrote:
> > 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);
> 
> The logic is different here as 'cache->dpagemap' never gets set to NULL
> under cache->lock when drm_pagemap_shrinker_cancel returns true.

I forgot to add this back in as a part of the v2 request, although in this case,
cache->dpagemap = NULL can just be assigned as such without checking the
return of drm_pagemap_shrinker_cancel.
I'll fix this for V3 and add the intel-xe list at that time.  Thank you.

-Jonathan Cavitt

> 
> Also be sure to include intel-xe list for CI too.
> 
> Matt
> 
> > -	}
> >  
> > -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: 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

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