Hi Krzysztof,
> On Aug 11, 2026, at 05:37, Krzysztof Karas <[email protected]> wrote: > > Hi Shuangpeng, > > On 2026-08-09 at 13:36:46 -0400, Shuangpeng Bai wrote: >> active_engine() walks timeline->requests in reverse under RCU and takes a >> temporary reference before inspecting each request. However, it drops that >> reference in the loop body before list_for_each_entry_reverse() advances >> the cursor. >> >> Concurrent retirement can unlink the same request and drop its base >> reference while active_engine() holds the temporary reference. The put in >> active_engine() may then be final, freeing or recycling the request before >> the loop step reads rq->link.prev. SLAB_TYPESAFE_BY_RCU does not defer that >> reuse. >> >> Open-code the reverse walk and cache the previous request while the current >> request is still referenced. The next request remains protected by >> i915_request_get_rcu() and validated against the timeline before use. >> >> An i915 mock selftest forced retirement between the active check and cursor >> advance. The vulnerable tree reached the final request release and >> kmem_cache_free(), while the fixed tree completed the same ordering without >> accessing rq after the put. > What mock selftest are you referring to? > Thanks for looking. The "i915 mock selftest" I referred to was a local temporary mock selftest. Because generic QEMU does not expose an Intel i915 PCI device, I used the mock engine/request infrastructure to validate only the ordering in active_engine(). In short, the test forces active_engine() to hold the temporary request reference while a worker on the other CPU retires the same request, so the subsequent i915_request_put(rq) in active_engine() becomes the final put. That was only meant to show that this interleaving is feasible. I can clarify that wording in v2. >> >> Fixes: 3cfea8c97c93 ("drm/i915/gem: Hold request reference for canceling an >> active context") >> Cc: [email protected] # v5.10+ >> Signed-off-by: Shuangpeng Bai <[email protected]> >> --- >> drivers/gpu/drm/i915/gem/i915_gem_context.c | 10 ++++++++-- >> 1 file changed, 8 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c >> b/drivers/gpu/drm/i915/gem/i915_gem_context.c >> index c58ffa5a8fa6..ff5c892a0176 100644 >> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c >> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c >> @@ -1361,7 +1361,7 @@ static bool __cancel_engine(struct intel_engine_cs >> *engine) >> static struct intel_engine_cs *active_engine(struct intel_context *ce) >> { >> struct intel_engine_cs *engine = NULL; >> - struct i915_request *rq; >> + struct i915_request *rq, *prev; >> >> if (intel_context_has_inflight(ce)) >> return intel_context_inflight(ce); >> @@ -1375,7 +1375,8 @@ static struct intel_engine_cs *active_engine(struct >> intel_context *ce) >> * (and onto a new timeline->requests list). >> */ >> rcu_read_lock(); >> - list_for_each_entry_reverse(rq, &ce->timeline->requests, link) { >> + rq = list_last_entry(&ce->timeline->requests, typeof(*rq), link); >> + while (!list_entry_is_head(rq, &ce->timeline->requests, link)) { >> bool found; >> >> /* timeline is already completed upto this point? */ >> @@ -1387,9 +1388,14 @@ static struct intel_engine_cs *active_engine(struct >> intel_context *ce) >> if (likely(rcu_access_pointer(rq->timeline) == ce->timeline)) >> found = i915_request_active_engine(rq, &engine); >> >> + /* Cache the cursor before the put, which may release rq. */ >> + if (!found) > You could skip this check here and unconditionally set "prev". > Its value is going to be used only once if found == false anyway. > Thanks for your comment. I'll simplify that by assigning prev unconditionally before i915_request_put(). Please let me know if this addresses your question. If it does, I'll send v2 with the wording clarified and prev assigned unconditionally before i915_request_put(). Thanks, Shuangpeng >> + prev = list_prev_entry(rq, link); >> i915_request_put(rq); >> if (found) >> break; >> + >> + rq = prev; >> } >> rcu_read_unlock(); >> >> >> base-commit: a59f57e2aa127c5354168d2ec4bac920df1be4f4 >> -- >> 2.43.0 >> > > -- > Best Regards, > Krzysztof
