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

[Matt Evans]

Hi both,

On 16/09/2026 15:19, Christian König wrote:
> On 9/15/26 16:22, Matt Evans wrote:
>> 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".

Leon, please see *** below.

>>>
>>> +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?
> 
> That sounds like a permanent revoke. 

Not quite; the priv->revoked flag tracks temporary periods of
inaccessibility.  An example is VFIO resetting a function; the BAR
mappings as seen by the CPU and DMABUFs made from the BARs are all made
inaccessible before the reset, and made accessible again after the reset.

>> 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.)
> 
> Yeah it's pretty much the naming I would clarify.
> 
> The original idea of notifying the importer that it need to re-create the mapping was resource reclaim.

I feel the (existing) VFIO function reset sequenece is a counterexample,
as the resource isn't being reclaimed/repurposed but users are "held
off" for a period of time across the reset.  (It's then the same
resource that a later access can touch.)

> But when you have some IOCTL or sysfs or whatever to disable a DMA-buf permanently I would call that revoke.

I see where you're coming from now, thanks.

If we were to just call VFIO's "permanently inacessible" state _revoked_
and "temporarily inaccessible" _reclaimed_, then it doesn't help clarity
unfortunately.  Those words are ~synonymous and don't convey permanent
vs momentary.

***

Leon, would you please explain this comment #5?  Were you suggesting
that (even ignoring this patch!) you want to rename priv->revoked to
priv->reclaimed?  If so, why?  (For context, this patch's state =
TEMP_REVOKED is identical to the existing revoked = true.)

In short:  TEMP_${X} vs PERM_${X} most clearly highlights the
meaning/behaviour of the current + new state, regardless of whether
X=REVOKED or X=RECLAIMED.  Although I prefer to use the exisitng
"revoked" I'm not wed to it, but mixing both "reclaimed" & "revoked"
makes the distinction (and semantics, expectations of what can happen
next) much less clear.

> I'm still not 100% sure what a temporary revoke should be. If you have some IOCTL/sysfs/whatever to temporary say to importers "You can't use that resource" then that most likely won't fly.

This patch only provides an ioctl action to "make this inacessible
forever".  There is no direct userspace manipulation to/from the
temporarily-inacessible state, nor need for that.

The only things that lead to a temporary revoke (as a side effect) are
the preexisting VFIO reset ioctls etc.

State is an enum, but this isn't a complex FSM, just an extra "sticky"
state.  And to labour the point, the new temp/perm distinction is only
about VFIO's internal tracking of why it revoked the buffer using the
existing mechanisms (meaning, whether it permits un-revoke); it does not
change the importer.

>> 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.
> 
> That sounds reasonable.
> 
> When a resource becomes unavailable you seriously need completely destroy it, re-create it and then import it again into other drivers which want to use it should it ever become available again.

Yep, quite.

Thanks,


Matt

PS: I should've rebased (eyeballed master and didn't spot the tiny
conflict), so no Sashiko review on v6; I want to resolve this for a v7 soon.

> 
> Regards,
> Christian.
> 
>>
>>
>> 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.)
>>
>>
>