Re: [PATCH v2 2/9] drm/xe/hw_error: Initialize hw_error_work per tile

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] drm/xe/hw_error: devres teardown race in xe_hw_error_work_disable()
- [Medium] drm/xe/hw_error: unchecked devm_add_action_or_reset() in xe_hw_error_init()
- [Medium] drm/xe/hw_error: uninitialized delayed_work in xe_hw_error_work_disable()
--

commit eb5d3c525869285977a7b6ae0cced93702f4f61e
Author: Raag Jadav <raag.jadav@intel.com>

drm/xe/hw_error: Initialize hw_error_work per tile

This commit initializes tile->hw_error_work across all tiles in
xe_hw_error_init() rather than only on the root tile, and registers a
xe_hw_error_work_disable() callback with devm_add_action_or_reset() to
disable the delayed work on teardown.

> diff --git a/drivers/gpu/drm/xe/xe_hw_error.c b/drivers/gpu/drm/xe/xe_hw_error.c
> index 41ddd997478b8..f7799c3c08aac 100644
> --- a/drivers/gpu/drm/xe/xe_hw_error.c
> +++ b/drivers/gpu/drm/xe/xe_hw_error.c
> @@ -537,6 +537,23 @@ static void process_hw_errors(struct xe_device *xe)
>  	}
>  }
>  
> +/**
> + * xe_hw_error_work_disable - Disable hw error work
> + * @arg: xe device instance
> + *
> + * This function disables the delayed work for hardware error handling
> + * for all tiles.
> + */
> +void xe_hw_error_work_disable(void *arg)
> +{
> +	struct xe_device *xe = arg;
> +	struct xe_tile *tile;
> +	u8 id;
> +
> +	for_each_tile(tile, xe, id)
> +		disable_delayed_work_sync(&tile->hw_error_work);
> +}

[Severity: Medium]
Should xe_hw_error_work_disable() also check !IS_DGFX(xe) || IS_SRIOV_VF(xe)
before calling disable_delayed_work_sync()?

In xe_hw_error_init(), tile->hw_error_work is only initialized via
INIT_DELAYED_WORK() when IS_DGFX(xe) && !IS_SRIOV_VF(xe):

drivers/gpu/drm/xe/xe_hw_error.c:xe_hw_error_init()
	if (!IS_DGFX(xe) || IS_SRIOV_VF(xe))
		return;

	for_each_tile(tile, xe, id)
		INIT_DELAYED_WORK(&tile->hw_error_work, hw_error_work);

However, xe_hw_error_work_disable() is exported in drivers/gpu/drm/xe/xe_hw_error.h
without that guard. In the commit "drm/xe/xe_ras: Disable hw error worker when
AER handling is in progress", xe_pci_error_detected() in
drivers/gpu/drm/xe/xe_pci_error.c calls xe_hw_error_work_disable(xe) whenever
XE_DEVICE_WA(xe, 14028434957) && is_aer_internal_error(pdev) is true.

Because Crescent Island (cri_desc) sets .has_sriov = true and enables
workaround 14028434957 at the platform level in xe_device_probe_early() before
xe_sriov_probe_early(), an SR-IOV VF on Crescent Island has
XE_DEVICE_WA(xe, 14028434957) set while tile->hw_error_work remains
uninitialized.

If pcie_do_recovery() invokes xe_pci_error_detected() on the VF, won't
xe_hw_error_work_disable() pass an uninitialized tile->hw_error_work (with
work->func == NULL) to disable_delayed_work_sync() -> __cancel_work_sync() ->
__flush_work(), triggering WARN_ON(!work->func) in kernel/workqueue.c?

> +
>  /**
>   * xe_hw_error_init - Initialize hw errors
>   * @xe: xe device instance
> @@ -546,12 +563,15 @@ static void process_hw_errors(struct xe_device *xe)
>   */
>  void xe_hw_error_init(struct xe_device *xe)
>  {
> -	struct xe_tile *tile = xe_device_get_root_tile(xe);
> +	struct xe_tile *tile;
> +	u8 id;
>  
>  	if (!IS_DGFX(xe) || IS_SRIOV_VF(xe))
>  		return;
>  
> -	INIT_DELAYED_WORK(&tile->hw_error_work, hw_error_work);
> +	for_each_tile(tile, xe, id)
> +		INIT_DELAYED_WORK(&tile->hw_error_work, hw_error_work);
>  
>  	process_hw_errors(xe);
> +	devm_add_action_or_reset(xe->drm.dev, xe_hw_error_work_disable, xe);
>  }

