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 32C366CA3 for ; Mon, 30 May 2016 16:23:08 +0200 (CEST) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga104.fm.intel.com with ESMTP; 30 May 2016 07:23:07 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.26,390,1459839600"; d="scan'208";a="977297641" Received: from yliu-dev.sh.intel.com (HELO yliu-dev) ([10.239.67.162]) by fmsmga001.fm.intel.com with ESMTP; 30 May 2016 07:23:06 -0700 Date: Mon, 30 May 2016 22:25:40 +0800 From: Yuanhan Liu To: Ilya Maximets Cc: dev@dpdk.org, Huawei Xie , Dyasly Sergey , Heetae Ahn , Jianfeng Tan Message-ID: <20160530142540.GH5641@yliu-dev.sh.intel.com> References: <1463748604-27251-1-git-send-email-i.maximets@samsung.com> <20160523105726.GI5641@yliu-dev.sh.intel.com> <5742E3C2.9090309@samsung.com> <574C1E63.3070000@samsung.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <574C1E63.3070000@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: Mon, 30 May 2016 14:23:08 -0000 Hi Ilya, Generically speaking, this patch looks good to me. But I guess still need more time to check this issue later; I still failed to reproduce it on my side after all. So, please allow a late merge. Thanks. --yliu On Mon, May 30, 2016 at 02:05:07PM +0300, Ilya Maximets wrote: > Ping. > > Best regards, Ilya Maximets. > > On 23.05.2016 14:04, Ilya Maximets wrote: > > On 23.05.2016 13:57, Yuanhan Liu wrote: > >> 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. > >>> > >>> 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 > >> > >> I actually didn't manage to reproduce it on my side, with the > >> vhost-example instead of OVS though. Is that all the commands > >> to reproduce it, and run them just after start test-pmd? > > > > Actually, I think, packet flow should be enabled while performing > > above actions and some traffic already should be sent through port > > to change last used idx on vhost side. > > > > Something like: > > start > > ..wait a while.. see that packets are flowing. > > stop > > port stop > > port config > > port config > > port start > >> > >>> [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. > >> > >> Agreed. > >> > >> --yliu > >> > >>