Re: [PATCH v8 2/5] drm/gpusvm: embed struct drm_device into drm_gpusvm_pages

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Device memory DMA mappings are leaked if an error occurs during the mapping loop in `drm_gpusvm_get_pages()`.
- [High] An uninitialized memory read occurs in `__drm_gpusvm_unmap_pages()` if IOVA allocation succeeds but linking fails on the very first page.
--

commit 4fe6a3e14e9c6c05c83b0d432b653dc56d06006d
Author: Honglei Huang <honghuan@amd.com>

drm/gpusvm: embed struct drm_device into drm_gpusvm_pages

Embeds the drm_device into drm_gpusvm_pages so that the pages object explicitly owns the drm handle. It routes DMA operations through svm_pages->drm instead of gpusvm->drm.

> diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c
> index abdfdaaf5e23c..604554e40f62d 100644
> --- a/drivers/gpu/drm/drm_gpusvm.c
> +++ b/drivers/gpu/drm/drm_gpusvm.c

[ ... ]

> @@ -1532,10 +1541,10 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm,
>  			}
>  			svm_pages->dma_addr[j] =
>  				dpagemap->ops->device_map(dpagemap,
> -							  gpusvm->drm->dev,
> +							  svm_pages->drm->dev,
>  							  page, order,
>  							  dma_dir);
> -			if (dma_mapping_error(gpusvm->drm->dev,
> +			if (dma_mapping_error(svm_pages->drm->dev,
>  					      svm_pages->dma_addr[j].addr)) {
>  				err = -EFAULT;
>  				goto err_unmap;

[Severity: High]
This isn't a bug introduced by this patch, but does this leak device memory DMA mappings if an error occurs here?

If dma_mapping_error() fails (or if a later operation fails and jumps to err_unmap), __drm_gpusvm_unmap_pages() is called. However, svm_pages->dpagemap is not updated until the very end of drm_gpusvm_get_pages() on the success path.

Because svm_pages->dpagemap is still NULL in the error path, __drm_gpusvm_unmap_pages() evaluates the condition else if (dpagemap && dpagemap->ops->device_unmap) as false and silently skips cleanup for all successfully mapped device memory pages.

> @@ -1555,11 +1564,11 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm,
>  			}
>  
>  			if (!i)
> -				dma_iova_try_alloc(gpusvm->drm->dev, state,
> +				dma_iova_try_alloc(svm_pages->drm->dev, state,
>  						   0, npages * PAGE_SIZE);
>  
>  			if (dma_use_iova(state)) {
> -				err = dma_iova_link(gpusvm->drm->dev, state,
> +				err = dma_iova_link(svm_pages->drm->dev, state,
>  						    hmm_pfn_to_phys(pfns[i]),
>  						    svm_pages->state_offset,
>  						    PAGE_SIZE << order,

[Severity: High]
This is a pre-existing issue, but can this lead to an uninitialized memory read if linking fails on the very first page?

If dma_iova_try_alloc() succeeds but dma_iova_link() fails on the first iteration (i == 0), the code jumps to err_unmap with num_dma_mapped = 0 before initializing svm_pages->dma_addr[0].

The error path then calls __drm_gpusvm_unmap_pages(..., 0). Because use_iova is true, it unconditionally evaluates svm_pages->dma_addr[0].dir to call dma_iova_destroy(), reading from the uninitialized memory of the dma_addr array (which was allocated via kvmalloc_objs without zeroing).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260630033132.361144-1-honghuan@amd.com?part=2