Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Pre-existing issues: - [Critical] Calling `sg_set_page` with a `NULL` page parameter triggers a kernel panic when `CONFIG_DEBUG_VM` and SPARSEMEM without VMEMMAP are enabled. - [High] Calculating the number of scatterlist entries (`nents`) using `DIV_ROUND_UP()` overflows on 32-bit architectures, resulting in `nents = 0` and uninitialized or empty scatterlists for large buffer lengths. - [High] dma_unmap_phys() is invoked iteratively on chunked scatterlist addresses and lengths rather than the exact address and size initially mapped by dma_map_phys(). - [High] The error path in `dma_buf_phys_vec_to_sgt` unmaps all allocated scatterlist entries, including unpopulated ones, leading to invalid DMA unmap requests. -- commit f6cce91b1ad2e1c68270e81a3cd60cffb903c456 Author: David Hu <[email protected]> dma-buf: Split sgl by largest page-aligned chunk Currently, fill_sg_entry() splits the scatterlist using UINT_MAX, resulting in non-page-aligned DMA addresses for subsequent entries. This patch changes the split size to the largest page-aligned chunk within UINT_MAX to satisfy hardware DMA alignment constraints. Reported-by: sashiko-bot <[email protected]> Closes: https://lore.kernel.org/all/[email protected]/ Fixes: 3aa31a8bb11e ("dma-buf: provide phys_vec to scatter-gather mapping routine") Cc: [email protected] Signed-off-by: David Hu <[email protected]> > diff --git a/drivers/dma-buf/dma-buf-mapping.c > b/drivers/dma-buf/dma-buf-mapping.c > index 794acff2546a3..f2bde38fdb1f7 100644 > --- a/drivers/dma-buf/dma-buf-mapping.c > +++ b/drivers/dma-buf/dma-buf-mapping.c > @@ -12,9 +15,9 @@ static struct scatterlist *fill_sg_entry(struct scatterlist > *sgl, size_t length, > unsigned int len, nents; > int i; > > - nents = DIV_ROUND_UP(length, UINT_MAX); > + nents = DIV_ROUND_UP(length, MAX_ENT_SZ); [Severity: High] This isn't a bug introduced by this patch, but does this DIV_ROUND_UP calculation overflow on 32-bit architectures? On 32-bit platforms, size_t is 32 bits and MAX_ENT_SZ evaluates to 0xFFFFF000 assuming 4KB pages. The DIV_ROUND_UP macro expands to: (length + MAX_ENT_SZ - 1) / MAX_ENT_SZ If the length is greater than PAGE_SIZE, the numerator wraps around in 32-bit arithmetic, resulting in a small value that evaluates to 0. This skips the scatterlist setup loop entirely for large buffer lengths. > for (i = 0; i < nents; i++) { > - len = min_t(size_t, length, UINT_MAX); > + len = min_t(size_t, length, MAX_ENT_SZ); > length -= len; > /* > * DMABUF abuses scatterlist to create a scatterlist > @@ -24,7 +27,7 @@ static struct scatterlist *fill_sg_entry(struct scatterlist > *sgl, size_t length, > * does not require the CPU list for mapping or unmapping. > */ > sg_set_page(sgl, NULL, 0, 0); [Severity: Critical] This is a pre-existing issue, but does passing NULL to sg_set_page here cause a kernel panic under certain configurations? When CONFIG_DEBUG_VM and classic SPARSEMEM without VMEMMAP are enabled, sg_set_page calls page_range_contiguous, which unconditionally calls page_to_pfn. On these setups, page_to_pfn determines the memory section by dereferencing the page flags. Dereferencing a NULL page pointer here causes an immediate kernel oops. [ ... ] > @@ -41,14 +44,14 @@ static unsigned int calc_sg_nents(struct dma_iova_state > *state, > > if (!state || !dma_use_iova(state)) { > for (i = 0; i < nr_ranges; i++) > - nents += DIV_ROUND_UP(phys_vec[i].len, UINT_MAX); > + nents += DIV_ROUND_UP(phys_vec[i].len, MAX_ENT_SZ); [Severity: High] This is a pre-existing issue, but does this calculation also suffer from the same 32-bit overflow mentioned above, resolving to 0 for large lengths? [ ... ] [Severity: High] This isn't a bug introduced by this patch, but looking at how the scatterlists populated by fill_sg_entry are mapped and unmapped, are we violating the DMA API contract in dma_buf_free_sgt? In dma_buf_phys_vec_to_sgt, a monolithic physical range is mapped using a single dma_map_phys call, and then fill_sg_entry splits it into chunks. However, in dma_buf_free_sgt, dma_unmap_phys is iteratively called on each individual scatterlist chunk: for_each_sgtable_dma_sg(sgt, sgl, i) dma_unmap_phys(attach->dev, sg_dma_address(sgl), sg_dma_len(sgl), dir, DMA_ATTR_MMIO); The DMA API requires that an allocation must be unmapped using the exact same address and size parameters passed during creation. Chunked unmapping violates this requirement. [Severity: High] This is also a pre-existing issue, but does the error path in dma_buf_phys_vec_to_sgt incorrectly handle unmapping? If dma_map_phys fails, the code jumps to the err_unmap_dma label. The error loop uses: for_each_sgtable_dma_sg(&dma->sgt, sgl, i) dma_unmap_phys(attach->dev, sg_dma_address(sgl), ...); Because the loop macro uses i as its internal iterator up to dma->sgt.nents, which was initialized to the total allocated entries, it overwrites the failure index. This causes it to iterate over all allocated entries, including unpopulated ones, passing 0 for the address and length to dma_unmap_phys, which leads to invalid DMA unmap requests. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
