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 B62221C52 for ; Wed, 13 Jul 2016 10:44:49 +0200 (CEST) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga103.fm.intel.com with ESMTP; 13 Jul 2016 01:44:48 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.28,356,1464678000"; d="scan'208";a="1005864701" Received: from yliu-dev.sh.intel.com (HELO yliu-dev) ([10.239.67.162]) by fmsmga001.fm.intel.com with ESMTP; 13 Jul 2016 01:44:46 -0700 Date: Wed, 13 Jul 2016 16:47:32 +0800 From: Yuanhan Liu To: Ilya Maximets Cc: dev@dpdk.org, Huawei Xie , Dyasly Sergey , Heetae Ahn , Jianfeng Tan , Stephen Hemminger , Thomas Monjalon , Rich Lane Message-ID: <20160713084732.GH26521@yliu-dev.sh.intel.com> References: <20160706122446.GO26521@yliu-dev.sh.intel.com> <577F9328.1030901@samsung.com> <20160710131731.GS26521@yliu-dev.sh.intel.com> <20160711083825.GY26521@yliu-dev.sh.intel.com> <57836BE0.2070401@samsung.com> <20160711110503.GZ26521@yliu-dev.sh.intel.com> <5783876C.1050103@samsung.com> <20160712024305.GB26521@yliu-dev.sh.intel.com> <578485CC.8070809@samsung.com> <5785EEEF.3080400@samsung.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5785EEEF.3080400@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: Wed, 13 Jul 2016 08:44:50 -0000 On Wed, Jul 13, 2016 at 10:34:07AM +0300, Ilya Maximets wrote: > On 12.07.2016 08:53, Ilya Maximets wrote: > > On 12.07.2016 05:43, Yuanhan Liu wrote: > >> On Mon, Jul 11, 2016 at 02:47:56PM +0300, Ilya Maximets wrote: > >>> On 11.07.2016 14:05, Yuanhan Liu wrote: > >>>> On Mon, Jul 11, 2016 at 12:50:24PM +0300, Ilya Maximets wrote: > >>>>> On 11.07.2016 11:38, Yuanhan Liu wrote: > >>>>>> On Sun, Jul 10, 2016 at 09:17:31PM +0800, Yuanhan Liu wrote: > >>>>>>> On Fri, Jul 08, 2016 at 02:48:56PM +0300, Ilya Maximets wrote: > >>>>>>>> > >>>>>>>> Another point is that crash constantly happens on queue_id=3 (second RX queue) in > >>>>>>>> my scenario. It is newly allocated virtqueue while reconfiguration from rxq=1 to > >>>>>>>> rxq=2. > >>>>>>> > >>>>>>> That's a valuable message: what's your DPDK HEAD commit while triggering > >>>>>>> this issue? > >>>>> > >>>>> fbfd99551ca3 ("mbuf: add raw allocation function") > >>>>> > >>>>>> > >>>>>> I guess I have understood what goes wrong in you case. > >>>>>> > >>>>>> I would guess that your vhost has 2 queues (here I mean queue-pairs, > >>>>>> including one Tx and Rx queue; below usage is the same) configured, > >>>>>> so does to your QEMU. However, you just enabled 1 queue while starting > >>>>>> testpmd inside the guest, and you want to enable 2 queues by running > >>>>>> following testpmd commands: > >>>>>> > >>>>>> stop > >>>>>> port stop all > >>>>>> port config all rxq 2 > >>>>>> port config all txq 2 > >>>>>> port start all > >>>>>> > >>>>>> Badly, that won't work for current virtio PMD implementation, and what's > >>>>>> worse, it triggers a vhost crash, the one you saw. > >>>>>> > >>>>>> Here is how it comes. Since you just enabled 1 queue while starting > >>>>>> testpmd, it will setup 1 queue only, meaning only one queue's **valid** > >>>>>> information will be sent to vhost. You might see SET_VRING_ADDR > >>>>>> (and related vhost messages) for the other queue as well, but they > >>>>>> are just the dummy messages: they don't include any valid/real > >>>>>> information about the 2nd queue: the driver don't setup it after all. > >>>>>> > >>>>>> So far, so good. It became broken when you run above commands. Those > >>>>>> commands do setup for the 2nd queue, however, they failed to trigger > >>>>>> the QEMU virtio device to start the vhost-user negotiation, meaning > >>>>>> no SET_VRING_ADDR will be sent for the 2nd queue, leaving vhost > >>>>>> untold and not updated. > >>>>>> > >>>>>> What's worse, above commands trigger the QEMU to send SET_VRING_ENABLE > >>>>>> messages, to enable all the vrings. And since the vrings for the 2nd > >>>>>> queue are not properly configured, the crash happens. > >>>>> > >>>>> Hmm, why 2nd queue works properly with my fix to vhost in this case? > >>>> > >>>> Hmm, really? You are sure that data flows in your 2nd queue after those > >>>> commands? From what I know is that your patch just avoid a crash, but > >>>> does not fix it. > >>> > >>> Oh, sorry. Yes, it doesn't work. With my patch applied I have a QEMU hang. > >> > >> The crash actually could be avoided by commit 0823c1cb0a73 ("vhost: > >> workaround stale vring base"), accidentally. That's why I asked you > >> above the HEAD commit you were using. > > > > Thanks for pointing this. I'll check it. > > I've checked my DPDK HEAD with above commit backported. Yes, it helps to > avoid vhost crash in my scenario. As expected, after reconfiguration new > virtqueue doesn't work, QEMU hangs sometimes. > >>>>>> So maybe we should do virtio reset on port start? > >>>>> > >>>>> I guess it was removed by this patch: > >>>>> a85786dc816f ("virtio: fix states handling during initialization"). > >>>> > >>>> Seems yes. > >> > >> Actually, we should not do that: do reset on port start. The right fix > >> should be allocating MAX queues virtio device supports (2 here). This > >> would allow us changing the queue number dynamically. > > > > Yes, I agree that this is the right way to fix this issue. > > > >> But this doesn't sound a simple fix; it involves many code changes, due > >> to it was not designed this way before. Therefore, we will not fix it > >> in this release, due to it's too late. Let's fix it in the next release > >> instead. For the crash issue, it will not happen with the latest HEAD. > >> Though it's accident fix, I think we are fine here. > > This scenario fixed somehow, I agree. But this patch still needed to protect > vhost from untrusted VM, from malicious or buggy virtio application. > Maybe we could change the commit-message and resend this patch as a > security enhancement? What do you think? Indeed, but I'm a bit concerned about the performance regression found by Rich, yet I am not quite sure why it happens, though Rich claimed that it seems to be a problem related to compiler. --yliu