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

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.

> 
> 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