From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: <3chas3@gmail.com> Received: from mail-qt1-f196.google.com (mail-qt1-f196.google.com [209.85.160.196]) by dpdk.org (Postfix) with ESMTP id E2A961B112 for ; Wed, 27 Mar 2019 16:31:17 +0100 (CET) Received: by mail-qt1-f196.google.com with SMTP id w5so19264737qtb.11 for ; Wed, 27 Mar 2019 08:31:17 -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=KPOyZcJXF8OlgHJt395hjGASXlY4NUtjBkbTwEdGz/0=; b=UomA4slnC4YzChJubjnfmUnMaIVXzCBgi2W/tYK5hvT11YzPdtjDQsKIk633ob5WDF rpZY6clHLuL4jUkMjiAw3rFvY6sk98tenQMLUQpdbkN4QV5MkjAdtmZglp9WSjDE4oPT HMG31LCerNi4vSS4TWtdNE7JEVQ/OxI+AS/RGzcrV3JKBdBXetj7hE7Z+I6zplV59yYx Ws+pvr1aE3zldq8jNNtK1jr6uU8YfGOx+an7njDS1/dH5wq/0LH4MSGklcSO5yKD1bVE 1op6k4fQqCvdllJDNp8WJCvXnuaoGsmdx6emkzoaP+07z7zidQwhwQ5hU0MEuJC/E+hT PbCQ== 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=KPOyZcJXF8OlgHJt395hjGASXlY4NUtjBkbTwEdGz/0=; b=Bbh5YG7n/6+6PdrQu5OMII15kC4WCy+ShuUmg7hrhsJVWYG8FHK3ziT/dvqs/rR+LC Lpzr6kTjMITBvIiCBXCCO0gd/CPKq0KwfSkY2SRNiBIByDhPFCyjZVUKCScMgKxFu6Tj HUD9RKll3FX2E103WXHSSvh3AzKqa/Yq8Ueq1O93CAPgbKPBaoRjK33mo0CKGAn1YWOG D6RDgW0HGpR1bE1iRbgi6OCVYrsq9l42B+R+7MdRtFh2u7r8aIGeu5+Gf99rfV4lVjE/ KHS05Ad3/zsmyatqKb3If6yvRoFg8RU1hEbl3if83u5NlZszTer/o48+AFTDtPaFTQuw PlhA== X-Gm-Message-State: APjAAAUCyjry00Eyluv9yQ/F5HnntRasXkHN3aANC6rhaYHzE6jcAA6J f4Hsho0AaDvAgDejEsy++ss= X-Google-Smtp-Source: APXvYqxvQZ4KTGKT9dzvoamOX6tNMgBFE/EXS8G6F3MFC09R/UAvcZbiuLctXfm41Rnawyru3X4Tgg== X-Received: by 2002:a0c:9973:: with SMTP id i48mr31741270qvd.9.1553700677310; Wed, 27 Mar 2019 08:31:17 -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 y6sm9871128qka.69.2019.03.27.08.31.15 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 27 Mar 2019 08:31:15 -0700 (PDT) To: Stephen Hemminger Cc: dev@dpdk.org, Stephen Hemminger References: <20190326191516.20675-1-stephen@networkplumber.org> <0889dffe-e9d8-f480-0480-5b9950fcbbf9@gmail.com> <20190327081838.46c46907@shemminger-XPS-13-9360> From: Chas Williams <3chas3@gmail.com> Message-ID: <71f77bcc-2eb5-e889-7458-71a9efd8d7e9@gmail.com> Date: Wed, 27 Mar 2019 11:31:14 -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: <20190327081838.46c46907@shemminger-XPS-13-9360> 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: Wed, 27 Mar 2019 15:31:18 -0000 On 3/27/19 11:18 AM, Stephen Hemminger wrote: > On Tue, 26 Mar 2019 18:38:57 -0400 > Chas Williams <3chas3@gmail.com> wrote: > >> 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. >> > > The problem is that indirect buffers probably can't be overwritten > because of lack of headroom. > > Actually, why not push the problem into the pktmbuf_headroom logic? That's not what the original code is checking and why it is checking. You should not modify the data of a packet that has other users. To check all the possible users, you need to check your refcnt and if a clone, check the parent to see if any other clones exist. If they do, you can't safely modify these packets. Yes, we have run into this bug. Yes, it was hard to find. Someone local write a slightly different version of rte_vlan_insert that clones the packet and prepends an mbuf so you can safely insert the VLAN information. I will see about getting it submitted. 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 30196A05D3 for ; Wed, 27 Mar 2019 16:31:19 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 84D211B129; Wed, 27 Mar 2019 16:31:18 +0100 (CET) Received: from mail-qt1-f196.google.com (mail-qt1-f196.google.com [209.85.160.196]) by dpdk.org (Postfix) with ESMTP id E2A961B112 for ; Wed, 27 Mar 2019 16:31:17 +0100 (CET) Received: by mail-qt1-f196.google.com with SMTP id w5so19264737qtb.11 for ; Wed, 27 Mar 2019 08:31:17 -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=KPOyZcJXF8OlgHJt395hjGASXlY4NUtjBkbTwEdGz/0=; b=UomA4slnC4YzChJubjnfmUnMaIVXzCBgi2W/tYK5hvT11YzPdtjDQsKIk633ob5WDF rpZY6clHLuL4jUkMjiAw3rFvY6sk98tenQMLUQpdbkN4QV5MkjAdtmZglp9WSjDE4oPT HMG31LCerNi4vSS4TWtdNE7JEVQ/OxI+AS/RGzcrV3JKBdBXetj7hE7Z+I6zplV59yYx Ws+pvr1aE3zldq8jNNtK1jr6uU8YfGOx+an7njDS1/dH5wq/0LH4MSGklcSO5yKD1bVE 1op6k4fQqCvdllJDNp8WJCvXnuaoGsmdx6emkzoaP+07z7zidQwhwQ5hU0MEuJC/E+hT PbCQ== 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=KPOyZcJXF8OlgHJt395hjGASXlY4NUtjBkbTwEdGz/0=; b=Bbh5YG7n/6+6PdrQu5OMII15kC4WCy+ShuUmg7hrhsJVWYG8FHK3ziT/dvqs/rR+LC Lpzr6kTjMITBvIiCBXCCO0gd/CPKq0KwfSkY2SRNiBIByDhPFCyjZVUKCScMgKxFu6Tj HUD9RKll3FX2E103WXHSSvh3AzKqa/Yq8Ueq1O93CAPgbKPBaoRjK33mo0CKGAn1YWOG D6RDgW0HGpR1bE1iRbgi6OCVYrsq9l42B+R+7MdRtFh2u7r8aIGeu5+Gf99rfV4lVjE/ KHS05Ad3/zsmyatqKb3If6yvRoFg8RU1hEbl3if83u5NlZszTer/o48+AFTDtPaFTQuw PlhA== X-Gm-Message-State: APjAAAUCyjry00Eyluv9yQ/F5HnntRasXkHN3aANC6rhaYHzE6jcAA6J f4Hsho0AaDvAgDejEsy++ss= X-Google-Smtp-Source: APXvYqxvQZ4KTGKT9dzvoamOX6tNMgBFE/EXS8G6F3MFC09R/UAvcZbiuLctXfm41Rnawyru3X4Tgg== X-Received: by 2002:a0c:9973:: with SMTP id i48mr31741270qvd.9.1553700677310; Wed, 27 Mar 2019 08:31:17 -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 y6sm9871128qka.69.2019.03.27.08.31.15 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 27 Mar 2019 08:31:15 -0700 (PDT) To: Stephen Hemminger Cc: dev@dpdk.org, Stephen Hemminger References: <20190326191516.20675-1-stephen@networkplumber.org> <0889dffe-e9d8-f480-0480-5b9950fcbbf9@gmail.com> <20190327081838.46c46907@shemminger-XPS-13-9360> From: Chas Williams <3chas3@gmail.com> Message-ID: <71f77bcc-2eb5-e889-7458-71a9efd8d7e9@gmail.com> Date: Wed, 27 Mar 2019 11:31:14 -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: <20190327081838.46c46907@shemminger-XPS-13-9360> 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: <20190327153114.d62-5y4O_TfL_fHeeqijBXqNYX0wgc5nxoEi57rQnVA@z> On 3/27/19 11:18 AM, Stephen Hemminger wrote: > On Tue, 26 Mar 2019 18:38:57 -0400 > Chas Williams <3chas3@gmail.com> wrote: > >> 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. >> > > The problem is that indirect buffers probably can't be overwritten > because of lack of headroom. > > Actually, why not push the problem into the pktmbuf_headroom logic? That's not what the original code is checking and why it is checking. You should not modify the data of a packet that has other users. To check all the possible users, you need to check your refcnt and if a clone, check the parent to see if any other clones exist. If they do, you can't safely modify these packets. Yes, we have run into this bug. Yes, it was hard to find. Someone local write a slightly different version of rte_vlan_insert that clones the packet and prepends an mbuf so you can safely insert the VLAN information. I will see about getting it submitted.