From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-vk0-f45.google.com (mail-vk0-f45.google.com [209.85.213.45]) by dpdk.org (Postfix) with ESMTP id 173FF2C10 for ; Thu, 2 Jun 2016 18:22:52 +0200 (CEST) Received: by mail-vk0-f45.google.com with SMTP id d127so78012360vkh.2 for ; Thu, 02 Jun 2016 09:22:52 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bigswitch-com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc; bh=YXQgsJJ5PjKPkxeUn938MWw4YwJ51LB528AzX0Vi7a4=; b=ibAxULRZRxoA530MMIqXFwkeYU5JyXkfn7qThe3I/cRb9r+oLu9svoelNeVLd5hFdC 586jSDdc8gm/UJnFmciXNAYGIVEnw2jj8GPJk+QxcMaNWGG399g6cjylXBll09N/D5Pz WSPn9mLiSSvpQEXx/FkdZx0JBXO0leGWmLXJC/GTGIsseHT60amah86fKHuxCzW1Wofi i6LSNBeuAOw3qHgggprJ9pgHJ3XmVgEfu1vVnV2y/XpGKaqo9O1P7/upnCOyvNOMaaiM piydaxCwcixFL7YdZ8jGfjvIRIwHivb/1vRV08K9973ksO3wW8SYaHqcU46LZe1RI8R+ 5F8w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc; bh=YXQgsJJ5PjKPkxeUn938MWw4YwJ51LB528AzX0Vi7a4=; b=UWXiLMh/K20lNec0/YT5fPr6/JR5t0BS3c+m8h35uX6Z1rdzdDn03YGFmQWJW1V6tN UL93InTVb5U1oNRktY9y5YSW2JHznHtdPfLtMV4FKVbQ/QTXAe9Ym5aLVtvNY1RFLGyq i5tnlu69XK9R+tIShv3WOM0+RztdZQwSBXtSFjUDH+4a66TgYa2EsGA2+mAb2thVukmq WDK7vHKKqDNLUCEIdRt5kssJGY2JR9mYUrxhOkP8/oNeTnURAaT0DUMjPXOvOao9xDgh jUzCCE3JnTtocrsqkwmC9DNreqXCjwL0Hfp4Dwjmkabqr2SZsTxyLHxJ8JwCy1gjfRdR NWrQ== X-Gm-Message-State: ALyK8tL0dze5UBU5q2wdUfkFLHH9VhwDuIWE/0Cl4c3eKRyF/fnlg2a6ZDSe+cmzv0Ux8veXcRahZ6qetTThfJO8 MIME-Version: 1.0 X-Received: by 10.31.48.139 with SMTP id w133mr4312695vkw.28.1464884571511; Thu, 02 Jun 2016 09:22:51 -0700 (PDT) Received: by 10.31.190.14 with HTTP; Thu, 2 Jun 2016 09:22:51 -0700 (PDT) In-Reply-To: <57500E86.3070104@samsung.com> References: <1463748604-27251-1-git-send-email-i.maximets@samsung.com> <57500E86.3070104@samsung.com> Date: Thu, 2 Jun 2016 09:22:51 -0700 Message-ID: From: Rich Lane To: Ilya Maximets Cc: dev@dpdk.org, Huawei Xie , Yuanhan Liu , Dyasly Sergey , Heetae Ahn , Jianfeng Tan Content-Type: text/plain; charset=UTF-8 X-Content-Filtered-By: Mailman/MimeDel 2.1.15 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: Thu, 02 Jun 2016 16:22:52 -0000 On Thu, Jun 2, 2016 at 3:46 AM, Ilya Maximets wrote: > Hi, Rich. > Thank you for testing and analysing. > > On 01.06.2016 01:06, Rich Lane wrote: > > On Fri, May 20, 2016 at 5:50 AM, 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. > > > > > > I see a performance regression with this patch at large packet sizes (> > 768 bytes). rte_vhost_enqueue_burst is consuming 10% more cycles. > Strangely, there's actually a ~1% performance improvement at small packet > sizes. > > > > The regression happens with GCC 4.8.4 and 5.3.0, but not 6.1.1. > > > > AFAICT this is just the compiler generating bad code. One difference is > that it's storing the offset on the stack instead of in a register. A > workaround is to move the !desc_addr check outside the unlikely macros. > > > > --- a/lib/librte_vhost/vhost_rxtx.c > > +++ b/lib/librte_vhost/vhost_rxtx.c > > @@ -147,10 +147,10 @@ copy_mbuf_to_desc(struct virtio_net *dev, > struct vhost_virtqueue *vq, > > struct virtio_net_hdr_mrg_rxbuf virtio_hdr = {{0, 0, 0, 0, > 0, 0}, 0}; > > > > desc = &vq->desc[desc_idx]; > > - if (unlikely(desc->len < vq->vhost_hlen)) > > + desc_addr = gpa_to_vva(dev, desc->addr); > > + if (unlikely(desc->len < vq->vhost_hlen || !desc_addr)) > > > > > > Workaround: change to "if (unlikely(desc->len < vq->vhost_hlen) || > !desc_addr)". > > > > return -1; > > > > > > - desc_addr = gpa_to_vva(dev, desc->addr); > > rte_prefetch0((void *)(uintptr_t)desc_addr); > > > > virtio_enqueue_offload(m, &virtio_hdr.hdr); > > @@ -184,6 +184,9 @@ copy_mbuf_to_desc(struct virtio_net *dev, struct > vhost_virtqueue *vq, > > > > desc = &vq->desc[desc->next]; > > desc_addr = gpa_to_vva(dev, desc->addr); > > + if (unlikely(!desc_addr)) > > > > > > Workaround: change to "if (!desc_addr)". > > > > > > + return -1; > > + > > desc_offset = 0; > > desc_avail = desc->len; > > } > > > > What about other places? Is there same issues or it's only inside > copy_mbuf_to_desc() ? > Only copy_mbuf_to_desc has the issue.