From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id 145912BB5 for ; Fri, 1 Jul 2016 09:34:25 +0200 (CEST) Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga104.fm.intel.com with ESMTP; 01 Jul 2016 00:34:25 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.26,555,1459839600"; d="scan'208";a="131771824" Received: from yliu-dev.sh.intel.com (HELO yliu-dev) ([10.239.67.162]) by fmsmga004.fm.intel.com with ESMTP; 01 Jul 2016 00:34:24 -0700 Date: Fri, 1 Jul 2016 15:35:06 +0800 From: Yuanhan Liu To: Ilya Maximets Cc: dev@dpdk.org, Huawei Xie , Dyasly Sergey , Heetae Ahn , Jianfeng Tan Message-ID: <20160701073506.GQ2831@yliu-dev.sh.intel.com> References: <1463748604-27251-1-git-send-email-i.maximets@samsung.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1463748604-27251-1-git-send-email-i.maximets@samsung.com> User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [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, 01 Jul 2016 07:34:26 -0000 Hi, Sorry for the long delay. On Fri, May 20, 2016 at 03:50:04PM +0300, Ilya Maximets wrote: > 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. Yes, you are right that vring will be reinitialized after restart. But even though, I don't see the reason it will cause a vhost crash, since the reinitialization will reset all the vring memeory by 0: memset(vq->vq_ring_virt_mem, 0, vq->vq_ring_size); That means those bad descriptors will be skipped, safely, at vhost side by: if (unlikely(desc->len < dev->vhost_hlen)) return -1; > > 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 Interesting, so it bypasses the above check since desc->len is non-zero while desc->addr is zero. The size (2056) also looks weird. Do you mind to check this issue a bit deeper, say why desc->addr is zero, however, desc->len is not? > (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. Yes, this sounds buggy. Maybe we could not reset the avail idx, in such case vhost dequeue/enqueue will just return as there are no more packets to dequeue and no more space to enqueue, respectively? --yliu