Re: [PATCH v14 13/17] drm/xe/vsec: Support late bind fw information

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] drm/xe/vsec: devres race and leak in cri_late_bind_probe_work()
  on remove
- [Medium] drm/xe/vsec: lock inversion holding pmt.lock in
  cri_late_bind_probe()
--

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

drm/xe/vsec: Support late bind fw information

This commit adds delayed-work polling and firmware readiness checks so
Crescent Island PMT probe and register/telemetry accesses wait for OOBMSM
late-bind firmware to become ready.

> diff --git a/drivers/gpu/drm/xe/xe_vsec.c b/drivers/gpu/drm/xe/xe_vsec.c
> index 4ca41a5ef5027..f400a6d6ec8de 100644
> --- a/drivers/gpu/drm/xe/xe_vsec.c
> +++ b/drivers/gpu/drm/xe/xe_vsec.c
[ ... ]
> @@ -476,12 +549,50 @@ static enum xe_vsec get_platform_info(struct xe_device *xe)
>  	return vsec_platforms[xe->info.platform];
>  }
>  
> +static void cri_late_bind_probe(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_CRI)
> +		return;
> +
> +	info = &xe_vsec_info[platform];
> +	if (!info->headers)
> +		return;
> +
> +	guard(mutex)(&xe->pmt.lock);
> +
> +	info->priv_data = &xe_cri_pmt_cb;
> +	xe->soc_remapper.set_telem_region(xe, CRI_IDX_TELEM_DISCOVERY);
> +
> +	intel_vsec_register(dev, info);
> +}

[Severity: Medium]
Can holding xe->pmt.lock across intel_vsec_register() cause a lock
inversion and ABBA deadlock with PMT sysfs attribute access and removal?

CPU0                              CPU1
----                              ----
lock(&xe->pmt.lock)
  lock(&auxdev->dev.mutex)
    lock(&ep_lock)
                                  kernfs_get_active(kn->dep_map)
                                    lock(&xe->pmt.lock)
    kernfs_drain(kn->dep_map)

 *** DEADLOCK ***

CPU0: cri_late_bind_probe() -> intel_vsec_register() ->
      auxiliary_device_add() -> pmt_telem_probe() ->
      intel_pmt_dev_register() (or pmt_telem_remove() on teardown/abort)
CPU1: intel_pmt_read() -> xe_pmt_telem_read()

intel_vsec_register() synchronously probes the PMT auxiliary devices under
device_lock(&auxdev->dev) and ep_lock, while PMT sysfs callbacks acquire
xe->pmt.lock under kernfs active protection (kn->dep_map), which PMT
removal and multi-entry probe error unwind drain under &auxdev->dev.mutex
and ep_lock.

Should xe->pmt.lock be released before calling intel_vsec_register(),
matching xe_vsec_init()?

> +
> +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
> + *
> + * Return:
> + * * -ENOMEM devm acction failed
> + * * 0 vsec is a best effort.  Only devm failure is tracked
>   */
> -void xe_vsec_init(struct xe_device *xe)
> +int xe_vsec_init(struct xe_device *xe)
>  {
>  	struct intel_vsec_platform_info *info;
>  	struct device *dev = xe->drm.dev;
> @@ -489,11 +600,11 @@ void xe_vsec_init(struct xe_device *xe)
>  
>  	platform = get_platform_info(xe);
>  	if (platform == XE_VSEC_UNKNOWN)
> -		return;
> +		return 0;
>  
>  	info = &xe_vsec_info[platform];
>  	if (!info->headers)
> -		return;
> +		return 0;
>  
>  	switch (platform) {
>  	case XE_VSEC_BMG:
> @@ -503,12 +614,26 @@ void xe_vsec_init(struct xe_device *xe)
>  
>  	case XE_VSEC_CRI:
>  		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_sysctrl_is_oobmsm_fw_ready(xe)) {
> +			queue_delayed_work(system_freezable_wq, &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 canceling xe->pmt.work from a devm action leak the VSEC auxiliary
devices and cause a use-after-free on struct xe_device if driver removal or
a late probe failure occurs while cri_late_bind_probe_work() is running?

CPU0                                    kworker
xe_pci_remove()
  xe_device_remove(xe)
  xe_pm_fini(xe)
  devres_release_all(dev)
    remove_nodes(dev, ..., &todo)
    release_nodes(dev, &todo)
                                        cri_late_bind_probe_work()
                                          cri_late_bind_probe(xe)
                                            intel_vsec_register(dev, info)
                                              ...
                                                devm_add_action_or_reset(
                                                  ...,
                                                  intel_vsec_remove_aux,
                                                  ...)
      vsec_disable_late_bind_work()
        disable_delayed_work_sync(...)
  drm_dev_put(&xe->drm) -> frees xe
                                        sysfs read -> xe_pmt_telem_read(xe)
                                          <-- use-after-free

When devres_release_all() runs, remove_nodes() splices dev->devres_head
onto a local todo list before release_nodes() invokes
vsec_disable_late_bind_work().  If cri_late_bind_probe_work() is already
running on another CPU and calls intel_vsec_register(), the new
intel_vsec_remove_aux devres actions are added to the already-drained
dev->devres_head and are never executed, leaving the PMT auxiliary devices
registered after xe is freed and causing subsequent re-probes to fail with
-EBUSY.  In addition, during xe_pci_remove(), cri_late_bind_probe_work()
can still touch MMIO and call xe_pm_runtime_put() after xe_device_remove()
and xe_pm_fini() have completed.

Can xe->pmt.work be stopped in xe_device_remove() (and on xe_device_probe()
error unwind) before devres teardown begins?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110183505.694124-19-michael.j.ruhl@intel.com?part=13