Re: [PATCH 03/10] drm/gem: Add helper for drm_gem_object resv assignment

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Direct assignments to `obj->resv` bypass `drm_gem_object_set_resv()`, causing reference counting underflows and memory leaks.
- [High] i915 driver open-coded GEM teardown misses `dma_resv_put(obj->base.resv)`, permanently leaking the reservation object references for all i915 GEM objects.
- [High] Memory leaks on the error path in virtgpu_gem_prime_import().
- [High] Calling drm_gem_object_set_resv() under a spinlock can trigger dma_resv and fence release callbacks, causing potential deadlocks.
--

commit ad191ee55ce616a6f881f13681e85331c2eeb4ec
Author: Christian König <ckoenig.leichtzumerken@gmail.com>

drm/gem: Add helper for drm_gem_object resv assignment
    
Introduce drm_gem_object_set_resv() to safely assign reservation objects
to GEM objects with proper reference counting. This helper replaces all
direct assignments to drm_gem_object.resv throughout the DRM subsystem.

[ ... ]

> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 88d60fb17f43b..bc87cc8fbeffa 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -230,7 +230,7 @@ void drm_gem_private_object_init(struct drm_device *dev,
>  	mutex_init(&obj->gpuva.lock);
>  	dma_resv_init(&obj->_resv);
>  	if (!obj->resv)
> -		obj->resv = &obj->_resv;
> +		obj->resv = dma_resv_get(&obj->_resv);

[Severity: Critical]
With this change, obj->resv holds a reference that must be released. It looks
like some drivers still use direct assignment (e.g., obj->resv = dma_buf->resv)
which will leak this initial reference and fail to acquire a reference on the
new object.

For example, these assignments appear to remain:
- drivers/accel/amdxdna/amdxdna_gem.c: gobj->resv = dma_buf->resv;
- drivers/accel/ivpu/ivpu_gem.c: obj->resv = dma_buf->resv;
- drivers/accel/qaic/qaic_data.c: obj->resv = dma_buf->resv;
- drivers/gpu/drm/i915/gt/intel_gtt.c: obj->base.resv = i915_vm_resv_get(vm);
- rust/kernel/drm/gem/shmem.rs: unsafe { (*obj.obj.get()).base.resv = parent_resv.raw_dma_resv() };

When these GEM objects are destroyed, drm_gem_private_object_fini() will call
dma_resv_put() on the external reservation object. Since a reference was never
acquired during assignment, could this cause a use-after-free? Should these
callers also be updated to use drm_gem_object_set_resv()?

>  
>  	drm_gem_gpuva_init(obj);
>  
> @@ -249,11 +249,40 @@ void drm_gem_private_object_fini(struct drm_gem_object *obj)
>  {
>  	WARN_ON(obj->dma_buf);
>  
> +	dma_resv_put(obj->resv);
>  	dma_resv_put(&obj->_resv);
>  	mutex_destroy(&obj->gpuva.lock);
>  }

[Severity: High]
This correctly releases the reference for objects using the standard teardown
path. However, the i915 driver open-codes GEM teardown and doesn't call
drm_gem_private_object_fini():

drivers/gpu/drm/i915/gem/i915_gem_object.c:__i915_gem_object_fini() {
    mutex_destroy(&obj->mm.get_page.lock);
    mutex_destroy(&obj->mm.get_dma_page.lock);
    dma_resv_put(&obj->base._resv);
}

Does __i915_gem_object_fini() also need a dma_resv_put(obj->base.resv) call to
prevent permanently leaking the reservation object references for i915 GEM
objects?

[ ... ]

> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index ef56c18ded1b8..216c83bb0a0bb 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -209,7 +209,7 @@ static int ttm_bo_individualize_resv(struct ttm_buffer_object *bo)
>  		 * the resv object while holding the lru_lock.
>  		 */
>  		spin_lock(&bo->bdev->lru_lock);
> -		bo->base.resv = &bo->base._resv;
> +		drm_gem_object_set_resv(&bo->base, &bo->base._resv);
>  		spin_unlock(&bo->bdev->lru_lock);
>  	}

[Severity: High]
Calling drm_gem_object_set_resv() here drops the reference to the old
reservation object while holding bo->bdev->lru_lock. If this drops the last
reference to a shared reservation object, dma_resv_release() will be invoked
synchronously under the spinlock.

Since dma_resv_release() frees the fence list and drops all attached fences,
could this invoke fence release callbacks that attempt to acquire the same
lru_lock (e.g., via ttm_bo_release()), leading to a deadlock?

[ ... ]

> diff --git a/drivers/gpu/drm/virtio/virtgpu_prime.c b/drivers/gpu/drm/virtio/virtgpu_prime.c
> index 216c77cd0d21b..79964b4362439 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_prime.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_prime.c
> @@ -361,7 +361,7 @@ struct drm_gem_object *virtgpu_gem_prime_import(struct drm_device *dev,
>  	INIT_LIST_HEAD(&bo->restore_node);
>  
>  	obj = &bo->base.base;
> -	obj->resv = buf->resv;
> +	drm_gem_object_set_resv(obj, buf->resv);
>  	obj->funcs = &virtgpu_gem_dma_buf_funcs;
>  	drm_gem_private_object_init(dev, obj, buf->size);
>  

[Severity: High]
If the subsequent calls to drm_gem_private_object_init() or
dma_buf_dynamic_attach() fail, the error paths just call kfree(bo):

	ret = drm_gem_private_object_init(dev, obj, buf->size);
	if (ret) {
		kfree(bo);
		return ERR_PTR(ret);
	}

	attach = dma_buf_dynamic_attach(buf, dev->dev,
					&virtgpu_dma_buf_attach_ops, obj);
	if (IS_ERR(attach)) {
		kfree(bo);
		return ERR_CAST(attach);
	}

Does this leak the new reference acquired by drm_gem_object_set_resv() to
buf->resv, as well as the resources initialized by drm_gem_private_object_init()
in the latter case?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260827124910.2245-1-christian.koenig@amd.com?part=3