Re: [PATCH v1 2/2] drm/msm: map page-less imported sg_tables from their DMA addresses

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] drm/msm: assuming physical contiguity for IOMMU-coalesced DMA segments
- [Medium] drm/msm: memory leak on error path when mapping page-less sg entries
--

commit 77e8df4ba7f003ef7b79e1521b4c58572c28169c
Author: Jianfeng Liu <liujianfeng1994@gmail.com>

drm/msm: map page-less imported sg_tables from their DMA addresses

With CONFIG_DMABUF_DEBUG=y, dma_buf_map_attachment() hands importers a
copy of the attachment sg_table with the struct page pointers stripped
and sg->length zeroed. This patch translates the DMA addresses back
to physical addresses using the drm device's DMA-API domain to fix
arm-smmu translation faults caused by the missing page pointers.

> diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c
> index da6782fca6bd2..8407a37f9efee 100644
> --- a/drivers/gpu/drm/msm/msm_iommu.c
> +++ b/drivers/gpu/drm/msm/msm_iommu.c
[ ... ]
> @@ -184,8 +202,35 @@ static int msm_iommu_pagetable_map(struct msm_mmu *mmu, u64 iova,
>  		return msm_iommu_pagetable_map_prr(mmu, iova, len, prot);
>  
>  	for_each_sgtable_sg(sgt, sg, i) {
> -		size_t size = sg->length;
> -		phys_addr_t phys = sg_phys(sg);
> +		size_t size;
> +		phys_addr_t phys;
> +
> +		if (sg_page(sg)) {
> +			/* CPU-view entry: native objects, and imported
> +			 * sg_tables that still carry struct page
> +			 */
> +			size = sg->length;
> +			phys = sg_phys(sg);
> +		} else {
> +			/*
> +			 * Page-less entry, e.g. the sg_table wrapper
> +			 * that dma_buf_map_attachment() hands out when
> +			 * CONFIG_DMABUF_DEBUG=y (page pointers stripped,
> +			 * sg->length zeroed, only the DMA fields carried
> +			 * over).  Recover the physical address by
> +			 * translating the DMA address through the drm
> +			 * device's DMA-API domain.
> +			 */
> +			size = sg_dma_len(sg);
> +			phys = msm_mmu_dma_to_phys(mmu, sg_dma_address(sg));
                        ^^^^

[Severity: Critical]
Does this assume that the entire DMA segment length mapped by the IOMMU is
physically contiguous? 

If the IOMMU DMA API coalesces physically discontiguous pages into a single
contiguous IOVA segment, wouldn't mapping sg_dma_len(sg) using just the
starting physical address map the pages after the first page to incorrect,
adjacent physical addresses instead of their actual disjoint addresses?

> +
> +			if (!size || !phys) {
> +				dev_err(mmu->dev,
> +					"cannot map page-less sg entry: dma=%pad len=%zu\n",
> +					&sg_dma_address(sg), size);
> +				return -EINVAL;
                                ^^^^

[Severity: Medium]
If this fails after some scatterlist entries have already been successfully
mapped, does returning -EINVAL directly cause a permanent leak of the IOMMU
pagetable mappings?

Should this use a cleanup path to unmap the already-mapped pages, similar to
how ops->map_pages failures are handled with msm_iommu_pagetable_unmap()?

> +			}
> +		}
>  
>  		if (!len)
>  			break;
[ ... ]
> @@ -697,6 +742,47 @@ static int msm_iommu_map(struct msm_mmu *mmu, uint64_t iova,
>  	if (iova & BIT_ULL(48))
>  		iova |= GENMASK_ULL(63, 49);
>  
> +	/*
> +	 * With CONFIG_DMABUF_DEBUG=y, imported sg_tables carry no struct
> +	 * page and sg->length is zeroed; iommu_map_sgtable() would consume
> +	 * zero length and silently map nothing.  Map from the (translated)
> +	 * DMA addresses instead.
> +	 */
> +	if (!sg_page(sgt->sgl)) {
> +		struct scatterlist *sg;
> +		size_t mapped = 0;
> +		unsigned int i;
> +
> +		for_each_sgtable_dma_sg(sgt, sg, i) {
> +			phys_addr_t phys =
> +				msm_mmu_dma_to_phys(mmu, sg_dma_address(sg));
> +			size_t size = sg_dma_len(sg);
> +
> +			if (!phys || !size) {
> +				ret = -EINVAL;
> +				goto err_unmap;
> +			}
> +
> +			ret = iommu_map(iommu->domain, iova + mapped, phys,
> +					size, prot, GFP_KERNEL);
                        ^^^^

[Severity: Critical]
Does this also incorrectly assume physical contiguity for the coalesced DMA
segment by passing the entire sg_dma_len(sg) to iommu_map() with only the
starting physical address?

> +			if (ret)
> +				goto err_unmap;
> +
> +			mapped += size;
> +		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260928053901.7270-1-liujianfeng1994@gmail.com?part=2