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

[Matt Evans <matt@ozlabs.org>]

Hi Christian,

On 15/09/2026 12:13, Christian König wrote:
> On 9/15/26 09:20, Leon Romanovsky wrote:
>> On Mon, Sep 14, 2026 at 01:13:36PM +0100, Matt Evans wrote:
>>> Hi Leon,
>>>
>>> On 14/09/2026 12:54, Leon Romanovsky wrote:
>>>> On Mon, Sep 14, 2026 at 08:36:47AM -0300, Jason Gunthorpe wrote:
>>>>> On Sun, Sep 13, 2026 at 07:52:44PM +0300, Leon Romanovsky wrote:
>>>>>> On Fri, Sep 11, 2026 at 10:41:57PM +0100, Matt Evans wrote:
>>>>>>> Expand the VFIO DMABUF revocation state to three states:
>>>>>>> Not revoked, temporarily revoked, and permanently revoked.
>>>>>>
>>>>>> The thing is that "temporarily revoked" is actually the standard
>>>>>> invalidate_mappings/move_notify mechanism of DMABUF, which wasn't good
>>>>>> for VFIO.
>>>>>
>>>>> I think temporarily revokes here means it is revoked from a dmabuf
>>>>> perspective
>>>>
>>>> My guess is that this is more of a "change owner" operation than a
>>>> revoke operation.
>>>>
>>>> The main issue here is that we have to guess the semantics instead of
>>>> having a properly named and documented operation.
>>>
>>> Apologies if the cover letter for the series and patch commit message (which
>>> cover this) are unclear about the motivations and semantics.  On the commit
>>> message, can you suggest clarifications:
>>>
>>> "This is useful for lifecycle management, to reclaim VFIO PCI BAR
>>> ranges previously delegated to a subordinate client process: by
>>> revoking, the driver process can ensure that the loaned resources are
>>> made inaccessible when the client is deemed "done".  The original
>>> DMABUF is defunct, and BAR resources can then be safely re-exported
>>> for use by new clients."
>>>
>>> Given what I'll explain below, do give suggestions please.  There is more
>>> context in the cover letter (the volume of which I didn't think appropriate
>>> for the commit message).
>>
>> 1. Do not mix "driver" and "client" in the same description. For a
>> non-native English speaker, "driver" has a very specific meaning in the
>> context of the Linux kernel.
> 
> In the context of DMA-buf it has also proven vital to clearly use the terms importer and exporter to describe the different roles a driver can have.
> 
> I can't count how often there was confusion because people (me included) just used "driver" and it wasn't clear which role was meant.

:)  I see where the quote can be clearer.  FWIW the "driver process" was
referring to a userspace driver (which is a legitimate use of the D-word
with VFIO, but still).  I'll clarify that, and will indicate this
mechanism is used by userspace to influence the VFIO _exporter_ behaviour.

>> 2. Explain the lifecycle in the commit message, and why "revoke", which
>> is effectively what the importer does, is not sufficient.
>>
>> 3. The more you put in the cover letter, the less likely people are to
>> read it.
>>
>> 4. Commit messages should describe the patches themselves, since they
>> are what remains visible in the git log, unlike the cover letter.
>>
>> 5. I would call what you describe as "temporarily revoke" is actually "reclaim".
> 
> +1

I don't follow here, sorry.  Would you please elaborate?

Currently VFIO uses the priv->revoked flag to track whether
it-the-exporter had previously done dma_buf_invalidate_mappings() on a
buffer and is now causing all .attach requests to fail.  Do you mean
that (even without this series) you want to call that concept
priv->reclaimed instead?

Or do you mean that you don't like the words "temporary"/"permanent" and
are looking for another name for a temporarily unavailable buffer?  (If
so, I find "not revoked", "reclaimed", "revoked" much less clear than
not/temp/perm revoked, as such names give no hint as to what to expect.
But I may have misunderstood what you're getting at.)

From the importer side, there is no change from this patch: they might
observe an invalidate_mappings() and find attach() of a given buffer now
fails, same as before this patch.  This is only about guaranteeing the
impossibility of an importer ever being able to re-attach in future.


Thanks,


Matt


PS:  A hypothetical alternative way of doing what this patch is doing is
instead to have:

 - existing priv->revoked
 - new 	    priv->revoked_flag_is_immutable

Then, the ioctl triggers an invalidate_mappings() for a targeted DMABUF,
sets revoked = true, and revoked_flag_is_immutable = true.  The new flag
prevents a future vfio_pci_dma_buf_move(false) from clearing revoked.
For example, a reset doing `move(true); reset; move(false);` can make
all DMABUFs available to attach again, except for those marked
immutably-revoked.  (That is equivalent to this new "permanent" state.)