DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] ether: check the first segment length on SW VLAN insertion
@ 2020-05-27 14:31 Andrew Rybchenko
  2020-05-29  5:43 ` Stephen Hemminger
  0 siblings, 1 reply; 4+ messages in thread
From: Andrew Rybchenko @ 2020-05-27 14:31 UTC (permalink / raw)
  To: Olivier Matz; +Cc: dev

SW VLAN insertion relies on Ethernet addresses location in contigous
memory (do not split across mbuf segments). There is no any formal
requirements on data location and mbuf structure which guarantee it.
So, check it explicitly to avoid corrupted packets if the condition
is violated. Typically software VLAN insertion is done on Tx prepare
stage and application will get indication that the packet is invalid
and cannot be transmitted.

Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
---
 lib/librte_net/rte_ether.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lib/librte_net/rte_ether.h b/lib/librte_net/rte_ether.h
index 0ae4e75b6c..d7c076bba8 100644
--- a/lib/librte_net/rte_ether.h
+++ b/lib/librte_net/rte_ether.h
@@ -357,6 +357,10 @@ static inline int rte_vlan_insert(struct rte_mbuf **m)
 	if (!RTE_MBUF_DIRECT(*m) || rte_mbuf_refcnt_read(*m) > 1)
 		return -EINVAL;
 
+	/* Can't insert header if the first segment is too short */
+	if (rte_pktmbuf_data_len(*m) < 2 * RTE_ETHER_ADDR_LEN)
+		return -EINVAL;
+
 	oh = rte_pktmbuf_mtod(*m, struct rte_ether_hdr *);
 	nh = (struct rte_ether_hdr *)
 		rte_pktmbuf_prepend(*m, sizeof(struct rte_vlan_hdr));
-- 
2.17.1


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [dpdk-dev] [PATCH] ether: check the first segment length on SW VLAN insertion
  2020-05-27 14:31 [dpdk-dev] [PATCH] ether: check the first segment length on SW VLAN insertion Andrew Rybchenko
@ 2020-05-29  5:43 ` Stephen Hemminger
  2020-06-25 12:27   ` Andrew Rybchenko
  0 siblings, 1 reply; 4+ messages in thread
From: Stephen Hemminger @ 2020-05-29  5:43 UTC (permalink / raw)
  To: Andrew Rybchenko; +Cc: Olivier Matz, dev

On Wed, 27 May 2020 15:31:41 +0100
Andrew Rybchenko <arybchenko@solarflare.com> wrote:

> SW VLAN insertion relies on Ethernet addresses location in contigous
> memory (do not split across mbuf segments). There is no any formal
> requirements on data location and mbuf structure which guarantee it.
> So, check it explicitly to avoid corrupted packets if the condition
> is violated. Typically software VLAN insertion is done on Tx prepare
> stage and application will get indication that the packet is invalid
> and cannot be transmitted.
> 
> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
> ---
>  lib/librte_net/rte_ether.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/lib/librte_net/rte_ether.h b/lib/librte_net/rte_ether.h
> index 0ae4e75b6c..d7c076bba8 100644
> --- a/lib/librte_net/rte_ether.h
> +++ b/lib/librte_net/rte_ether.h
> @@ -357,6 +357,10 @@ static inline int rte_vlan_insert(struct rte_mbuf **m)
>  	if (!RTE_MBUF_DIRECT(*m) || rte_mbuf_refcnt_read(*m) > 1)
>  		return -EINVAL;
>  
> +	/* Can't insert header if the first segment is too short */
> +	if (rte_pktmbuf_data_len(*m) < 2 * RTE_ETHER_ADDR_LEN)
> +		return -EINVAL;

Looks good, but you could also make it handle the fragment case with:

diff --git a/lib/librte_net/rte_ether.h b/lib/librte_net/rte_ether.h
index 0ae4e75b6c58..4d0e310a4fac 100644
--- a/lib/librte_net/rte_ether.h
+++ b/lib/librte_net/rte_ether.h
@@ -350,14 +350,18 @@ static inline int rte_vlan_strip(struct rte_mbuf *m)
  */
 static inline int rte_vlan_insert(struct rte_mbuf **m)
 {
-	struct rte_ether_hdr *oh, *nh;
+	struct rte_ether_hdr *nh, tmp;
+	const struct rte_ether_hdr *oh;
 	struct rte_vlan_hdr *vh;
 
 	/* Can't insert header if mbuf is shared */
 	if (!RTE_MBUF_DIRECT(*m) || rte_mbuf_refcnt_read(*m) > 1)
 		return -EINVAL;
 
-	oh = rte_pktmbuf_mtod(*m, struct rte_ether_hdr *);
+	oh = rte_pktmbuf_read(*m, 0, sizeof(*oh), &tmp);
+	if (unlikely(oh == NULL))
+		return -EINVAL;
+
 	nh = (struct rte_ether_hdr *)
 		rte_pktmbuf_prepend(*m, sizeof(struct rte_vlan_hdr));
 	if (nh == NULL)

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [dpdk-dev] [PATCH] ether: check the first segment length on SW VLAN insertion
  2020-05-29  5:43 ` Stephen Hemminger
