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

[Matt Evans <matt@ozlabs.org>]

Hi Kevin,


Digging this one up again,

On 17/06/2026 17:08, Matt Evans wrote:
> Hi Kevin,
> 
> On 16/06/2026 10:26, Tian, Kevin wrote:
>>> From: Matt Evans <matt@ozlabs.org>
>>> 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.

Well, after the D0/D3 reset thread, I noticed while testing that a
double-revoke will naturally happen when cleaning up a buffer that was
already revoked by a device having previously transitioned to D3.

Similarly, cleaning up a buffer that was explicitly (permanently)
revoked leads to an attempt to set TEMP whilst PERM, and this is OK too.

So the only "surprising" case is a buffer already in the PERM_REVOKED
state getting a second PERM_REVOKED (which is weeded out in the caller
as you point out).  Any new caller asking for PERM_REVOKED repeatedly is
odd, but still gets what it wants.  I really don't think a warning is
warranted just for that (it's safe either way).  Sending this
explanation separately, so you are not too disappointed if v4 reverts to
this existing condition above...  :)


Thanks,


Matt


> 
>>> +
>>> +	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@intel.com>
> 
> 
> Thank you.
> 
> 
> Matt
>