Re: [PATCH 1/3] drm/panthor: Don't use the racy drm_gem_lru_remove() helper

[Boris Brezillon <boris.brezillon@collabora.com>]

On Wed, 6 May 2026 16:40:22 +0100
Steven Price <steven.price@arm.com> wrote:

> On 06/05/2026 13:16, Boris Brezillon wrote:
> > drm_gem_lru_remove() dereference stores drm_gem_object::lru in a local
> > variable that's then dereferenced to acquire the LRU lock. Because this
> > assignment in done without the LRU lock held, it can race with  
> s/in/is/     ^^
> > drm_gem_lru_scan() where drm_gem_object::lru is temporarily assigned
> > a stack-allcated LRU that goes away when leaving the function. By
> > the time we dereference this local lru variable, the object might already
> > be gone.
> > 
> > It feels like drm_gem_lru_move_tail() was never meant to be used this
> > way, because there's no easy way we can avoid this race unless we defer
> > the locking to the caller. Let's add an explicit LRU for unreclaimable
> > BOs instead, and have all BOs added to this LRU at creation time.
> > 
> > Fixes: fb42964e2a76 ("drm/panthor: Add a GEM shrinker")
> > Reported-by: Chia-I Wu <olvaffe@gmail.com>
> > Closes: https://gitlab.freedesktop.org/panfrost/linux/-/work_items/86
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > Reviewed-by: Chia-I Wu <olvaffe@gmail.com>  
> 
> With minor typos fixed
> 
> Reviewed-by: Steven Price <steven.price@arm.com>
> 
> Although an alternative would be to expose drm_gem_lru_remove_locked()
> in some form (maybe a wrapper which requires passing in the lock?)

I considered that too, but I thought it was less invasive to just have
a default LRU to start in at creation time, and end in there's nothing
left to reclaim. It's also what MSM does, so I figured I'd do that too.

> 
> > ---
> >  drivers/gpu/drm/panthor/panthor_device.h | 10 ++++++++++
> >  drivers/gpu/drm/panthor/panthor_gem.c    |  5 ++++-
> >  2 files changed, 14 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h
> > index 4e4607bca7cc..45b71546f83c 100644
> > --- a/drivers/gpu/drm/panthor/panthor_device.h
> > +++ b/drivers/gpu/drm/panthor/panthor_device.h
> > @@ -190,6 +190,16 @@ struct panthor_device {
> >  		/** @reclaim.lock: Lock protecting all LRUs */
> >  		struct mutex lock;
> >  
> > +		/**
> > +		 * @reclaim.unreclaimable: unreclaimable BOs
> > +		 *
> > +		 * Either the BO is unreclaimable because it has no pages allocated,
> > +		 * or it's unreclaimable because pages are pinned.
> > +		 *
> > +		 * All BOs start in that list at creation time.  
> s/that/this/                        ^^^^
> 
> Thanks,
> Steve
> 
> > +		 */
> > +		struct drm_gem_lru unreclaimable;
> > +
> >  		/**
> >  		 * @reclaim.unused: BOs with unused pages
> >  		 *
> > diff --git a/drivers/gpu/drm/panthor/panthor_gem.c b/drivers/gpu/drm/panthor/panthor_gem.c
> > index 13295d7a593d..8e31740126e7 100644
> > --- a/drivers/gpu/drm/panthor/panthor_gem.c
> > +++ b/drivers/gpu/drm/panthor/panthor_gem.c
> > @@ -204,7 +204,7 @@ void panthor_gem_update_reclaim_state_locked(struct panthor_gem_object *bo,
> >  		drm_gem_lru_move_tail(&ptdev->reclaim.gpu_mapped_shared, &bo->base);
> >  		break;
> >  	case PANTHOR_GEM_UNRECLAIMABLE:
> > -		drm_gem_lru_remove(&bo->base);
> > +		drm_gem_lru_move_tail(&ptdev->reclaim.unreclaimable, &bo->base);
> >  		break;
> >  	default:
> >  		drm_WARN(&ptdev->base, true, "invalid GEM reclaim state (%d)\n", new_state);
> > @@ -994,6 +994,7 @@ static struct panthor_gem_object *
> >  panthor_gem_create(struct drm_device *dev, size_t size, uint32_t flags,
> >  		   struct panthor_vm *exclusive_vm, u32 usage_flags)
> >  {
> > +	struct panthor_device *ptdev = container_of(dev, struct panthor_device, base);
> >  	struct panthor_gem_object *bo;
> >  	int ret;
> >  
> > @@ -1026,6 +1027,7 @@ panthor_gem_create(struct drm_device *dev, size_t size, uint32_t flags,
> >  	}
> >  
> >  	panthor_gem_debugfs_set_usage_flags(bo, usage_flags);
> > +	drm_gem_lru_move_tail(&ptdev->reclaim.unreclaimable, &bo->base);
> >  	return bo;
> >  
> >  err_put:
> > @@ -1551,6 +1553,7 @@ int panthor_gem_shrinker_init(struct panthor_device *ptdev)
> >  		return ret;
> >  
> >  	INIT_LIST_HEAD(&ptdev->reclaim.vms);
> > +	drm_gem_lru_init(&ptdev->reclaim.unreclaimable, &ptdev->reclaim.lock);
> >  	drm_gem_lru_init(&ptdev->reclaim.unused, &ptdev->reclaim.lock);
> >  	drm_gem_lru_init(&ptdev->reclaim.mmapped, &ptdev->reclaim.lock);
> >  	drm_gem_lru_init(&ptdev->reclaim.gpu_mapped_shared, &ptdev->reclaim.lock);
> >   
>