Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] The first invocation of `begin_cpu_udmabuf` skips CPU cache synchronization entirely, potentially leading to data corruption. - [High] Reusing the cached SG table for subsequent CPU accesses with different directions causes a DMA API violation. -- commit 924482c7f4134e0b3fa3b6a2fe23f17b9acba538 Author: Bryam Vargas <[email protected]> udmabuf: serialize the sg_table cache under the reservation lock This commit adds reservation lock serialization to begin_cpu_udmabuf() and end_cpu_udmabuf() to protect the sg_table cache. It resolves a race condition where concurrent SYNC ioctls could observe a NULL sg_table, leading to permanent leaks of the sg_table and unbalanced DMA mappings. Single-threaded callers are unaffected by this change. > diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c > index bced421c0d658..702ae13b97d15 100644 > --- a/drivers/dma-buf/udmabuf.c > +++ b/drivers/dma-buf/udmabuf.c > @@ -226,6 +226,8 @@ static int begin_cpu_udmabuf(struct dma_buf *buf, > struct device *dev = ubuf->device->this_device; > int ret = 0; > > + dma_resv_lock(buf->resv, NULL); > + > if (!ubuf->sg) { > ubuf->sg = get_sg_table(dev, buf, direction); > if (IS_ERR(ubuf->sg)) { > ret = PTR_ERR(ubuf->sg); > ubuf->sg = NULL; > } else { > ubuf->sg_dir = direction; > } [Severity: High] This isn't a bug introduced by this patch, but does the first invocation of begin_cpu_udmabuf() skip CPU cache synchronization entirely? Since get_sg_table() intentionally maps the SG table with the DMA_ATTR_SKIP_CPU_SYNC attribute, and the dma_sync_sgtable_for_cpu() call is isolated inside the else block, the freshly mapped table doesn't get synchronized. If an unprivileged user calls the DMA_BUF_IOCTL_SYNC ioctl with SYNC_START on a udmabuf, it seems this could lead to data corruption where the CPU reads stale cache lines instead of device-written data. > } else { > dma_sync_sgtable_for_cpu(dev, ubuf->sg, direction); [Severity: High] This is a pre-existing issue, but might this result in syncing DMA memory with a mismatched direction? begin_cpu_udmabuf() lazily allocates ubuf->sg using the direction of its very first invocation and saves it in ubuf->sg_dir. If a subsequent DMA_BUF_IOCTL_SYNC call uses a different direction (for example, the first sync is DMA_FROM_DEVICE, and the next is DMA_TO_DEVICE), the cached sg_table is reused. Here, dma_sync_sgtable_for_cpu() is called with the caller's new direction rather than the original mapping direction stored in ubuf->sg_dir. This can lead to incorrect cache flushes on non-coherent architectures and trigger CONFIG_DMA_API_DEBUG warnings. > } > > + dma_resv_unlock(buf->resv); > + > return ret; > } [ ... ] > @@ -246,12 +250,18 @@ static int end_cpu_udmabuf(struct dma_buf *buf, > { > struct udmabuf *ubuf = buf->priv; > struct device *dev = ubuf->device->this_device; > + int ret = 0; > + > + dma_resv_lock(buf->resv, NULL); > > if (!ubuf->sg) > - return -EINVAL; > + ret = -EINVAL; > + else > + dma_sync_sgtable_for_device(dev, ubuf->sg, direction); [Severity: High] This is a pre-existing issue, but similar to the concern above, does this sync the cached sg_table for the device using the caller's new direction instead of the original mapping direction stored in ubuf->sg_dir? > > - dma_sync_sgtable_for_device(dev, ubuf->sg, direction); > - return 0; > + dma_resv_unlock(buf->resv); > + > + return ret; > } -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
