Re: [PATCH] drm/xe: Add wa_14028434957 to Graphics Version 35.11

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] drm: missing error return after drmm_add_action_or_reset() failure leads to leaked work and UAF
- [High] xe: Use-after-free/MMIO fault due to deferred workqueue cancellation in drmm
- [High] xe: nonfatal_msi_work accesses MMIO without waking device, causing suspended MMIO accesses
- [High] drm/xe: UAF and uninitialized work_struct in csc_hw_error_work
--

commit c1e7bdf12138e40e4c7ecaaa2d05a9ee296aa085
Author: Varun Gupta <varun.gupta@intel.com>
Subject: drm/xe: Add wa_14028434957 to Graphics Version 35.11

This patch bypasses a faulty AER path by intercepting uncorrectable 
non-fatal MSI errors and routing them to a deferred workqueue. The new 
workqueue reuses the GFSP error containment logic and wedges the device 
if a reset is required.

> diff --git a/drivers/gpu/drm/xe/xe_hw_error.c b/drivers/gpu/drm/xe/xe_hw_error.c
> index 5f2abc9485ff5..847ca1ae411ef 100644
> --- a/drivers/gpu/drm/xe/xe_hw_error.c
> +++ b/drivers/gpu/drm/xe/xe_hw_error.c
[ ... ]
> @@ -173,6 +177,27 @@ static void csc_hw_error_work(struct work_struct *work)
>  	xe_survivability_mode_runtime_enable(xe);
>  }
>  
> +static void nonfatal_msi_work(struct work_struct *work)
> +{
> +	struct xe_drm_ras *ras = container_of(work, typeof(*ras), nonfatal_msi_work);
> +	struct xe_device *xe = container_of(ras, typeof(*xe), ras);
> +	enum xe_ras_recovery_action action = xe_ras_process_errors(xe);

[Severity: High]
Does this workqueue execution need to resume the device first?

Because this runs asynchronously in a worker thread, the runtime PM 
reference from the interrupt might already be released, allowing the device 
to auto-suspend to D3. The nested call to xe_sysctrl_send_command() uses 
guard(xe_pm_runtime_noresume)(xe), which blocks future suspends but does 
not wake a device that is already suspended. This means MMIO accesses 
to SYSCTRL_MB_CTRL might target suspended hardware.

Other workers like xe_sysctrl_work() seem to use guard(xe_pm_runtime)(xe) 
to explicitly wake the device.

> +
> +	/*
> +	 * TODO: Request an SBR instead of wedging, once VR driver
> +	 * quiescing support is in place.
> +	 */
> +	if (action != XE_RAS_RECOVERY_ACTION_RECOVERED)
> +		xe_device_declare_wedged(xe);
> +}
> +
> +static void hw_error_fini(struct drm_device *drm, void *arg)
> +{
> +	struct xe_device *xe = arg;
> +
> +	cancel_work_sync(&xe->ras.nonfatal_msi_work);

[Severity: High]
Should this function also cancel csc_hw_error_work?

The code initializes csc_hw_error_work in xe_hw_error_init(), but it 
doesn't appear to be cancelled during teardown. With a fini function now 
added, leaving csc_hw_error_work uncancelled might allow it to run after 
the xe_device structures are freed.

Also, it looks like csc_hw_error_work is only initialized for the root tile 
in xe_hw_error_init():

    struct xe_tile *tile = xe_device_get_root_tile(xe);
    ...
    INIT_WORK(&tile->csc_hw_error_work, csc_hw_error_work);

If a non-root tile receives a hardware error and schedules this work, might 
it operate on an uninitialized work_struct?

> +}
> +
[ ... ]
> @@ -436,6 +461,15 @@ static void hw_error_source_handler(struct xe_tile *tile, const enum hardware_er
>  	 * here for those platforms.
>  	 */
>  	if (xe->info.has_sysctrl) {
> +		/*
> +		 * Defer non-fatal MSI handling to process context because the GFSP
> +		 * query can sleep.
> +		 */
> +		if (hw_err == HARDWARE_ERROR_NONFATAL && XE_DEVICE_WA(xe, 14028434957)) {
> +			schedule_work(&xe->ras.nonfatal_msi_work);

[Severity: High]
Could this work execute after the MMIO mappings have been unmapped?

Since hw_error_fini() is registered as a drmm action, its execution is 
deferred until the last DRM reference is dropped (like when userspace 
closes the device file). However, devres actions unmap the device's MMIO 
mappings synchronously during PCI unbind. 

If this work is queued just before the IRQ is torn down and executes after 
the unbind finishes, the nested xe_ras_process_errors() could attempt to 
access unmapped MMIO regions via xe_sysctrl_send_command(), resulting in a 
fatal page fault.

> +			return;
> +		}
> +
>  		drm_err_ratelimited(&xe->drm, HW_ERR "Invalid error routing\n");
>  		return;
>  	}
> @@ -546,6 +580,11 @@ void xe_hw_error_init(struct xe_device *xe)
>  {
>  	struct xe_tile *tile = xe_device_get_root_tile(xe);
>  
> +	INIT_WORK(&xe->ras.nonfatal_msi_work, nonfatal_msi_work);
> +
> +	if (drmm_add_action_or_reset(&xe->drm, hw_error_fini, xe))
> +		drm_err(&xe->drm, "Failed to register hw_error_fini\n");

[Severity: High]
Is a return statement missing here?

If drmm_add_action_or_reset() fails, it logs an error but initialization 
continues. This means the driver can be fully initialized without 
hw_error_fini() being registered. If nonfatal_msi_work gets scheduled, 
it will never be cancelled on teardown, potentially leading to a 
use-after-free regression.

> +
>  	if (!IS_DGFX(xe) || IS_SRIOV_VF(xe))
>  		return;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009065748.1304076-2-varun.gupta@intel.com?part=1