On 16/09/2026 13:53, Christian König wrote:
On 9/16/26 11:51, Tvrtko Ursulin wrote:
...
[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.
Yes, of course.
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?
Hui?
dma_resv_replace_fences() asserts on holding the reservation lock and uses
dma_resv_list_set() to update the entry with the replacement.
dma_resv_list_set() then uses RCU_INIT_POINTER().
Never mind, on retrospect I think it is fine. I wasn't grasping the
consequences of the outer rcu_read_lock() in i915_gem_busy_ioctl() to
how this interacts with the restart detection.
Could be that using rcu_assign_pointer() instead of RCU_INIT_POINTER() would be
better, but apart from that I don't see why that shouldn't be correctly RCU
protected.
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.
Please let me know when it landed in drm-tip since I want to rebase on that.
It passed CI and is in drm-tip now.
Regards,
Tvrtko
Thanks,
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);
}
/**
