vhost_vsock_set_features() leaves the device IOTLB attached when
userspace clears VIRTIO_F_ACCESS_PLATFORM. Descriptors can therefore
continue to use translations installed before the feature change,
including HVAs made stale by a later memory table update.

Detach the IOTLB before acknowledging a feature mask without
ACCESS_PLATFORM. Hold all virtqueue mutexes in index order while clearing
the device and virtqueue IOTLB pointers, resetting metadata caches, and
updating the acknowledged features. This prevents a kick handler from
observing a mixed translation state.

Free the old IOTLB after releasing the virtqueue mutexes. Also drop
the old IOTLB miss messages and wake readers now that the device no
longer accepts IOTLB updates.

Fixes: e13a6915a03f ("vhost/vsock: add IOTLB API support")
Signed-off-by: Jia Jia <physicalmtea@gmail.com>
---
 drivers/vhost/vsock.c | 43 ++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 38 insertions(+), 5 deletions(-)

diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
index 9aaab6bb8061..562b9e139a76 100644
--- a/drivers/vhost/vsock.c
+++ b/drivers/vhost/vsock.c
@@ -851,6 +851,34 @@ static int vhost_vsock_set_cid(struct vhost_vsock *vsock, u64 guest_cid)
 	return 0;
 }
 
+/* Caller must hold the device mutex. */
+static void vhost_vsock_clear_iotlb(struct vhost_vsock *vsock, u64 features)
+{
+	struct vhost_iotlb *iotlb;
+	struct vhost_virtqueue *vq;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(vsock->vqs); i++)
+		mutex_lock_nested(&vsock->vqs[i].mutex, i);
+
+	iotlb = vsock->dev.iotlb;
+	vsock->dev.iotlb = NULL;
+
+	for (i = 0; i < ARRAY_SIZE(vsock->vqs); i++) {
+		vq = &vsock->vqs[i];
+		vq->iotlb = NULL;
+		memset(vq->meta_iotlb, 0, sizeof(vq->meta_iotlb));
+		vq->acked_features = features;
+	}
+
+	for (i = ARRAY_SIZE(vsock->vqs); i-- > 0;)
+		mutex_unlock(&vsock->vqs[i].mutex);
+
+	vhost_clear_msg(&vsock->dev);
+	vhost_iotlb_free(iotlb);
+	wake_up_interruptible_poll(&vsock->dev.wait, EPOLLIN | EPOLLRDNORM);
+}
+
 static int vhost_vsock_set_features(struct vhost_vsock *vsock, u64 features)
 {
 	struct vhost_virtqueue *vq;
@@ -872,11 +900,16 @@ static int vhost_vsock_set_features(struct vhost_vsock *vsock, u64 features)
 
 	vsock->seqpacket_allow = features & (1ULL << VIRTIO_VSOCK_F_SEQPACKET);
 
-	for (i = 0; i < ARRAY_SIZE(vsock->vqs); i++) {
-		vq = &vsock->vqs[i];
-		mutex_lock(&vq->mutex);
-		vq->acked_features = features;
-		mutex_unlock(&vq->mutex);
+	if (!(features & (1ULL << VIRTIO_F_ACCESS_PLATFORM)) &&
+	    vsock->dev.iotlb) {
+		vhost_vsock_clear_iotlb(vsock, features);
+	} else {
+		for (i = 0; i < ARRAY_SIZE(vsock->vqs); i++) {
+			vq = &vsock->vqs[i];
+			mutex_lock(&vq->mutex);
+			vq->acked_features = features;
+			mutex_unlock(&vq->mutex);
+		}
 	}
 	mutex_unlock(&vsock->dev.mutex);
 	return 0;
-- 
2.34.1

On Fri, Jul 31, 2026 at 06:34:13PM +0800, Jia Jia wrote:
> vhost_vsock_set_features() leaves the device IOTLB attached when
> userspace clears VIRTIO_F_ACCESS_PLATFORM. Descriptors can therefore
> continue to use translations installed before the feature change,
> including HVAs made stale by a later memory table update.
> 
> Detach the IOTLB before acknowledging a feature mask without
> ACCESS_PLATFORM. Hold all virtqueue mutexes in index order while clearing
> the device and virtqueue IOTLB pointers, resetting metadata caches, and
> updating the acknowledged features. This prevents a kick handler from
> observing a mixed translation state.
> 
> Free the old IOTLB after releasing the virtqueue mutexes. Also drop
> the old IOTLB miss messages and wake readers now that the device no
> longer accepts IOTLB updates.
> 
> Fixes: e13a6915a03f ("vhost/vsock: add IOTLB API support")
> Signed-off-by: Jia Jia <physicalmtea@gmail.com>
> ---
>  drivers/vhost/vsock.c | 43 ++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 38 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
> index 9aaab6bb8061..562b9e139a76 100644
> --- a/drivers/vhost/vsock.c
> +++ b/drivers/vhost/vsock.c
> @@ -851,6 +851,34 @@ static int vhost_vsock_set_cid(struct vhost_vsock *vsock, u64 guest_cid)
>  	return 0;
>  }
>  
> +/* Caller must hold the device mutex. */
> +static void vhost_vsock_clear_iotlb(struct vhost_vsock *vsock, u64 features)
> +{
> +	struct vhost_iotlb *iotlb;
> +	struct vhost_virtqueue *vq;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(vsock->vqs); i++)
> +		mutex_lock_nested(&vsock->vqs[i].mutex, i);
> +
> +	iotlb = vsock->dev.iotlb;
> +	vsock->dev.iotlb = NULL;
> +
> +	for (i = 0; i < ARRAY_SIZE(vsock->vqs); i++) {
> +		vq = &vsock->vqs[i];
> +		vq->iotlb = NULL;
> +		memset(vq->meta_iotlb, 0, sizeof(vq->meta_iotlb));
> +		vq->acked_features = features;
> +	}
> +
> +	for (i = ARRAY_SIZE(vsock->vqs); i-- > 0;)
> +		mutex_unlock(&vsock->vqs[i].mutex);

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?


> +	vhost_clear_msg(&vsock->dev);
> +	vhost_iotlb_free(iotlb);
> +	wake_up_interruptible_poll(&vsock->dev.wait, EPOLLIN | EPOLLRDNORM);
> +}
> +
>  static int vhost_vsock_set_features(struct vhost_vsock *vsock, u64 features)
>  {
>  	struct vhost_virtqueue *vq;
> @@ -872,11 +900,16 @@ static int vhost_vsock_set_features(struct vhost_vsock *vsock, u64 features)
>  
>  	vsock->seqpacket_allow = features & (1ULL << VIRTIO_VSOCK_F_SEQPACKET);
>  
> -	for (i = 0; i < ARRAY_SIZE(vsock->vqs); i++) {
> -		vq = &vsock->vqs[i];
> -		mutex_lock(&vq->mutex);
> -		vq->acked_features = features;
> -		mutex_unlock(&vq->mutex);
> +	if (!(features & (1ULL << VIRTIO_F_ACCESS_PLATFORM)) &&
> +	    vsock->dev.iotlb) {
> +		vhost_vsock_clear_iotlb(vsock, features);
> +	} else {
> +		for (i = 0; i < ARRAY_SIZE(vsock->vqs); i++) {
> +			vq = &vsock->vqs[i];
> +			mutex_lock(&vq->mutex);
> +			vq->acked_features = features;
> +			mutex_unlock(&vq->mutex);
> +		}
>  	}
>  	mutex_unlock(&vsock->dev.mutex);
>  	return 0;
> -- 
> 2.34.1

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.

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?

Thanks for the review.

My understanding is as follows. The proposed sequence protects the
lifetime of the old IOTLB, but it does not keep the translation state
consistent during the transition.

dev->iotlb is shared by all VQs, while vq->iotlb, meta_iotlb, and
acked_features are per-VQ state. A kick handler only holds its own VQ
mutex. If dev->iotlb is cleared first, a handler that already holds a VQ
mutex can continue using 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 observe both the IOVA/IOTLB and GPA/umem
views.

Locking each VQ in turn before freeing the old IOTLB prevents a lifetime
issue, but it does not remove this mixed-state window. Taking all VQ
mutexes before changing dev->iotlb lets active handlers finish and
prevents new handlers from running until the shared and per-VQ state has
been updated consistently.

If VHOST_SET_FEATURES is guaranteed to run only while all VQs are stopped
or otherwise quiesced, then the shorter sequence should be sufficient.
Since the ioctl itself does not enforce that, I thought this transition
also needed to be safe while a VQ may still be active.

Please correct me if I have misunderstood anything. Thank you very much.