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 <[email protected]> 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/[email protected]?part=3
