Re: [PATCH 1/2] drm/gpusvm: avoid reading uninitialized dma_addr[0].dir on IOVA unmap

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Device memory mappings created by `device_map` are leaked on the error path in `drm_gpusvm_get_pages()` because `svm_pages->dpagemap` is NULL.
--

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

drm/gpusvm: avoid reading uninitialized dma_addr[0].dir on IOVA unmap

This commit fixes an issue where an uninitialized DMA direction could be
passed to dma_iova_destroy() on the get_pages() error path. It guards
the direction argument with a check on npages and zeroes the dma_addr array.

> diff --git a/drivers/gpu/drm/drm_gpusvm.c b/drivers/gpu/drm/drm_gpusvm.c
> index 958cb605aedd5..2d74e2aae0a77 100644
> --- a/drivers/gpu/drm/drm_gpusvm.c
> +++ b/drivers/gpu/drm/drm_gpusvm.c
> @@ -1146,10 +1146,17 @@ static void __drm_gpusvm_unmap_pages(struct drm_gpusvm *gpusvm,
>  		};
>  		bool use_iova = dma_use_iova(&svm_pages->state);
>  
> +		/*
> +		 * On the get_pages() error path npages can be 0 with
> +		 * dma_addr[0] still unpopulated, while the IOVA reservation
> +		 * already needs tearing down. Pass DMA_BIDIRECTIONAL in that
> +		 * case instead of reading dma_addr[0].dir.
> +		 */
>  		if (use_iova)
>  			dma_iova_destroy(dev, &svm_pages->state,
>  					 svm_pages->state_offset,
> -					 svm_pages->dma_addr[0].dir, 0);
> +					 npages ? svm_pages->dma_addr[0].dir :
> +						  DMA_BIDIRECTIONAL, 0);
>  
>  		for (i = 0, j = 0; i < npages; j++) {
>  			struct drm_pagemap_addr *addr = &svm_pages->dma_addr[j];

[Severity: High]
This is a pre-existing issue, but does this error path leak device mappings?

When drm_gpusvm_get_pages() maps device pages successfully using
dpagemap->ops->device_map(), but then fails on a subsequent page (for
example, returning -EOPNOTSUPP because ctx->allow_mixed is false), it jumps
to the err_unmap label:

drm_gpusvm_get_pages() {
        ...
err_unmap:
        svm_pages->flags.has_dma_mapping = true;
        __drm_gpusvm_unmap_pages(gpusvm, svm_pages, num_dma_mapped);
}

However, svm_pages->dpagemap hasn't been assigned the local dpagemap yet.
When __drm_gpusvm_unmap_pages() runs, it reads the uninitialized
svm_pages->dpagemap as NULL.

This causes the cleanup loop in __drm_gpusvm_unmap_pages() to skip
device_unmap():

__drm_gpusvm_unmap_pages() {
        struct drm_pagemap *dpagemap = svm_pages->dpagemap;
        ...
        for (i = 0, j = 0; i < npages; j++) {
            ...
            } else if (dpagemap && dpagemap->ops->device_unmap) {
                dpagemap->ops->device_unmap(dpagemap, dev, addr);
            }
        }
}

Since dpagemap is NULL, device_unmap() is never called for the already
mapped pages. Can this permanently leak the device mappings?

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