[dpdk-dev] [PATCH] net: fix rte_vlan_insert with shared mbuf

[dpdk-dev] [PATCH] net: fix rte_vlan_insert with shared mbuf

[Stephen Hemminger]

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)
+		return -EPERM;
 
 	oh = rte_pktmbuf_mtod(*m, struct ether_hdr *);
 	nh = (struct ether_hdr *)
-- 
2.17.1

[dpdk-dev] [PATCH] net: fix rte_vlan_insert with shared mbuf

[Stephen Hemminger]

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)
+		return -EPERM;
 
 	oh = rte_pktmbuf_mtod(*m, struct ether_hdr *);
 	nh = (struct ether_hdr *)
-- 
2.17.1

Re: [dpdk-dev] [PATCH] net: fix rte_vlan_insert with shared mbuf

[Chas Williams]

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.

> +		return -EPERM;
>   
>   	oh = rte_pktmbuf_mtod(*m, struct ether_hdr *);
>   	nh = (struct ether_hdr *)
> 

Re: [dpdk-dev] [PATCH] net: fix rte_vlan_insert with shared mbuf

[Chas Williams]

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.

> +		return -EPERM;
>   
>   	oh = rte_pktmbuf_mtod(*m, struct ether_hdr *);
>   	nh = (struct ether_hdr *)
> 

Re: [dpdk-dev] [PATCH] net: fix rte_vlan_insert with shared mbuf

[Stephen Hemminger]

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?

Re: [dpdk-dev] [PATCH] net: fix rte_vlan_insert with shared mbuf

[Stephen Hemminger]

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?

Re: [dpdk-dev] [PATCH] net: fix rte_vlan_insert with shared mbuf

[Chas Williams]

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.

Re: [dpdk-dev] [PATCH] net: fix rte_vlan_insert with shared mbuf

[Chas Williams]

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.

Re: [dpdk-dev] [PATCH] net: fix rte_vlan_insert with shared mbuf

[Ferruh Yigit]

On 3/27/2019 3:31 PM, Chas Williams wrote:
> 
> 
> 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. 

+1, commit log mentions from the same problem I think.

> 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.

Thanks for it, it looks like duplicating the mbuf was the original intention of
the code.
But if we can't get updated 'rte_vlan_insert' timely, I suggest getting this
patch with extended checks that you suggested. So can it be possible to make new
version of this patch in any case?

Re: [dpdk-dev] [PATCH] net: fix rte_vlan_insert with shared mbuf

[Ferruh Yigit]

On 3/27/2019 3:31 PM, Chas Williams wrote:
> 
> 
> 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. 

+1, commit log mentions from the same problem I think.

> 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.

Thanks for it, it looks like duplicating the mbuf was the original intention of
the code.
But if we can't get updated 'rte_vlan_insert' timely, I suggest getting this
patch with extended checks that you suggested. So can it be possible to make new
version of this patch in any case?

[dpdk-dev] [RFC v2] net: fix rte_vlan_insert with shared mbuf

[Stephen Hemminger]

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 <sthemmin@microsoft.com>
---
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 *);
-- 
2.17.1

[dpdk-dev] [RFC v2] net: fix rte_vlan_insert with shared mbuf

[Stephen Hemminger]

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 <sthemmin@microsoft.com>
---
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 *);
-- 
2.17.1

Re: [dpdk-dev] [RFC v2] net: fix rte_vlan_insert with shared mbuf

[Chas Williams]

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

Re: [dpdk-dev] [RFC v2] net: fix rte_vlan_insert with shared mbuf

[Chas Williams]

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

Re: [dpdk-dev] [RFC v2] net: fix rte_vlan_insert with shared mbuf

[Stephen Hemminger]

On Sat, 30 Mar 2019 08:41:33 -0400
Chas Williams <3chas3@gmail.com> wrote:

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

The virtio driver is buggy, it saves original mbuf and doesn't handle
rewrite. Dpaa2 is fine, should never happen since it checks refcnt etc.
Netvsc PMD is using this on rx path and is safe.
AF_packet PMD should work fine as well.

