From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: <3chas3@gmail.com> 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) 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=tQKih8oMppXvt757UutLGulhHR48juZPWdJQFYtW+LI=; b=QXq+nSO2ZoH4SEoahTE5OJsNO8EW8THjLgGuzqoqhDnm3byGDDNp+o78nPaMKi3gxQ MN3frFa5Ktv8rL8LNoxXqjskfAkbv/+ObJQwPzCtH9E8y5ykJBG2C5uFdCEGx9xoNKKj 8o6zkw3yAgQnn6AFhYO1DBXv4V9Bi8EwX6U++JfYhNynQ2OAWYOIPifIKsuCdmLZIjYk U7gp/ZQWjGlAjIsB/oCTkJUucGAl5zkjICeZIRH6hpKpQptNc7WQD9NnlZ3ixLiLWzOp A7j/ya9pxvGnCdinJ9Xzue2MQzKlojhqCFA2/JbBO9OR6P56eUyfyarTnmlmPTP5k6y3 jxNw== 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=tQKih8oMppXvt757UutLGulhHR48juZPWdJQFYtW+LI=; b=DeiMHo5lzox+/p2cqz1ZPD+OpI0JF3ZQppdvXV1Y/TSxXsObbMjvpxkk8BpkRqXjhl Ft7A7Iy8+AaJdx6yrvUyy0TCMVnaaHjRPv8HK7JBawiTtZPMtbG74+LyB0opttGXJSpD okypqoG4ut4O36i9jnk7t956Nr/cNON1JBKya1czqtTUf7QrqPrwx+z+7kTk8EMYX6hC ZKk3T3fSXskgETZa2b5LOErRk02pc/IaBLFHfaU04pGGFOmlJOKSbGC5GJJBnXwFK5jk zGS1xIPSt3PsFmYndcHcZTK5Ez3P3oElI8N/P1Xyyi3jACqhOHG4HXpx9VH4lUmAfo9M QRWQ== X-Gm-Message-State: APjAAAV/cX7ZTLjOzMSbDAAIpAVjQhrWsI0Kaf07xZRMtbML0vHYUtUH ZN52pqhh2pD9qsQmJlg1+LI= X-Google-Smtp-Source: APXvYqwI0v5QCmiBFuI082lCW6LKGhcXoJxAIB+sqWeo7A7/EaW1fhnR4QS/dQ+VxAGUXe0tBTFjlw== X-Received: by 2002:ae9:c310:: with SMTP id n16mr26353392qkg.8.1553639938277; Tue, 26 Mar 2019 15:38:58 -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 s54sm4855093qtb.83.2019.03.26.15.38.57 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 26 Mar 2019 15:38:57 -0700 (PDT) To: Stephen Hemminger , dev@dpdk.org Cc: Stephen Hemminger References: <20190326191516.20675-1-stephen@networkplumber.org> From: Chas Williams <3chas3@gmail.com> Message-ID: <0889dffe-e9d8-f480-0480-5b9950fcbbf9@gmail.com> Date: Tue, 26 Mar 2019 18:38:57 -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: <20190326191516.20675-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] [PATCH] 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: , X-List-Received-Date: Tue, 26 Mar 2019 22:38:59 -0000 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 *) > 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 4D085A05D3 for ; Tue, 26 Mar 2019 23:39:01 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 6F3E51B3D7; Tue, 26 Mar 2019 23:39:00 +0100 (CET) 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) 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=tQKih8oMppXvt757UutLGulhHR48juZPWdJQFYtW+LI=; b=QXq+nSO2ZoH4SEoahTE5OJsNO8EW8THjLgGuzqoqhDnm3byGDDNp+o78nPaMKi3gxQ MN3frFa5Ktv8rL8LNoxXqjskfAkbv/+ObJQwPzCtH9E8y5ykJBG2C5uFdCEGx9xoNKKj 8o6zkw3yAgQnn6AFhYO1DBXv4V9Bi8EwX6U++JfYhNynQ2OAWYOIPifIKsuCdmLZIjYk U7gp/ZQWjGlAjIsB/oCTkJUucGAl5zkjICeZIRH6hpKpQptNc7WQD9NnlZ3ixLiLWzOp A7j/ya9pxvGnCdinJ9Xzue2MQzKlojhqCFA2/JbBO9OR6P56eUyfyarTnmlmPTP5k6y3 jxNw== 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=tQKih8oMppXvt757UutLGulhHR48juZPWdJQFYtW+LI=; b=DeiMHo5lzox+/p2cqz1ZPD+OpI0JF3ZQppdvXV1Y/TSxXsObbMjvpxkk8BpkRqXjhl Ft7A7Iy8+AaJdx6yrvUyy0TCMVnaaHjRPv8HK7JBawiTtZPMtbG74+LyB0opttGXJSpD okypqoG4ut4O36i9jnk7t956Nr/cNON1JBKya1czqtTUf7QrqPrwx+z+7kTk8EMYX6hC ZKk3T3fSXskgETZa2b5LOErRk02pc/IaBLFHfaU04pGGFOmlJOKSbGC5GJJBnXwFK5jk zGS1xIPSt3PsFmYndcHcZTK5Ez3P3oElI8N/P1Xyyi3jACqhOHG4HXpx9VH4lUmAfo9M QRWQ== X-Gm-Message-State: APjAAAV/cX7ZTLjOzMSbDAAIpAVjQhrWsI0Kaf07xZRMtbML0vHYUtUH ZN52pqhh2pD9qsQmJlg1+LI= X-Google-Smtp-Source: APXvYqwI0v5QCmiBFuI082lCW6LKGhcXoJxAIB+sqWeo7A7/EaW1fhnR4QS/dQ+VxAGUXe0tBTFjlw== X-Received: by 2002:ae9:c310:: with SMTP id n16mr26353392qkg.8.1553639938277; Tue, 26 Mar 2019 15:38:58 -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 s54sm4855093qtb.83.2019.03.26.15.38.57 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 26 Mar 2019 15:38:57 -0700 (PDT) To: Stephen Hemminger , dev@dpdk.org Cc: Stephen Hemminger References: <20190326191516.20675-1-stephen@networkplumber.org> From: Chas Williams <3chas3@gmail.com> Message-ID: <0889dffe-e9d8-f480-0480-5b9950fcbbf9@gmail.com> Date: Tue, 26 Mar 2019 18:38:57 -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: <20190326191516.20675-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] [PATCH] 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: <20190326223857.ZmyRd0UGvp837oOPlEqw0hAnCi3bL55wol3YThDWwxs@z> 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 *) >