vhost: crash under heavy traffic condition due to memory corruption (VPP-1016)

With heavy traffic, tx code path may crash due to memory corruption

Thread 5 "vpp_wk_2" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fff3995c700 (LWP 2505)]
0x00007ffff73675e8 in vhost_user_if_input (vm=0x7fffb5f5bf9c,
    vum=0x7ffff7882a40 <vhost_user_main>, vui=0x7fffb65570c4, qid=0,
    node=0x7fffb6577dac, mode=VNET_HW_INTERFACE_RX_MODE_POLLING)
    at /home/sluong/vpp-master/vpp/build-data/../src/vnet/devices/virtio/vhost-user.c:1610
1610		  bi_current = (vum->cpus[thread_index].rx_buffers)
	                       [vum->cpus[thread_index].rx_buffers_len];
(gdb) p vum->cpus[thread_index].rx_buffers_len
$2 = 793212607
(gdb)

Apparently, some code accidentally wrote the bad value in rx_buffers_len.
rx_buffers_len should never be greater than 1024 since that is how many buffers
we request each time.

After debugging many hours, I discovered that the memory corruption happens
in the tx code path right here on line 2176.

	  {
	    vhost_copy_t *cpy = &vum->cpus[thread_index].copy[copy_len];
	    copy_len++;
	    cpy->len = bytes_left;
	    cpy->len = (cpy->len > buffer_len) ? buffer_len : cpy->len;
	    cpy->dst = buffer_map_addr;
	    cpy->src = (uword) vlib_buffer_get_current (current_b0) +
	      current_b0->current_length - bytes_left;

(gdb) p cpy
$3 = (vhost_copy_t *) 0x7fffb554077c
(gdb) p copy_len
$4 = 1025
(gdb) p &vum->cpus[3].rx_buffers_len
$8 = (u32 *) 0x7fffb5540784

copy_len is picking up the index entry 1024 before it was incremented. copy array has only
1024 members (0 - 1023 are valid).
The assignment here in cpy surely causes memory corruption. It is only discovered later
when the memory location that it corrupted is used.

The condition for the crash is to transmit jumbo frames under heavy volume. Since ring
size is 1024, with one packet taking up one index for frame size (less 2048), it does
not cause overflow. With jumbo frames, it requires multiple indices for one packet,
it can cause the overflow under heavy traffic.

The fix is to do copy out when we have 1000 entries in the array to avoid
overflow.

Change-Id: Iefbc739b8e80470f1cf13123113f8331ffcd0eb2
Signed-off-by: Steven <sluong@cisco.com>

1 files changed, 33 insertions, 2 deletions

diff --git a/src/vnet/devices/virtio/vhost-user.c b/src/vnet/devices/virtio/vhost-user.c
index 6b2c898acd5..7470f15b1c3 100644
--- a/src/vnet/devices/virtio/vhost-user.c
+++ b/src/vnet/devices/virtio/vhost-user.c
@@ -86,6 +86,16 @@
  * The value 64 was obtained by testing (48 and 128 were not as good).
  */
 #define VHOST_USER_RX_COPY_THRESHOLD 64
+/*
+ * On the transmit side, we keep processing the buffers from vlib in the while
+ * loop and prepare the copy order to be executed later. However, the static
+ * array which we keep the copy order is limited to VHOST_USER_COPY_ARRAY_N
+ * entries. In order to not corrupt memory, we have to do the copy when the
+ * static array reaches the copy threshold. We subtract 40 in case the code
+ * goes into the inner loop for a maximum of 64k frames which may require
+ * more array entries.
+ */
+#define VHOST_USER_TX_COPY_THRESHOLD (VHOST_USER_COPY_ARRAY_N - 40)
 
 #define UNIX_GET_FD(unixfd_idx) \
     (unixfd_idx != ~0) ? \
@@ -2000,7 +2010,7 @@ vhost_user_tx (vlib_main_t * vm,
 
   qid =
     VHOST_VRING_IDX_RX (*vec_elt_at_index
-			(vui->per_cpu_tx_qid, vlib_get_thread_index ()));
+			(vui->per_cpu_tx_qid, thread_index));
   rxvq = &vui->vrings[qid];
   if (PREDICT_FALSE (vui->use_tx_spinlock))
     vhost_user_vring_lock (vui, qid);
@@ -2212,6 +2222,27 @@ retry:
 	}
 
       n_left--;			//At the end for error counting when 'goto done' is invoked
+
+      /*
+       * Do the copy periodically to prevent
+       * vum->cpus[thread_index].copy array overflow and corrupt memory
+       */
+      if (PREDICT_FALSE (copy_len >= VHOST_USER_TX_COPY_THRESHOLD))
+	{
+	  if (PREDICT_FALSE
+	      (vhost_user_tx_copy (vui, vum->cpus[thread_index].copy,
+				   copy_len, &map_hint)))
+	    {
+	      vlib_error_count (vm, node->node_index,
+				VHOST_USER_TX_FUNC_ERROR_MMAP_FAIL, 1);
+	    }
+	  copy_len = 0;
+
+	  /* give buffers back to driver */
+	  CLIB_MEMORY_BARRIER ();
+	  rxvq->used->idx = rxvq->last_used_idx;
+	  vhost_user_log_dirty_ring (vui, rxvq, idx);
+	}
       buffers++;
     }
 
@@ -2266,7 +2297,7 @@ done3:
       vlib_increment_simple_counter
 	(vnet_main.interface_main.sw_if_counters
 	 + VNET_INTERFACE_COUNTER_DROP,
-	 vlib_get_thread_index (), vui->sw_if_index, n_left);
+	 thread_index, vui->sw_if_index, n_left);
     }
 
   vlib_buffer_free (vm, vlib_frame_args (frame), frame->n_vectors);