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?
>
> 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.
> + 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
