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

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

[Kazuki Hanai]

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;
+		}
+
 		vc4_bo_remove_from_purgeable_pool_locked(bo);
 
 		/* 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) {

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

[Maíra Canal]

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) {