Re: [PATCH v5 2/4] drm/xe/forcewake: add delayed-release state machine

On 8/12/2026 5:23 PM, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Use-After-Free in delayed-release forcewake due to devm/drmm teardown asymmetry and timer re-arming
> --
>
> --- Patch [2]: [PATCH v5 2/4] drm/xe/forcewake: add delayed-release state machine ---
> commit 7a90eb7b65ee38383fb6f1973ddea842f2c9c391
> Author: Zongyao Bai <[email protected]>
>
> drm/xe/forcewake: add delayed-release state machine
>
> Add an opt-in forcewake release path that keeps an idle domain awake
> for a short hold interval. A subsequent get can reuse the domain
> without issuing another wake request or waiting for its ACK.
>
>> diff --git a/drivers/gpu/drm/xe/xe_force_wake.c b/drivers/gpu/drm/xe/xe_force_wake.c
>> index 197e2197bd0a3..3f5013c7424c8 100644
>> --- a/drivers/gpu/drm/xe/xe_force_wake.c
>> +++ b/drivers/gpu/drm/xe/xe_force_wake.c
> [ ... ]
>
>> +static void xe_force_wake_fini(void *arg)
>> +{
>> +	struct xe_force_wake *fw = arg;
>> +	struct xe_force_wake_domain *domain;
>> +	unsigned int tmp;
>> +
>> +	for_each_fw_domain(domain, fw, tmp)
>> +		hrtimer_cancel(&domain->sleep_timer);
>> +}
>> +
>> -void xe_force_wake_init_gt(struct xe_gt *gt, struct xe_force_wake *fw)
>> +int xe_force_wake_init_gt(struct xe_gt *gt, struct xe_force_wake *fw)
>>   {
> [ ... ]
>
>>   	}
>> +
>> +	return devm_add_action_or_reset(xe->drm.dev, xe_force_wake_fini, fw);
>>   }
> [Severity: High]
> Does registering this teardown via devm create a use-after-free window?
>
> Since devm teardown (PCI unbind) happens before drmm teardown, background
> tasks like gt->ordered_wq (which are destroyed during drmm teardown) can
> still be running after xe_force_wake_fini() has executed.
>
> Also, if a timer was pending here, canceling it without putting the domain
> to sleep could leave the hardware permanently awake. Is there a need to
> clear the state or issue domain_sleep() during teardown?

The devm/drmm lifetime asymmetry is real, but this fini is not exposed 
to it in practice.
The only delayed-release caller is query_engine_cycles(),
an ioctl that holds a runtime-PM reference and runs inside a 
drm_dev_enter() section.
On remove/unbind the driver calls drm_dev_unplug() and then flushes 
forcewake on
all GTs before devres_release_all() runs xe_force_wake_fini(),
so no timer is pending by the time this runs. gt->ordered_wq only runs 
the GT reset worker,
which never uses delayed release.

The bare hrtimer_cancel() without domain_sleep() is intentional:
the power-boundary/unplug flush has already settled the domains,
and teardown is followed by device power-off/FLR, so leaving a domain 
awake is harmless here.

Zongyao

> [ ... ]
>
>> @@ -242,12 +330,24 @@ void xe_force_wake_put(struct xe_force_wake *fw, unsigned int fw_ref)
>>   
>>   	spin_lock_irqsave(&fw->lock, flags);
>>   	for_each_fw_domain_masked(domain, fw_ref, fw, tmp) {
>> +		assert_domain_state(fw, domain);
>>   		xe_gt_assert(gt, domain->ref);
>>   
>>   		if (!--domain->ref) {
>> -			sleep |= BIT(domain->id);
>> -			domain_sleep(gt, domain);
>> +			if (delayed_release || domain->delayed_release_requested) {
>> +				domain->delayed_release_requested = false;
>> +				fw->delayed_release_domains |= BIT(domain->id);
>> +				hrtimer_start(&domain->sleep_timer,
>> +					      us_to_ktime(xe->forcewake_hold_delay_us),
>> +					      HRTIMER_MODE_REL);
> [Severity: High]
> If a background task drops the last reference here after xe_force_wake_fini()
> has already run, could this unconditionally re-arm the timer?
>
> If the timer is re-armed during this devm/drmm teardown window, the timer
> callback might fire after the struct xe_gt memory has been completely freed
> by drmm, resulting in a use-after-free.
>
> Would it be safer to use drmm_add_action_or_reset() for teardown, or add a
> mechanism to ensure we don't start timers when the device is being destroyed?
This cannot re-arm the timer during teardown.
The delayed branch is only taken when either the caller used 
xe_force_wake_put_delay()
or a delayed release was previously recorded, and the sole producer of 
both is query_engine_cycles().
That ioctl is drained by drm_dev_unplug() before the all-GT forcewake 
flush, which runs before devres_release_all()/xe_force_wake_fini().
No background task (including gt->ordered_wq) drops a forcewake 
reference through the delayed path.

So there is no path where a delayed timer is re-armed after fini and 
fires against freed gt memory in this series.
I agree that switching the fini to drmm_add_action_or_reset() would 
additionally harden this against any future caller that releases 
forcewake outside the unplug/flush fence;
I'll keep that as a follow-up rather than fold it into this patch

Zongyao

>> +			} else {
>> +				sleep |= BIT(domain->id);
>> +				domain_sleep(gt, domain);
>> +			}
>> +		} else if (delayed_release) {
>> +			domain->delayed_release_requested = true;
>>   		}
>> +		assert_domain_state(fw, domain);
>>   	}