On 9/8/26 09:25, Tvrtko Ursulin wrote: > On 07/09/2026 12:53, Christian König wrote: >> On 9/7/26 11:54, Tvrtko Ursulin wrote: >>> >>> On 03/09/2026 12:36, Christian König wrote: >>>> i915_gem_busy_ioctl uses dma_resv_for_each_fence_unlocked() to iterate >>>> over the fences in an GEM object without holding a reference but only >>>> the RCU read side lock. >>>> >>>> What can happen here is that the GEM object is destroyed concurrently >>>> while i915_gem_busy_ioctl is still running. This won't free the GEM >>>> objects memory, but still drops all the dma_fence references. >>>> >>>> Now when dma_resv_for_each_fence_unlocked() sees a destroyed dma_fence it >>>> assumes that a new fence list was installed and re-starts the loop. >>>> >>>> But in the case of a destroyed GEM object a new fence list is never >>>> installed, only the old one freed and therefore the iteration never >>>> finishes resulting in an endless loop. >>> >>> Only i915_busy can get into this failure mode? None of the other users of >>> the iterator? >> >> Yes, at least as far as I can see. >> >> The problem is completely i915 specific because it is the only driver (I >> could find) which protects GEM objects by RCU. >> >>> Also, the reference counting series makes the fix irrelevant? >> >> No, that series just helped uncover the issue. >> >> Sashiko-bot correctly complained that i915 is dropping the new dma-resv >> reference to early resulting in potential use after free. And I was thinking >> wait a second when the dma_resv_fini() is called to early in the existing >> code then the dma_fence references are dropped to early as well... so that >> is an pre-existing bug. >> >> Before the commit mentioned in the fixes tag the i915_gem_busy_ioctl() could >> just return nonsense, but after that change it could result in an endless >> loop and that is problematic. > > Where is this sashiko report, associated with which patch I mean?
See the comment here https://patchwork.freedesktop.org/patch/748868/#comment_1379963: >> @@ -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()? > Is the dma_fence_get_rcu() inside dma_resv_iter_walk_unlocked() what triggers > the endless restarts? Yes, exactly that one. When it can't grab a fence reference it tries to get a new list, but when there isn't any new list it just tries that forever. > It's been some time since I looked at the dma-resv walks.. but fences on the > list have reference held so that can trigger either via dma_resv_fini() or > dma_resv_replace_fences(), right? No, dma_resv_replace_fences() replaces an old fence with a valid new one. So the loop never becomes endless. Same for dma_resv_reserve_fences(), here we replace a whole list with a new one and make sure that we free up the old one only after an RCU grace period. The problem happens only when drivers incorrectly call dma_resv_fini() while a call to dma_resv_for_each_fence_unlocked() is still ongoing at the same time. And that is pretty obviously a bug. > If second is true then how does i915 having the dma-resv containing object > RCU freed cause the problem? I also considered setting obj->fences to NULL in dma_resv_fini() as alternative workaround, but that would break again when I try to reference count the dma_resv object in the future. So I would need to free the dma_resv object RCU safe as well just because of the problem in i915 and that is not something I like to do when it is actually a trivial fix in i915. Regards, Christian. > > Regards, > > Tvrtko > >>>> The solution is to drop the fence references only after the RCU grace >>>> period. >>>> >>>> The fixes tag is not necessary the patch introducing the problem, but the >>>> one making it so worse that we need to address it. >>>> >>>> This problem was pointed out by Sashiko-bot. >>>> >>>> Signed-off-by: Christian König <[email protected]> >>>> Fixes: 912ff2ebd695 ("drm/i915: use the new iterator in >>>> i915_gem_busy_ioctl v2") >>>> CC: [email protected] >>>> --- >>>> drivers/gpu/drm/i915/gem/i915_gem_object.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c >>>> b/drivers/gpu/drm/i915/gem/i915_gem_object.c >>>> index 5172d3982654..9e01f8b2079a 100644 >>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c >>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c >>>> @@ -89,6 +89,7 @@ struct drm_i915_gem_object *i915_gem_object_alloc(void) >>>> void i915_gem_object_free(struct drm_i915_gem_object *obj) >>>> { >>>> + dma_resv_fini(&obj->base._resv); >>>> return kmem_cache_free(slab_objects, obj); >>>> } >>>> @@ -144,7 +145,6 @@ void __i915_gem_object_fini(struct >>>> drm_i915_gem_object *obj) >>>> { >>>> mutex_destroy(&obj->mm.get_page.lock); >>>> mutex_destroy(&obj->mm.get_dma_page.lock); >>>> - dma_resv_fini(&obj->base._resv); >>>> } >>>> /** >>> >> >
