[PATCH 2/3] vhost: fix virtqueue access lock in datapath

[PATCH 2/3] vhost: fix virtqueue access lock in datapath

[David Marchand]

Now that a r/w lock is used, the access_ok field should only be updated
under a write lock.

Since the datapath code only takes a read lock on the virtqueue to check
access_ok, this lock must be released and a write lock taken before
calling vring_translate().

Fixes: 03f77d66d966 ("vhost: change virtqueue access lock to a read/write one")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 lib/vhost/virtio_net.c | 60 +++++++++++++++++++++++++++++++-----------
 1 file changed, 44 insertions(+), 16 deletions(-)

diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c
index 759a78e3e3..4116f79d4f 100644
--- a/lib/vhost/virtio_net.c
+++ b/lib/vhost/virtio_net.c
@@ -1694,6 +1694,17 @@ virtio_dev_rx_packed(struct virtio_net *dev,
 	return pkt_idx;
 }
 
+static void
+virtio_dev_vring_translate(struct virtio_net *dev, struct vhost_virtqueue *vq)
+{
+	rte_rwlock_write_lock(&vq->access_lock);
+	vhost_user_iotlb_rd_lock(vq);
+	if (!vq->access_ok)
+		vring_translate(dev, vq);
+	vhost_user_iotlb_rd_unlock(vq);
+	rte_rwlock_write_unlock(&vq->access_lock);
+}
+
 static __rte_always_inline uint32_t
 virtio_dev_rx(struct virtio_net *dev, struct vhost_virtqueue *vq,
 	struct rte_mbuf **pkts, uint32_t count)
