Re: [PATCH] drm/i915: fix incorrect RCU teardown order

["Christian König" <christian.koenig@amd.com>]

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 <christian.koenig@amd.com>
>>>> Fixes: 912ff2ebd695 ("drm/i915: use the new iterator in i915_gem_busy_ioctl v2")
>>>> CC: stable@vger.kernel.org
>>>> ---
>>>>    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);
>>>>    }
>>>>      /**
>>>
>>
>