Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Low] Lockless plain load of `priv->vdev` races with concurrent modification in cleanup path. - [Low] Missing `WRITE_ONCE()` for `priv->revoked` leads to data race with `READ_ONCE()`. --
commit d524eab36e9482385fe1c1a6c6280e667bf56a9c Author: Zhiping Zhang <[email protected]> Subject: vfio/pci: implement get_pci_tph and DMA_BUF_TPH feature This commit implements dma-buf get_pci_tph for vfio-pci exported dma-bufs and adds VFIO_DEVICE_FEATURE_DMA_BUF_TPH. This allows userspace to publish TPH metadata for a VFIO-owned device. > diff --git a/drivers/vfio/pci/vfio_pci_dmabuf.c > b/drivers/vfio/pci/vfio_pci_dmabuf.c > index c16f460c01d68..8de72f9e75024 100644 > --- a/drivers/vfio/pci/vfio_pci_dmabuf.c > +++ b/drivers/vfio/pci/vfio_pci_dmabuf.c > @@ -30,7 +38,7 @@ static int vfio_pci_dma_buf_attach(struct dma_buf *dmabuf, > if (!attachment->peer2peer) > return -EOPNOTSUPP; > > - if (priv->revoked) > + if (READ_ONCE(priv->revoked)) [Severity: Low] Since this updates the read of priv->revoked to use READ_ONCE(), should the corresponding writers in vfio_pci_dma_buf_move() also be updated to use WRITE_ONCE()? For example, when a concurrent state change like a device reset writes to this field: vfio_pci_dma_buf_move() { ... priv->revoked = true; ... } Will the lack of WRITE_ONCE() here trigger KCSAN data race warnings when evaluated concurrently by the new READ_ONCE() check? > return -ENODEV; > > if (!dma_buf_attach_revocable(attachment)) [ ... ] > @@ -333,6 +362,72 @@ int vfio_pci_core_feature_dma_buf(struct > vfio_pci_core_device *vdev, u32 flags, > return ret; > } > > +int vfio_pci_core_feature_dma_buf_tph(struct vfio_pci_core_device *vdev, > + u32 flags, > + struct vfio_device_feature_dma_buf_tph > __user *arg, > + size_t argsz) > +{ [ ... ] > + dmabuf = dma_buf_get(set_tph.dmabuf_fd); > + if (IS_ERR(dmabuf)) > + return PTR_ERR(dmabuf); > + > + if (dmabuf->ops != &vfio_pci_dmabuf_ops) { > + ret = -EINVAL; > + goto out_put; > + } > + > + priv = dmabuf->priv; > + if (priv->vdev != vdev) { [Severity: Low] Can this lockless read of priv->vdev race with concurrent modifications in the cleanup path? If userspace calls the VFIO_DEVICE_FEATURE_DMA_BUF_TPH ioctl on device A, passing a dmabuf_fd belonging to device B, while device B is concurrently being closed, could this race with vfio_pci_dma_buf_cleanup()? vfio_pci_dma_buf_cleanup() { ... priv->vdev = NULL; ... } Will this concurrent plain store trigger KCSAN warnings during the pointer equality check? > + ret = -EINVAL; > + goto out_put; > + } > + > + ret = dma_resv_lock_interruptible(dmabuf->resv, NULL); -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
