Re: [PATCH v3 12/33] drm/xe: Add debugfs stats for DMA-mapped pages per order

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Missing decrement of DMA mapped pages counter in `xe_bo_dma_unmap_pinned()` leads to leaked accounting and kernel assertion failure during device teardown.

Pre-existing issues:
- [High] `xe_bo_dev_fini()` is called before `xe->destroy_wq` is flushed, leading to Use-After-Free of the TTM device and false positive assertion failures on device teardown.
--

--- Patch [12]: [PATCH v3 12/33] drm/xe: Add debugfs stats for DMA-mapped pages per order ---
commit 670e42692f03a24b49683eec7ff3afb39e28a858
Author: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Subject: drm/xe: Add debugfs stats for DMA-mapped pages per order

Expose per-page-order DMA mapping counts for the system memory that the
xe driver maps for GPU access, split into two categories: TTM buffer
objects, and the GPU SVM / userptr ranges.

> diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
> index 85e6d9a0f575b..60e0e568aa31f 100644
> --- a/drivers/gpu/drm/xe/xe_bo.c
> +++ b/drivers/gpu/drm/xe/xe_bo.c
[ ... ]
> @@ -404,6 +449,7 @@ static void xe_tt_unmap_sg(struct xe_device *xe, struct ttm_tt *tt)
>  	struct xe_ttm_tt *xe_tt = container_of(tt, struct xe_ttm_tt, ttm);
>  
>  	if (xe_tt->sg) {
> +		xe_tt_account_dma_pages(xe, xe_tt, -1);
>  		dma_unmap_sgtable(xe->drm.dev, xe_tt->sg,
>  				  DMA_BIDIRECTIONAL, 0);
>  		sg_free_table(xe_tt->sg);

[Severity: Critical]
Will this accounting leak if the scatter-gather table is unmapped elsewhere?

Looking at xe_bo_dma_unmap_pinned() in drivers/gpu/drm/xe/xe_bo.c, it unmaps
and frees the sg table without updating the accounting:

	} else if (xe_tt->sg) {
		dma_unmap_sgtable(ttm_to_xe_device(ttm_bo->bdev)->drm.dev,
				  xe_tt->sg,
				  DMA_BIDIRECTIONAL, 0);
		sg_free_table(xe_tt->sg);
		xe_tt->sg = NULL;
	}

Because xe_bo_dma_unmap_pinned() clears xe_tt->sg, a later call to
xe_tt_unmap_sg() will see xe_tt->sg as NULL and skip the decrement, leading to
a permanent accounting leak and a guaranteed assertion failure on device
teardown.

> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index ad7f3e61d4579..97cc2d185e45d 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
[ ... ]
>  static void xe_device_destroy(struct drm_device *dev, void *dummy)
>  {
>  	struct xe_device *xe = to_xe_device(dev);
>  
>  	xe_bo_dev_fini(&xe->bo_device);
>  
> +	/*
> +	 * Assert the DMA-mapped-pages accounting only after xe_bo_dev_fini()
> +	 * has flushed the async BO-free worker: BOs pending async free still
> +	 * hold their DMA mappings (and counts) until the worker runs, so
> +	 * asserting before the flush could trip a false positive.
> +	 */
> +	xe_device_assert_dma_pages_zero(xe);
> +
>  	if (xe->preempt_fence_wq)
>  		destroy_workqueue(xe->preempt_fence_wq);

[Severity: High]
This is a pre-existing issue, but will this new assertion falsely trip due
to the teardown order?

Because xe->destroy_wq is destroyed after this assertion, and work items in
xe->destroy_wq might still hold final references to BOs, those BOs won't be
unmapped until the workqueue is drained.

Since the assertion now runs before the workqueues drop their BO references,
it will incorrectly trigger because those BOs still have active DMA mappings.

Additionally, since xe_bo_dev_fini() flushes the async BO-free worker before
the workqueues drop BOs, the worker can run concurrently with or after
ttm_device_fini(), which is a pre-existing use-after-free risk exposed by
this change.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260711025619.2540575-1-matthew.brost@intel.com?part=12