[PATCH v2 0/2] vhost fixes for OVS SIGSEGV in PMD

[PATCH v2 0/2] vhost fixes for OVS SIGSEGV in PMD

[Claudio Fontana]

This is an alternative, more general fix compared with PATCH v1.

The series fixes a segmentation fault in the OVS PMD thread when
resynchronizing with QEMU after the guest application has been killed
with SIGKILL (patch 1/2),

The segmentation fault can be caused by the guest DPDK application,
which is able this way to crash the OVS process on the host,
see the backtrace in patch 1/2.

Patch 2/2 is an additional improvement in the current error handling.

---

Changes from v1:

* patch 1/2: instead of only fixing virtio_dev_tx_split, put the check
  for nr_vec == 0 inside desc_to_mbuf and mbuf_to_desc, so that in no
  case they attempt to read and dereference addresses from the buf_vec[]
  array when it does not contain any valid elements.

---

For your review and comments,

Claudio

Claudio Fontana (2):
  vhost: check for nr_vec == 0 in desc_to_mbuf, mbuf_to_desc
  vhost: improve error handling in desc_to_mbuf

 lib/vhost/virtio_net.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

-- 
2.26.2

[PATCH 1/2] vhost: check for nr_vec == 0 in desc_to_mbuf, mbuf_to_desc

[Claudio Fontana]

in virtio_dev_split we cannot currently call desc_to_mbuf with
nr_vec == 0, or we end up trying to rte_memcpy from a source address
buf_vec[0] that is an uninitialized stack variable.

Improve this in general by having desc_to_mbuf and mbuf_to_desc
return -1 when called with an invalid nr_vec == 0, which should
fix any other instance of this problem.

This should fix errors that have been reported in multiple occasions
from telcos to the DPDK, OVS and QEMU projects, as this affects in
particular the openvswitch/DPDK, QEMU vhost-user setup when the
guest DPDK application abruptly goes away via SIGKILL and then
reconnects.

The back trace looks roughly like this, depending on the specific
rte_memcpy selected, etc, in any case the "src" parameter is garbage
(in this example containing 0 + dev->host_hlen(12 = 0xc)).

Thread 153 "pmd-c88/id:150" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7f64e5e6b700 (LWP 141373)]
rte_mov128blocks (n=2048, src=0xc <error: Cannot access memory at 0xc>,
             dst=0x150da4480) at ../lib/eal/x86/include/rte_memcpy.h:384
(gdb) bt
0  rte_mov128blocks (n=2048, src=0xc, dst=0x150da4480)
1  rte_memcpy_generic (n=2048, src=0xc, dst=0x150da4480)
2  rte_memcpy (n=2048, src=0xc, dst=<optimized out>)
3  sync_fill_seg
4  desc_to_mbuf
5  virtio_dev_tx_split
6  virtio_dev_tx_split_legacy
7  0x00007f676fea0fef in rte_vhost_dequeue_burst
8  0x00007f6772005a62 in netdev_dpdk_vhost_rxq_recv
9  0x00007f6771f38116 in netdev_rxq_recv
10 0x00007f6771f03d96 in dp_netdev_process_rxq_port
11 0x00007f6771f04239 in pmd_thread_main
12 0x00007f6771f92aff in ovsthread_wrapper
13 0x00007f6771c1b6ea in start_thread
14 0x00007f6771933a8f in clone

Tested-by: Claudio Fontana <cfontana@suse.de>
Signed-off-by: Claudio Fontana <cfontana@suse.de>
---
 lib/vhost/virtio_net.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c
index 35fa4670fd..8d0d223983 100644
--- a/lib/vhost/virtio_net.c
+++ b/lib/vhost/virtio_net.c
@@ -1153,7 +1153,7 @@ mbuf_to_desc(struct virtio_net *dev, struct vhost_virtqueue *vq,
 	struct virtio_net_hdr_mrg_rxbuf tmp_hdr, *hdr = NULL;
 	struct vhost_async *async = vq->async;
 
-	if (unlikely(m == NULL))
+	if (unlikely(m == NULL) || nr_vec == 0)
 		return -1;
 
 	buf_addr = buf_vec[vec_idx].buf_addr;
@@ -2673,6 +2673,9 @@ desc_to_mbuf(struct virtio_net *dev, struct vhost_virtqueue *vq,
 	struct vhost_async *async = vq->async;
 	struct async_inflight_info *pkts_info;
 
+        if (unlikely(nr_vec == 0)) {
+		return -1;
+        }
 	buf_addr = buf_vec[vec_idx].buf_addr;
 	buf_iova = buf_vec[vec_idx].buf_iova;
 	buf_len = buf_vec[vec_idx].buf_len;
@@ -2917,9 +2920,11 @@ virtio_dev_tx_split(struct virtio_net *dev, struct vhost_virtqueue *vq,
 						vq->last_avail_idx + i,
 						&nr_vec, buf_vec,
 						&head_idx, &buf_len,
-						VHOST_ACCESS_RO) < 0))
+						VHOST_ACCESS_RO) < 0)) {
+			dropped += 1;
+			i++;
 			break;
-
+		}
 		update_shadow_used_ring_split(vq, head_idx, 0);
 
 		err = virtio_dev_pktmbuf_prep(dev, pkts[i], buf_len);
