Re: [PATCH] drm/vc4: Hold a GEM reference while purging userspace BOs

["Maíra Canal" <mcanal@igalia.com>]

Hi Kazuki,

On 30/08/26 10:07, Kazuki Hanai wrote:
> vc4_bo_userspace_cache_purge() removes a BO from the purgeable list and
> then drops the list lock before taking the BO madvise lock. A concurrent
> final GEM put can therefore release the BO while the purge path still
> holds only a raw pointer.
> 
> Acquire a non-zero GEM reference while the purgeable list lock still
> protects the lookup. If final release has already started, drop the list
> lock so the release path can remove the entry and retry. Put the temporary
> reference after dropping the madvise lock to avoid recursing into final
> release while that lock is held.
> 
> Fixes: b9f19259b84d ("drm/vc4: Add the DRM_IOCTL_VC4_GEM_MADVISE ioctl")
> Cc: stable@vger.kernel.org
> Signed-off-by: Kazuki Hanai <hnkz.64@gmail.com>
> ---
>   drivers/gpu/drm/vc4/vc4_bo.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_bo.c b/drivers/gpu/drm/vc4/vc4_bo.c
> index 49ea2ed0996b4..5289e69f3345f 100644
> --- a/drivers/gpu/drm/vc4/vc4_bo.c
> +++ b/drivers/gpu/drm/vc4/vc4_bo.c
> @@ -319,6 +319,19 @@ static void vc4_bo_userspace_cache_purge(struct drm_device *dev)
>   		struct drm_gem_object *obj = &bo->base.base;
>   		size_t purged_size = 0;
>   
> +		/*
> +		 * The last GEM reference can be dropped concurrently. Its free
> +		 * callback removes the BO from this list before releasing it, so
> +		 * the list lock makes an unless-zero get safe. Never carry only a
> +		 * raw list pointer across the lock drop below.
> +		 */
> +		if (!kref_get_unless_zero(&obj->refcount)) {
> +			mutex_unlock(&vc4->purgeable.lock);
> +			cond_resched();
> +			mutex_lock(&vc4->purgeable.lock);
> +			continue;
> +		}

Hum, your fix doesn't look correct to me. First, I don't understand why
you are using something like cond_resched() here. Next, with this
approach, the BO with refcount=0 will never be removed from the list.

> +
>   		vc4_bo_remove_from_purgeable_pool_locked(bo);

What you could do instead is calling:

if (!kref_get_unless_zero(&obj->refcount))
       continue;

here.

Best regards,
- Maíra

>   
>   		/* Release the purgeable lock while we're purging the BO so
> @@ -345,6 +358,7 @@ static void vc4_bo_userspace_cache_purge(struct drm_device *dev)
>   			vc4_bo_purge(obj);
>   		}
>   		mutex_unlock(&bo->madv_lock);
> +		drm_gem_object_put(obj);
>   		mutex_lock(&vc4->purgeable.lock);
>   
>   		if (purged_size) {