Re: [PATCH v7 17/20] drm/xe/vsec: Support late bind fw information

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Use-After-Free and Resource Leak due to devres addition race during teardown.
- [Low] Build Error: The function `xe_is_oobmsm_fw_ready` is called but neither declared nor defined anywhere in the kernel tree.
--

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

drm/xe/vsec: Support late bind fw information

This patch adds support for late bind firmware information for CRI FW, which
is loaded on power on. It updates the XE PMT probe and access routines to
check for late bind devices, and uses a delayed workqueue to wait for the
FW state before probing or accessing PMT telemetry.

> diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
> index 58421e3157a8f..bf75adb73f27e 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: Low]
Does this code compile successfully? It appears xe_is_oobmsm_fw_ready() is
called here (and in wait_for_fw() and xe_vsec_init()) but it does not seem
to be declared or defined anywhere in the kernel tree.

> +		cri_late_bind_probe(xe);
> +		xe_pm_runtime_put(xe);
> +		return;
> +	}

[ ... ]

> +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))
> +		xe_pm_runtime_put(xe);
> +}
> +
>  /**
>   * xe_vsec_init - Initialize resources and add intel_vsec auxiliary
>   * interface
>   * @xe: valid xe instance
>   */
> -void xe_vsec_init(struct xe_device *xe)
> +int 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));
> +			return devm_add_action_or_reset(xe->drm.dev,
> +							vsec_disable_late_bind_work,
> +							xe);
> +		}

[Severity: High]
Can this lead to a devres leak and use-after-free during driver teardown?

If the driver teardown is initiated concurrently, devres_release_all() will
splice the devres list to a local todo list before executing the cleanup
actions. 

When vsec_disable_late_bind_work() executes from that local list, it blocks
on the delayed work using disable_delayed_work_sync(). If the worker thread
(cri_late_bind_probe_work) is already executing, it proceeds to call
intel_vsec_register(), which adds new devm actions to the parent device.

Since the parent devres list was already spliced, these newly added devm
actions sit on the empty devres_head and are never processed. Could this
cause the auxiliary device to be leaked and accessed after the parent
device is freed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909154739.1515514-22-michael.j.ruhl@intel.com?part=17