On Wed, 6 May 2026 16:40:22 +0100
Steven Price <[email protected]> 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 <[email protected]>
> > Closes: https://gitlab.freedesktop.org/panfrost/linux/-/work_items/86
> > Signed-off-by: Boris Brezillon <[email protected]>
> > Reviewed-by: Chia-I Wu <[email protected]>
>
> With minor typos fixed
>
> Reviewed-by: Steven Price <[email protected]>
>
> 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);
> >
>
