Re: [PATCH v2 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 4 potential issue(s) to consider:

New issues:
- [High] xe_device_assert_dma_pages_zero() is called too early in xe_device_destroy(), leading to spurious kernel assertion failures.
- [High] Uncapped `addr->order` in SVM and userptr DMA accounting leads to out-of-bounds array writes.
- [Low] Too many arguments to `drm_gpusvm_init` in `xe_svm.h` causes a compilation error when `CONFIG_DRM_GPUSVM` is enabled but `CONFIG_DRM_XE_GPUSVM` is disabled.

Pre-existing issues:
- [Critical] Asynchronous execution of `vm_destroy_work_func` can cause a Use-After-Free of the `xe` device structure.
--

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

This commit adds per-page-order DMA mapping counters for system memory
accessible by the GPU. The stats cover both TTM buffer objects and SVM/userptr
ranges, exposing them through debugfs.

> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index ad7f3e61d4579..0c4ee874a56d7 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
[ ... ]
> @@ -475,8 +499,10 @@ static void xe_device_destroy(struct drm_device *dev, void *dummy)
>  {
>  	struct xe_device *xe = to_xe_device(dev);
>  
> +	xe_device_assert_dma_pages_zero(xe);
> +
>  	xe_bo_dev_fini(&xe->bo_device);

[Severity: High]
Does this assertion trigger prematurely if the asynchronous BO destruction
worker has pending buffers to free?

In xe_device_destroy(), xe_device_assert_dma_pages_zero() is called
before xe_bo_dev_fini() flushes the async_free worker. If there are
delayed destructions, wouldn't the DMA mappings still be active, causing
the counters to be non-zero and triggering a kernel warning?

> diff --git a/drivers/gpu/drm/xe/xe_svm.c b/drivers/gpu/drm/xe/xe_svm.c
> index b228a737cfd6d..2f639fc0cac4e 100644
> --- a/drivers/gpu/drm/xe/xe_svm.c
> +++ b/drivers/gpu/drm/xe/xe_svm.c
[ ... ]
> +static void xe_svm_dma_map_account(struct drm_gpusvm *gpusvm,
> +				   const struct drm_pagemap_addr *addr,
> +				   int sign)
> +{
> +	struct xe_device *xe = gpusvm_to_vm(gpusvm)->xe;
> +	unsigned int order = addr->order;
> +
> +	if (addr->proto != DRM_INTERCONNECT_SYSTEM)
> +		return;
> +
> +	atomic_long_add((long)(1UL << order) * sign,
> +			&xe->mem.dma_mapped_pages_svm[order]);

[Severity: High]
Can this cause an out-of-bounds write if addr->order exceeds the array
bounds of dma_mapped_pages_svm?

In xe_svm_dma_map_account(), addr->order is used directly as an array
index. If an unprivileged user maps a large huge page (such as a 1GB page
with an order of 18) and faults it into the GPU, wouldn't this exceed the
NR_PAGE_ORDERS limit and corrupt adjacent kernel memory?

> +}
> +#endif

> diff --git a/drivers/gpu/drm/xe/xe_svm.h b/drivers/gpu/drm/xe/xe_svm.h
> index a921556d34662..edd668c415512 100644
> --- a/drivers/gpu/drm/xe/xe_svm.h
> +++ b/drivers/gpu/drm/xe/xe_svm.h
[ ... ]
> @@ -236,7 +236,8 @@ int xe_svm_init(struct xe_vm *vm)
>  {
>  #if IS_ENABLED(CONFIG_DRM_GPUSVM)
>  	return drm_gpusvm_init(&vm->svm.gpusvm, "Xe SVM (simple)",
> -			       NULL, 0, 0, 0, NULL, NULL, 0);
> +			       NULL, 0, 0, 0, NULL, NULL,
> +			       xe_userptr_gpusvm_ops_get(), 0);

[Severity: Low]
Does this function call pass too many arguments to drm_gpusvm_init()?

In the xe_svm_init() inline stub, the xe_userptr_gpusvm_ops_get() argument
was added without removing one of the NULL placeholders. Since
drm_gpusvm_init() expects 9 arguments and this now provides 10, won't this
cause a compilation error when CONFIG_DRM_GPUSVM is enabled but
CONFIG_DRM_XE_GPUSVM is disabled?

>  #else
>  	return 0;
>  #endif

> diff --git a/drivers/gpu/drm/xe/xe_userptr.c b/drivers/gpu/drm/xe/xe_userptr.c
> index 8b2d461ea0b28..79c2a81001f2e 100644
> --- a/drivers/gpu/drm/xe/xe_userptr.c
> +++ b/drivers/gpu/drm/xe/xe_userptr.c
[ ... ]
> +static void xe_userptr_dma_map_account(struct drm_gpusvm *gpusvm,
> +				       const struct drm_pagemap_addr *addr,
> +				       int sign)
> +{
> +	struct xe_vm *vm = container_of(gpusvm, struct xe_vm, svm.gpusvm);
> +	unsigned int order = addr->order;
> +
> +	if (addr->proto != DRM_INTERCONNECT_SYSTEM)
> +		return;
> +
> +	atomic_long_add((long)(1UL << order) * sign,
> +			&vm->xe->mem.dma_mapped_pages_svm[order]);

[Severity: High]
Similarly to xe_svm_dma_map_account(), can this cause an out-of-bounds
write if addr->order is larger than NR_PAGE_ORDERS?

Using addr->order directly as an array index without capping it could allow
unprivileged user-space applications mapping large huge pages to corrupt
memory when invoking xe_userptr_dma_map_account().

> +}


While reviewing the VM lifecycle code related to this patch, I noticed a
separate pre-existing issue in drivers/gpu/drm/xe/xe_vm.c.

[Severity: Critical]
This is a pre-existing issue, but does queueing the destroy work
asynchronously lead to a use-after-free if the last reference to the DRM
device is dropped before the worker executes?

In xe_vm_free(), the destruction work is queued to system_dfl_wq:

	queue_work(system_dfl_wq, &vm->destroy_work);

If this drops the last device reference, the xe device could be freed via
RCU before vm_destroy_work_func() runs. Since vm_destroy_work_func()
dereferences the xe pointer (for example, when calling
xe_pm_runtime_put()), wouldn't this result in a use-after-free?

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