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@xxxxxxxxx> 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@xxxxxxxxxxxxxxx # v5.10+
>> Signed-off-by: Shuangpeng Bai <shuangpeng.kernel@xxxxxxxxx>
>> ---
>> 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