Re: [PATCH v2 11/33] drm/gpusvm: Add a DMA-mapping accounting callback

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Device memory mappings are leaked on the error path because `dpagemap` is not yet stored in `svm_pages`.
- [High] Reading uninitialized memory (`svm_pages->dma_addr[0].dir`) when unwinding an IOVA allocation failure.
- [High] System pages are incorrectly routed to `device_unmap` during unmapping of mixed ranges when IOVA is used.
--

--- Patch [11]: [PATCH v2 11/33] drm/gpusvm: Add a DMA-mapping accounting callback ---
commit 98db73f178129cc5ccc6277eaada197044dfbd3b
Author: Thomas Hellström <thomas.hellstrom@linux.intel.com>

drm/gpusvm: Add a DMA-mapping accounting callback

This commit adds an optional @dma_map_account callback to struct
drm_gpusvm_ops to track mapped pages. It is invoked during mapping and
unmapping to keep symmetric accounting. It also relaxes drm_gpusvm_init() to
allow restricted ops without full-SVM mode.

> diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c
> index fcfe635bc1956..512f52471419e 100644
> --- a/drivers/gpu/drm/drm_gpusvm.c
> +++ b/drivers/gpu/drm/drm_gpusvm.c

[ ... ]

> @@ -1246,6 +1253,8 @@ static void __drm_gpusvm_unmap_pages(struct drm_gpusvm *gpusvm,
>  			else if (dpagemap && dpagemap->ops->device_unmap)
>  				dpagemap->ops->device_unmap(dpagemap,
>  							    dev, addr);

[Severity: High]
This isn't a bug introduced by this patch, but can this leak device memory
mappings on the error path?

In drm_gpusvm_get_pages(), if an error occurs after successfully mapping
one or more device pages, the code jumps to err_unmap and calls
__drm_gpusvm_unmap_pages().

However, svm_pages->dpagemap is purposefully only updated after the mapping
loop completes successfully. During the early error unwind, it remains NULL.

Because of this, in __drm_gpusvm_unmap_pages():

    struct drm_pagemap *dpagemap = svm_pages->dpagemap;

The dpagemap evaluated here will be NULL, causing this else if condition to
fail and completely bypassing the unmap callback for all successfully mapped
device pages in that chunk.

[Severity: High]
This is also a pre-existing issue, but could this incorrectly unmap system
pages as device pages in a mixed chunk?

In drm_gpusvm_get_pages(), if a chunk has a mix of device and system pages
(ctx->allow_mixed is true) and IOVA is used:

When iterating over the pages in __drm_gpusvm_unmap_pages(), system pages will
have addr->proto == DRM_INTERCONNECT_SYSTEM. However, if use_iova is true, the
previous condition:

    if (!use_iova && addr->proto == DRM_INTERCONNECT_SYSTEM)
        dma_unmap_page(...)

will evaluate to false.

Execution then falls through to this else if branch. If a previous device page
set dpagemap, it will inappropriately call dpagemap->ops->device_unmap() on the
system page.

> +			if (gpusvm->ops && gpusvm->ops->dma_map_account)
> +				gpusvm->ops->dma_map_account(gpusvm, addr, -1);
>  			i += 1 << addr->order;
>  		}
>  

[ ... ]

> @@ -1654,6 +1663,10 @@ int drm_gpusvm_get_pages(struct drm_gpusvm *gpusvm,
>  				(addr, DRM_INTERCONNECT_SYSTEM, order,
>  				 dma_dir);
>  		}
> +		if (gpusvm->ops && gpusvm->ops->dma_map_account)
> +			gpusvm->ops->dma_map_account(gpusvm,
> +						     &svm_pages->dma_addr[j],
> +						     1);

[Severity: High]
This is a pre-existing issue, but is it possible to read uninitialized memory
here if the first page mapping fails?

In drm_gpusvm_get_pages(), svm_pages->dma_addr is allocated via
kvmalloc_objs() without zero-initialization.

If an error occurs on the very first page (i = 0), such as an IOVA allocation
failure, the code jumps to err_unmap with num_dma_mapped = 0, and
__drm_gpusvm_unmap_pages() is called with npages = 0.

Inside __drm_gpusvm_unmap_pages(), if dma_use_iova(&svm_pages->state) is true,
it will unconditionally call:

    dma_iova_destroy(dev, &svm_pages->state,
             svm_pages->state_offset,
             svm_pages->dma_addr[0].dir, 0);

Since npages = 0, svm_pages->dma_addr[0] was never initialized, leading to
svm_pages->dma_addr[0].dir being read from uninitialized heap memory.

>  		i += 1 << order;
>  		num_dma_mapped = i;
>  		flags.has_dma_mapping = true;

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