@@ -1708,9 +1719,13 @@ virtio_dev_rx(struct virtio_net *dev, struct vhost_virtqueue *vq,
 
 	vhost_user_iotlb_rd_lock(vq);
 
-	if (unlikely(!vq->access_ok))
-		if (unlikely(vring_translate(dev, vq) < 0))
-			goto out;
+	if (unlikely(!vq->access_ok)) {
+		vhost_user_iotlb_rd_unlock(vq);
+		rte_rwlock_read_unlock(&vq->access_lock);
+
+		virtio_dev_vring_translate(dev, vq);
+		goto out_no_unlock;
+	}
 
 	count = RTE_MIN((uint32_t)MAX_PKT_BURST, count);
 	if (count == 0)
@@ -1729,6 +1744,7 @@ virtio_dev_rx(struct virtio_net *dev, struct vhost_virtqueue *vq,
 out_access_unlock:
 	rte_rwlock_read_unlock(&vq->access_lock);
 
+out_no_unlock:
 	return nb_tx;
 }
 
@@ -2523,9 +2539,13 @@ virtio_dev_rx_async_submit(struct virtio_net *dev, struct vhost_virtqueue *vq,
 
 	vhost_user_iotlb_rd_lock(vq);
 
-	if (unlikely(!vq->access_ok))
-		if (unlikely(vring_translate(dev, vq) < 0))
-			goto out;
+	if (unlikely(!vq->access_ok)) {
+		vhost_user_iotlb_rd_unlock(vq);
+		rte_rwlock_read_unlock(&vq->access_lock);
+
+		virtio_dev_vring_translate(dev, vq);
+		goto out_no_unlock;
+	}
 
 	count = RTE_MIN((uint32_t)MAX_PKT_BURST, count);
 	if (count == 0)
@@ -2546,6 +2566,7 @@ virtio_dev_rx_async_submit(struct virtio_net *dev, struct vhost_virtqueue *vq,
 out_access_unlock:
 	rte_rwlock_write_unlock(&vq->access_lock);
 
+out_no_unlock:
 	return nb_tx;
 }
 
@@ -3576,11 +3597,13 @@ rte_vhost_dequeue_burst(int vid, uint16_t queue_id,
 
 	vhost_user_iotlb_rd_lock(vq);
 
-	if (unlikely(!vq->access_ok))
-		if (unlikely(vring_translate(dev, vq) < 0)) {
-			count = 0;
-			goto out;
-		}
+	if (unlikely(!vq->access_ok)) {
+		vhost_user_iotlb_rd_unlock(vq);
+		rte_rwlock_read_unlock(&vq->access_lock);
+
+		virtio_dev_vring_translate(dev, vq);
+		goto out_no_unlock;
+	}
 
 	/*
 	 * Construct a RARP broadcast packet, and inject it to the "pkts"
@@ -3641,6 +3664,7 @@ rte_vhost_dequeue_burst(int vid, uint16_t queue_id,
 	if (unlikely(rarp_mbuf != NULL))
 		count += 1;
 
+out_no_unlock:
 	return count;
 }
 
@@ -4190,11 +4214,14 @@ rte_vhost_async_try_dequeue_burst(int vid, uint16_t queue_id,
 
 	vhost_user_iotlb_rd_lock(vq);
 
-	if (unlikely(vq->access_ok == 0))
-		if (unlikely(vring_translate(dev, vq) < 0)) {
-			count = 0;
-			goto out;
-		}
+	if (unlikely(vq->access_ok == 0)) {
+		vhost_user_iotlb_rd_unlock(vq);
+		rte_rwlock_read_unlock(&vq->access_lock);
+
+		virtio_dev_vring_translate(dev, vq);
+		count = 0;
+		goto out_no_unlock;
+	}
 
 	/*
 	 * Construct a RARP broadcast packet, and inject it to the "pkts"
@@ -4260,5 +4287,6 @@ rte_vhost_async_try_dequeue_burst(int vid, uint16_t queue_id,
 	if (unlikely(rarp_mbuf != NULL))
 		count += 1;
 
+out_no_unlock:
 	return count;
 }
-- 
2.41.0

Re: [PATCH 2/3] vhost: fix virtqueue access lock in datapath

[Eelco Chaudron]

On 23 Oct 2023, at 11:55, David Marchand wrote:

> Now that a r/w lock is used, the access_ok field should only be updated
> under a write lock.
>
> Since the datapath code only takes a read lock on the virtqueue to check
> access_ok, this lock must be released and a write lock taken before
> calling vring_translate().
>
> Fixes: 03f77d66d966 ("vhost: change virtqueue access lock to a read/write one")
> Cc: stable@dpdk.org
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>

Only one question, but whatever the outcome is the change looks good to me.

Acked-by: Eelco Chaudron <echaudro@redhat.com>

> ---
>  lib/vhost/virtio_net.c | 60 +++++++++++++++++++++++++++++++-----------
>  1 file changed, 44 insertions(+), 16 deletions(-)
>
> diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c
> index 759a78e3e3..4116f79d4f 100644
> --- a/lib/vhost/virtio_net.c
> +++ b/lib/vhost/virtio_net.c
> @@ -1694,6 +1694,17 @@ virtio_dev_rx_packed(struct virtio_net *dev,
>  	return pkt_idx;
>  }
>
> +static void
> +virtio_dev_vring_translate(struct virtio_net *dev, struct vhost_virtqueue *vq)
> +{

Would it be an idea to annotate this function that it needs to be called with the “read locks” (and that it will free them) to avoid the duplicate:

+		vhost_user_iotlb_rd_unlock(vq);
+		rte_rwlock_read_unlock(&vq->access_lock);

> +	rte_rwlock_write_lock(&vq->access_lock);
> +	vhost_user_iotlb_rd_lock(vq);
> +	if (!vq->access_ok)
> +		vring_translate(dev, vq);
> +	vhost_user_iotlb_rd_unlock(vq);
> +	rte_rwlock_write_unlock(&vq->access_lock);
> +}
> +
>  static __rte_always_inline uint32_t
>  virtio_dev_rx(struct virtio_net *dev, struct vhost_virtqueue *vq,
>  	struct rte_mbuf **pkts, uint32_t count)
> @@ -1708,9 +1719,13 @@ virtio_dev_rx(struct virtio_net *dev, struct vhost_virtqueue *vq,
>
>  	vhost_user_iotlb_rd_lock(vq);
>
> -	if (unlikely(!vq->access_ok))
> -		if (unlikely(vring_translate(dev, vq) < 0))
> -			goto out;
> +	if (unlikely(!vq->access_ok)) {
> +		vhost_user_iotlb_rd_unlock(vq);
> +		rte_rwlock_read_unlock(&vq->access_lock);
> +
> +		virtio_dev_vring_translate(dev, vq);
> +		goto out_no_unlock;
> +	}
>
>  	count = RTE_MIN((uint32_t)MAX_PKT_BURST, count);
>  	if (count == 0)
> @@ -1729,6 +1744,7 @@ virtio_dev_rx(struct virtio_net *dev, struct vhost_virtqueue *vq,
>  out_access_unlock:
>  	rte_rwlock_read_unlock(&vq->access_lock);
>
> +out_no_unlock:
>  	return nb_tx;
>  }
>
> @@ -2523,9 +2539,13 @@ virtio_dev_rx_async_submit(struct virtio_net *dev, struct vhost_virtqueue *vq,
>
>  	vhost_user_iotlb_rd_lock(vq);
>
> -	if (unlikely(!vq->access_ok))
> -		if (unlikely(vring_translate(dev, vq) < 0))
> -			goto out;
> +	if (unlikely(!vq->access_ok)) {
> +		vhost_user_iotlb_rd_unlock(vq);
> +		rte_rwlock_read_unlock(&vq->access_lock);
> +
> +		virtio_dev_vring_translate(dev, vq);
> +		goto out_no_unlock;
> +	}
>
>  	count = RTE_MIN((uint32_t)MAX_PKT_BURST, count);
>  	if (count == 0)
> @@ -2546,6 +2566,7 @@ virtio_dev_rx_async_submit(struct virtio_net *dev, struct vhost_virtqueue *vq,
>  out_access_unlock:
>  	rte_rwlock_write_unlock(&vq->access_lock);
>
> +out_no_unlock:
>  	return nb_tx;
>  }
>
> @@ -3576,11 +3597,13 @@ rte_vhost_dequeue_burst(int vid, uint16_t queue_id,
>
>  	vhost_user_iotlb_rd_lock(vq);
>
> -	if (unlikely(!vq->access_ok))
> -		if (unlikely(vring_translate(dev, vq) < 0)) {
> -			count = 0;
> -			goto out;
> -		}
> +	if (unlikely(!vq->access_ok)) {
> +		vhost_user_iotlb_rd_unlock(vq);
> +		rte_rwlock_read_unlock(&vq->access_lock);
> +
> +		virtio_dev_vring_translate(dev, vq);
> +		goto out_no_unlock;
> +	}
>
>  	/*
>  	 * Construct a RARP broadcast packet, and inject it to the "pkts"
> @@ -3641,6 +3664,7 @@ rte_vhost_dequeue_burst(int vid, uint16_t queue_id,
>  	if (unlikely(rarp_mbuf != NULL))
>  		count += 1;
>
> +out_no_unlock:
>  	return count;
>  }
>
> @@ -4190,11 +4214,14 @@ rte_vhost_async_try_dequeue_burst(int vid, uint16_t queue_id,
>
>  	vhost_user_iotlb_rd_lock(vq);
>
> -	if (unlikely(vq->access_ok == 0))
> -		if (unlikely(vring_translate(dev, vq) < 0)) {
> -			count = 0;
> -			goto out;
> -		}
> +	if (unlikely(vq->access_ok == 0)) {
> +		vhost_user_iotlb_rd_unlock(vq);
> +		rte_rwlock_read_unlock(&vq->access_lock);
> +
> +		virtio_dev_vring_translate(dev, vq);
> +		count = 0;
> +		goto out_no_unlock;
> +	}
>
>  	/*
>  	 * Construct a RARP broadcast packet, and inject it to the "pkts"
> @@ -4260,5 +4287,6 @@ rte_vhost_async_try_dequeue_burst(int vid, uint16_t queue_id,
>  	if (unlikely(rarp_mbuf != NULL))
>  		count += 1;
>
> +out_no_unlock:
>  	return count;
>  }
> -- 
> 2.41.0

Re: [PATCH 2/3] vhost: fix virtqueue access lock in datapath

[David Marchand]

On Fri, Oct 27, 2023 at 11:05 AM Eelco Chaudron <echaudro@redhat.com> wrote:
> > diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c
> > index 759a78e3e3..4116f79d4f 100644
> > --- a/lib/vhost/virtio_net.c
> > +++ b/lib/vhost/virtio_net.c
> > @@ -1694,6 +1694,17 @@ virtio_dev_rx_packed(struct virtio_net *dev,
> >       return pkt_idx;
> >  }
> >
> > +static void
> > +virtio_dev_vring_translate(struct virtio_net *dev, struct vhost_virtqueue *vq)
> > +{
>
> Would it be an idea to annotate this function that it needs to be called with the “read locks” (and that it will free them) to avoid the duplicate:
>
> +               vhost_user_iotlb_rd_unlock(vq);
> +               rte_rwlock_read_unlock(&vq->access_lock);

The "unlock" annotations do not express read/write concerns for locks.
So that would make the code less readable and potentially hide some issues.

I prefer to keep as is, with clear calls to rd_lock / rd_unlock in
those functions.

>
> > +     rte_rwlock_write_lock(&vq->access_lock);
> > +     vhost_user_iotlb_rd_lock(vq);
> > +     if (!vq->access_ok)
> > +             vring_translate(dev, vq);
> > +     vhost_user_iotlb_rd_unlock(vq);
> > +     rte_rwlock_write_unlock(&vq->access_lock);
> > +}
> > +

-- 
David Marchand

Re: [PATCH 2/3] vhost: fix virtqueue access lock in datapath

[Eelco Chaudron]

On 27 Oct 2023, at 11:22, David Marchand wrote:

> On Fri, Oct 27, 2023 at 11:05 AM Eelco Chaudron <echaudro@redhat.com> wrote:
>>> diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c
>>> index 759a78e3e3..4116f79d4f 100644
>>> --- a/lib/vhost/virtio_net.c
>>> +++ b/lib/vhost/virtio_net.c
>>> @@ -1694,6 +1694,17 @@ virtio_dev_rx_packed(struct virtio_net *dev,
>>>       return pkt_idx;
>>>  }
>>>
>>> +static void
>>> +virtio_dev_vring_translate(struct virtio_net *dev, struct vhost_virtqueue *vq)
>>> +{
>>
>> Would it be an idea to annotate this function that it needs to be called with the “read locks” (and that it will free them) to avoid the duplicate:
>>
>> +               vhost_user_iotlb_rd_unlock(vq);
>> +               rte_rwlock_read_unlock(&vq->access_lock);
>
> The "unlock" annotations do not express read/write concerns for locks.
> So that would make the code less readable and potentially hide some issues.
>
> I prefer to keep as is, with clear calls to rd_lock / rd_unlock in
> those functions.

ACK, keeping this as is fine by me.

Acked-by: Eelco Chaudron <echaudro@redhat.com>

>>> +     rte_rwlock_write_lock(&vq->access_lock);
>>> +     vhost_user_iotlb_rd_lock(vq);
>>> +     if (!vq->access_ok)
>>> +             vring_translate(dev, vq);
>>> +     vhost_user_iotlb_rd_unlock(vq);
>>> +     rte_rwlock_write_unlock(&vq->access_lock);
>>> +}
>>> +
>
> -- 
> David Marchand

Re: [PATCH 2/3] vhost: fix virtqueue access lock in datapath

[Maxime Coquelin]

On 10/23/23 11:55, David Marchand wrote:
> Now that a r/w lock is used, the access_ok field should only be updated
> under a write lock.
> 
> Since the datapath code only takes a read lock on the virtqueue to check
> access_ok, this lock must be released and a write lock taken before
> calling vring_translate().
> 
> Fixes: 03f77d66d966 ("vhost: change virtqueue access lock to a read/write one")
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>   lib/vhost/virtio_net.c | 60 +++++++++++++++++++++++++++++++-----------
>   1 file changed, 44 insertions(+), 16 deletions(-)
> 

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

[PATCH v2 1/5] vhost: fix virtqueue access check in datapath

[David Marchand]

Now that a r/w lock is used, the access_ok field should only be updated
under a write lock.

Since the datapath code only takes a read lock on the virtqueue to check
access_ok, this lock must be released and a write lock taken before
calling vring_translate().

Fixes: 03f77d66d966 ("vhost: change virtqueue access lock to a read/write one")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
Acked-by: Eelco Chaudron <echaudro@redhat.com>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/vhost/virtio_net.c | 60 +++++++++++++++++++++++++++++++-----------
 1 file changed, 44 insertions(+), 16 deletions(-)

diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c
index 8af20f1487..d00f4b03aa 100644
--- a/lib/vhost/virtio_net.c
+++ b/lib/vhost/virtio_net.c
@@ -1696,6 +1696,17 @@ virtio_dev_rx_packed(struct virtio_net *dev,
 	return pkt_idx;
 }
 
+static void
+virtio_dev_vring_translate(struct virtio_net *dev, struct vhost_virtqueue *vq)
+{
+	rte_rwlock_write_lock(&vq->access_lock);
+	vhost_user_iotlb_rd_lock(vq);
+	if (!vq->access_ok)
+		vring_translate(dev, vq);
+	vhost_user_iotlb_rd_unlock(vq);
+	rte_rwlock_write_unlock(&vq->access_lock);
+}
+
 static __rte_always_inline uint32_t
 virtio_dev_rx(struct virtio_net *dev, struct vhost_virtqueue *vq,
 	struct rte_mbuf **pkts, uint32_t count)
@@ -1710,9 +1721,13 @@ virtio_dev_rx(struct virtio_net *dev, struct vhost_virtqueue *vq,
 
 	vhost_user_iotlb_rd_lock(vq);
 
-	if (unlikely(!vq->access_ok))
-		if (unlikely(vring_translate(dev, vq) < 0))
-			goto out;
+	if (unlikely(!vq->access_ok)) {
+		vhost_user_iotlb_rd_unlock(vq);
+		rte_rwlock_read_unlock(&vq->access_lock);
+
+		virtio_dev_vring_translate(dev, vq);
+		goto out_no_unlock;
+	}
 
 	count = RTE_MIN((uint32_t)MAX_PKT_BURST, count);
 	if (count == 0)
@@ -1731,6 +1746,7 @@ virtio_dev_rx(struct virtio_net *dev, struct vhost_virtqueue *vq,
 out_access_unlock:
 	rte_rwlock_read_unlock(&vq->access_lock);
 
+out_no_unlock:
 	return nb_tx;
 }
 
@@ -2528,9 +2544,13 @@ virtio_dev_rx_async_submit(struct virtio_net *dev, struct vhost_virtqueue *vq,
 
 	vhost_user_iotlb_rd_lock(vq);
 
-	if (unlikely(!vq->access_ok))
-		if (unlikely(vring_translate(dev, vq) < 0))
-			goto out;
+	if (unlikely(!vq->access_ok)) {
+		vhost_user_iotlb_rd_unlock(vq);
+		rte_rwlock_read_unlock(&vq->access_lock);
+
+		virtio_dev_vring_translate(dev, vq);
+		goto out_no_unlock;
+	}
 
 	count = RTE_MIN((uint32_t)MAX_PKT_BURST, count);
 	if (count == 0)
@@ -2551,6 +2571,7 @@ virtio_dev_rx_async_submit(struct virtio_net *dev, struct vhost_virtqueue *vq,
 out_access_unlock:
 	rte_rwlock_write_unlock(&vq->access_lock);
 
+out_no_unlock:
 	return nb_tx;
 }
 
@@ -3581,11 +3602,13 @@ rte_vhost_dequeue_burst(int vid, uint16_t queue_id,
 
 	vhost_user_iotlb_rd_lock(vq);
 
-	if (unlikely(!vq->access_ok))
-		if (unlikely(vring_translate(dev, vq) < 0)) {
-			count = 0;
-			goto out;
-		}
+	if (unlikely(!vq->access_ok)) {
+		vhost_user_iotlb_rd_unlock(vq);
+		rte_rwlock_read_unlock(&vq->access_lock);
+
+		virtio_dev_vring_translate(dev, vq);
+		goto out_no_unlock;
+	}
 
 	/*
 	 * Construct a RARP broadcast packet, and inject it to the "pkts"
@@ -3646,6 +3669,7 @@ rte_vhost_dequeue_burst(int vid, uint16_t queue_id,
 	if (unlikely(rarp_mbuf != NULL))
 		count += 1;
 
+out_no_unlock:
 	return count;
 }
 
@@ -4196,11 +4220,14 @@ rte_vhost_async_try_dequeue_burst(int vid, uint16_t queue_id,
 
 	vhost_user_iotlb_rd_lock(vq);
 
-	if (unlikely(vq->access_ok == 0))
-		if (unlikely(vring_translate(dev, vq) < 0)) {
-			count = 0;
-			goto out;
-		}
+	if (unlikely(vq->access_ok == 0)) {
+		vhost_user_iotlb_rd_unlock(vq);
+		rte_rwlock_read_unlock(&vq->access_lock);
+
+		virtio_dev_vring_translate(dev, vq);
+		count = 0;
+		goto out_no_unlock;
+	}
 
 	/*
 	 * Construct a RARP broadcast packet, and inject it to the "pkts"
@@ -4266,5 +4293,6 @@ rte_vhost_async_try_dequeue_burst(int vid, uint16_t queue_id,
 	if (unlikely(rarp_mbuf != NULL))
 		count += 1;
 
+out_no_unlock:
 	return count;
 }
-- 
2.42.0

[PATCH v2 2/5] vhost: fix virtqueue access check in VDUSE setup

[David Marchand]

vring_translate and vring_invalidate change the vq access_ok field.
The access_ok field should only be updated under a (write) lock.

Fixes: a9120db8b98b ("vhost: add VDUSE device startup")
Fixes: ad67c65efda1 ("vhost: add VDUSE device stop")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
Acked-by: Eelco Chaudron <echaudro@redhat.com>
---
Changes since v1:
- moved fix out of patch 3,

---
 lib/vhost/vduse.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lib/vhost/vduse.c b/lib/vhost/vduse.c
index 080b58f7de..e198eeef64 100644
--- a/lib/vhost/vduse.c
+++ b/lib/vhost/vduse.c
@@ -196,6 +196,7 @@ vduse_vring_setup(struct virtio_net *dev, unsigned int index)
 				vq->size * sizeof(struct batch_copy_elem),
 				RTE_CACHE_LINE_SIZE, 0);
 
+	rte_rwlock_write_lock(&vq->access_lock);
 	vhost_user_iotlb_rd_lock(vq);
 	if (vring_translate(dev, vq))
 		VHOST_LOG_CONFIG(dev->ifname, ERR, "Failed to translate vring %d addresses\n",
@@ -206,6 +207,7 @@ vduse_vring_setup(struct virtio_net *dev, unsigned int index)
 				"Failed to disable guest notifications on vring %d\n",
 				index);
 	vhost_user_iotlb_rd_unlock(vq);
+	rte_rwlock_write_unlock(&vq->access_lock);
 
 	vq_efd.index = index;
 	vq_efd.fd = vq->kickfd;
@@ -259,7 +261,9 @@ vduse_vring_cleanup(struct virtio_net *dev, unsigned int index)
 	close(vq->kickfd);
 	vq->kickfd = VIRTIO_UNINITIALIZED_EVENTFD;
 
+	rte_rwlock_write_lock(&vq->access_lock);
 	vring_invalidate(dev, vq);
+	rte_rwlock_write_unlock(&vq->access_lock);
 
 	rte_free(vq->batch_copy_elems);
 	vq->batch_copy_elems = NULL;
-- 
2.42.0

Re: [PATCH v2 2/5] vhost: fix virtqueue access check in VDUSE setup

[Maxime Coquelin]

On 12/5/23 10:45, David Marchand wrote:
> vring_translate and vring_invalidate change the vq access_ok field.
> The access_ok field should only be updated under a (write) lock.
> 
> Fixes: a9120db8b98b ("vhost: add VDUSE device startup")
> Fixes: ad67c65efda1 ("vhost: add VDUSE device stop")
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> Acked-by: Eelco Chaudron <echaudro@redhat.com>
> ---
> Changes since v1:
> - moved fix out of patch 3,
> 
> ---
>   lib/vhost/vduse.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

[PATCH v2 3/5] vhost: fix virtqueue access check in vhost-user setup

[David Marchand]

Calling vring_invalidate must be done with a (write) lock taken on the
virtqueue.

Fixes: 72d002b3ebda ("vhost: fix vring address handling during live migration")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
Acked-by: Eelco Chaudron <echaudro@redhat.com>
---
Changes since v1:
- moved fix out of patch 3,

---
 lib/vhost/vhost_user.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/vhost/vhost_user.c b/lib/vhost/vhost_user.c
index e36312181a..a323ce5fbf 100644
--- a/lib/vhost/vhost_user.c
+++ b/lib/vhost/vhost_user.c
@@ -2198,7 +2198,9 @@ vhost_user_get_vring_base(struct virtio_net **pdev,
 
 	vhost_user_iotlb_flush_all(dev);
 
+	rte_rwlock_write_lock(&vq->access_lock);
 	vring_invalidate(dev, vq);
+	rte_rwlock_write_unlock(&vq->access_lock);
 
 	return RTE_VHOST_MSG_RESULT_REPLY;
 }
-- 
2.42.0

Re: [PATCH v2 3/5] vhost: fix virtqueue access check in vhost-user setup

[Maxime Coquelin]

On 12/5/23 10:45, David Marchand wrote:
> Calling vring_invalidate must be done with a (write) lock taken on the
> virtqueue.
> 
> Fixes: 72d002b3ebda ("vhost: fix vring address handling during live migration")
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> Acked-by: Eelco Chaudron <echaudro@redhat.com>
> ---
> Changes since v1:
> - moved fix out of patch 3,
> 
> ---
>   lib/vhost/vhost_user.c | 2 ++
>   1 file changed, 2 insertions(+)

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

Re: [PATCH v2 1/5] vhost: fix virtqueue access check in datapath

[Maxime Coquelin]

On 12/5/23 10:45, David Marchand wrote:
> Now that a r/w lock is used, the access_ok field should only be updated
> under a write lock.
> 
> Since the datapath code only takes a read lock on the virtqueue to check
> access_ok, this lock must be released and a write lock taken before
> calling vring_translate().
> 
> Fixes: 03f77d66d966 ("vhost: change virtqueue access lock to a read/write one")
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> Acked-by: Eelco Chaudron <echaudro@redhat.com>
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>   lib/vhost/virtio_net.c | 60 +++++++++++++++++++++++++++++++-----------
>   1 file changed, 44 insertions(+), 16 deletions(-)
> 
> diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c
> index 8af20f1487..d00f4b03aa 100644
> --- a/lib/vhost/virtio_net.c
> +++ b/lib/vhost/virtio_net.c
> @@ -1696,6 +1696,17 @@ virtio_dev_rx_packed(struct virtio_net *dev,
>   	return pkt_idx;
>   }
>   
> +static void
> +virtio_dev_vring_translate(struct virtio_net *dev, struct vhost_virtqueue *vq)
> +{
> +	rte_rwlock_write_lock(&vq->access_lock);
> +	vhost_user_iotlb_rd_lock(vq);
> +	if (!vq->access_ok)
> +		vring_translate(dev, vq);
> +	vhost_user_iotlb_rd_unlock(vq);
> +	rte_rwlock_write_unlock(&vq->access_lock);
> +}
> +
>   static __rte_always_inline uint32_t
>   virtio_dev_rx(struct virtio_net *dev, struct vhost_virtqueue *vq,
>   	struct rte_mbuf **pkts, uint32_t count)
> @@ -1710,9 +1721,13 @@ virtio_dev_rx(struct virtio_net *dev, struct vhost_virtqueue *vq,
>   
>   	vhost_user_iotlb_rd_lock(vq);
>   
> -	if (unlikely(!vq->access_ok))
> -		if (unlikely(vring_translate(dev, vq) < 0))
> -			goto out;
> +	if (unlikely(!vq->access_ok)) {
> +		vhost_user_iotlb_rd_unlock(vq);
> +		rte_rwlock_read_unlock(&vq->access_lock);
> +
> +		virtio_dev_vring_translate(dev, vq);
> +		goto out_no_unlock;
> +	}
>   
>   	count = RTE_MIN((uint32_t)MAX_PKT_BURST, count);
>   	if (count == 0)
> @@ -1731,6 +1746,7 @@ virtio_dev_rx(struct virtio_net *dev, struct vhost_virtqueue *vq,
>   out_access_unlock:
>   	rte_rwlock_read_unlock(&vq->access_lock);
>   
> +out_no_unlock:
>   	return nb_tx;
>   }
>   
> @@ -2528,9 +2544,13 @@ virtio_dev_rx_async_submit(struct virtio_net *dev, struct vhost_virtqueue *vq,
>   
>   	vhost_user_iotlb_rd_lock(vq);
>   
> -	if (unlikely(!vq->access_ok))
> -		if (unlikely(vring_translate(dev, vq) < 0))
> -			goto out;
> +	if (unlikely(!vq->access_ok)) {
> +		vhost_user_iotlb_rd_unlock(vq);
> +		rte_rwlock_read_unlock(&vq->access_lock);
> +
> +		virtio_dev_vring_translate(dev, vq);
> +		goto out_no_unlock;
> +	}
>   
>   	count = RTE_MIN((uint32_t)MAX_PKT_BURST, count);
>   	if (count == 0)
> @@ -2551,6 +2571,7 @@ virtio_dev_rx_async_submit(struct virtio_net *dev, struct vhost_virtqueue *vq,
>   out_access_unlock:
>   	rte_rwlock_write_unlock(&vq->access_lock);
>   
> +out_no_unlock:
>   	return nb_tx;
>   }
>   
> @@ -3581,11 +3602,13 @@ rte_vhost_dequeue_burst(int vid, uint16_t queue_id,
>   
>   	vhost_user_iotlb_rd_lock(vq);
>   
> -	if (unlikely(!vq->access_ok))
> -		if (unlikely(vring_translate(dev, vq) < 0)) {
> -			count = 0;
> -			goto out;
> -		}
> +	if (unlikely(!vq->access_ok)) {
> +		vhost_user_iotlb_rd_unlock(vq);
> +		rte_rwlock_read_unlock(&vq->access_lock);
> +
> +		virtio_dev_vring_translate(dev, vq);
> +		goto out_no_unlock;
> +	}
>   
>   	/*
>   	 * Construct a RARP broadcast packet, and inject it to the "pkts"
> @@ -3646,6 +3669,7 @@ rte_vhost_dequeue_burst(int vid, uint16_t queue_id,
>   	if (unlikely(rarp_mbuf != NULL))
>   		count += 1;
>   
> +out_no_unlock:
>   	return count;
>   }
>   
> @@ -4196,11 +4220,14 @@ rte_vhost_async_try_dequeue_burst(int vid, uint16_t queue_id,
>   
>   	vhost_user_iotlb_rd_lock(vq);
>   
> -	if (unlikely(vq->access_ok == 0))
> -		if (unlikely(vring_translate(dev, vq) < 0)) {
> -			count = 0;
> -			goto out;
> -		}
> +	if (unlikely(vq->access_ok == 0)) {
> +		vhost_user_iotlb_rd_unlock(vq);
> +		rte_rwlock_read_unlock(&vq->access_lock);
> +
> +		virtio_dev_vring_translate(dev, vq);
> +		count = 0;
> +		goto out_no_unlock;
> +	}
>   
>   	/*
>   	 * Construct a RARP broadcast packet, and inject it to the "pkts"
> @@ -4266,5 +4293,6 @@ rte_vhost_async_try_dequeue_burst(int vid, uint16_t queue_id,
>   	if (unlikely(rarp_mbuf != NULL))
>   		count += 1;
>   
> +out_no_unlock:
>   	return count;
>   }

Series applied to next-virtio/for-next-net

Thanks,
Maxime