Re: [PATCH v3 09/10] 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:

New issues:
- [High] Registering devm resources from a delayed work that is cancelled via a devm action causes the devm resources to be leaked and outlive the driver binding, leading to a Use-After-Free.
- [High] Holding the DRM SRCU read lock while sleeping for up to 20 seconds blocks device hot-unplug and causes userspace tasks to hang unresponsively.

Pre-existing issues:
- [High] The PMT VSEC initialization does not restrict execution on Virtual Functions (VFs), violating the restriction on VF access to physical SOC controllers.
--

--- Patch [9]: [PATCH v3 09/10] drm/xe/vsec: Support late bind fw information ---
commit baf7715ba5240a405fca0471519228b0c43828df
Author: Michael J. Ruhl <michael.j.ruhl@intel.com>

drm/xe/vsec: Support late bind fw information

This commit updates the XE PMT probe to support late bind devices by
waiting for the appropriate firmware state before proceeding with
initialization. It introduces a delayed work queue to retry the probe
and pauses telemetry reads until the firmware is ready.

> diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
> index 578d59048b39c..edc20c24137ed 100644
> --- a/drivers/gpu/drm/xe/xe_vsec.c
> +++ b/drivers/gpu/drm/xe/xe_vsec.c

[ ... ]

> @@ -272,6 +282,56 @@ static int xe_guid_decode(u32 guid, int *index, u32 *offset)

[ ... ]

> +static bool wait_for_fw(struct xe_device *xe)
> +{
> +	int retries = VSEC_LATE_BIND_RETRY;  /* wait up to 20 secs */
> +
> +	if (xe->info.platform != XE_CRESCENTISLAND)
> +		return true;
> +
> +	while (retries--) {
> +		if (xe_is_oobmsm_fw_ready(xe))
> +			return true;
> +
> +		msleep(VSEC_LATE_BIND_DELAY_MSEC);
> +	}
> +
> +	drm_warn(&xe->drm, "Late Binding failed to complete\n");
> +
> +	return false;
> +}

[ ... ]

> @@ -318,6 +378,11 @@ int xe_pmt_telem_read(struct device *dev, u32 guid, u64 *data, loff_t user_offse

[ ... ]

>  	if (!drm_dev_enter(&xe->drm, &idx))
>  		return -ENODEV;

[ ... ]

> +	if (!wait_for_fw(xe)) {
> +		ret = -ENODATA;
> +		goto runtime_exit;
> +	}

[Severity: High]
Does holding the DRM SRCU read lock while sleeping block device hot-unplug?

Earlier in xe_pmt_telem_read(), drm_dev_enter() is called, which acquires
the SRCU read lock. Inside this critical section, wait_for_fw() can sleep
for up to 20 seconds using msleep().

Could this uninterruptible sleep under SRCU prevent synchronize_srcu() from
completing during drm_dev_unplug(), causing a 20-second stall and leaving
user processes unkillable?

[ ... ]

> @@ -454,12 +532,44 @@ static enum xe_vsec get_platform_info(struct xe_device *xe)

[ ... ]

> +static void cri_late_bind_probe(struct xe_device *xe)
> +{

[ ... ]

> +	info->priv_data = &xe_cri_pmt_cb;
> +	xe->soc_remapper.set_telem_region(xe, CRI_IDX_TELEM_DISCOVERY);
> +
> +	intel_vsec_register(dev, info);
> +}
> +
> +static void vsec_disable_late_bind_work(void *arg)
> +{
> +	struct xe_device *xe = arg;
> +
> +	/*
> +	 * If was work was cancelled while it was still pending, we need to
> +	 * take care of releasing the runtime reference
> +	 */
> +	if (disable_delayed_work_sync(&xe->pmt.work))
> +		xe_pm_runtime_put(xe);
> +}

[Severity: High]
Can this sequence leak devres resources and cause a use-after-free?

When the driver unbinds, devres_release_all() detaches existing resources
to a local list and executes them. vsec_disable_late_bind_work() is
registered as a devm action and runs during this process.

If vsec_disable_late_bind_work() blocks in disable_delayed_work_sync() while
the work queue is actively executing cri_late_bind_probe(), the work thread
will call intel_vsec_register(), which allocates and attaches new devm
resources.

Since devres_release_all() only processes its detached list, will these
newly added devres nodes be added to the empty devres head, leaving them
behind to outlive the driver?

[ ... ]

> @@ -467,30 +577,44 @@ void xe_vsec_init(struct xe_device *xe)
>  {
>  	struct intel_vsec_platform_info *info;
>  	struct device *dev = xe->drm.dev;
>  	enum xe_vsec platform;
>  
>  	platform = get_platform_info(xe);
>  	if (platform == XE_VSEC_UNKNOWN)
> -		return;
> +		return 0;
>  
>  	info = &xe_vsec_info[platform];
>  	if (!info->headers)
> -		return;
> +		return 0;

[Severity: High]
This is a pre-existing issue, but does this initialization path lack an
IS_SRIOV_VF() guard before accessing physical SOC controllers?

If xe_vsec_init() is called during Virtual Function (VF) initialization, it
proceeds to access the SOC remapper and register the VSEC PMT auxiliary
device.

Since these are PF-only hardware resources, will accessing them from a VF
context result in hardware faults or MMIO timeouts?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260824162317.2450380-12-michael.j.ruhl@intel.com?part=9