-- 
2.26.2

[PATCH 2/2] vhost: improve error handling in desc_to_mbuf

[Claudio Fontana]

check when increasing vec_idx that it is still valid
in the (buf_len < dev->vhost_hlen) case too.

Signed-off-by: Claudio Fontana <cfontana@suse.de>
---
 lib/vhost/virtio_net.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c
index 8d0d223983..229e484f2d 100644
--- a/lib/vhost/virtio_net.c
+++ b/lib/vhost/virtio_net.c
@@ -2704,12 +2704,15 @@ desc_to_mbuf(struct virtio_net *dev, struct vhost_virtqueue *vq,
 	if (unlikely(buf_len < dev->vhost_hlen)) {
 		buf_offset = dev->vhost_hlen - buf_len;
 		vec_idx++;
+		if (unlikely(vec_idx >= nr_vec))
+			goto error;
 		buf_addr = buf_vec[vec_idx].buf_addr;
 		buf_iova = buf_vec[vec_idx].buf_iova;
 		buf_len = buf_vec[vec_idx].buf_len;
 		buf_avail  = buf_len - buf_offset;
 	} else if (buf_len == dev->vhost_hlen) {
-		if (unlikely(++vec_idx >= nr_vec))
+		vec_idx++;
+		if (unlikely(vec_idx >= nr_vec))
 			goto error;
 		buf_addr = buf_vec[vec_idx].buf_addr;
 		buf_iova = buf_vec[vec_idx].buf_iova;
-- 
2.26.2

[PATCH 2/2] vhost: improve error handling in desc_to_mbuf

[Claudio Fontana]

check when increasing vec_idx that it is still valid
in the (buf_len < dev->vhost_hlen) case too.

Signed-off-by: Claudio Fontana <cfontana@suse.de>
---
 lib/vhost/virtio_net.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c
index 0b8db2046e..6d34feaf73 100644
--- a/lib/vhost/virtio_net.c
+++ b/lib/vhost/virtio_net.c
@@ -2701,12 +2701,15 @@ desc_to_mbuf(struct virtio_net *dev, struct vhost_virtqueue *vq,
 	if (unlikely(buf_len < dev->vhost_hlen)) {
 		buf_offset = dev->vhost_hlen - buf_len;
 		vec_idx++;
+		if (unlikely(vec_idx >= nr_vec))
+			goto error;
 		buf_addr = buf_vec[vec_idx].buf_addr;
 		buf_iova = buf_vec[vec_idx].buf_iova;
 		buf_len = buf_vec[vec_idx].buf_len;
 		buf_avail  = buf_len - buf_offset;
 	} else if (buf_len == dev->vhost_hlen) {
-		if (unlikely(++vec_idx >= nr_vec))
+		vec_idx++;
+		if (unlikely(vec_idx >= nr_vec))
 			goto error;
 		buf_addr = buf_vec[vec_idx].buf_addr;
 		buf_iova = buf_vec[vec_idx].buf_iova;
-- 
2.26.2

[PATCH 2/2] vhost: improve error handling in desc_to_mbuf

[Claudio Fontana]

check when increasing vec_idx that it is still valid
in the (buf_len < dev->vhost_hlen) case too.

Signed-off-by: Claudio Fontana <cfontana@suse.de>
---
 lib/vhost/virtio_net.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c
index 0b8db2046e..6d34feaf73 100644
--- a/lib/vhost/virtio_net.c
+++ b/lib/vhost/virtio_net.c
@@ -2701,12 +2701,15 @@ desc_to_mbuf(struct virtio_net *dev, struct vhost_virtqueue *vq,
 	if (unlikely(buf_len < dev->vhost_hlen)) {
 		buf_offset = dev->vhost_hlen - buf_len;
 		vec_idx++;
+		if (unlikely(vec_idx >= nr_vec))
+			goto error;
 		buf_addr = buf_vec[vec_idx].buf_addr;
 		buf_iova = buf_vec[vec_idx].buf_iova;
 		buf_len = buf_vec[vec_idx].buf_len;
 		buf_avail  = buf_len - buf_offset;
 	} else if (buf_len == dev->vhost_hlen) {
-		if (unlikely(++vec_idx >= nr_vec))
+		vec_idx++;
+		if (unlikely(vec_idx >= nr_vec))
 			goto error;
 		buf_addr = buf_vec[vec_idx].buf_addr;
 		buf_iova = buf_vec[vec_idx].buf_iova;
-- 
2.26.2