From patchwork Wed Oct 7 12:53:18 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Olivier Matz X-Patchwork-Id: 79871 X-Patchwork-Delegate: maxime.coquelin@redhat.com Return-Path: X-Original-To: patchwork@inbox.dpdk.org Delivered-To: patchwork@inbox.dpdk.org Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 82084A04BA; Wed, 7 Oct 2020 14:53:34 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 30F461BAB5; Wed, 7 Oct 2020 14:53:32 +0200 (CEST) Received: from proxy.6wind.com (host.76.145.23.62.rev.coltfrance.com [62.23.145.76]) by dpdk.org (Postfix) with ESMTP id 691E81BA45; Wed, 7 Oct 2020 14:53:29 +0200 (CEST) Received: from glumotte.dev.6wind.com. (unknown [10.16.0.195]) by proxy.6wind.com (Postfix) with ESMTP id 29BB546BBC6; Wed, 7 Oct 2020 14:53:29 +0200 (CEST) From: Olivier Matz To: dev@dpdk.org Cc: Maxime Coquelin , Chenbo Xia , Zhihong Wang , Flavio Leitner , yang_y_yi , stable@dpdk.org Date: Wed, 7 Oct 2020 14:53:18 +0200 Message-Id: <20201007125318.9850-1-olivier.matz@6wind.com> X-Mailer: git-send-email 2.25.1 MIME-Version: 1.0 Subject: [dpdk-dev] [PATCH] vhost: fix external mbuf creation X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" In virtio_dev_extbuf_alloc(), the shinfo structure used to store the reference counter and the free callback of the external buffer is by default stored inside the mbuf data. This is wrong because the mbuf (and its data) can be freed before the external buffer, for instance in the following situation: pkt2 = rte_pktmbuf_alloc(mp); rte_pktmbuf_attach(pkt2, pkt); rte_pktmbuf_free(pkt); After this, pkt is freed, but it still contains shinfo, which is referenced by pkt2. Fix this by always storing the shinfo beside the external buffer. Fixes: c3ff0ac70acb ("vhost: improve performance by supporting large buffer") Cc: stable@dpdk.org Signed-off-by: Olivier Matz Reviewed-by: Maxime Coquelin Tested-by: Yi Yang --- Hi, I found this issue by code review while discussing about this patchset [1]. I did not tested the patch, but as I'm only removing one path among the two, I don't expect that it breaks anything. [1] http://inbox.dpdk.org/dev/20200730120900.108232-1-yang_y_yi@163.com/ Regards, Olivier lib/librte_vhost/virtio_net.c | 30 ++++++++---------------------- 1 file changed, 8 insertions(+), 22 deletions(-) diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c index 0a0bea1a5a..008f5ceb04 100644 --- a/lib/librte_vhost/virtio_net.c +++ b/lib/librte_vhost/virtio_net.c @@ -2098,16 +2098,8 @@ virtio_dev_extbuf_alloc(struct rte_mbuf *pkt, uint32_t size) rte_iova_t iova; void *buf; - /* Try to use pkt buffer to store shinfo to reduce the amount of memory - * required, otherwise store shinfo in the new buffer. - */ - if (rte_pktmbuf_tailroom(pkt) >= sizeof(*shinfo)) - shinfo = rte_pktmbuf_mtod(pkt, - struct rte_mbuf_ext_shared_info *); - else { - total_len += sizeof(*shinfo) + sizeof(uintptr_t); - total_len = RTE_ALIGN_CEIL(total_len, sizeof(uintptr_t)); - } + total_len += sizeof(*shinfo) + sizeof(uintptr_t); + total_len = RTE_ALIGN_CEIL(total_len, sizeof(uintptr_t)); if (unlikely(total_len > UINT16_MAX)) return -ENOSPC; @@ -2118,18 +2110,12 @@ virtio_dev_extbuf_alloc(struct rte_mbuf *pkt, uint32_t size) return -ENOMEM; /* Initialize shinfo */ - if (shinfo) { - shinfo->free_cb = virtio_dev_extbuf_free; - shinfo->fcb_opaque = buf; - rte_mbuf_ext_refcnt_set(shinfo, 1); - } else { - shinfo = rte_pktmbuf_ext_shinfo_init_helper(buf, &buf_len, - virtio_dev_extbuf_free, buf); - if (unlikely(shinfo == NULL)) { - rte_free(buf); - VHOST_LOG_DATA(ERR, "Failed to init shinfo\n"); - return -1; - } + shinfo = rte_pktmbuf_ext_shinfo_init_helper(buf, &buf_len, + virtio_dev_extbuf_free, buf); + if (unlikely(shinfo == NULL)) { + rte_free(buf); + VHOST_LOG_DATA(ERR, "Failed to init shinfo\n"); + return -1; } iova = rte_malloc_virt2iova(buf);