Re: [PATCH v5 15/18] drm/xe/vsec: Support late bind fw information

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Asynchronous `devm` registration race causes memory leak and Use-After-Free of the auxiliary device.
- [High] Asynchronous runtime PM put during devm teardown leaves hardware in an incorrect power state.
- [High] Delayed work accesses hardware MMIO during system suspend.
--

commit 1b67a780c3fbc619f441f6dfdc210d11c50f6476
Author: Michael J. Ruhl <michael.j.ruhl@intel.com>

drm/xe/vsec: Support late bind fw information

CRI FW is loaded on power on. Because of this, access to the FW cannot
be done until it is running. Update the XE PMT probe and access to check
for late bind devices, verify, and wait for the appropriate FW state
before probe or access.

> diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
> index 336c4b7d284bc..48cacd23b071e 100644
> --- a/drivers/gpu/drm/xe/xe_vsec.c
> +++ b/drivers/gpu/drm/xe/xe_vsec.c
[ ... ]
> @@ -281,6 +291,48 @@ static int xe_guid_decode(u32 guid, int *index, u32 *offset)
>  	return -ENODEV;
>  }
>  
> +static void cri_late_bind_probe_work(struct work_struct *work)
> +{
> +	struct xe_device *xe = container_of(work, struct xe_device, pmt.work.work);
> +
> +	if (xe_is_oobmsm_fw_ready(xe)) {
[Severity: High]
Does this code access hardware MMIO during system suspend?

If a system suspend (e.g., S3) is initiated while the worker is actively
polling, the device is forced into a suspended state (D3) by the system PM
core, bypassing the runtime PM usage count held by the worker.

Because the work is on a non-freezable workqueue and not explicitly
cancelled in the driver's system suspend callbacks, the worker can wake up
while the hardware is powered down and call xe_is_oobmsm_fw_ready().
Reading MMIO from a device in the D3 state causes master aborts, unhandled
PCI errors, or silent 0xFFFFFFFF reads leading to undefined behavior.

> +		cri_late_bind_probe(xe);
> +		xe_pm_runtime_put(xe);
> +		return;
> +	}
[ ... ]
> @@ -464,12 +534,44 @@ static enum xe_vsec get_platform_info(struct xe_device *xe)
[ ... ]
> +static void cri_late_bind_probe(struct xe_device *xe)
> +{
[ ... ]
> +	intel_vsec_register(dev, info);
[Severity: High]
Can this asynchronous devm registration leak the auxiliary device?

Because intel_vsec_register() attaches its own devres actions to the parent
device (xe->drm.dev), calling it from a delayed worker can race with the
parent device teardown.

If the worker evaluates the firmware as ready and calls
intel_vsec_register() while the parent's devres list has already been
spliced during unbinding, these new devm actions are left behind on the
empty devres_head and are never executed. This causes a permanent memory
leak of the auxiliary device, and subsequent accesses can lead to a
use-after-free since the parent xe_device has been freed.

> +}
> +
> +static void vsec_disable_late_bind_work(void *arg)
> +{
> +	struct xe_device *xe = arg;
> +
> +	/*
> +	 * If the queued work is canceled, the runtime reference needs to be
> +	 * released here.
> +	 */
> +	if (disable_delayed_work_sync(&xe->pmt.work))
[Severity: High]
Does blocking to cancel the work here open the devres race window mentioned
above?

Since devres_release_all() has already spliced the devres list by the time
this devm action executes, any concurrent worker that bypassed the
cancellation check will attach its new devres actions to the already
spliced parent list.

> +		xe_pm_runtime_put(xe);
[Severity: High]
Could this asynchronous PM put leave the hardware active?

Using xe_pm_runtime_put() (which executes asynchronously) immediately before
device removal allows the pending async idle/suspend work to be cancelled
by the core teardown's pm_runtime_disable().

According to the PM subsystem guidelines, this requires a synchronous put
(like pm_runtime_put_sync()) to ensure the hardware is suspended before
teardown continues.

> +}
[ ... ]
> @@ -477,30 +579,45 @@ void xe_vsec_init(struct xe_device *xe)
[ ... ]
>  	case XE_VSEC_CRI:
>  		if (IS_SRIOV_VF(xe))
> -			return;
> +			return 0;
> +
>  		xe->pmt.base_offset = CRI_PMT_OFFSET;
> +
> +		xe->pmt.retry_count = 0;
> +		INIT_DELAYED_WORK(&xe->pmt.work, cri_late_bind_probe_work);
> +
> +		xe_pm_runtime_get_noresume(xe);
> +		if (!xe_is_oobmsm_fw_ready(xe)) {
> +			schedule_delayed_work(&xe->pmt.work,
> +					      msecs_to_jiffies(VSEC_LATE_BIND_DELAY_MSEC));
[Severity: High]
Is it safe to schedule this work on a non-freezable workqueue without
suspend hooks?

As noted earlier, scheduling this on the default workqueue allows the
worker to execute during system suspend. This permits hardware MMIO
accesses when the device is in a suspended state (D3).

> +			return devm_add_action_or_reset(xe->drm.dev,
> +							vsec_disable_late_bind_work,
> +							xe);
> +		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903201002.1064889-20-michael.j.ruhl@intel.com?part=15