Re: [PATCH v10 07/10] drm/xe/pm: Introduce xe_device_suspend/resume()

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Critical] External pinned BOs lose their DMA mappings during FLR suspend but are not unbound from GPU VMs and not restored on FLR resume, leading to an exploitable DMA UAF.
- [High] Deadlock in FLR prepare path due to waiting for migration jobs after disabling interrupts and stopping GuC.

Pre-existing issues:
- [High] Incomplete rollback on GT suspend failure leaves device in an inconsistent state.
--

--- Patch [7]: [PATCH v10 07/10] drm/xe/pm: Introduce xe_device_suspend/resume() ---
commit ee6e16bf1e226eef6a606f3d7137d9fbf63f7e82
Author: Raag Jadav <[email protected]>

drm/xe/pm: Introduce xe_device_suspend/resume()
    
PCIe FLR prepare/re-initialization flows pretty much reflect system
suspend/resume flows with a few notable exceptions. Repurpose existing
helpers for PCIe FLR with an explicit flag to distinguish between both
flows.

> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index 396d02eb2af83..012ba47660982 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
[ ... ]
> +int xe_device_suspend(struct xe_device *xe, bool flr)
> +{
> +	struct xe_gt *gt;
> +	u8 id;
> +	int err;
> +
> +	err = xe_pxp_pm_suspend(xe->pxp);
> +	if (err)
> +		return err;
> +
> +	xe_late_bind_wait_for_worker_completion(&xe->late_bind);
> +
> +	for_each_gt(gt, xe, id)
> +		xe_gt_suspend_prepare(gt);
> +
> +	if (flr) {
> +		for_each_gt(gt, xe, id)
> +			xe_gt_flr_prepare(gt);
> +	} else {
> +		xe_display_pm_suspend(xe);
> +
> +		/* FIXME: Super racey... */
> +		err = xe_bo_evict_all(xe);
> +		if (err)
> +			goto err_display;
> +
> +		for_each_gt(gt, xe, id) {
> +			err = xe_gt_suspend(gt);
> +			if (err)
> +				goto err_display;
> +		}
> +	}
> +
> +	xe_irq_suspend(xe);
> +
> +	if (flr) {
> +		/* TODO: Drop all user bos */
> +		xe_bo_pci_dev_remove_pinned(xe);

[Severity: High]
Can this cause a deadlock during the PCIe FLR prepare sequence?

When flr is true, xe_gt_flr_prepare() stops GuC scheduling. The
guc_exec_queue_stop() function explicitly skips banning and triggering
cleanup for kernel queues like the migrate engine, leaving pending migration
jobs in the scheduler.

Later we call xe_irq_suspend() which disables hardware interrupts. Then
xe_bo_pci_dev_remove_pinned() calls xe_tile_migrate_wait(), performing a
blocking dma_fence_wait() on the migrate engine's last job fence.

Since GuC is stopped, interrupts are disabled, and TDR wasn't queued, will
the fence ever signal?

> +
> +		unmap_mapping_range(xe->drm.anon_inode->i_mapping, 0, 0, 1);
> +	} else {
> +		xe_display_pm_suspend_late(xe);
> +
> +		xe_i2c_pm_suspend(xe);
> +	}
> +
> +	return 0;
> +
> +err_display:
> +	xe_display_pm_resume(xe);
> +	xe_pxp_pm_resume(xe->pxp);
> +	return err;
> +}

[Severity: High]
This is a pre-existing issue, but does this error path leave the system in an
inconsistent state if we fail during system suspend?

If xe_bo_evict_all() fails (e.g., due to system memory pressure), or
xe_gt_suspend() fails on a subsequent GT, we jump to err_display.

The error path only resumes the display and PXP, but doesn't resume the already
suspended GTs, undo xe_gt_suspend_prepare(), or restore the evicted BOs. Could
this leave the device permanently broken until a reboot?

[ ... ]
> +int xe_device_resume(struct xe_device *xe, bool flr)
> +{
[ ... ]
> +	if (flr) {
> +		if (err)
> +			return err;
> +	} else {
> +		/*
> +		 * Try to bring up display before bailing from GT resume failure,
> +		 * so we don't leave the user clueless with a blank screen.
> +		 */
> +		xe_display_pm_resume(xe);
> +		if (err)
> +			return err;
> +
> +		err = xe_bo_restore_late(xe);

[Severity: Critical]
Does skipping xe_bo_restore_late() during FLR resume create an IOMMU bypass or
Use-After-Free risk?

During FLR suspend, xe_bo_pci_dev_remove_pinned() unmaps DMA for external BOs
(dma-bufs), but the GPU page tables (which may reside in system memory and
survive FLR) are not updated to remove the old IOVAs.

When resuming from FLR, we take the "flr == true" branch above and skip
xe_bo_restore_late(). Since we skip remapping the external BOs, are those BOs
left unmapped while their old IOVAs remain in the surviving GPU VMs?

If userspace submits jobs to the old GPU VMs, and the GPU accesses the unmapped
IOVAs, could the GPU silently corrupt or exfiltrate memory if the IOMMU has
reallocated those IOVAs to another device or process?

> +		if (err)
> +			return err;
> +
> +		if (IS_VF_CCS_READY(xe))
> +			xe_sriov_vf_ccs_register_context(xe);
> +	}
> +
> +	xe_pxp_pm_resume(xe->pxp);
> +
> +	xe_late_bind_fw_load(&xe->late_bind);
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7