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

> On Aug 11, 2026, at 06:50, Krzysztof Karas <[email protected]> wrote:
> 
> Hi Shuangpeng,
> 
> On 2026-08-08 at 14:43:02 -0400, Shuangpeng Bai wrote:
>> heartbeat() detaches engine->heartbeat.systole with xchg() before
>> checking whether the request has completed. If the request is complete,
>> dropping the detached systole reference may free it. Concurrently,
>> idle_pulse() can observe the empty slot and publish a newer request.
>> 
>> The worker then sees a non-NULL engine->heartbeat.systole, but continues
>> to dereference its stale local rq. The newer request can therefore make
>> the shared-slot check succeed after the old request has been freed,
>> causing use-after-free accesses to emitted_jiffies, submit, and sched.
>> 
>> If the detached request is incomplete, heartbeat() can also overwrite a
>> newer request published by idle_pulse() when it unconditionally restores
>> the old pointer.
>> 
>> Use cmpxchg() both when idle_pulse() publishes a request and when
>> heartbeat() restores the detached request. Drop the detached reference
>> when restoration loses the race, and reload rq from the shared slot
>> before dereferencing it.
>> 
>> A KASAN-enabled i915 mock selftest reproduces the use-after-free before
>> this change and completes without a report after the fix.
> What is this mock selftest? Can I see it somewhere and confirm
> if it works myself? A link to a public issue would be cool too,
> something that gives more context as to how this was discovered.
> 

Hi Krzysztof,

I've put the reproducer here:

https://gist.github.com/shuangpengbai/1410664bc909e8c834311576d6242103

Tested on commit a59f57e2aa127c5354168d2ec4bac920df1be4f4 (2026-08-07).

It uses i915's mock selftest path, so no Intel GPU hardware is required. 
Please apply 1_repro.patch, build with 2_config, and boot with:
i915.mock_selftests=1 i915.igt__36__mock_heartbeat_uaf=1

Expected result:
BUG: KASAN: slab-use-after-free in heartbeat

Best,
Shuangpeng

>> 
>> Fixes: bf9bd6a5128a ("drm/i915/gt: Track the most recent pulse for the heartbeat")
>> Cc: [email protected] # v5.11+
>> Signed-off-by: Shuangpeng Bai <[email protected]>
>> ---
>> .../gpu/drm/i915/gt/intel_engine_heartbeat.c  | 22 ++++++++++++++-----
>> 1 file changed, 16 insertions(+), 6 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
>> index 6424ecce8bcb..fc6c53c93cf5 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c
>> @@ -81,10 +81,18 @@ heartbeat_create(struct intel_context *ce, gfp_t gfp)
>> 
>> static void idle_pulse(struct intel_engine_cs *engine, struct i915_request *rq)
>> {
>> + struct i915_request *systole;
>> +
>> engine->wakeref_serial = READ_ONCE(engine->serial) + 1;
>> i915_request_add_active_barriers(rq);
>> - if (!engine->heartbeat.systole && intel_engine_has_heartbeat(engine))
>> - engine->heartbeat.systole = i915_request_get(rq);
>> + if (READ_ONCE(engine->heartbeat.systole) ||
>> +     !intel_engine_has_heartbeat(engine))
>> + return;
>> +
>> + systole = i915_request_get(rq);
>> + /* The worker may have restored its detached systole in the meantime. */
>> + if (cmpxchg(&engine->heartbeat.systole, NULL, systole))
>> + i915_request_put(systole);
>> }
>> 
>> static void heartbeat_commit(struct i915_request *rq,
>> @@ -152,9 +160,11 @@ static void heartbeat(struct work_struct *wrk)
>> if (rq) {
>> if (i915_request_completed(rq))
>> i915_request_put(rq);
>> - else
>> - engine->heartbeat.systole = rq;
>> + /* Keep a newer pulse that raced with the detached systole. */
>> + else if (cmpxchg(&engine->heartbeat.systole, NULL, rq))
>> + i915_request_put(rq);
>> }
>> + rq = READ_ONCE(engine->heartbeat.systole);
>> 
>> if (!intel_engine_pm_get_if_awake(engine))
>> return;
>> @@ -163,11 +173,11 @@ static void heartbeat(struct work_struct *wrk)
>> goto out;
>> 
>> if (i915_sched_engine_disabled(engine->sched_engine)) {
>> - reset_engine(engine, engine->heartbeat.systole);
>> + reset_engine(engine, rq);
>> goto out;
>> }
>> 
>> - if (engine->heartbeat.systole) {
>> + if (rq) {
>> long delay = READ_ONCE(engine->props.heartbeat_interval_ms);
>> 
>> /* Safeguard against too-fast worker invocations */
>> 
>> base-commit: a59f57e2aa127c5354168d2ec4bac920df1be4f4
>> -- 
>> 2.43.0
>> 
> 
> The fix looks sane at first glance to me, but I'd like to see
> how it was spotted before diving in further.
> 
> -- 
> Best Regards,
> Krzysztof