Re: [PATCH 2/6] drm/i915/pmu: use the expiry injecting hrtimer callback

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Race condition in `i915_pmu_unregister()` allowing the timer to be rearmed after cancellation, leading to a use-after-free.
--

--- Patch [2]: [PATCH 2/6] drm/i915/pmu: use the expiry injecting hrtimer callback ---
commit 3544b866c523f970d22c7a2ae5ea50d0bc15978f
Author: Andreas Hindborg <[email protected]>

drm/i915/pmu: use the expiry injecting hrtimer callback
    
The commit converts the i915 PMU sampling timer to use the expiry injecting
callback variant. This prevents a race condition between the timer callback
forwarding itself and concurrent timer restarts when the GT is unparked.

> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> index 1c3bafda9c708..c18587e200c9f 100644
> --- a/drivers/gpu/drm/i915/i915_pmu.c
> +++ b/drivers/gpu/drm/i915/i915_pmu.c

[ ... ]

> @@ -1157,7 +1159,7 @@ void i915_pmu_register(struct drm_i915_private *i915)
>  	int ret = -ENOMEM;
>  
>  	spin_lock_init(&pmu->lock);
> -	hrtimer_setup(&pmu->timer, i915_sample, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> +	hrtimer_setup_ext(&pmu->timer, i915_sample, CLOCK_MONOTONIC, HRTIMER_MODE_REL);

[Severity: High]
This is a pre-existing issue, but is there a race condition between the PMU
unregistration and GT unparking that can lead to a use-after-free?

Looking at i915_pmu_gt_unparked() in drivers/gpu/drm/i915/i915_pmu.c, it
performs a lockless check on pmu->registered before acquiring the lock:

drivers/gpu/drm/i915/i915_pmu.c:i915_pmu_gt_unparked() {
	if (!pmu->registered)
		return;

	spin_lock_irq(&pmu->lock);

	/*
	 * Re-enable sampling timer when GPU goes active.
	 */
	if (pmu->unparked == 0)
		__i915_pmu_maybe_start_timer(pmu);
    ...
}

Concurrently, i915_pmu_unregister() sets pmu->registered to false and
cancels the timer:

drivers/gpu/drm/i915/i915_pmu.c:i915_pmu_unregister() {
    ...
	/* Disconnect the PMU callbacks */
	pmu->registered = false;

	hrtimer_cancel(&pmu->timer);
    ...
}

If a user unparks the GT (e.g., by submitting a request) while the driver is
being removed, could i915_pmu_gt_unparked() read pmu->registered as true,
wait for the lock, and then rearm the timer after i915_pmu_unregister() has
already cancelled it?

When the drm_i915_private struct is later freed, would the rearmed timer
fire on freed memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2