So virtio is the one that needs fixing

Re: [dpdk-dev] [RFC v2] net: fix rte_vlan_insert with shared mbuf

[Stephen Hemminger]

On Sat, 30 Mar 2019 08:41:33 -0400
Chas Williams <3chas3@gmail.com> wrote:

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

The virtio driver is buggy, it saves original mbuf and doesn't handle
rewrite. Dpaa2 is fine, should never happen since it checks refcnt etc.
Netvsc PMD is using this on rx path and is safe.
AF_packet PMD should work fine as well.

So virtio is the one that needs fixing

Re: [dpdk-dev] [RFC v2] net: fix rte_vlan_insert with shared mbuf

[Chas Williams]

On 4/4/19 7:54 PM, Stephen Hemminger wrote:
> On Sat, 30 Mar 2019 08:41:33 -0400
> Chas Williams <3chas3@gmail.com> wrote:
> 
>> 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 <sthemmin@microsoft.com>
> 
> The virtio driver is buggy, it saves original mbuf and doesn't handle
> rewrite. Dpaa2 is fine, should never happen since it checks refcnt etc.
> Netvsc PMD is using this on rx path and is safe.
> AF_packet PMD should work fine as well.

af_packet is broken as well. It needs to defer rte_vlan_insert until
after it gets the next incoming frame.  Otherwise the break can exit
the loop early.

> So virtio is the one that needs fixing

I agree with this.

Re: [dpdk-dev] [RFC v2] net: fix rte_vlan_insert with shared mbuf

[Chas Williams]

On 4/4/19 7:54 PM, Stephen Hemminger wrote:
> On Sat, 30 Mar 2019 08:41:33 -0400
> Chas Williams <3chas3@gmail.com> wrote:
> 
>> 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 <sthemmin@microsoft.com>
> 
> The virtio driver is buggy, it saves original mbuf and doesn't handle
> rewrite. Dpaa2 is fine, should never happen since it checks refcnt etc.
> Netvsc PMD is using this on rx path and is safe.
> AF_packet PMD should work fine as well.

af_packet is broken as well. It needs to defer rte_vlan_insert until
after it gets the next incoming frame.  Otherwise the break can exit
the loop early.

> So virtio is the one that needs fixing

I agree with this.

Re: [dpdk-dev] [RFC v2] net: fix rte_vlan_insert with shared mbuf

[Ferruh Yigit]

On 3/28/2019 8: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.

Hi Stephen, Chas,

Since this patch is still an RFC, and we are after RC1, I think it is better to
consider this patch for next release.

But for this release, what do you think having a patch to return error on
'rte_vlan_insert()' for shared mbufs?


> 
> Fixes: c974021a5949 ("ether: add soft vlan encap/decap")
> Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> ---
> 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 *);
> 

Re: [dpdk-dev] [RFC v2] net: fix rte_vlan_insert with shared mbuf

[Ferruh Yigit]

On 3/28/2019 8: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.

Hi Stephen, Chas,

Since this patch is still an RFC, and we are after RC1, I think it is better to
consider this patch for next release.

But for this release, what do you think having a patch to return error on
'rte_vlan_insert()' for shared mbufs?


> 
> Fixes: c974021a5949 ("ether: add soft vlan encap/decap")
> Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> ---
> 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 *);
> 

Re: [dpdk-dev] [RFC v2] net: fix rte_vlan_insert with shared mbuf

[Ferruh Yigit]

On 4/12/2019 5:35 PM, Ferruh Yigit wrote:
> On 3/28/2019 8: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.
> 
> Hi Stephen, Chas,
> 
> Since this patch is still an RFC, and we are after RC1, I think it is better to
> consider this patch for next release.
> 
> But for this release, what do you think having a patch to return error on
> 'rte_vlan_insert()' for shared mbufs?

nack for this patch

The other approach to not support the shared mbuf merged:
https://git.dpdk.org/dpdk/commit/?id=15a74163b12