[dpdk-stable] [PATCH 1/3] vhost: do not realloc device and queues during running

[dpdk-stable] [PATCH 1/3] vhost: do not realloc device and queues during running

[Tiwei Bie]

When the device has been started, don't do the reallocation anymore.
Otherwise the pointers used in application threads can be invalidated
without proper protection. Instead of introducing a global lock to
protect the change of device pointers which will hurt the performance,
let's just do the reallocation during setup.

Fixes: af295ad4698c ("vhost: realloc device and queues to same numa node as vring desc")
Cc: stable@dpdk.org

Reported-by: Yinan Wang <yinan.wang@intel.com>
Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
---
 lib/librte_vhost/vhost_user.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 0b72648a5..ea43ab139 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -410,6 +410,9 @@ numa_realloc(struct virtio_net *dev, int index)
 	struct batch_copy_elem *new_batch_copy_elems;
 	int ret;
 
+	if (dev->flags & VIRTIO_DEV_RUNNING)
+		return dev;
+
 	old_dev = dev;
 	vq = old_vq = dev->virtqueue[index];
 
-- 
2.17.1

[dpdk-stable] [PATCH 2/3] vhost: fix vring addr handling during live migration

[Tiwei Bie]

When live migration starts, QEMU will set ring addrs again for
each virtqueue. In this case, we should try to translate ring
addrs after we invalidating the ring, otherwise virtqueues can
be enabled with the addrs untranslated. Besides, also leverage
the access_ok flag in non-IOMMU case to prevent the data path
accessing invalidated virtqueues.

Fixes: 5a4933e56be4 ("vhost: postpone ring address translations at kick time only")
Cc: stable@dpdk.org

Reported-by: Yilong Lv <lvyilong.lyl@alibaba-inc.com>
Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
---
 lib/librte_vhost/vhost.c      |  3 +--
 lib/librte_vhost/vhost_user.c | 12 ++++++++++--
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
index 981837b5d..77be16069 100644
--- a/lib/librte_vhost/vhost.c
+++ b/lib/librte_vhost/vhost.c
@@ -358,7 +358,7 @@ vring_translate(struct virtio_net *dev, struct vhost_virtqueue *vq)
 {
 
 	if (!(dev->features & (1ULL << VIRTIO_F_IOMMU_PLATFORM)))
-		goto out;
+		return -1;
 
 	if (vq_is_packed(dev)) {
 		if (vring_translate_packed(dev, vq) < 0)
@@ -367,7 +367,6 @@ vring_translate(struct virtio_net *dev, struct vhost_virtqueue *vq)
 		if (vring_translate_split(dev, vq) < 0)
 			return -1;
 	}
-out:
 	vq->access_ok = 1;
 
 	return 0;
diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index ea43ab139..381250cbc 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -622,6 +622,7 @@ translate_ring_addresses(struct virtio_net *dev, int vq_index)
 			return dev;
 		}
 
+		vq->access_ok = 1;
 		return dev;
 	}
 
@@ -680,6 +681,7 @@ translate_ring_addresses(struct virtio_net *dev, int vq_index)
 	}
 
 	vq->log_guest_addr = addr->log_guest_addr;
+	vq->access_ok = 1;
 
 	VHOST_LOG_DEBUG(VHOST_CONFIG, "(%d) mapped address desc: %p\n",
 			dev->vid, vq->desc);
