Re: [PATCH] drm/i915/gem: Fix request use-after-free in active_engine()

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