@ 2020-06-25 12:27   ` Andrew Rybchenko
  2020-09-14 13:09     ` Ferruh Yigit
  0 siblings, 1 reply; 4+ messages in thread
From: Andrew Rybchenko @ 2020-06-25 12:27 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: Olivier Matz, dev

On 5/29/20 8:43 AM, Stephen Hemminger wrote:
> On Wed, 27 May 2020 15:31:41 +0100
> Andrew Rybchenko <arybchenko@solarflare.com> wrote:
> 
>> SW VLAN insertion relies on Ethernet addresses location in contigous
>> memory (do not split across mbuf segments). There is no any formal
>> requirements on data location and mbuf structure which guarantee it.
>> So, check it explicitly to avoid corrupted packets if the condition
>> is violated. Typically software VLAN insertion is done on Tx prepare
>> stage and application will get indication that the packet is invalid
>> and cannot be transmitted.
>>
>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
>> ---
>>  lib/librte_net/rte_ether.h | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/lib/librte_net/rte_ether.h b/lib/librte_net/rte_ether.h
>> index 0ae4e75b6c..d7c076bba8 100644
>> --- a/lib/librte_net/rte_ether.h
>> +++ b/lib/librte_net/rte_ether.h
>> @@ -357,6 +357,10 @@ static inline int rte_vlan_insert(struct rte_mbuf **m)
>>  	if (!RTE_MBUF_DIRECT(*m) || rte_mbuf_refcnt_read(*m) > 1)
>>  		return -EINVAL;
>>  
>> +	/* Can't insert header if the first segment is too short */
>> +	if (rte_pktmbuf_data_len(*m) < 2 * RTE_ETHER_ADDR_LEN)
>> +		return -EINVAL;
> 
> Looks good, but you could also make it handle the fragment case with:
> 
> diff --git a/lib/librte_net/rte_ether.h b/lib/librte_net/rte_ether.h
> index 0ae4e75b6c58..4d0e310a4fac 100644
> --- a/lib/librte_net/rte_ether.h
> +++ b/lib/librte_net/rte_ether.h
> @@ -350,14 +350,18 @@ static inline int rte_vlan_strip(struct rte_mbuf *m)
>   */
>  static inline int rte_vlan_insert(struct rte_mbuf **m)
>  {
> -	struct rte_ether_hdr *oh, *nh;
> +	struct rte_ether_hdr *nh, tmp;
> +	const struct rte_ether_hdr *oh;
>  	struct rte_vlan_hdr *vh;
>  
>  	/* Can't insert header if mbuf is shared */
>  	if (!RTE_MBUF_DIRECT(*m) || rte_mbuf_refcnt_read(*m) > 1)
>  		return -EINVAL;
>  
> -	oh = rte_pktmbuf_mtod(*m, struct rte_ether_hdr *);
> +	oh = rte_pktmbuf_read(*m, 0, sizeof(*oh), &tmp);
> +	if (unlikely(oh == NULL))
> +		return -EINVAL;
> +
>  	nh = (struct rte_ether_hdr *)
>  		rte_pktmbuf_prepend(*m, sizeof(struct rte_vlan_hdr));
>  	if (nh == NULL)
> 

It is more complicated since memmove() below should be
rewritten  in a similar way as rte_pktmbuf_read(), but
write. I'm not sure it worse the effort.

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [dpdk-dev] [PATCH] ether: check the first segment length on SW VLAN insertion
  2020-06-25 12:27   ` Andrew Rybchenko
@ 2020-09-14 13:09     ` Ferruh Yigit
  0 siblings, 0 replies; 4+ messages in thread
From: Ferruh Yigit @ 2020-09-14 13:09 UTC (permalink / raw)
  To: Andrew Rybchenko, Stephen Hemminger; +Cc: Olivier Matz, dev

On 6/25/2020 1:27 PM, Andrew Rybchenko wrote:
> On 5/29/20 8:43 AM, Stephen Hemminger wrote:
>> On Wed, 27 May 2020 15:31:41 +0100
>> Andrew Rybchenko <arybchenko@solarflare.com> wrote:
>>
>>> SW VLAN insertion relies on Ethernet addresses location in contigous
>>> memory (do not split across mbuf segments). There is no any formal
>>> requirements on data location and mbuf structure which guarantee it.
>>> So, check it explicitly to avoid corrupted packets if the condition
>>> is violated. Typically software VLAN insertion is done on Tx prepare
>>> stage and application will get indication that the packet is invalid
>>> and cannot be transmitted.
>>>
>>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
>>> ---
>>>  lib/librte_net/rte_ether.h | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/lib/librte_net/rte_ether.h b/lib/librte_net/rte_ether.h
>>> index 0ae4e75b6c..d7c076bba8 100644
>>> --- a/lib/librte_net/rte_ether.h
>>> +++ b/lib/librte_net/rte_ether.h
>>> @@ -357,6 +357,10 @@ static inline int rte_vlan_insert(struct rte_mbuf **m)
>>>  	if (!RTE_MBUF_DIRECT(*m) || rte_mbuf_refcnt_read(*m) > 1)
>>>  		return -EINVAL;
>>>  
>>> +	/* Can't insert header if the first segment is too short */
>>> +	if (rte_pktmbuf_data_len(*m) < 2 * RTE_ETHER_ADDR_LEN)
>>> +		return -EINVAL;
>>
>> Looks good, but you could also make it handle the fragment case with:
>>
>> diff --git a/lib/librte_net/rte_ether.h b/lib/librte_net/rte_ether.h
>> index 0ae4e75b6c58..4d0e310a4fac 100644
>> --- a/lib/librte_net/rte_ether.h
>> +++ b/lib/librte_net/rte_ether.h
>> @@ -350,14 +350,18 @@ static inline int rte_vlan_strip(struct rte_mbuf *m)
>>   */
>>  static inline int rte_vlan_insert(struct rte_mbuf **m)
>>  {
>> -	struct rte_ether_hdr *oh, *nh;
>> +	struct rte_ether_hdr *nh, tmp;
>> +	const struct rte_ether_hdr *oh;
>>  	struct rte_vlan_hdr *vh;
>>  
>>  	/* Can't insert header if mbuf is shared */
>>  	if (!RTE_MBUF_DIRECT(*m) || rte_mbuf_refcnt_read(*m) > 1)
>>  		return -EINVAL;
>>  
>> -	oh = rte_pktmbuf_mtod(*m, struct rte_ether_hdr *);
>> +	oh = rte_pktmbuf_read(*m, 0, sizeof(*oh), &tmp);
>> +	if (unlikely(oh == NULL))
>> +		return -EINVAL;
>> +
>>  	nh = (struct rte_ether_hdr *)
>>  		rte_pktmbuf_prepend(*m, sizeof(struct rte_vlan_hdr));
>>  	if (nh == NULL)
>>
> 
> It is more complicated since memmove() below should be
> rewritten  in a similar way as rte_pktmbuf_read(), but
> write. I'm not sure it worse the effort.
> 

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

Applied to dpdk-next-net/main, thanks.

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2020-09-14 13:09 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-27 14:31 [dpdk-dev] [PATCH] ether: check the first segment length on SW VLAN insertion Andrew Rybchenko
2020-05-29  5:43 ` Stephen Hemminger
2020-06-25 12:27   ` Andrew Rybchenko
2020-09-14 13:09     ` Ferruh Yigit

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