@@ -704,6 +706,7 @@ vhost_user_set_vring_addr(struct virtio_net **pdev, struct VhostUserMsg *msg,
 	struct virtio_net *dev = *pdev;
 	struct vhost_virtqueue *vq;
 	struct vhost_vring_addr *addr = &msg->payload.addr;
+	bool access_ok;
 
 	if (dev->mem == NULL)
 		return RTE_VHOST_MSG_RESULT_ERR;
@@ -711,6 +714,8 @@ vhost_user_set_vring_addr(struct virtio_net **pdev, struct VhostUserMsg *msg,
 	/* addr->index refers to the queue index. The txq 1, rxq is 0. */
 	vq = dev->virtqueue[msg->payload.addr.index];
 
+	access_ok = vq->access_ok;
+
 	/*
 	 * Rings addresses should not be interpreted as long as the ring is not
 	 * started and enabled
@@ -719,8 +724,9 @@ vhost_user_set_vring_addr(struct virtio_net **pdev, struct VhostUserMsg *msg,
 
 	vring_invalidate(dev, vq);
 
-	if (vq->enabled && (dev->features &
-				(1ULL << VHOST_USER_F_PROTOCOL_FEATURES))) {
+	if ((vq->enabled && (dev->features &
+				(1ULL << VHOST_USER_F_PROTOCOL_FEATURES))) ||
+			access_ok) {
 		dev = translate_ring_addresses(dev, msg->payload.addr.index);
 		if (!dev)
 			return RTE_VHOST_MSG_RESULT_ERR;
@@ -1325,6 +1331,8 @@ vhost_user_get_vring_base(struct virtio_net **pdev,
 	msg->size = sizeof(msg->payload.state);
 	msg->fd_num = 0;
 
+	vring_invalidate(dev, vq);
+
 	return RTE_VHOST_MSG_RESULT_REPLY;
 }
 
-- 
2.17.1

[dpdk-stable] [PATCH 3/3] vhost: protect vring access done by application

[Tiwei Bie]

Besides the enqueue/dequeue API, other APIs of the builtin net
backend should also be protected.

Fixes: a3688046995f ("vhost: protect active rings from async ring changes")
Cc: stable@dpdk.org

Reported-by: Peng He <xnhp0320@icloud.com>
Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
---
 lib/librte_vhost/vhost.c | 50 +++++++++++++++++++++++++++++++---------
 1 file changed, 39 insertions(+), 11 deletions(-)

diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
index 77be16069..cea44df8c 100644
--- a/lib/librte_vhost/vhost.c
+++ b/lib/librte_vhost/vhost.c
@@ -785,22 +785,33 @@ rte_vhost_avail_entries(int vid, uint16_t queue_id)
 {
 	struct virtio_net *dev;
 	struct vhost_virtqueue *vq;
+	uint16_t ret = 0;
 
 	dev = get_device(vid);
 	if (!dev)
 		return 0;
 
 	vq = dev->virtqueue[queue_id];
-	if (!vq->enabled)
-		return 0;
 
-	return *(volatile uint16_t *)&vq->avail->idx - vq->last_used_idx;
+	rte_spinlock_lock(&vq->access_lock);
+
+	if (unlikely(!vq->enabled || vq->avail == NULL))
+		goto out;
+
+	ret = *(volatile uint16_t *)&vq->avail->idx - vq->last_used_idx;
+
+out:
+	rte_spinlock_unlock(&vq->access_lock);
+	return ret;
 }
 
-static inline void
+static inline int
 vhost_enable_notify_split(struct virtio_net *dev,
 		struct vhost_virtqueue *vq, int enable)
 {
+	if (vq->used == NULL)
+		return -1;
+
 	if (!(dev->features & (1ULL << VIRTIO_RING_F_EVENT_IDX))) {
 		if (enable)
 			vq->used->flags &= ~VRING_USED_F_NO_NOTIFY;
@@ -810,17 +821,21 @@ vhost_enable_notify_split(struct virtio_net *dev,
 		if (enable)
 			vhost_avail_event(vq) = vq->last_avail_idx;
 	}
+	return 0;
 }
 
-static inline void
+static inline int
 vhost_enable_notify_packed(struct virtio_net *dev,
 		struct vhost_virtqueue *vq, int enable)
 {
 	uint16_t flags;
 
+	if (vq->device_event == NULL)
+		return -1;
+
 	if (!enable) {
 		vq->device_event->flags = VRING_EVENT_F_DISABLE;
-		return;
+		return 0;
 	}
 
 	flags = VRING_EVENT_F_ENABLE;
@@ -833,6 +848,7 @@ vhost_enable_notify_packed(struct virtio_net *dev,
 	rte_smp_wmb();
 
 	vq->device_event->flags = flags;
+	return 0;
 }
 
 int
@@ -840,18 +856,23 @@ rte_vhost_enable_guest_notification(int vid, uint16_t queue_id, int enable)
 {
 	struct virtio_net *dev = get_device(vid);
 	struct vhost_virtqueue *vq;
+	int ret;
 
 	if (!dev)
 		return -1;
 
 	vq = dev->virtqueue[queue_id];
 
+	rte_spinlock_lock(&vq->access_lock);
+
 	if (vq_is_packed(dev))
-		vhost_enable_notify_packed(dev, vq, enable);
+		ret = vhost_enable_notify_packed(dev, vq, enable);
 	else
-		vhost_enable_notify_split(dev, vq, enable);
+		ret = vhost_enable_notify_split(dev, vq, enable);
 
-	return 0;
+	rte_spinlock_unlock(&vq->access_lock);
+
+	return ret;
 }
 
 void
@@ -890,6 +911,7 @@ rte_vhost_rx_queue_count(int vid, uint16_t qid)
 {
 	struct virtio_net *dev;
 	struct vhost_virtqueue *vq;
+	uint32_t ret = 0;
 
 	dev = get_device(vid);
 	if (dev == NULL)
@@ -905,10 +927,16 @@ rte_vhost_rx_queue_count(int vid, uint16_t qid)
 	if (vq == NULL)
 		return 0;
 
+	rte_spinlock_lock(&vq->access_lock);
+
 	if (unlikely(vq->enabled == 0 || vq->avail == NULL))
-		return 0;
+		goto out;
 
-	return *((volatile uint16_t *)&vq->avail->idx) - vq->last_avail_idx;
+	ret = *((volatile uint16_t *)&vq->avail->idx) - vq->last_avail_idx;
+
+out:
+	rte_spinlock_unlock(&vq->access_lock);
+	return ret;
 }
 
 int rte_vhost_get_vdpa_device_id(int vid)
-- 
2.17.1

Re: [dpdk-stable] [PATCH 1/3] vhost: do not realloc device and queues during running

[Maxime Coquelin]

On 8/19/19 1:34 PM, Tiwei Bie wrote:
> When the device has been started, don't do the reallocation anymore.
> Otherwise the pointers used in application threads can be invalidated
> without proper protection. Instead of introducing a global lock to
> protect the change of device pointers which will hurt the performance,
> let's just do the reallocation during setup.
> 
> Fixes: af295ad4698c ("vhost: realloc device and queues to same numa node as vring desc")
> Cc: stable@dpdk.org
> 
> Reported-by: Yinan Wang <yinan.wang@intel.com>
> Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> ---
>  lib/librte_vhost/vhost_user.c | 3 +++
>  1 file changed, 3 insertions(+)

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

Thanks,
Maxime

Re: [dpdk-stable] [PATCH 2/3] vhost: fix vring addr handling during live migration

[Maxime Coquelin]

On 8/19/19 1:34 PM, Tiwei Bie wrote:
> When live migration starts, QEMU will set ring addrs again for
> each virtqueue. In this case, we should try to translate ring
> addrs after we invalidating the ring, otherwise virtqueues can
> be enabled with the addrs untranslated. Besides, also leverage
> the access_ok flag in non-IOMMU case to prevent the data path
> accessing invalidated virtqueues.
> 
> Fixes: 5a4933e56be4 ("vhost: postpone ring address translations at kick time only")
> Cc: stable@dpdk.org
> 
> Reported-by: Yilong Lv <lvyilong.lyl@alibaba-inc.com>
> Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> ---
>  lib/librte_vhost/vhost.c      |  3 +--
>  lib/librte_vhost/vhost_user.c | 12 ++++++++++--
>  2 files changed, 11 insertions(+), 4 deletions(-)
> 


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

Thanks,
Maxime

Re: [dpdk-stable] [PATCH 3/3] vhost: protect vring access done by application

[Maxime Coquelin]

On 8/19/19 1:34 PM, Tiwei Bie wrote:
> Besides the enqueue/dequeue API, other APIs of the builtin net
> backend should also be protected.
> 
> Fixes: a3688046995f ("vhost: protect active rings from async ring changes")
> Cc: stable@dpdk.org
> 
> Reported-by: Peng He <xnhp0320@icloud.com>
> Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> ---
>  lib/librte_vhost/vhost.c | 50 +++++++++++++++++++++++++++++++---------
>  1 file changed, 39 insertions(+), 11 deletions(-)
> 

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

Thanks,
Maxime