Re: [PATCH v2 1/2] vhost/vsock: discard IOTLB when ACCESS_PLATFORM is cleared

On Mon, Aug 03, 2026 at 11:18:50PM -0400, Michael S. Tsirkin wrote:
> Why lock down all vqs like this?  Would this work just as well instead?
>
>       iotlb = vsock->dev.iotlb;
>       vsock->dev.iotlb = NULL;
>
>       for (i = 0; i < ARRAY_SIZE(vsock->vqs); i++) {
>               mutex_lock(&vsock->vqs[i].mutex);
>               vq = &vsock->vqs[i];
>               vq->iotlb = NULL;
>               memset(vq->meta_iotlb, 0, sizeof(vq->meta_iotlb));
>               vq->acked_features = features;
>               mutex_unlock(&vsock->vqs[i].mutex);
>       }
>
> and if no why not?

Let me add a little more detail to my earlier reasoning. Locking the VQs
one at a time does reduce lock hold time and avoids blocking one queue
while waiting for another. However, the additional blocking from taking
all VQ mutexes is confined to the `VHOST_SET_FEATURES` transition and
does not add any steady-state data-path overhead. vsock has only two VQs,
and once the locks have been acquired, the critical section only updates
a few pointers, metadata caches, and feature fields.

`dev->iotlb` is shared by all VQs, while `vq->iotlb`, `meta_iotlb`, and
`acked_features` are per-VQ state protected by that VQ's mutex. A kick
handler only holds its own VQ mutex. The following interleaving therefore
seems possible:

```text
worker: holds vq->mutex with the old vq->iotlb
ioctl:  sets dev->iotlb = NULL
ioctl:  waits for vq->mutex
worker: continues processing with the old per-VQ state
```

During this window, the state can be:

```text
vq->iotlb          = old_iotlb
vq->meta_iotlb     = old mappings
vq->acked_features = ACCESS_PLATFORM enabled
dev->iotlb         = NULL
```

`vq_meta_prefetch()` may still use the old `vq->iotlb` and metadata
cache, while `translate_desc()` sees `dev->iotlb == NULL` and falls back
to `dev->umem`. The same handler could therefore access the vring through
the old IOTLB and then interpret a descriptor address as a GPA when
translating the payload.

If that IOVA has no corresponding GPA mapping, `translate_desc()`
returns `-EFAULT` and aborts the current queue-processing pass. If it
happens to fall within a valid GPA mapping, the translation may produce
an iovec for a different HVA.

Clearing `dev->iotlb` is also different from replacing one mapping table
with another under the same address model, since it changes the address
interpretation from IOVA to GPA.

As I mentioned in my earlier reply, I do not see any check in the
vhost-vsock `VHOST_SET_FEATURES` ioctl path that guarantees all VQs are
stopped or otherwise quiesced, so I thought the transition also needed
to be safe while a VQ may still be active.

Please let me know if I am missing such a guarantee elsewhere. Thanks.