From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chas Williams <3chas3@gmail.com> Subject: Re: [PATCH] net: fix rte_vlan_insert with shared mbuf Date: Tue, 26 Mar 2019 18:38:57 -0400 Message-ID: <0889dffe-e9d8-f480-0480-5b9950fcbbf9@gmail.com> References: <20190326191516.20675-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-qk1-f195.google.com (mail-qk1-f195.google.com [209.85.222.195]) by dpdk.org (Postfix) with ESMTP id E4696559A for ; Tue, 26 Mar 2019 23:38:58 +0100 (CET) Received: by mail-qk1-f195.google.com with SMTP id c20so8725449qkc.10 for ; Tue, 26 Mar 2019 15:38:58 -0700 (PDT) In-Reply-To: <20190326191516.20675-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" On 3/26/19 3:15 PM, Stephen Hemminger wrote: > If mbuf refcnt was > 1 then rte_vlan_insert() would incorrectly > modify the original copy. Original code was expecting clone to make > a copy (it doesn't). Better to let the caller deal with making > a copy or setting up mbuf chain to allow for header to be added. > > Also fix docbook comment about parameters (function takes > pointer to mbuf). > > Fixes: c974021a5949 ("ether: add soft vlan encap/decap") > Signed-off-by: Stephen Hemminger > --- > lib/librte_net/rte_ether.h | 15 ++++----------- > 1 file changed, 4 insertions(+), 11 deletions(-) > > diff --git a/lib/librte_net/rte_ether.h b/lib/librte_net/rte_ether.h > index c2c5e249ffe9..bab2b198fa79 100644 > --- a/lib/librte_net/rte_ether.h > +++ b/lib/librte_net/rte_ether.h > @@ -374,7 +374,7 @@ 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 > @@ -385,16 +385,9 @@ 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; > - > - copy = rte_pktmbuf_clone(*m, (*m)->pool); > - if (unlikely(copy == NULL)) > - return -ENOMEM; > - rte_pktmbuf_free(*m); > - *m = copy; > - } > + /* Can't directly insert header if mbuf is shared */ > + if (rte_mbuf_refcnt_read(*m) > 1) This check probably isn't sufficient. You need something more like: if (rte_mbuf_refcnt_read(mbuf) > 1 || (RTE_MBUF_INDIRECT(mbuf) && rte_mbuf_refcnt_read(rte_mbuf_from_indirect(mbuf)) > 1)) { If this is a cloned packet, the refcnt will be 1. So you need to examine the parent mbuf to see if other clones exist. > + return -EPERM; > > oh = rte_pktmbuf_mtod(*m, struct ether_hdr *); > nh = (struct ether_hdr *) >