From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id 5293A5A66 for ; Mon, 23 May 2016 12:56:08 +0200 (CEST) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga103.fm.intel.com with ESMTP; 23 May 2016 03:56:07 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.26,355,1459839600"; d="scan'208";a="812799344" Received: from yliu-dev.sh.intel.com (HELO yliu-dev) ([10.239.67.162]) by orsmga003.jf.intel.com with ESMTP; 23 May 2016 03:56:05 -0700 Date: Mon, 23 May 2016 18:57:26 +0800 From: Yuanhan Liu To: Ilya Maximets Cc: dev@dpdk.org, Huawei Xie , Dyasly Sergey , Heetae Ahn , Jianfeng Tan Message-ID: <20160523105726.GI5641@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: Mon, 23 May 2016 10:56:08 -0000 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? > [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