DPDK patches and discussions
 help / color / mirror / Atom feed
From: Chas Williams <3chas3@gmail.com>
To: Stephen Hemminger <stephen@networkplumber.org>
Cc: dev@dpdk.org, Stephen Hemminger <sthemmin@microsoft.com>
Subject: Re: [dpdk-dev] [PATCH] net: fix rte_vlan_insert with shared mbuf
Date: Wed, 27 Mar 2019 11:31:14 -0400	[thread overview]
Message-ID: <71f77bcc-2eb5-e889-7458-71a9efd8d7e9@gmail.com> (raw)
Message-ID: <20190327153114.d62-5y4O_TfL_fHeeqijBXqNYX0wgc5nxoEi57rQnVA@z> (raw)
In-Reply-To: <20190327081838.46c46907@shemminger-XPS-13-9360>



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 <sthemmin@microsoft.com>
>>> ---
>>>    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.

  parent reply	other threads:[~2019-03-27 15:31 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-03-26 19:15 Stephen Hemminger
2019-03-26 19:15 ` Stephen Hemminger
2019-03-26 22:38 ` Chas Williams
2019-03-26 22:38   ` Chas Williams
2019-03-27 15:18   ` Stephen Hemminger
2019-03-27 15:18     ` Stephen Hemminger
2019-03-27 15:31     ` Chas Williams [this message]
2019-03-27 15:31       ` Chas Williams
2019-03-28 14:04       ` Ferruh Yigit
2019-03-28 14:04         ` Ferruh Yigit
2019-03-28 20:53         ` [dpdk-dev] [RFC v2] " Stephen Hemminger
2019-03-28 20:53           ` Stephen Hemminger
2019-03-30 12:41           ` Chas Williams
2019-03-30 12:41             ` Chas Williams
2019-04-04 23:54             ` Stephen Hemminger
2019-04-04 23:54               ` Stephen Hemminger
2019-04-06 23:11               ` Chas Williams
2019-04-06 23:11                 ` Chas Williams
2019-04-12 16:35           ` Ferruh Yigit
2019-04-12 16:35             ` Ferruh Yigit
2019-07-04 18:40             ` Ferruh Yigit

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=71f77bcc-2eb5-e889-7458-71a9efd8d7e9@gmail.com \
    --to=3chas3@gmail.com \
    --cc=dev@dpdk.org \
    --cc=stephen@networkplumber.org \
    --cc=sthemmin@microsoft.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).