summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorSteven <sluong@cisco.com>2017-06-09 18:49:17 -0700
committerDamjan Marion <dmarion.lists@gmail.com>2017-10-14 09:11:19 +0000
commitd77275307b6ad6459ecba01912a302fb7dbf0f02 (patch)
tree97404007d5458d887d57ef8403bb1ea2d3172186
parent781b99ddc9565a6c8bbe078ecb3258d91a2bbc06 (diff)
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>
-rw-r--r--src/vnet/devices/virtio/vhost-user.c35
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);