On 09/09/2026 08:24, Christian König wrote:
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()?
I guess the shared dma-resv part you will solve in the context of the
reference counting series.
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.
What I was wondering about is that dma_resv_replace_fences() has no RCU
protection so how does it co-operate with unlocked walks?
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.
Fix looks plausible to me but I still wonder of the implication any
driver which would use dma_resv_for_each_fence_unlocked would need to
ensure a RCU grace before calling dma_resv_fini, no? I do not see it
documented in dma_resv_for_each_fence_unlocked kerneldoc so if that is
true we should add it.
For this patch:
Reviewed-by: Tvrtko Ursulin <[email protected]>
There were some CI failures so I have queued a re-test. If things will
look reasonable I will merge it.
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);
}
/**
