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

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

[Shuangpeng Bai]

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.

Fixes: 3cfea8c97c93 ("drm/i915/gem: Hold request reference for canceling an active context")
Cc: stable@vger.kernel.org # v5.10+
Signed-off-by: Shuangpeng Bai <shuangpeng.kernel@gmail.com>
---
 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)
+			prev = list_prev_entry(rq, link);
 		i915_request_put(rq);
 		if (found)
 			break;
+
+		rq = prev;
 	}
 	rcu_read_unlock();
 

base-commit: a59f57e2aa127c5354168d2ec4bac920df1be4f4
-- 
2.43.0

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

[Krzysztof Karas]

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: stable@vger.kernel.org # v5.10+
> Signed-off-by: Shuangpeng Bai <shuangpeng.kernel@gmail.com>
> ---
>  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

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

[Shuangpeng]

Hi Krzysztof,



> On Aug 11, 2026, at 05:37, Krzysztof Karas <krzysztof.karas@intel.com> 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: stable@vger.kernel.org # v5.10+
>> Signed-off-by: Shuangpeng Bai <shuangpeng.kernel@gmail.com>
>> ---
>> 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