Re: [PATCH v5 3/5] drm/xe: have xe_svm_range embed one drm_gpusvm_pages

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Deferring IOMMU unmapping to the range free callback creates a window where a range is unlinked from the MMU interval tree but its pages remain mapped, breaking the MMU notifier security guarantees and allowing DMA use-after-free.
--

commit 178f1e1f04007ff8f673ad61f9e9152f411fac61
Author: Honglei Huang <honghuan@amd.com>

drm/xe: have xe_svm_range embed one drm_gpusvm_pages

This commit modifies the drm_gpusvm framework so that the driver takes
responsibility for the page lifecycle by embedding drm_gpusvm_pages. As part
of this, the synchronous unmap in drm_gpusvm_range_remove() was removed, and
xe now unmaps the pages from its range_free callback instead.

> diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c
> index 155e3c2e703d9..85fbadc9716a4 100644
> --- a/drivers/gpu/drm/drm_gpusvm.c
> +++ b/drivers/gpu/drm/drm_gpusvm.c

[ ... ]

> @@ -1245,8 +1248,6 @@ void drm_gpusvm_range_remove(struct drm_gpusvm *gpusvm,
>  		return;
>  
>  	drm_gpusvm_notifier_lock(gpusvm);
> -	__drm_gpusvm_unmap_pages(gpusvm, &range->pages, npages);
> -	__drm_gpusvm_free_pages(gpusvm, &range->pages);
>  	__drm_gpusvm_range_remove(notifier, range);
>  	drm_gpusvm_notifier_unlock(gpusvm);

[Severity: Critical]
Does this create a window where the pages remain mapped in the IOMMU but the
range is no longer tracked in the MMU interval tree?

If a concurrent CPU munmap() occurs right after this function removes the
range from the interval tree, the MMU notifier (xe_svm_invalidate) won't
find the range. It might return early without waiting for the GPU to finish
unbinding or unmapping the IOMMU, allowing the OS to reallocate physical pages
while the GPU can still access them.

[ ... ]

> diff --git a/drivers/gpu/drm/xe/xe_svm.c b/drivers/gpu/drm/xe/xe_svm.c
> index 63da149f3b7fd..77af0a8de63e0 100644
> --- a/drivers/gpu/drm/xe/xe_svm.c
> +++ b/drivers/gpu/drm/xe/xe_svm.c

[ ... ]

> @@ -122,8 +123,10 @@ xe_svm_range_alloc(struct drm_gpusvm *gpusvm)
>  
>  static void xe_svm_range_free(struct drm_gpusvm_range *range)
>  {
> +	drm_gpusvm_free_pages(range->gpusvm, &(to_xe_range(range)->pages),
> +			      drm_gpusvm_range_size(range) >> PAGE_SHIFT);
>  	xe_vm_put(range_to_vm(range));
> -	kfree(range);
> +	kfree(to_xe_range(range));
>  }

[Severity: Critical]
Could deferring the unmap to the free callback lead to a DMA use-after-free
regression if callers hold extra references?

For example, in xe_svm_unmap_address_range(), the code takes an extra
reference via drm_gpusvm_range_get() before triggering the garbage collector.
The garbage collector calls drm_gpusvm_range_remove(), removing the range
from the interval tree, but this free callback won't run until the extra
reference is dropped later. This keeps the IOMMU mappings alive while the
interval tree tracking is gone, opening up the race with munmap() described
above.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260627035418.3796481-1-honglei1.huang@amd.com?part=3