* [PATCH 2/3] vhost: fix virtqueue access lock in datapath [not found] <20231023095520.2864868-1-david.marchand@redhat.com> @ 2023-10-23 9:55 ` David Marchand 2023-10-27 9:03 ` Eelco Chaudron 2023-12-05 9:10 ` Maxime Coquelin 2023-12-05 9:45 ` [PATCH v2 1/5] vhost: fix virtqueue access check " David Marchand 1 sibling, 2 replies; 11+ messages in thread From: David Marchand @ 2023-10-23 9:55 UTC (permalink / raw) To: dev; +Cc: stable, Maxime Coquelin, Chenbo Xia, Eelco Chaudron 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] vhost: fix virtqueue access lock in datapath 2023-10-23 9:55 ` [PATCH 2/3] vhost: fix virtqueue access lock in datapath David Marchand @ 2023-10-27 9:03 ` Eelco Chaudron 2023-10-27 9:22 ` David Marchand 2023-12-05 9:10 ` Maxime Coquelin 1 sibling, 1 reply; 11+ messages in thread From: Eelco Chaudron @ 2023-10-27 9:03 UTC (permalink / raw) To: David Marchand; +Cc: dev, stable, Maxime Coquelin, Chenbo Xia 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] vhost: fix virtqueue access lock in datapath 2023-10-27 9:03 ` Eelco Chaudron @ 2023-10-27 9:22 ` David Marchand 2023-10-27 10:11 ` Eelco Chaudron 0 siblings, 1 reply; 11+ messages in thread From: David Marchand @ 2023-10-27 9:22 UTC (permalink / raw) To: Eelco Chaudron; +Cc: dev, stable, Maxime Coquelin, Chenbo Xia 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] vhost: fix virtqueue access lock in datapath 2023-10-27 9:22 ` David Marchand @ 2023-10-27 10:11 ` Eelco Chaudron 0 siblings, 0 replies; 11+ messages in thread From: Eelco Chaudron @ 2023-10-27 10:11 UTC (permalink / raw) To: David Marchand; +Cc: dev, stable, Maxime Coquelin, Chenbo Xia 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] vhost: fix virtqueue access lock in datapath 2023-10-23 9:55 ` [PATCH 2/3] vhost: fix virtqueue access lock in datapath David Marchand 2023-10-27 9:03 ` Eelco Chaudron @ 2023-12-05 9:10 ` Maxime Coquelin 1 sibling, 0 replies; 11+ messages in thread From: Maxime Coquelin @ 2023-12-05 9:10 UTC (permalink / raw) To: David Marchand, dev; +Cc: stable, Chenbo Xia, Eelco Chaudron 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> ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 1/5] vhost: fix virtqueue access check in datapath [not found] <20231023095520.2864868-1-david.marchand@redhat.com> 2023-10-23 9:55 ` [PATCH 2/3] vhost: fix virtqueue access lock in datapath David Marchand @ 2023-12-05 9:45 ` David Marchand 2023-12-05 9:45 ` [PATCH v2 2/5] vhost: fix virtqueue access check in VDUSE setup David Marchand ` (2 more replies) 1 sibling, 3 replies; 11+ messages in thread From: David Marchand @ 2023-12-05 9:45 UTC (permalink / raw) To: dev; +Cc: stable, Eelco Chaudron, Maxime Coquelin, Chenbo Xia 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 2/5] vhost: fix virtqueue access check in VDUSE setup 2023-12-05 9:45 ` [PATCH v2 1/5] vhost: fix virtqueue access check " David Marchand @ 2023-12-05 9:45 ` David Marchand 2023-12-05 9:57 ` Maxime Coquelin 2023-12-05 9:45 ` [PATCH v2 3/5] vhost: fix virtqueue access check in vhost-user setup David Marchand 2023-12-12 11:37 ` [PATCH v2 1/5] vhost: fix virtqueue access check in datapath Maxime Coquelin 2 siblings, 1 reply; 11+ messages in thread From: David Marchand @ 2023-12-05 9:45 UTC (permalink / raw) To: dev; +Cc: stable, Eelco Chaudron, Maxime Coquelin, Chenbo Xia 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/5] vhost: fix virtqueue access check in VDUSE setup 2023-12-05 9:45 ` [PATCH v2 2/5] vhost: fix virtqueue access check in VDUSE setup David Marchand @ 2023-12-05 9:57 ` Maxime Coquelin 0 siblings, 0 replies; 11+ messages in thread From: Maxime Coquelin @ 2023-12-05 9:57 UTC (permalink / raw) To: David Marchand, dev; +Cc: stable, Eelco Chaudron, Chenbo Xia 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 3/5] vhost: fix virtqueue access check in vhost-user setup 2023-12-05 9:45 ` [PATCH v2 1/5] vhost: fix virtqueue access check " David Marchand 2023-12-05 9:45 ` [PATCH v2 2/5] vhost: fix virtqueue access check in VDUSE setup David Marchand @ 2023-12-05 9:45 ` David Marchand 2023-12-05 9:59 ` Maxime Coquelin 2023-12-12 11:37 ` [PATCH v2 1/5] vhost: fix virtqueue access check in datapath Maxime Coquelin 2 siblings, 1 reply; 11+ messages in thread From: David Marchand @ 2023-12-05 9:45 UTC (permalink / raw) To: dev; +Cc: stable, Eelco Chaudron, Maxime Coquelin, Chenbo Xia, Tiwei Bie 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 3/5] vhost: fix virtqueue access check in vhost-user setup 2023-12-05 9:45 ` [PATCH v2 3/5] vhost: fix virtqueue access check in vhost-user setup David Marchand @ 2023-12-05 9:59 ` Maxime Coquelin 0 siblings, 0 replies; 11+ messages in thread From: Maxime Coquelin @ 2023-12-05 9:59 UTC (permalink / raw) To: David Marchand, dev; +Cc: stable, Eelco Chaudron, Chenbo Xia, Tiwei Bie 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/5] vhost: fix virtqueue access check in datapath 2023-12-05 9:45 ` [PATCH v2 1/5] vhost: fix virtqueue access check " David Marchand 2023-12-05 9:45 ` [PATCH v2 2/5] vhost: fix virtqueue access check in VDUSE setup David Marchand 2023-12-05 9:45 ` [PATCH v2 3/5] vhost: fix virtqueue access check in vhost-user setup David Marchand @ 2023-12-12 11:37 ` Maxime Coquelin 2 siblings, 0 replies; 11+ messages in thread From: Maxime Coquelin @ 2023-12-12 11:37 UTC (permalink / raw) To: David Marchand, dev; +Cc: stable, Eelco Chaudron, Chenbo Xia 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2023-12-12 11:37 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20231023095520.2864868-1-david.marchand@redhat.com> 2023-10-23 9:55 ` [PATCH 2/3] vhost: fix virtqueue access lock in datapath David Marchand 2023-10-27 9:03 ` Eelco Chaudron 2023-10-27 9:22 ` David Marchand 2023-10-27 10:11 ` Eelco Chaudron 2023-12-05 9:10 ` Maxime Coquelin 2023-12-05 9:45 ` [PATCH v2 1/5] vhost: fix virtqueue access check " David Marchand 2023-12-05 9:45 ` [PATCH v2 2/5] vhost: fix virtqueue access check in VDUSE setup David Marchand 2023-12-05 9:57 ` Maxime Coquelin 2023-12-05 9:45 ` [PATCH v2 3/5] vhost: fix virtqueue access check in vhost-user setup David Marchand 2023-12-05 9:59 ` Maxime Coquelin 2023-12-12 11:37 ` [PATCH v2 1/5] vhost: fix virtqueue access check in datapath Maxime Coquelin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).