Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [Medium] Unconditional validation of the flag-gated `ph` parameter causes valid requests clearing metadata to be rejected. - [Low] Mixing `goto`-based error handling with scope-based cleanup helpers (`scoped_guard`) in the same function. - [Low] Unprotected read of `priv->vdev` races with concurrent write to `NULL`. --
commit f7be31969e4c92a551932ef245b8d9697e571272 Author: Zhiping Zhang <[email protected]> vfio/pci: implement get_tph and DMA_BUF_TPH feature This commit implements the dma-buf get_tph callback for vfio-pci-exported dma-bufs. It also adds the VFIO_DEVICE_FEATURE_DMA_BUF_TPH feature to allow userspace to attach TPH metadata to a dma-buf. > diff --git a/drivers/vfio/pci/vfio_pci_dmabuf.c > b/drivers/vfio/pci/vfio_pci_dmabuf.c > index 1a177ce7de546..f1bc3733d3db5 100644 > --- a/drivers/vfio/pci/vfio_pci_dmabuf.c > +++ b/drivers/vfio/pci/vfio_pci_dmabuf.c [ ... ] > @@ -327,12 +358,71 @@ int vfio_pci_core_feature_dma_buf(struct > vfio_pci_core_device *vdev, u32 flags, [ ... ] > +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) > +{ > + struct vfio_device_feature_dma_buf_tph set_tph; > + struct vfio_pci_dma_buf *priv; > + struct dma_buf *dmabuf; > + int ret; > + > + if (!pcie_tph_supported(vdev->pdev)) > + return -EOPNOTSUPP; > + > + ret = vfio_check_feature(flags, argsz, VFIO_DEVICE_FEATURE_SET, > + sizeof(set_tph)); > + if (ret != 1) > + return ret; > + > + if (copy_from_user(&set_tph, arg, sizeof(set_tph))) > + return -EFAULT; > + > + if (set_tph.flags & ~(VFIO_DMA_BUF_TPH_ST | VFIO_DMA_BUF_TPH_ST_EXT)) > + return -EINVAL; > + > + /* PCIe TLP Processing Hint is a 2-bit field. */ > + if (set_tph.ph & ~0x3) > + return -EINVAL; [Severity: Medium] Will this unconditionally reject requests clearing the metadata? The documentation mentions that setting flags to 0 clears previously published metadata. However, because this check is evaluated unconditionally, userspace leaving the ph field uninitialized (containing stack garbage) when passing flags = 0 will cause the kernel to erroneously reject a valid clear request with -EINVAL. Should this validation only occur if the relevant flags are set? > + > + 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] Does this unprotected read of priv->vdev race with concurrent unbound operations? In vfio_pci_dma_buf_cleanup(), there is a concurrent write setting priv->vdev = NULL under the vdev->memory_lock semaphore. Since this read occurs without locks, can it race with the concurrent write and cause KCSAN warnings or load tearing? Would using READ_ONCE() here and WRITE_ONCE() in vfio_pci_dma_buf_cleanup() help prevent this? > + ret = -EINVAL; > + goto out_put; > + } > + > + scoped_guard(mutex, &priv->tph_lock) { > + priv->tph_st = set_tph.steering_tag; > + priv->tph_st_ext = set_tph.steering_tag_ext; > + priv->tph_ph = set_tph.ph; > + priv->tph_st_valid = !!(set_tph.flags & VFIO_DMA_BUF_TPH_ST); > + priv->tph_st_ext_valid = > + !!(set_tph.flags & VFIO_DMA_BUF_TPH_ST_EXT); > + } > + ret = 0; > + > +out_put: [Severity: Low] Is it okay to mix goto-based error handling and scope-based cleanup helpers in this function? The kernel cleanup guidelines discourage mixing goto labels for cleanup (like goto out_put here) and scoped_guard() in the same function to avoid confusing ownership semantics. Would it be better to either use a __free() wrapper for the dmabuf or stick to traditional mutex_lock()/mutex_unlock() here? > + dma_buf_put(dmabuf); > + return ret; > +} -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
