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?)
> ---
> 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);
>
