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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Thu, Aug 13, 2026 at 02:28:27PM -0700, Bai, Zongyao wrote:
> 
> On 8/12/2026 5:23 PM, sashiko-bot@kernel.org 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 <zongyao.bai@intel.com>
> > 
> > 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.

I'm honestly not comfortable with this dismissal here.
It is not only a left-over domain awake, but some potential UAF.

Why did you chose the hrtimer instead of a delayed workqueue?
The workqueue you can flush and wait in the finish.

> 
> 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);
> > >   	}