From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by dpdk.space (Postfix) with ESMTP id 473DEA05D3 for ; Sat, 30 Mar 2019 13:41:37 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 404F7493D; Sat, 30 Mar 2019 13:41:36 +0100 (CET) Received: from mail-qt1-f194.google.com (mail-qt1-f194.google.com [209.85.160.194]) by dpdk.org (Postfix) with ESMTP id 3DF012C57 for ; Sat, 30 Mar 2019 13:41:35 +0100 (CET) Received: by mail-qt1-f194.google.com with SMTP id w5so5473429qtb.11 for ; Sat, 30 Mar 2019 05:41:35 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=dLA9LiNiEySkiq4sh7ICoVVeB28hhb2/E4K0AWpRilc=; b=vfCicQHA30HQC16R5+oKT2Ia6hSXiGo9ZlmnDswwxjHX1UN7JXJ0oq2P2BIQFdDten YX03hl+EpuiYmBTA65SVyWZA+lgkZBtBsRKy53zBM5FdQXEp5DL/p+WrY1NiG/FCsitv AUSNhbYCXXo1OqxND5eGDz3y41Mip3OVpQgBQnr6AJTtnlOCutV+dWHAz5oBdn8E+Cd6 SPFiyx7dS1ylOxsaqS89CA055XTW7OxBUKBS294hGcodVoE54aBMTDDkUnkFJ2DTEHSP Qht22DsvUSUxlvm5JCoLkOyJeoCbW7tB05QRBdb+Fsv+PRm02KdqFquZcYV4Ji3l+AxU VISA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=dLA9LiNiEySkiq4sh7ICoVVeB28hhb2/E4K0AWpRilc=; b=LJLt8kq2gM4duGydCNsghav6VYEwiuIi+9Tpm2EWhSkPwO53pwYuBUAlLSHmS/hud/ LDMACXnU5Ou2g1FPIpCKephJ+pRFT0V/cxhwggib55GKu4FCo8tpBv2YARjg57M2JdAE 055pHl6og8fbV7wG0uYu57ZfXKAteefq64jhnKxZdj9unvgJh5DgWbRG17KuOXUxbf04 0VQtqmJOD7uKluQ8gl11aSFu5zNo+tq8NXf6kgQrMdED73ZL521vpi0LaGXoyXBP5LHS S3/jtNvVV8nOc10QwYzr75sDk31MgTK78zPUDdLlBCDC+JnZ2ahj+geoEDWkG95SBWcY cgCw== X-Gm-Message-State: APjAAAWu0va14ifAOMgfMLvN6OXyvDWVA3GDrpYx44JWULjmhtjmwgKc N6swKIgiMjxvK5q5cPXLq4Y= X-Google-Smtp-Source: APXvYqyk6MW43bDZSsm1/0y3BJWk913Vr7/3PzBXYK7d14HzGXkLoOOmWm9NK4e/xcVXCGUImOYGAA== X-Received: by 2002:ac8:3fbc:: with SMTP id d57mr43183720qtk.96.1553949694632; Sat, 30 Mar 2019 05:41:34 -0700 (PDT) Received: from [192.168.1.10] (pool-96-255-82-34.washdc.fios.verizon.net. [96.255.82.34]) by smtp.gmail.com with ESMTPSA id y55sm3024375qtk.13.2019.03.30.05.41.33 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sat, 30 Mar 2019 05:41:33 -0700 (PDT) To: Stephen Hemminger , dev@dpdk.org Cc: Stephen Hemminger References: <20190328205322.852-1-stephen@networkplumber.org> From: Chas Williams <3chas3@gmail.com> Message-ID: Date: Sat, 30 Mar 2019 08:41:33 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.4.0 MIME-Version: 1.0 In-Reply-To: <20190328205322.852-1-stephen@networkplumber.org> Content-Type: text/plain; charset="UTF-8"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [RFC v2] net: fix rte_vlan_insert with shared mbuf 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" Message-ID: <20190330124133.uqX_n2IPDkkrKWkxw4yHmPgYo-Gyvi9v9doIw5gIhII@z> Unfortunately, I think the complete fix is more complicated than this. Drivers that use rte_vlan_insert don't anticipate that the mbuf might change and that (hardware) transmit can fail. They make a copy of the mbuf pointer from the incoming transmit list and don't update the original if rte_vlan_insert creates a new mbuf. If transmit fails, the application needs to be the given the new mbuf for re-transmission or freeing. On 3/28/19 4:53 PM, Stephen Hemminger wrote: > If mbuf is shared then rte_vlan_insert() would clobber the original > Ethernet header. The changed version handles this by getting > an mbuf that will hold the new Ethernet and VLAN header followed > by another mbuf (cloned) for the data. > > Fixes: c974021a5949 ("ether: add soft vlan encap/decap") > Signed-off-by: Stephen Hemminger > --- > v2 - compile tested only, do copy/clone. > > lib/librte_net/rte_ether.h | 37 ++++++++++++++++++++++++++++--------- > 1 file changed, 28 insertions(+), 9 deletions(-) > > diff --git a/lib/librte_net/rte_ether.h b/lib/librte_net/rte_ether.h > index c2c5e249ffe9..5fc306e5d08c 100644 > --- a/lib/librte_net/rte_ether.h > +++ b/lib/librte_net/rte_ether.h > @@ -374,10 +374,10 @@ static inline int rte_vlan_strip(struct rte_mbuf *m) > * Software version of VLAN unstripping > * > * @param m > - * The packet mbuf. > + * Pointer to the packet mbuf. > * @return > * - 0: On success > - * -EPERM: mbuf is is shared overwriting would be unsafe > + * -ENOMEM: could not allocate mbuf for header > * -ENOSPC: not enough headroom in mbuf > */ > static inline int rte_vlan_insert(struct rte_mbuf **m) > @@ -385,15 +385,34 @@ static inline int rte_vlan_insert(struct rte_mbuf **m) > struct ether_hdr *oh, *nh; > struct vlan_hdr *vh; > > - /* Can't insert header if mbuf is shared */ > - if (rte_mbuf_refcnt_read(*m) > 1) { > - struct rte_mbuf *copy; > + /* Can't safely directly insert header if mbuf is shared or indirect */ > + if (!RTE_MBUF_DIRECT(*m) || rte_mbuf_refcnt_read(*m) > 1) { > + struct rte_mempool *mp = (*m)->pool; > + struct rte_mbuf *md, *mh; > + > + mh = rte_pktmbuf_alloc(mp); > + if (unlikely(mh == NULL)) > + return -ENOMEM; > + > + mh->tx_offload = (*m)->tx_offload; > + mh->vlan_tci = (*m)->vlan_tci; > + mh->vlan_tci_outer = (*m)->vlan_tci_outer; > + mh->port = (*m)->port; > + mh->ol_flags = (*m)->ol_flags; > + mh->packet_type = (*m)->packet_type; > > - copy = rte_pktmbuf_clone(*m, (*m)->pool); > - if (unlikely(copy == NULL)) > + md = rte_pktmbuf_clone(*m, mp); > + if (unlikely(md == NULL)) { > + rte_pktmbuf_free(mh); > return -ENOMEM; > - rte_pktmbuf_free(*m); > - *m = copy; > + } > + > + mh->next = md; > + mh->nb_segs = md->nb_segs + 1; > + memcpy(rte_pktmbuf_append(mh, ETHER_HDR_LEN), > + rte_pktmbuf_mtod(md, void *), ETHER_HDR_LEN); > + rte_pktmbuf_adj(md, ETHER_HDR_LEN); > + *m = mh; > } > > oh = rte_pktmbuf_mtod(*m, struct ether_hdr *); >