From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mailout3.w1.samsung.com (mailout3.w1.samsung.com [210.118.77.13]) by dpdk.org (Postfix) with ESMTP id 9B68C2BD2 for ; Tue, 31 May 2016 11:12:30 +0200 (CEST) Received: from eucpsbgm1.samsung.com (unknown [203.254.199.244]) by mailout3.w1.samsung.com (Oracle Communications Messaging Server 7.0.5.31.0 64bit (built May 5 2014)) with ESMTP id <0O8100FJPAWTIR70@mailout3.w1.samsung.com> for dev@dpdk.org; Tue, 31 May 2016 10:12:29 +0100 (BST) X-AuditID: cbfec7f4-f796c6d000001486-25-574d557d8ee1 Received: from eusync4.samsung.com ( [203.254.199.214]) by eucpsbgm1.samsung.com (EUCPMTA) with SMTP id F2.2E.05254.D755D475; Tue, 31 May 2016 10:12:29 +0100 (BST) Received: from [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 <0O8100CO4AWSQUA0@eusync4.samsung.com>; Tue, 31 May 2016 10:12:29 +0100 (BST) To: Yuanhan Liu 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> <20160530142540.GH5641@yliu-dev.sh.intel.com> Cc: dev@dpdk.org, Huawei Xie , Dyasly Sergey , Heetae Ahn , Jianfeng Tan From: Ilya Maximets Message-id: <574D557C.5040300@samsung.com> Date: Tue, 31 May 2016 12:12:28 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.8.0 MIME-version: 1.0 In-reply-to: <20160530142540.GH5641@yliu-dev.sh.intel.com> Content-type: text/plain; charset=windows-1252 Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFrrMLMWRmVeSWpSXmKPExsVy+t/xa7q1ob7hBptaJCzefdrOZDHt8212 i/aZZ5ksrrT/ZLfonv2FzWLybCmL6xMusDqwe/xasJTVY/Gel0we804GevRtWcUYwBLFZZOS mpNZllqkb5fAlbH/2wmWgtOKFZtbLzI3MO6T6mLk5JAQMJGY2PyGHcIWk7hwbz1bFyMXh5DA UkaJ/x/PskI4Lxgl3kxYAlYlLOAsMeFsHyOILSKgK/F0zjpWEFtI4AmjxLWXpiANzAKrGSXu vl7FApJgE9CROLX6CFADBwevgJbE82WiIGEWAVWJHV2fwXpFBSIkZm3/wQRi8woISvyYfI8F pJxTwEriyW8wk1lAT+L+RS2QCmYBeYnNa94yT2AUmIWkYRZC1SwkVQsYmVcxiqaWJhcUJ6Xn GuoVJ+YWl+al6yXn525ihAT1lx2Mi49ZHWIU4GBU4uGN7PYJF2JNLCuuzD3EKMHBrCTCW+/l Gy7Em5JYWZValB9fVJqTWnyIUZqDRUmcd+6u9yFCAumJJanZqakFqUUwWSYOTqkGRq6s7l1G sw5ICxwMtbi/6fFZ1r9BSq2tZbvy0pNfNMVPKm2W8V816539bUONKptTeZY56YczJvx3ecxk xnx2KtPPvMuHTOK/Zl7etfjBN/UQ/brpZuHX/1q27lAuWe7aytzPdOHDmUOHklrWPz5zxdi1 cKZd3V/N9f33s2PE/TPOzisOFvS2VmIpzkg01GIuKk4EAHqm7itmAgAA 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: Tue, 31 May 2016 09:12:30 -0000 OK. On 30.05.2016 17:25, Yuanhan Liu wrote: > 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 >>>> >>>> > >