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 <[email protected]> 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/[email protected]?part=2
