Re: [PATCH v3] dma-buf: Split sgl by largest page-aligned chunk

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but in the error handling path `err_unmap_dma`, `for_each_sgtable_dma_sg` incorrectly iterates over all allocated entries and calls `dma_unmap_phys` on zero-initialized memory.
- [High] This problem wasn't introduced by this patch, but in the error handling path `err_unmap_dma`, if mapping fails on the very first iteration (`i == 0`), it skips cleaning up the IOVA state, permanently leaking the IOVA space.
--

--- Patch [1]: [PATCH v3] dma-buf: Split sgl by largest page-aligned chunk ---
commit 4626127ac68c48120c89f9454d7c72b3a29300b8
Author: David Hu <xuehaohu@google.com>

dma-buf: Split sgl by largest page-aligned chunk

This commit modifies the scatterlist splitting logic to use the largest
page-aligned chunk instead of UINT_MAX. This ensures that scatterlist
DMA lengths and addresses remain page-aligned. The change prevents
hardware DMA engine failures and addresses an edge case in
ib_umem_find_best_pgsz().

Closes: https://lore.kernel.org/all/20260609165431.778061F00893@smtp.kernel.org/
Fixes: 3aa31a8bb11e ("dma-buf: provide phys_vec to scatter-gather mapping routine")
Cc: stable@vger.kernel.org

> diff --git a/drivers/dma-buf/dma-buf-mapping.c b/drivers/dma-buf/dma-buf-mapping.c
> index 794acff2546a3..50ded9daf5fb3 100644
> --- a/drivers/dma-buf/dma-buf-mapping.c
> +++ b/drivers/dma-buf/dma-buf-mapping.c

[ ... ]

While looking at this file, I noticed a couple of pre-existing issues in
dma_buf_phys_vec_to_sgt().

> err_unmap_dma:
> 	if (!i || !dma->state) {
> 		; /* Do nothing */

[Severity: High]
This isn't a bug introduced by this patch, but if mapping fails on the very
first iteration (i == 0), will this skip cleaning up the IOVA state?

If PCI_P2PDMA_MAP_THRU_HOST_BRIDGE is used, dma_iova_try_alloc() allocates
IOVA space before the mapping loop. If the very first dma_iova_link() fails,
the error path jumps here. Since !i evaluates to true, the code skips
calling dma_iova_destroy().

Does this permanently leak the device's IOVA space?

> 	} else if (dma_use_iova(dma->state)) {
> 		dma_iova_destroy(attach->dev, dma->state, mapped_len, dir,
> 				 DMA_ATTR_MMIO);
> 	} 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);

[Severity: High]
This isn't a bug introduced by this patch, but does this loop over
uninitialized entries?

The for_each_sgtable_dma_sg macro shadows the outer loop variable i, which
represents the number of successfully mapped entries. Instead of bounding
the loop to the mapped entries, it resets i to 0 and unconditionally
iterates up to the total allocated entries (dma->sgt.nents).

Since sg_alloc_table zero-initializes the memory via __GFP_ZERO, the unmapped
elements will have DMA addresses and lengths of 0. Can calling dma_unmap_phys
with a 0 address and 0 length cause IOMMU translation corruption or explicit
DMA faults?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260722233806.3922093-1-dhu@x6u.co?part=1