Re: [PATCH v5 9/9] vfio/pci: Permanently revoke a DMABUF on request

Dear robot,

On 15/07/2026 19:11, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Low] The function `vfio_pci_core_feature_dma_buf_revoke` mixes `goto`-based error handling with scope-based cleanup helpers (`scoped_guard`).
> --
> 
> --- Patch [9]: [PATCH v5 9/9] vfio/pci: Permanently revoke a DMABUF on request ---
> commit 668793566cfbdb5cee2b0a69188ebbada38dd4d5
> Author: Matt Evans <[email protected]>
> 
> vfio/pci: Permanently revoke a DMABUF on request
> 
> This commit expands the VFIO DMABUF revocation state to allow permanent
> revocation via a new VFIO_DEVICE_FEATURE_DMA_BUF_REVOKE feature request.
> It refactors the revocation logic into a common function used by both
> the move path and the new feature request path.
> 
>> diff --git a/drivers/vfio/pci/vfio_pci_dmabuf.c b/drivers/vfio/pci/vfio_pci_dmabuf.c
>> index ccfe861f88b66..36bf07530840d 100644
>> --- a/drivers/vfio/pci/vfio_pci_dmabuf.c
>> +++ b/drivers/vfio/pci/vfio_pci_dmabuf.c
> [ ... ]
>> @@ -671,9 +696,72 @@ void vfio_pci_dma_buf_cleanup(struct vfio_pci_core_device *vdev)
> [ ... ]
>> +	if (dmabuf->ops != &vfio_pci_dmabuf_ops ||
>> +	    READ_ONCE(priv->vdev) != vdev) {
>> +		ret = -ENODEV;
>> +		goto out_put_buf;
>> +	}
>> +
>> +	scoped_guard(rwsem_write, &vdev->memory_lock) {
> 
> [Severity: Low]
> Is it intentional to mix goto-based error handling and scope-based cleanup
> helpers in the same function?

Yes.

> The subsystem guidelines for cleanup helpers recommend against mixing goto
> labels with scoped_guard in the same routine, to avoid confusing ownership
> semantics and potential resource leaks during future refactoring.
> 
> Could the dma_buf_put cleanup at the out_put_buf label be converted to use a
> scope-based helper, or could the scoped_guard be replaced with traditional
> locking, to keep the error handling style consistent throughout
> vfio_pci_core_feature_dma_buf_revoke?

Those guidelines are good, and exist for clarity.  It's a good idea to
avoid jumping into or out of scoped_guard blocks.  In the case of this
function, it is clear what is going on regarding the scoped_guard, and
the pattern is:

- Verify input values; exit if unpalatable, else:
- Do the scoped_guard block to perform action

There is, IMO, very low scope for confusion from mixing one goto +
scoped_guard here.

(Mixing goto-out and return is worse, and this function is guilty of
that.  However, converting four if()s to ret = -EFOO; goto out; would
make the function longer and actually a little harder to read.

IOW I think it's fine as is.


Matt


> 
>> +		if (priv->status == VFIO_PCI_DMABUF_PERM_REVOKED) {
>> +			ret = -EBADFD;
>> +		} else {
>> +			vfio_pci_dma_buf_set_status(priv,
>> +						    VFIO_PCI_DMABUF_PERM_REVOKED);
>> +			ret = 0;
>> +		}
>> +	}
>> +
>> +out_put_buf:
>> +	dma_buf_put(dmabuf);
>> +
>> +	return ret;
>> +}
>> +#endif /* CONFIG_VFIO_PCI_DMABUF */
>