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(). 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. 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); >>>>>> } >>>>>> /** >>>>> >>>> >>> >> >
