Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Pre-existing issues: - [Critical] dma_iova_link() is called with a hardcoded offset of 0, causing all physical ranges to overlap at the start of the IOVA space. - [High] IOVA space allocated by dma_iova_try_alloc() is leaked on multiple error paths. - [High] The fallback error path unconditionally unmaps unpopulated, zero-initialized scatterlist entries by overwriting the loop counter. - [High] Mapping a large region as a single chunk but unmapping it in smaller split chunks violates the DMA API. -- commit 139d9e764ec01d51d2adf94f612620ee800a32bb Author: Davidlohr Bueso <[email protected]> dma-buf: validate the attachment before asserting its reservation lock dma_buf_phys_vec_to_sgt() asserts the reservation lock before its NULL checks run. Validate first, then assert. > diff --git a/drivers/dma-buf/dma-buf-mapping.c > b/drivers/dma-buf/dma-buf-mapping.c > index 833be519e1e6a..12674d89b23eb 100644 > --- a/drivers/dma-buf/dma-buf-mapping.c > +++ b/drivers/dma-buf/dma-buf-mapping.c > @@ -111,12 +111,12 @@ struct sg_table *dma_buf_phys_vec_to_sgt(struct > dma_buf_attachment *attach, > size_t i; > int ret; > > - dma_resv_assert_held(attach->dmabuf->resv); > - > if (WARN_ON(!attach || !attach->dmabuf || !provider)) > /* This function is supposed to work on MMIO memory only */ > return ERR_PTR(-EINVAL); > > + dma_resv_assert_held(attach->dmabuf->resv); > + > dma = kzalloc_obj(*dma); > if (!dma) > return ERR_PTR(-ENOMEM); [Severity: High] This is a pre-existing issue, but does this code leak the IOVA space allocated by dma_iova_try_alloc() on multiple error paths? If sg_alloc_table() fails, the code jumps to err_free_state: ret = sg_alloc_table(&dma->sgt, nents, GFP_KERNEL | __GFP_ZERO); if (ret) goto err_free_state; This frees dma->state but skips calling dma_iova_free() to release the IOVA allocation. Additionally, if the very first physical range fails to link (when i == 0), the error handler skips destruction: err_unmap_dma: if (!i || !dma->state) { ; /* Do nothing */ } else if (dma_use_iova(dma->state)) { dma_iova_destroy(attach->dev, dma->state, mapped_len, dir, DMA_ATTR_MMIO); Can this exhaust device IOMMU IOVA space under memory pressure? [Severity: Critical] This is a pre-existing issue, but does this loop incorrectly map all physical ranges to the exact same starting IOVA address? } else if (dma_use_iova(dma->state)) { ret = dma_iova_link(attach->dev, dma->state, phys_vec[i].paddr, 0, phys_vec[i].len, dir, DMA_ATTR_MMIO); When nr_ranges > 1, dma_iova_link() is called repeatedly with the offset hardcoded to 0. Since the target IOVA address is computed as (state->addr + offset), each physical range overwrites the previous mapping. Should the offset be passed as mapped_len to ensure segments are appended sequentially? [Severity: High] This is a pre-existing issue, but could this teardown logic violate the DMA API by mapping a large region as a single chunk and unmapping it piece-by-piece? During mapping, a physical range might be mapped in a single contiguous block: addr = dma_map_phys(attach->dev, phys_vec[i].paddr, phys_vec[i].len, dir, DMA_ATTR_MMIO); Later during teardown, the fallback error path iterates over the individual split SG entries and unmaps them separately: err_unmap_dma: ... } else { for_each_sgtable_dma_sg(&dma->sgt, sgl, i) dma_unmap_phys(attach->dev, sg_dma_address(sgl), sg_dma_len(sgl), dir, DMA_ATTR_MMIO); The same piece-by-piece unmapping also happens in dma_buf_free_sgt(). Since the DMA API requires that the size and address passed to dma_unmap_phys() exactly match the original call to dma_map_phys(), could these partial unmaps corrupt IOMMU tracking structures? [Severity: High] This is a pre-existing issue, but does the fallback error path unconditionally unmap unpopulated, zero-initialized scatterlist entries? } else { for_each_sgtable_dma_sg(&dma->sgt, sgl, i) dma_unmap_phys(attach->dev, sg_dma_address(sgl), sg_dma_len(sgl), dir, DMA_ATTR_MMIO); The for_each_sgtable_dma_sg macro shadows and resets 'i' to 0, looping over all allocated entries (dma->sgt.nents) rather than just the successfully mapped ranges. Since the table was allocated with __GFP_ZERO, the unpopulated entries will have a length and address of 0. Could passing these to dma_unmap_phys() trigger IOMMU faults? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