[Severity: High]
Can registering xe_hw_error_work_disable() via devm_add_action_or_reset() race
with an in-flight hw_error_work() callback during devres teardown and leave
behind dangling devres nodes and sysfs attributes after struct xe_device is
freed?

When devres teardown runs during probe failure unwind, driver unbind
(__device_release_driver() -> xe_pci_remove() -> device_unbind_cleanup() ->
devres_release_all()), or PCI slot reset (xe_pci_error_slot_reset() ->
devres_release_group()), drivers/base/devres.c moves all queued nodes from
dev->devres_head onto a local todo list before running any release callbacks:

drivers/base/devres.c:devres_release_all()
	spin_lock_irqsave(&dev->devres_lock, flags);
	cnt = remove_nodes(dev, dev->devres_head.next, &dev->devres_head, &todo);
	spin_unlock_irqrestore(&dev->devres_lock, flags);

	release_nodes(dev, &todo);

If tile->hw_error_work was scheduled by process_hw_errors() or an interrupt and
begins executing before release_nodes() invokes xe_hw_error_work_disable()
(including while xe_hw_error_work_disable() is waiting inside
disable_delayed_work_sync(&tile->hw_error_work) for the running worker to
finish), the worker executes:

drivers/gpu/drm/xe/xe_hw_error.c:hw_error_work()
  -> xe_survivability_mode_runtime_enable(xe)
    -> create_survivability_sysfs(pdev)

Inside drivers/gpu/drm/xe/xe_survivability_mode.c:create_survivability_sysfs(),
the worker creates sysfs files and registers new devres actions on
xe->drm.dev:

	ret = device_create_file(dev, &dev_attr_survivability_mode);
	if (ret)
		goto failed;

	ret = devm_add_action_or_reset(xe->drm.dev,
				       xe_survivability_mode_fini, xe);
	if (ret)
		goto failed;

	if (needs_info) {
		ret = devm_device_add_group(dev, &survivability_info_group);
		if (ret)
			goto failed;
	}

Because remove_nodes() already drained dev->devres_head into todo before
release_nodes() started, those newly registered devres actions are appended to
dev->devres_head and are not run by the active release_nodes(dev, &todo) pass,
while devm_drm_dev_init_release on todo frees struct xe_device *xe.

Wouldn't this leave the survivability_mode and survivability_info sysfs files
exposed on pdev->dev after xe is freed and drvdata is cleared (leading to a
NULL pointer dereference or use-after-free in survivability_mode_show() and
survivability_info_show()), leave xe_survivability_mode_fini(xe) queued on
dev->devres_head with a dangling xe pointer, and cause subsequent driver
re-probing in really_probe() to fail with -EBUSY because dev->devres_head is
non-empty?

Additionally, what happens in xe_hw_error_init() if devm_add_action_or_reset()
fails with -ENOMEM?

include/linux/device/devres.h:__devm_add_action_or_reset()
	ret = __devm_add_action(dev, action, data, name);
	if (ret)
		action(data);

	return ret;

On allocation failure, __devm_add_action_or_reset() immediately runs
xe_hw_error_work_disable(xe), which cancels any delayed work just scheduled by
process_hw_errors(xe) (after the hardware error registers and GFX_MSTR_IRQ
have already been cleared) and permanently disables tile->hw_error_work across
all tiles via WORK_CANCEL_DISABLE. Because xe_hw_error_init() returns void and
ignores the return value, xe_irq_install() and xe_device_probe() continue probing
with tile->hw_error_work permanently disabled for the lifetime of the device.
Should xe_hw_error_init() propagate the error to xe_irq_install()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026060459.771448-11-riana.tauro@intel.com?part=2