Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [High] The use of dma_resv_lock_interruptible in end_cpu_udmabuf allows signal interruptions to permanently bypass device cache synchronization. Pre-existing issues: - [High] The first CPU access fails to synchronize CPU caches due to omitting the manual sync after mapping with DMA_ATTR_SKIP_CPU_SYNC. - [High] dma_sync_sgtable_for_cpu and dma_sync_sgtable_for_device are called with a potentially mismatched DMA direction compared to the original mapping. -- commit c61af695e850a91fed44895b4a5acba2b9e7c206 Author: Bryam Vargas <[email protected]> udmabuf: serialize the sg_table cache under the reservation lock begin_cpu_udmabuf() builds and caches ubuf->sg with an unserialised check-then-set, and end_cpu_udmabuf() reads the same field unlocked. The core invokes both cpu-access hooks without holding the reservation lock and DMA_BUF_IOCTL_SYNC is unlocked, so concurrent SYNC ioctls on a shared udmabuf fd race on ubuf->sg. Serialize both hooks under the buffer's reservation lock. > diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c > index bced421c0d658..d6a137f0de1f5 100644 > --- a/drivers/dma-buf/udmabuf.c > +++ b/drivers/dma-buf/udmabuf.c > @@ -226,6 +226,10 @@ static int begin_cpu_udmabuf(struct dma_buf *buf, > struct device *dev = ubuf->device->this_device; > int ret = 0; > > + ret = dma_resv_lock_interruptible(buf->resv, NULL); > + if (ret) > + return ret; > + > 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; > } > } else { [Severity: High] This is a pre-existing issue, but does this skip synchronizing CPU caches on the first access? Because the manual sync is isolated within the else branch, it looks like the initial mapping via get_sg_table() (which uses DMA_ATTR_SKIP_CPU_SYNC) leaves the CPU to read stale cached data instead of device-written data. > dma_sync_sgtable_for_cpu(dev, ubuf->sg, direction); [Severity: High] This is a pre-existing issue, but does this violate the DMA API requirement that sync directions must match the mapped direction? The cached sg_table was mapped with the initial direction and stored in ubuf->sg_dir, but subsequent calls to dma_sync_sgtable_for_cpu() blindly pass the new user-provided direction. > } > > + dma_resv_unlock(buf->resv); > + > return ret; > } > > @@ -246,12 +252,20 @@ static int end_cpu_udmabuf(struct dma_buf *buf, > { > struct udmabuf *ubuf = buf->priv; > struct device *dev = ubuf->device->this_device; > + int ret = 0; > + > + ret = dma_resv_lock_interruptible(buf->resv, NULL); > + if (ret) > + return ret; [Severity: High] Does this allow signal interruptions to permanently bypass device cache synchronization? If a signal (e.g., SIGINT) is delivered while waiting to acquire the reservation lock, end_cpu_udmabuf() returns early. Teardown and completion hooks generally shouldn't be bypassable by signals, and kernel callers do not retry on -EINTR. Should this use uninterruptible locking instead? > > 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 can this trigger DMA API debug warnings or IOMMU faults? Similar to begin_cpu_udmabuf(), this calls dma_sync_sgtable_for_device() with the user-provided direction instead of the originally mapped direction (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
