Re: [PATCH v3 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] Uninitialized memory read of `svm_pages->dma_addr[0].dir` passed to `dma_iova_destroy()` on the error path.
- [High] System memory pages are erroneously unmapped using the device-specific unmap callback in mixed SVM ranges.
- [High] Device memory DMA mappings are leaked on the error path because `svm_pages->dpagemap` is assigned too late.
--

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

drm/gpusvm: Add a DMA-mapping accounting callback

The patch adds a dma_map_account callback to drm_gpusvm_ops to allow drivers
to symmetrically account for DMA-mapped pages during map and unmap operations.

> 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 does the error path in
drm_gpusvm_get_pages() trigger an uninitialized memory read here?

If the first page's mapping fails (e.g., dma_iova_link() fails), the code
jumps to the err_unmap label with num_dma_mapped = 0. Because
dma_iova_try_alloc() succeeded, use_iova evaluates to true in
__drm_gpusvm_unmap_pages(). This unconditionally executes dma_iova_destroy()
just before this loop, reading svm_pages->dma_addr[0].dir by value:

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

Since svm_pages->dma_addr was allocated using kvmalloc_objs without
zero-initialization, this passes an uninitialized enum dma_data_direction into
a kernel DMA API, which can trigger KMSAN panics or BUG_ON checks in DMA
debugging routines.

[Severity: High]
This is a pre-existing issue, but does this logic misroute system memory pages
to the device unmap callback?

When ctx->allow_mixed is true, an SVM range can contain both system and device
pages. In this scenario, use_iova evaluates to true, and svm_pages->dpagemap
is initialized to the device's pagemap. During __drm_gpusvm_unmap_pages(), the
loop iterates over all pages. For a system page, addr->proto ==
DRM_INTERCONNECT_SYSTEM is true, but !use_iova evaluates to false.

This causes the execution to fall through to the else if condition without
validating the page type:

    else if (dpagemap && dpagemap->ops->device_unmap)
        dpagemap->ops->device_unmap(dpagemap, dev, addr);

This incorrectly executes the device's unmap callback on a system memory page
(containing an IOVA address), which seems likely to cause memory corruption or
a crash.

[Severity: High]
This is also a pre-existing issue, but are device memory DMA mappings leaked
on the error path because svm_pages->dpagemap is assigned too late?

If an error occurs during drm_gpusvm_get_pages() after successfully mapping
one or more device private pages, the code jumps to the err_unmap cleanup
path. However, svm_pages->dpagemap is only updated at the very end of the
success path:

    if (pagemap) {
        flags.has_devmem_pages = true;
        drm_pagemap_get(dpagemap);
        drm_pagemap_put(svm_pages->dpagemap);
        svm_pages->dpagemap = dpagemap;
    }

Consequently, inside __drm_gpusvm_unmap_pages(), the local dpagemap variable
is NULL, causing the condition dpagemap && dpagemap->ops->device_unmap to
evaluate to false. The device unmap callback is skipped, leaking the device
memory mappings. The newly added dma_map_account callback will un-account
these leaked pages, causing internal accounting to irreversibly drift from the
hardware state.

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

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