Re: [PATCH 4/4] virtio-iommu: Clarify hot-unplug behavior

[Akihiko Odaki]

On 2023/08/11 0:10, Jean-Philippe Brucker wrote:
> On Fri, Aug 04, 2023 at 03:19:27PM +0900, Akihiko Odaki wrote:
>> On 2023/08/04 0:32, Jean-Philippe Brucker wrote:
>>> Since it is not obvious what the virtio-iommu device or driver should do
>>> when an endpoint is removed [1], add some guidance in the specification.
>>> Follow the same principle as other (hardware) IOMMU devices: clearing
>>> endpoint ID state on endpoint removal is the responsibility of the
>>> driver, not the IOMMU device.
>>>
>>> On most hardware IOMMU devices, the endpoint ID state is kept in tables
>>> managed by the driver, so intuitively the driver should be updating the
>>> tables on hot-unplug, so that a new endpoint plugged later with the same
>>> ID does not inherit stale translations. Besides, hardware IOMMUs have no
>>> knowledge of endpoint removal. It is less obvious for virtio-iommu
>>> because endpoint states are managed with ATTACH/DETACH requests, and a
>>> virtual platform could in theory update the endpoint state when it is
>>> removed. Existing VMMs like QEMU don't do it, and rightly expect the
>>> driver to manage endpoint ID state like with other IOMMUs. It is not
>>> invalid for a VMM to clean up state on endpoint removal, but a driver
>>> shouldn't count on it.
>>
>> I think it's better to say it's invalid to detach on endpoint removal.
>
> It's certainly not a clear cut choice and I'm still hesitating whether we
> should allow, deny or let the VMM choose what to do.
>
> However, it looks like crosvm already detaches the domain when an endpoint
> is unplugged:
>
> https://github.com/google/crosvm/blob/e8b2cd080e8174948563567d395c2a42416f2807/devices/src/virtio/iommu/sys/unix.rs#L68
>
> If I understood correctly, this function is called on PCIe hot-unplug, and
> endpoint_map represents the domain attached to an endpoint ID. So the
> domain is detached on endpoint removal. If a DETACH request is sent
> afterwards, it will still succeed (detach_endpoint() in
> devices/src/virtio/iommu.rs returns detached=true, which causes DETACH to
> succeed, even if the endpoint is not in endpoint_map). But I could be
> wrong as I'm not familiar with crosvm or rust.
>
> That does make the decision a little easier, because if we did
> retroactively specify that detaching on removal is invalid, this code
> would become a bug. But in my opinion crosvm isn't doing anything wrong
> here. It feels valid and even good practice to clean up all state
> associated to an endpoint when it is removed, to avoid leaks and stale
> objects.

In this case I think it's creating use-after-free hazard. The guest
believes it is managing domains and assume a domain is available until
it makes a DETACH request for the last endpoint attached to the domain.

However, if the host automatically detaches an endpoint from a domain
and if the domain has no other endpoints, the domain will be gone and
the domain ID may be reused for another domain. If the guest makes a
request with the stale domain ID before it becomes aware that the device
is unplugged, the request will be performed on some arbitrary domain and
may break things.

By the way, crosvm's logic to detach endpoint on removal looks incorrect
for me. A domain may have several endpoints attached, but the code looks
like it's always destroying a domain whether there are other endpoints
attached to the domain. I'm adding Zide Chen, who wrote the code
according to git blame, and crosv...@chromium.org to CC.

Regards,
Akihiko Odaki

[Jean-Philippe Brucker]

That's an excellent point, there is a race:

VMM GUEST
Remove ep 1
detach domain 1
destroy domain 1
ATTACH ep 2 to domain 1
Handle ATTACH
create domain 1

Notify that ep 1 is gone
Handle notification, DETACH ep 1

The guest tries to attach ep 2 to domain 1, which succeeds. So the
guest expects that ep 2 is now able to access the mappings that were on
domain 1. But instead it's a new empty domain.

I think that's a good justification for adding a device normative
statement, something like:

The device SHOULD only detach endpoints from domains when requested by
the driver; either when handling ATTACH and DETACH requests or when
performing a device reset.

>
> By the way, crosvm's logic to detach endpoint on removal looks incorrect for
> me. A domain may have several endpoints attached, but the code looks like
> it's always destroying a domain whether there are other endpoints attached
> to the domain. I'm adding Zide Chen, who wrote the code according to git
> blame, and crosv...@chromium.org to CC.

Link to this thread for more context:
https://lore.kernel.org/virtio-dev/20230803153238.541...@linaro.org/

I thought crosvm rejected attaching multiple endpoints to one domain but I
think I misread. Rejecting multiple attach would be a straightforward fix
(it's allowed by the spec), though it would prevent assigning endpoints
that cannot be isolated from each others by the hardware (the driver won't
attach those to different domains, if it's made aware that they should be
in the same IOMMU group, for example if they are on a conventional PCI
bus).

Thanks,
Jean

[Akihiko Odaki]

Now we figured out an endpoint should not be detached from a domain
without a request from the driver anyway so the code to detach an
endpoint can be simply removed.

Regards,
Akihiko Odaki

[Jean-Philippe Brucker]

On Sat, Aug 12, 2023 at 03:25:10PM +0900, Akihiko Odaki wrote:
> > > By the way, crosvm's logic to detach endpoint on removal looks incorrect for
> > > me. A domain may have several endpoints attached, but the code looks like
> > > it's always destroying a domain whether there are other endpoints attached
> > > to the domain. I'm adding Zide Chen, who wrote the code according to git
> > > blame, and crosv...@chromium.org to CC.
> >
> > Link to this thread for more context:
> > https://lore.kernel.org/virtio-dev/20230803153238.541...@linaro.org/
> >
> > I thought crosvm rejected attaching multiple endpoints to one domain but I
> > think I misread. Rejecting multiple attach would be a straightforward fix
> > (it's allowed by the spec), though it would prevent assigning endpoints
> > that cannot be isolated from each others by the hardware (the driver won't
> > attach those to different domains, if it's made aware that they should be
> > in the same IOMMU group, for example if they are on a conventional PCI
> > bus).
>
> Now we figured out an endpoint should not be detached from a domain without
> a request from the driver anyway so the code to detach an endpoint can be
> simply removed.

Yes, but I think the other detach path, when handling ATTACH or DETACH
requests, doesn't support domains with multiple endpoints attached either:

// Currently, we only support detaching an endpoint if it is the only endpoint attached
// to its domain.

But the ATTACH handler seems to accept attaching multiple endpoints to the
same domain?

Thanks,
Jean

[Akihiko Odaki]

We saw no response from crosvm people for a few weeks so I opened an
issue on their bug tracker for heads-up:
https://issuetracker.google.com/u/1/issues/298297288

Regards,
Akihiko Odaki