From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mailout4.w1.samsung.com (mailout4.w1.samsung.com [210.118.77.14]) by dpdk.org (Postfix) with ESMTP id 0E70DADB9 for ; Fri, 20 May 2016 14:50:16 +0200 (CEST) Received: from eucpsbgm2.samsung.com (unknown [203.254.199.245]) by mailout4.w1.samsung.com (Oracle Communications Messaging Server 7.0.5.31.0 64bit (built May 5 2014)) with ESMTP id <0O7H0098E7NQOP10@mailout4.w1.samsung.com> for dev@dpdk.org; Fri, 20 May 2016 13:50:14 +0100 (BST) X-AuditID: cbfec7f5-f792a6d000001302-9d-573f08058185 Received: from eusync4.samsung.com ( [203.254.199.214]) by eucpsbgm2.samsung.com (EUCPMTA) with SMTP id 16.59.04866.5080F375; Fri, 20 May 2016 13:50:13 +0100 (BST) Received: from imaximets.rnd.samsung.ru ([106.109.129.180]) by eusync4.samsung.com (Oracle Communications Messaging Server 7.0.5.31.0 64bit (built May 5 2014)) with ESMTPA id <0O7H00H757NKRR90@eusync4.samsung.com>; Fri, 20 May 2016 13:50:13 +0100 (BST) From: Ilya Maximets To: dev@dpdk.org, Huawei Xie , Yuanhan Liu Cc: Dyasly Sergey , Heetae Ahn , Jianfeng Tan , Ilya Maximets Date: Fri, 20 May 2016 15:50:04 +0300 Message-id: <1463748604-27251-1-git-send-email-i.maximets@samsung.com> X-Mailer: git-send-email 2.5.0 X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFvrCJMWRmVeSWpSXmKPExsVy+t/xa7qsHPbhBjPvGFm8+7SdyWLa59vs Fu0zzzJZXGn/yW7RPfsLm8Xk2VIW1ydcYHVg9/i1YCmrx+I9L5k85p0M9OjbsooxgCWKyyYl NSezLLVI3y6BK+P27Q6Wgm+qFf/2sjcwzpXrYuTkkBAwkbi05i4zhC0mceHeerYuRi4OIYGl jBKb1sxnhXBamSSmnZvIDlLFJqAjcWr1EUYQW0QgQeLI/t9gRcwCixgl9m/bBFYkLGAvcfbU JpYuRg4OFgFVic558iBhXgE3iffPjrKBhCUE5CQWXEifwMi9gJFhFaNoamlyQXFSeq6RXnFi bnFpXrpecn7uJkZIWHzdwbj0mNUhRgEORiUe3gw7u3Ah1sSy4srcQ4wSHMxKIrxRbPbhQrwp iZVVqUX58UWlOanFhxilOViUxHln7nofIiSQnliSmp2aWpBaBJNl4uCUamAstfnK9fHGosfi 76p7k09qxevE8TU6blhVGjFpYczKlkf3/M/6y5fzHzpY57zPpFjo74qa9rbK4w4OZ72ver6q Fl/132xdp8z8OqXni13jspvPJTG+SGn7OPPaDs1/VUvjytYFFva+stLtES0pNVC+2fUt/+uH Wbp9M4K9/TqFow24vzws3KjEUpyRaKjFXFScCABCQ6ACBwIAAA== Subject: [dpdk-dev] [PATCH] vhost: fix segfault on bad descriptor address. X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 20 May 2016 12:50:16 -0000 In current implementation guest application can reinitialize vrings by executing start after stop. In the same time host application can still poll virtqueue while device stopped in guest and it will crash with segmentation fault while vring reinitialization because of dereferencing of bad descriptor addresses. OVS crash for example: <------------------------------------------------------------------------> [test-pmd inside guest VM] testpmd> port stop all Stopping ports... Checking link statuses... Port 0 Link Up - speed 10000 Mbps - full-duplex Done testpmd> port config all rxq 2 testpmd> port config all txq 2 testpmd> port start all Configuring Port 0 (socket 0) Port 0: 52:54:00:CB:44:C8 Checking link statuses... Port 0 Link Up - speed 10000 Mbps - full-duplex Done [OVS on host] Program received signal SIGSEGV, Segmentation fault. rte_memcpy (n=2056, src=0xc, dst=0x7ff4d5247000) at rte_memcpy.h (gdb) bt #0 rte_memcpy (n=2056, src=0xc, dst=0x7ff4d5247000) #1 copy_desc_to_mbuf #2 rte_vhost_dequeue_burst #3 netdev_dpdk_vhost_rxq_recv ... (gdb) bt full #0 rte_memcpy ... #1 copy_desc_to_mbuf desc_addr = 0 mbuf_offset = 0 desc_offset = 12 ... <------------------------------------------------------------------------> Fix that by checking addresses of descriptors before using them. Note: For mergeable buffers this patch checks only guest's address for zero, but in non-meargeable case host's address checked. This is done because checking of host's address in mergeable case requires additional refactoring to keep virtqueue in consistent state in case of error. Signed-off-by: Ilya Maximets --- Actually, current virtio implementation looks broken for me. Because 'virtio_dev_start' breaks virtqueue while it still available from the vhost side. There was 2 patches about this behaviour: 1. a85786dc816f ("virtio: fix states handling during initialization") 2. 9a0615af7746 ("virtio: fix restart") The second patch fixes somehow issue intoduced in the first patch, but actually also breaks vhost in the way described above. It's not pretty clear for me what to do in current situation with virtio, because it will be broken for guest application even if vhost will not crash. May be it'll be better to forbid stopping of virtio device and force user to exit and start again (may be implemented in hidden from user way)? This patch adds additional sane checks, so it should be applied anyway, IMHO. lib/librte_vhost/vhost_rxtx.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/lib/librte_vhost/vhost_rxtx.c b/lib/librte_vhost/vhost_rxtx.c index 750821a..9d05739 100644 --- a/lib/librte_vhost/vhost_rxtx.c +++ b/lib/librte_vhost/vhost_rxtx.c @@ -147,10 +147,10 @@ copy_mbuf_to_desc(struct virtio_net *dev, struct vhost_virtqueue *vq, struct virtio_net_hdr_mrg_rxbuf virtio_hdr = {{0, 0, 0, 0, 0, 0}, 0}; desc = &vq->desc[desc_idx]; - if (unlikely(desc->len < vq->vhost_hlen)) + desc_addr = gpa_to_vva(dev, desc->addr); + if (unlikely(desc->len < vq->vhost_hlen || !desc_addr)) return -1; - desc_addr = gpa_to_vva(dev, desc->addr); rte_prefetch0((void *)(uintptr_t)desc_addr); virtio_enqueue_offload(m, &virtio_hdr.hdr); @@ -184,6 +184,9 @@ copy_mbuf_to_desc(struct virtio_net *dev, struct vhost_virtqueue *vq, desc = &vq->desc[desc->next]; desc_addr = gpa_to_vva(dev, desc->addr); + if (unlikely(!desc_addr)) + return -1; + desc_offset = 0; desc_avail = desc->len; } @@ -344,7 +347,8 @@ fill_vec_buf(struct vhost_virtqueue *vq, uint32_t avail_idx, uint32_t len = *allocated; while (1) { - if (unlikely(vec_id >= BUF_VECTOR_MAX || idx >= vq->size)) + if (unlikely(vec_id >= BUF_VECTOR_MAX || idx >= vq->size + || !vq->desc[idx].addr)) return -1; len += vq->desc[idx].len; @@ -747,10 +751,10 @@ copy_desc_to_mbuf(struct virtio_net *dev, struct vhost_virtqueue *vq, uint32_t nr_desc = 1; desc = &vq->desc[desc_idx]; - if (unlikely(desc->len < vq->vhost_hlen)) + desc_addr = gpa_to_vva(dev, desc->addr); + if (unlikely(desc->len < vq->vhost_hlen || !desc_addr)) return -1; - desc_addr = gpa_to_vva(dev, desc->addr); rte_prefetch0((void *)(uintptr_t)desc_addr); /* Retrieve virtio net header */ @@ -769,6 +773,9 @@ copy_desc_to_mbuf(struct virtio_net *dev, struct vhost_virtqueue *vq, desc = &vq->desc[desc->next]; desc_addr = gpa_to_vva(dev, desc->addr); + if (unlikely(!desc_addr)) + return -1; + rte_prefetch0((void *)(uintptr_t)desc_addr); desc_offset = 0; -- 2.5.0