From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chas Williams <3chas3@gmail.com> Subject: Re: [RFC v2] net: fix rte_vlan_insert with shared mbuf Date: Sat, 30 Mar 2019 08:41:33 -0400 Message-ID: References: <20190328205322.852-1-stephen@networkplumber.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Cc: Stephen Hemminger To: Stephen Hemminger , dev@dpdk.org Return-path: 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) In-Reply-To: <20190328205322.852-1-stephen@networkplumber.org> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 *); >