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

[Matt Evans]

Hi Kevin,

On 16/06/2026 10:26, Tian, Kevin wrote:
>> From: Matt Evans <matt@xxxxxxxxxx>
>> Sent: Wednesday, June 10, 2026 11:43 PM
>>
>> Expand the VFIO DMABUF revocation state to three states:
>> Not revoked, temporarily revoked, and permanently revoked.
>>
>> The first two are for existing transient revocation, e.g. across a
>> function reset, and the DMABUF is put into the last in response to a
>> new VFIO feature VFIO_DEVICE_FEATURE_DMA_BUF.
> 
> VFIO_DEVICE_FEATURE_DMA_BUF_REVOKE
> 
>>
>> VFIO_DEVICE_FEATURE_DMA_BUF passes a DMABUF by fd and requests that
>> the DMABUF is permanently revoked.  On success, it's guaranteed that
> 
> ditto

Argh, thanks for catching these.  Fixed.

>> the buffer can never be imported/attached/mmap()ed in future, that
>> dynamic imports have been cleanly detached, and that all mappings have
>> been made inaccessible/PTEs zapped.
>>
>> This is useful for lifecycle management, to reclaim VFIO PCI BAR
>> ranges previously delegated to a subordinate client process: The
>> driver process can ensure that the loaned resources are revoked when
>> the client is deemed "done", and exported ranges can be safely re-used
>> elsewhere.
> 
> probably clarify that re-use by creating a new dmabuf fd as the original
> one is essentially zombie now.

Reworded this, plus added a note re the change below.

>>
>> +/* Set the DMABUF's revocation status (OK or temporarily/permanently
>> revoked) */
>> +static void vfio_pci_dma_buf_set_status(struct vfio_pci_dma_buf *priv,
>> +					enum vfio_pci_dma_buf_status
>> new_status)
>> +{
>> +	bool was_revoked;
>> +
>> +	lockdep_assert_held_write(&priv->vdev->memory_lock);
>> +
>> +	if (priv->status == VFIO_PCI_DMABUF_PERM_REVOKED ||
>> +	    priv->status == new_status) {
>> +		return;
>> +	}
> 
> the only interface to request PERM_REVOKED is via the new ioctl.
> 
> vfio_pci_core_feature_dma_buf_revoke() returns -EBADFD if
> it's already in PERM_REVOKED.
> 
> so this check shouldn't be reached, suggesting a warning.

Good point, both any change to PERM_REVOKED or a double-set of the same
state indicate some caller has gone wrong.  Added a warning.

>> +
>> +	dma_buf_invalidate_mappings(priv->dmabuf);
>> +	dma_resv_wait_timeout(priv->dmabuf->resv,
>> +			      DMA_RESV_USAGE_BOOKKEEP, false,
>> +			      MAX_SCHEDULE_TIMEOUT);
>> +	dma_resv_unlock(priv->dmabuf->resv);
> 
> It's existing code but while at it let's make above conditional to
> the actual revoke path. for unrevoked it's not required given the
> previous revoke already cleans up everything.

I noticed this too though I was consciously trying to keep the diff as
small as possible.  But with this feedback from both you and Praan, I'll
move this.  It's still pretty readable before/after.

> otherwise,
> 
> Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>


Thank you.


Matt