Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Critical] Synchronous freeing of shared dma_resv objects causes Use-After-Free for concurrent lockless RCU readers. - [High] Drivers like i915 leak dynamically allocated dma_resv objects when destroying GEM objects because they bypass standard DRM cleanup routines. --
commit d696682993c0c4cb40a8a895d00e6e7b158307dd Author: Christian König <[email protected]> dma-buf: Inline dma_resv_init and remove allocated flag This commit migrates all dma_resv object initializations directly into dma_resv_alloc() and removes the dma_resv_init() function. It also makes dma_resv objects strictly dynamically allocated, removing the allocated flag and simplifying the cleanup in dma_resv_release() to an unconditional kfree(). > diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c > index 1251b18e1b540..b553ad9c87445 100644 > --- a/drivers/dma-buf/dma-resv.c > +++ b/drivers/dma-buf/dma-resv.c [ ... ] > -/** > - * dma_resv_init - initialize a reservation object > - * @obj: the reservation object > - */ > -void dma_resv_init(struct dma_resv *obj) > -{ > - kref_init(&obj->refcount); > - obj->allocated = false; > - ww_mutex_init(&obj->lock, &reservation_ww_class); > - > - RCU_INIT_POINTER(obj->fences, NULL); > -} > -EXPORT_SYMBOL(dma_resv_init); [Severity: High] Does replacing the static initialization with dynamic allocation via dma_resv_alloc() cause memory leaks in drivers that bypass standard DRM cleanup routines? Drivers like i915 use custom cleanup logic. For instance, i915_gem_object_alloc() calls drm_gem_private_object_init(), which dynamically allocates obj->resv via dma_resv_alloc(). When the object is freed, i915 uses __i915_gem_free_object(), which bypasses drm_gem_object_release() and only calls __i915_gem_object_fini(). Since dma_resv_fini() was removed from __i915_gem_object_fini() and not replaced with a call to dma_resv_put(obj->base.resv), the reference is never dropped. Could this permanently leak the dma_resv allocation on every GEM object destruction? [ ... ] > @@ -159,8 +146,7 @@ static void dma_resv_release(struct kref *kref) > > dma_resv_list_free(rcu_dereference_protected(obj->fences, true)); > ww_mutex_destroy(&obj->lock); > - if (obj->allocated) > - kfree(obj); > + kfree(obj); > } [Severity: Critical] Does this synchronous free cause a use-after-free for concurrent lockless RCU readers? In the i915 driver, GEM objects are destroyed using call_rcu() (via __i915_gem_free_object_rcu). Lockless readers like i915_gem_busy_ioctl() look up objects under rcu_read_lock() and access obj->base.resv. If an object shares its resv instance (e.g., via obj->shares_resv_from with an i915_address_space), __i915_gem_free_object() drops the lock reference synchronously via i915_vm_resv_put(). If that drops the last reference, dma_resv_release() synchronously frees the memory here. However, the GEM object itself remains valid during the RCU grace period. Can concurrent RCU readers dereference the already freed dma_resv pointer when calling dma_resv_iter_begin(&cursor, obj->base.resv, ...) in i915_gem_busy_ioctl()? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=10
