[dpdk-stable] [PATCH 1/4] net/tap: fix Rx cksum flags on IP options packets

[dpdk-stable] [PATCH 1/4] net/tap: fix Rx cksum flags on IP options packets

[Olivier Matz]

When packet type is IPV4_EXT, the checksum is always marked as good in
the mbuf offload flags.

Since we know the header lengths, we can easily call
rte_ipv4_udptcp_cksum() in this case too.

Fixes: 8ae3023387e9 ("net/tap: add Rx/Tx checksum offload support")
Cc: stable@dpdk.org

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 drivers/net/tap/rte_eth_tap.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 68baa18523..e7b185a4b5 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -350,7 +350,7 @@ tap_verify_csum(struct rte_mbuf *mbuf)
 		/* Don't verify checksum for multi-segment packets. */
 		if (mbuf->nb_segs > 1)
 			return;
-		if (l3 == RTE_PTYPE_L3_IPV4) {
+		if (l3 == RTE_PTYPE_L3_IPV4 || l3 == RTE_PTYPE_L3_IPV4_EXT) {
 			if (l4 == RTE_PTYPE_L4_UDP) {
 				udp_hdr = (struct rte_udp_hdr *)l4_hdr;
 				if (udp_hdr->dgram_cksum == 0) {
@@ -364,7 +364,7 @@ tap_verify_csum(struct rte_mbuf *mbuf)
 				}
 			}
 			cksum = ~rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
-		} else if (l3 == RTE_PTYPE_L3_IPV6) {
+		} else { /* l3 == RTE_PTYPE_L3_IPV6, checked above */
 			cksum = ~rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
 		}
 		mbuf->ol_flags |= cksum ?
-- 
2.29.2

[dpdk-stable] [PATCH 2/4] net/tap: fix Rx cksum flags on TCP packets

[Olivier Matz]

Since commit d5df2ae0428a ("net: fix unneeded replacement of TCP
checksum 0"), the functions rte_ipv4_udptcp_cksum() or
rte_ipv6_udptcp_cksum() can return either 0x0000 or 0xffff when used to
verify a packet containing a valid checksum.

This new behavior broke the checksum verification in tap driver for TCP
packets: these packets are marked with PKT_RX_L4_CKSUM_BAD.

Fix this by checking the 2 possible values. A next commit will introduce
a checksum verification helper to simplify this a bit.

Fixes: d5df2ae0428a ("net: fix unneeded replacement of TCP checksum 0")
Cc: stable@dpdk.org

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 drivers/net/tap/rte_eth_tap.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index e7b185a4b5..71282e8065 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -346,6 +346,8 @@ tap_verify_csum(struct rte_mbuf *mbuf)
 		return;
 	}
 	if (l4 == RTE_PTYPE_L4_UDP || l4 == RTE_PTYPE_L4_TCP) {
+		int cksum_ok;
+
 		l4_hdr = rte_pktmbuf_mtod_offset(mbuf, void *, l2_len + l3_len);
 		/* Don't verify checksum for multi-segment packets. */
 		if (mbuf->nb_segs > 1)
@@ -363,13 +365,13 @@ tap_verify_csum(struct rte_mbuf *mbuf)
 					return;
 				}
 			}
-			cksum = ~rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
+			cksum = rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
 		} else { /* l3 == RTE_PTYPE_L3_IPV6, checked above */
-			cksum = ~rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
+			cksum = rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
 		}
-		mbuf->ol_flags |= cksum ?
-			PKT_RX_L4_CKSUM_BAD :
-			PKT_RX_L4_CKSUM_GOOD;
+		cksum_ok = (cksum == 0) || (cksum == 0xffff);
+		mbuf->ol_flags |= cksum_ok ?
+			PKT_RX_L4_CKSUM_GOOD : PKT_RX_L4_CKSUM_BAD;
 	}
 }
 
-- 
2.29.2

Re: [dpdk-stable] [PATCH 1/4] net/tap: fix Rx cksum flags on IP options packets

[Ferruh Yigit]

On 4/27/2021 2:57 PM, Olivier Matz wrote:
> When packet type is IPV4_EXT, the checksum is always marked as good in
> the mbuf offload flags.
> 
> Since we know the header lengths, we can easily call
> rte_ipv4_udptcp_cksum() in this case too.
> 
> Fixes: 8ae3023387e9 ("net/tap: add Rx/Tx checksum offload support")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> ---
>  drivers/net/tap/rte_eth_tap.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index 68baa18523..e7b185a4b5 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -350,7 +350,7 @@ tap_verify_csum(struct rte_mbuf *mbuf)
>  		/* Don't verify checksum for multi-segment packets. */
>  		if (mbuf->nb_segs > 1)
>  			return;
> -		if (l3 == RTE_PTYPE_L3_IPV4) {
> +		if (l3 == RTE_PTYPE_L3_IPV4 || l3 == RTE_PTYPE_L3_IPV4_EXT) {

Should we take 'RTE_PTYPE_L3_IPV4_EXT_UNKNOWN' into account?

>  			if (l4 == RTE_PTYPE_L4_UDP) {
>  				udp_hdr = (struct rte_udp_hdr *)l4_hdr;
>  				if (udp_hdr->dgram_cksum == 0) {
> @@ -364,7 +364,7 @@ tap_verify_csum(struct rte_mbuf *mbuf)
>  				}
>  			}
>  			cksum = ~rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
> -		} else if (l3 == RTE_PTYPE_L3_IPV6) {
> +		} else { /* l3 == RTE_PTYPE_L3_IPV6, checked above */
>  			cksum = ~rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
>  		}
>  		mbuf->ol_flags |= cksum ?
> 

Re: [dpdk-stable] [dpdk-dev] [PATCH 1/4] net/tap: fix Rx cksum flags on IP options packets

[Andrew Rybchenko]

On 4/30/21 5:48 PM, Ferruh Yigit wrote:
> On 4/27/2021 2:57 PM, Olivier Matz wrote:
>> When packet type is IPV4_EXT, the checksum is always marked as good in
>> the mbuf offload flags.
>>
>> Since we know the header lengths, we can easily call
>> rte_ipv4_udptcp_cksum() in this case too.
>>
>> Fixes: 8ae3023387e9 ("net/tap: add Rx/Tx checksum offload support")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
>> ---
>>  drivers/net/tap/rte_eth_tap.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
>> index 68baa18523..e7b185a4b5 100644
>> --- a/drivers/net/tap/rte_eth_tap.c
>> +++ b/drivers/net/tap/rte_eth_tap.c
>> @@ -350,7 +350,7 @@ tap_verify_csum(struct rte_mbuf *mbuf)
>>  		/* Don't verify checksum for multi-segment packets. */
>>  		if (mbuf->nb_segs > 1)
>>  			return;
>> -		if (l3 == RTE_PTYPE_L3_IPV4) {
>> +		if (l3 == RTE_PTYPE_L3_IPV4 || l3 == RTE_PTYPE_L3_IPV4_EXT) {
> 
> Should we take 'RTE_PTYPE_L3_IPV4_EXT_UNKNOWN' into account?

I think we should.

> 
>>  			if (l4 == RTE_PTYPE_L4_UDP) {
>>  				udp_hdr = (struct rte_udp_hdr *)l4_hdr;
>>  				if (udp_hdr->dgram_cksum == 0) {
>> @@ -364,7 +364,7 @@ tap_verify_csum(struct rte_mbuf *mbuf)
>>  				}
>>  			}
>>  			cksum = ~rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
>> -		} else if (l3 == RTE_PTYPE_L3_IPV6) {
>> +		} else { /* l3 == RTE_PTYPE_L3_IPV6, checked above */
>>  			cksum = ~rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
>>  		}
>>  		mbuf->ol_flags |= cksum ?
>>

Re: [dpdk-stable] [dpdk-dev] [PATCH 2/4] net/tap: fix Rx cksum flags on TCP packets

[Andrew Rybchenko]

On 4/27/21 4:57 PM, Olivier Matz wrote:
> Since commit d5df2ae0428a ("net: fix unneeded replacement of TCP
> checksum 0"), the functions rte_ipv4_udptcp_cksum() or
> rte_ipv6_udptcp_cksum() can return either 0x0000 or 0xffff when used to
> verify a packet containing a valid checksum.
> 
> This new behavior broke the checksum verification in tap driver for TCP
> packets: these packets are marked with PKT_RX_L4_CKSUM_BAD.
> 
> Fix this by checking the 2 possible values. A next commit will introduce
> a checksum verification helper to simplify this a bit.
> 
> Fixes: d5df2ae0428a ("net: fix unneeded replacement of TCP checksum 0")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>

Acked-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>

Re: [dpdk-stable] [dpdk-dev] [PATCH 1/4] net/tap: fix Rx cksum flags on IP options packets

[Olivier Matz]

Hi Ferruh, Andrew,

On Tue, Jun 08, 2021 at 01:13:59PM +0300, Andrew Rybchenko wrote:
> On 4/30/21 5:48 PM, Ferruh Yigit wrote:
> > On 4/27/2021 2:57 PM, Olivier Matz wrote:
> >> When packet type is IPV4_EXT, the checksum is always marked as good in
> >> the mbuf offload flags.
> >>
> >> Since we know the header lengths, we can easily call
> >> rte_ipv4_udptcp_cksum() in this case too.
> >>
> >> Fixes: 8ae3023387e9 ("net/tap: add Rx/Tx checksum offload support")
> >> Cc: stable@dpdk.org
> >>
> >> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> >> ---
> >>  drivers/net/tap/rte_eth_tap.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> >> index 68baa18523..e7b185a4b5 100644
> >> --- a/drivers/net/tap/rte_eth_tap.c
> >> +++ b/drivers/net/tap/rte_eth_tap.c
> >> @@ -350,7 +350,7 @@ tap_verify_csum(struct rte_mbuf *mbuf)
> >>  		/* Don't verify checksum for multi-segment packets. */
> >>  		if (mbuf->nb_segs > 1)
> >>  			return;
> >> -		if (l3 == RTE_PTYPE_L3_IPV4) {
> >> +		if (l3 == RTE_PTYPE_L3_IPV4 || l3 == RTE_PTYPE_L3_IPV4_EXT) {
> > 
> > Should we take 'RTE_PTYPE_L3_IPV4_EXT_UNKNOWN' into account?
> 
> I think we should.

I think 'RTE_PTYPE_L3_IPV4_EXT_UNKNOWN' cannot happen here:

- mbuf->packet_type is generated by rte_net_get_ptype(), which cannot
  return 'RTE_PTYPE_L3_IPV4_EXT_UNKNOWN'
- right above this code, we already returned if l3 is not in
  (RTE_PTYPE_L3_IPV4, RTE_PTYPE_L3_IPV4_EXT, RTE_PTYPE_L3_IPV6)

> > 
> >>  			if (l4 == RTE_PTYPE_L4_UDP) {
> >>  				udp_hdr = (struct rte_udp_hdr *)l4_hdr;
> >>  				if (udp_hdr->dgram_cksum == 0) {
> >> @@ -364,7 +364,7 @@ tap_verify_csum(struct rte_mbuf *mbuf)
> >>  				}
> >>  			}
> >>  			cksum = ~rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
> >> -		} else if (l3 == RTE_PTYPE_L3_IPV6) {
> >> +		} else { /* l3 == RTE_PTYPE_L3_IPV6, checked above */
> >>  			cksum = ~rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
> >>  		}
> >>  		mbuf->ol_flags |= cksum ?
> >>
> 

Re: [dpdk-stable] [dpdk-dev] [PATCH 1/4] net/tap: fix Rx cksum flags on IP options packets

[Andrew Rybchenko]

On 6/8/21 3:29 PM, Olivier Matz wrote:
> Hi Ferruh, Andrew,
> 
> On Tue, Jun 08, 2021 at 01:13:59PM +0300, Andrew Rybchenko wrote:
>> On 4/30/21 5:48 PM, Ferruh Yigit wrote:
>>> On 4/27/2021 2:57 PM, Olivier Matz wrote:
>>>> When packet type is IPV4_EXT, the checksum is always marked as good in
>>>> the mbuf offload flags.
>>>>
>>>> Since we know the header lengths, we can easily call
>>>> rte_ipv4_udptcp_cksum() in this case too.
>>>>
>>>> Fixes: 8ae3023387e9 ("net/tap: add Rx/Tx checksum offload support")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
>>>> ---
>>>>  drivers/net/tap/rte_eth_tap.c | 4 ++--
>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
>>>> index 68baa18523..e7b185a4b5 100644
>>>> --- a/drivers/net/tap/rte_eth_tap.c
>>>> +++ b/drivers/net/tap/rte_eth_tap.c
>>>> @@ -350,7 +350,7 @@ tap_verify_csum(struct rte_mbuf *mbuf)
>>>>  		/* Don't verify checksum for multi-segment packets. */
>>>>  		if (mbuf->nb_segs > 1)
>>>>  			return;
>>>> -		if (l3 == RTE_PTYPE_L3_IPV4) {
>>>> +		if (l3 == RTE_PTYPE_L3_IPV4 || l3 == RTE_PTYPE_L3_IPV4_EXT) {
>>>
>>> Should we take 'RTE_PTYPE_L3_IPV4_EXT_UNKNOWN' into account?
>>
>> I think we should.
> 
> I think 'RTE_PTYPE_L3_IPV4_EXT_UNKNOWN' cannot happen here:
> 
> - mbuf->packet_type is generated by 

(), which cannot
>   return 'RTE_PTYPE_L3_IPV4_EXT_UNKNOWN'

My question if it is guaranteed and the only possible branch.
Can application set packet_type itself and do not call
rte_net_get_ptype(). Yes, typically application knows
if it has IPv4 options in the header or not, but theoretically
could be unaware as well.

> - right above this code, we already returned if l3 is not in
>   (RTE_PTYPE_L3_IPV4, RTE_PTYPE_L3_IPV4_EXT, RTE_PTYPE_L3_IPV6)

If so, it sounds like it should be allowed above as well.

> 
>>>
>>>>  			if (l4 == RTE_PTYPE_L4_UDP) {
>>>>  				udp_hdr = (struct rte_udp_hdr *)l4_hdr;
>>>>  				if (udp_hdr->dgram_cksum == 0) {
>>>> @@ -364,7 +364,7 @@ tap_verify_csum(struct rte_mbuf *mbuf)
>>>>  				}
>>>>  			}
>>>>  			cksum = ~rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
>>>> -		} else if (l3 == RTE_PTYPE_L3_IPV6) {
>>>> +		} else { /* l3 == RTE_PTYPE_L3_IPV6, checked above */
>>>>  			cksum = ~rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
>>>>  		}
>>>>  		mbuf->ol_flags |= cksum ?
>>>>
>>

Re: [dpdk-stable] [dpdk-dev] [PATCH 1/4] net/tap: fix Rx cksum flags on IP options packets

[Olivier Matz]

On Tue, Jun 08, 2021 at 03:34:36PM +0300, Andrew Rybchenko wrote:
> On 6/8/21 3:29 PM, Olivier Matz wrote:
> > Hi Ferruh, Andrew,
> > 
> > On Tue, Jun 08, 2021 at 01:13:59PM +0300, Andrew Rybchenko wrote:
> >> On 4/30/21 5:48 PM, Ferruh Yigit wrote:
> >>> On 4/27/2021 2:57 PM, Olivier Matz wrote:
> >>>> When packet type is IPV4_EXT, the checksum is always marked as good in
> >>>> the mbuf offload flags.
> >>>>
> >>>> Since we know the header lengths, we can easily call
> >>>> rte_ipv4_udptcp_cksum() in this case too.
> >>>>
> >>>> Fixes: 8ae3023387e9 ("net/tap: add Rx/Tx checksum offload support")
> >>>> Cc: stable@dpdk.org
> >>>>
> >>>> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> >>>> ---
> >>>>  drivers/net/tap/rte_eth_tap.c | 4 ++--
> >>>>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> >>>> index 68baa18523..e7b185a4b5 100644
> >>>> --- a/drivers/net/tap/rte_eth_tap.c
> >>>> +++ b/drivers/net/tap/rte_eth_tap.c
> >>>> @@ -350,7 +350,7 @@ tap_verify_csum(struct rte_mbuf *mbuf)
> >>>>  		/* Don't verify checksum for multi-segment packets. */
> >>>>  		if (mbuf->nb_segs > 1)
> >>>>  			return;
> >>>> -		if (l3 == RTE_PTYPE_L3_IPV4) {
> >>>> +		if (l3 == RTE_PTYPE_L3_IPV4 || l3 == RTE_PTYPE_L3_IPV4_EXT) {
> >>>
> >>> Should we take 'RTE_PTYPE_L3_IPV4_EXT_UNKNOWN' into account?
> >>
> >> I think we should.
> > 
> > I think 'RTE_PTYPE_L3_IPV4_EXT_UNKNOWN' cannot happen here:
> > 
> > - mbuf->packet_type is generated by 
> 
> (), which cannot
> >   return 'RTE_PTYPE_L3_IPV4_EXT_UNKNOWN'
> 
> My question if it is guaranteed and the only possible branch.
> Can application set packet_type itself and do not call
> rte_net_get_ptype(). Yes, typically application knows
> if it has IPv4 options in the header or not, but theoretically
> could be unaware as well.

This function is called on the Rx path from pmd_rx_burst(), so
the application does not have access to the mbuf.

The software parser that sets the packet type returns either
RTE_PTYPE_L3_IPV4 if there is no option, or RTE_PTYPE_L3_IPV4_EXT
else. The value RTE_PTYPE_L3_IPV4_EXT_UNKNOWN is used by PMDs that don't
know if there are options.

> > - right above this code, we already returned if l3 is not in
> >   (RTE_PTYPE_L3_IPV4, RTE_PTYPE_L3_IPV4_EXT, RTE_PTYPE_L3_IPV6)
> 
> If so, it sounds like it should be allowed above as well.
> 
> > 
> >>>
> >>>>  			if (l4 == RTE_PTYPE_L4_UDP) {
> >>>>  				udp_hdr = (struct rte_udp_hdr *)l4_hdr;
> >>>>  				if (udp_hdr->dgram_cksum == 0) {
> >>>> @@ -364,7 +364,7 @@ tap_verify_csum(struct rte_mbuf *mbuf)
> >>>>  				}
> >>>>  			}
> >>>>  			cksum = ~rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
> >>>> -		} else if (l3 == RTE_PTYPE_L3_IPV6) {
> >>>> +		} else { /* l3 == RTE_PTYPE_L3_IPV6, checked above */
> >>>>  			cksum = ~rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
> >>>>  		}
> >>>>  		mbuf->ol_flags |= cksum ?
> >>>>
> >>
> 

Re: [dpdk-stable] [dpdk-dev] [PATCH 1/4] net/tap: fix Rx cksum flags on IP options packets

[Andrew Rybchenko]

On 6/8/21 3:49 PM, Olivier Matz wrote:
> On Tue, Jun 08, 2021 at 03:34:36PM +0300, Andrew Rybchenko wrote:
>> On 6/8/21 3:29 PM, Olivier Matz wrote:
>>> Hi Ferruh, Andrew,
>>>
>>> On Tue, Jun 08, 2021 at 01:13:59PM +0300, Andrew Rybchenko wrote:
>>>> On 4/30/21 5:48 PM, Ferruh Yigit wrote:
>>>>> On 4/27/2021 2:57 PM, Olivier Matz wrote:
>>>>>> When packet type is IPV4_EXT, the checksum is always marked as good in
>>>>>> the mbuf offload flags.
>>>>>>
>>>>>> Since we know the header lengths, we can easily call
>>>>>> rte_ipv4_udptcp_cksum() in this case too.
>>>>>>
>>>>>> Fixes: 8ae3023387e9 ("net/tap: add Rx/Tx checksum offload support")
>>>>>> Cc: stable@dpdk.org
>>>>>>
>>>>>> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
>>>>>> ---
>>>>>>  drivers/net/tap/rte_eth_tap.c | 4 ++--
>>>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
>>>>>> index 68baa18523..e7b185a4b5 100644
>>>>>> --- a/drivers/net/tap/rte_eth_tap.c
>>>>>> +++ b/drivers/net/tap/rte_eth_tap.c
>>>>>> @@ -350,7 +350,7 @@ tap_verify_csum(struct rte_mbuf *mbuf)
>>>>>>  		/* Don't verify checksum for multi-segment packets. */
>>>>>>  		if (mbuf->nb_segs > 1)
>>>>>>  			return;
>>>>>> -		if (l3 == RTE_PTYPE_L3_IPV4) {
>>>>>> +		if (l3 == RTE_PTYPE_L3_IPV4 || l3 == RTE_PTYPE_L3_IPV4_EXT) {
>>>>>
>>>>> Should we take 'RTE_PTYPE_L3_IPV4_EXT_UNKNOWN' into account?
>>>>
>>>> I think we should.
>>>
>>> I think 'RTE_PTYPE_L3_IPV4_EXT_UNKNOWN' cannot happen here:
>>>
>>> - mbuf->packet_type is generated by 
>>
>> (), which cannot
>>>   return 'RTE_PTYPE_L3_IPV4_EXT_UNKNOWN'
>>
>> My question if it is guaranteed and the only possible branch.
>> Can application set packet_type itself and do not call
>> rte_net_get_ptype(). Yes, typically application knows
>> if it has IPv4 options in the header or not, but theoretically
>> could be unaware as well.
> 
> This function is called on the Rx path from pmd_rx_burst(), so
> the application does not have access to the mbuf.
> 
> The software parser that sets the packet type returns either
> RTE_PTYPE_L3_IPV4 if there is no option, or RTE_PTYPE_L3_IPV4_EXT
> else. The value RTE_PTYPE_L3_IPV4_EXT_UNKNOWN is used by PMDs that don't
> know if there are options.

I see. What I'm trying to say that there are non
obvious assumptions here on rte_net_get_ptype()
behaviour which can be changed. May be it makes
sense to add comments here to highlight it.

> 
>>> - right above this code, we already returned if l3 is not in
>>>   (RTE_PTYPE_L3_IPV4, RTE_PTYPE_L3_IPV4_EXT, RTE_PTYPE_L3_IPV6)
>>
>> If so, it sounds like it should be allowed above as well.
>>
>>>
>>>>>
>>>>>>  			if (l4 == RTE_PTYPE_L4_UDP) {
>>>>>>  				udp_hdr = (struct rte_udp_hdr *)l4_hdr;
>>>>>>  				if (udp_hdr->dgram_cksum == 0) {
>>>>>> @@ -364,7 +364,7 @@ tap_verify_csum(struct rte_mbuf *mbuf)
>>>>>>  				}
>>>>>>  			}
>>>>>>  			cksum = ~rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
>>>>>> -		} else if (l3 == RTE_PTYPE_L3_IPV6) {
>>>>>> +		} else { /* l3 == RTE_PTYPE_L3_IPV6, checked above */
>>>>>>  			cksum = ~rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
>>>>>>  		}
>>>>>>  		mbuf->ol_flags |= cksum ?
>>>>>>
>>>>
>>

Re: [dpdk-stable] [dpdk-dev] [PATCH 1/4] net/tap: fix Rx cksum flags on IP options packets

[Olivier Matz]

On Tue, Jun 08, 2021 at 04:57:00PM +0300, Andrew Rybchenko wrote:
> On 6/8/21 3:49 PM, Olivier Matz wrote:
> > On Tue, Jun 08, 2021 at 03:34:36PM +0300, Andrew Rybchenko wrote:
> >> On 6/8/21 3:29 PM, Olivier Matz wrote:
> >>> Hi Ferruh, Andrew,
> >>>
> >>> On Tue, Jun 08, 2021 at 01:13:59PM +0300, Andrew Rybchenko wrote:
> >>>> On 4/30/21 5:48 PM, Ferruh Yigit wrote:
> >>>>> On 4/27/2021 2:57 PM, Olivier Matz wrote:
> >>>>>> When packet type is IPV4_EXT, the checksum is always marked as good in
> >>>>>> the mbuf offload flags.
> >>>>>>
> >>>>>> Since we know the header lengths, we can easily call
> >>>>>> rte_ipv4_udptcp_cksum() in this case too.
> >>>>>>
> >>>>>> Fixes: 8ae3023387e9 ("net/tap: add Rx/Tx checksum offload support")
> >>>>>> Cc: stable@dpdk.org
> >>>>>>
> >>>>>> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> >>>>>> ---
> >>>>>>  drivers/net/tap/rte_eth_tap.c | 4 ++--
> >>>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> >>>>>> index 68baa18523..e7b185a4b5 100644
> >>>>>> --- a/drivers/net/tap/rte_eth_tap.c
> >>>>>> +++ b/drivers/net/tap/rte_eth_tap.c
> >>>>>> @@ -350,7 +350,7 @@ tap_verify_csum(struct rte_mbuf *mbuf)
> >>>>>>  		/* Don't verify checksum for multi-segment packets. */
> >>>>>>  		if (mbuf->nb_segs > 1)
> >>>>>>  			return;
> >>>>>> -		if (l3 == RTE_PTYPE_L3_IPV4) {
> >>>>>> +		if (l3 == RTE_PTYPE_L3_IPV4 || l3 == RTE_PTYPE_L3_IPV4_EXT) {
> >>>>>
> >>>>> Should we take 'RTE_PTYPE_L3_IPV4_EXT_UNKNOWN' into account?
> >>>>
> >>>> I think we should.
> >>>
> >>> I think 'RTE_PTYPE_L3_IPV4_EXT_UNKNOWN' cannot happen here:
> >>>
> >>> - mbuf->packet_type is generated by 
> >>
> >> (), which cannot
> >>>   return 'RTE_PTYPE_L3_IPV4_EXT_UNKNOWN'
> >>
> >> My question if it is guaranteed and the only possible branch.
> >> Can application set packet_type itself and do not call
> >> rte_net_get_ptype(). Yes, typically application knows
> >> if it has IPv4 options in the header or not, but theoretically
> >> could be unaware as well.
> > 
> > This function is called on the Rx path from pmd_rx_burst(), so
> > the application does not have access to the mbuf.
> > 
> > The software parser that sets the packet type returns either
> > RTE_PTYPE_L3_IPV4 if there is no option, or RTE_PTYPE_L3_IPV4_EXT
> > else. The value RTE_PTYPE_L3_IPV4_EXT_UNKNOWN is used by PMDs that don't
> > know if there are options.
> 
> I see. What I'm trying to say that there are non
> obvious assumptions here on rte_net_get_ptype()
> behaviour which can be changed. May be it makes
> sense to add comments here to highlight it.

Ok, I'll add some words about it.

Thanks!

> 
> > 
> >>> - right above this code, we already returned if l3 is not in
> >>>   (RTE_PTYPE_L3_IPV4, RTE_PTYPE_L3_IPV4_EXT, RTE_PTYPE_L3_IPV6)
> >>
> >> If so, it sounds like it should be allowed above as well.
> >>
> >>>
> >>>>>
> >>>>>>  			if (l4 == RTE_PTYPE_L4_UDP) {
> >>>>>>  				udp_hdr = (struct rte_udp_hdr *)l4_hdr;
> >>>>>>  				if (udp_hdr->dgram_cksum == 0) {
> >>>>>> @@ -364,7 +364,7 @@ tap_verify_csum(struct rte_mbuf *mbuf)
> >>>>>>  				}
> >>>>>>  			}
> >>>>>>  			cksum = ~rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
> >>>>>> -		} else if (l3 == RTE_PTYPE_L3_IPV6) {
> >>>>>> +		} else { /* l3 == RTE_PTYPE_L3_IPV6, checked above */
> >>>>>>  			cksum = ~rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
> >>>>>>  		}
> >>>>>>  		mbuf->ol_flags |= cksum ?
> >>>>>>
> >>>>
> >>
> 

[dpdk-stable] [PATCH v2 1/4] net/tap: fix Rx cksum flags on IP options packets

[Olivier Matz]

When packet type is IPV4_EXT, the checksum is always marked as good in
the mbuf offload flags.

Since we know the header lengths, we can easily call
rte_ipv4_udptcp_cksum() in this case too.

Fixes: 8ae3023387e9 ("net/tap: add Rx/Tx checksum offload support")
Cc: stable@dpdk.org

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 drivers/net/tap/rte_eth_tap.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 5735988e7c..5513cfd2d7 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -342,7 +342,11 @@ tap_verify_csum(struct rte_mbuf *mbuf)
 				rte_pktmbuf_data_len(mbuf))
 			return;
 	} else {
-		/* IPv6 extensions are not supported */
+		/* - RTE_PTYPE_L3_IPV4_EXT_UNKNOWN cannot happen because
+		 *   mbuf->packet_type is filled by rte_net_get_ptype() which
+		 *   never returns this value.
+		 * - IPv6 extensions are not supported.
+		 */
 		return;
 	}
 	if (l4 == RTE_PTYPE_L4_UDP || l4 == RTE_PTYPE_L4_TCP) {
@@ -350,7 +354,7 @@ tap_verify_csum(struct rte_mbuf *mbuf)
 		/* Don't verify checksum for multi-segment packets. */
 		if (mbuf->nb_segs > 1)
 			return;
-		if (l3 == RTE_PTYPE_L3_IPV4) {
+		if (l3 == RTE_PTYPE_L3_IPV4 || l3 == RTE_PTYPE_L3_IPV4_EXT) {
 			if (l4 == RTE_PTYPE_L4_UDP) {
 				udp_hdr = (struct rte_udp_hdr *)l4_hdr;
 				if (udp_hdr->dgram_cksum == 0) {
@@ -364,7 +368,7 @@ tap_verify_csum(struct rte_mbuf *mbuf)
 				}
 			}
 			cksum = ~rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
-		} else if (l3 == RTE_PTYPE_L3_IPV6) {
+		} else { /* l3 == RTE_PTYPE_L3_IPV6, checked above */
 			cksum = ~rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
 		}
 		mbuf->ol_flags |= cksum ?
-- 
2.29.2

[dpdk-stable] [PATCH v2 2/4] net/tap: fix Rx cksum flags on TCP packets

[Olivier Matz]

Since commit d5df2ae0428a ("net: fix unneeded replacement of TCP
checksum 0"), the functions rte_ipv4_udptcp_cksum() or
rte_ipv6_udptcp_cksum() can return either 0x0000 or 0xffff when used to
verify a packet containing a valid checksum.

This new behavior broke the checksum verification in tap driver for TCP
packets: these packets are marked with PKT_RX_L4_CKSUM_BAD.

Fix this by checking the 2 possible values. A next commit will introduce
a checksum verification helper to simplify this a bit.

Fixes: d5df2ae0428a ("net: fix unneeded replacement of TCP checksum 0")
Cc: stable@dpdk.org

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
Acked-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
---
 drivers/net/tap/rte_eth_tap.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 5513cfd2d7..5429f611c1 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -350,6 +350,8 @@ tap_verify_csum(struct rte_mbuf *mbuf)
 		return;
 	}
 	if (l4 == RTE_PTYPE_L4_UDP || l4 == RTE_PTYPE_L4_TCP) {
+		int cksum_ok;
+
 		l4_hdr = rte_pktmbuf_mtod_offset(mbuf, void *, l2_len + l3_len);
 		/* Don't verify checksum for multi-segment packets. */
 		if (mbuf->nb_segs > 1)
@@ -367,13 +369,13 @@ tap_verify_csum(struct rte_mbuf *mbuf)
 					return;
 				}
 			}
-			cksum = ~rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
+			cksum = rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
 		} else { /* l3 == RTE_PTYPE_L3_IPV6, checked above */
-			cksum = ~rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
+			cksum = rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
 		}
-		mbuf->ol_flags |= cksum ?
-			PKT_RX_L4_CKSUM_BAD :
-			PKT_RX_L4_CKSUM_GOOD;
+		cksum_ok = (cksum == 0) || (cksum == 0xffff);
+		mbuf->ol_flags |= cksum_ok ?
+			PKT_RX_L4_CKSUM_GOOD : PKT_RX_L4_CKSUM_BAD;
 	}
 }
 
-- 
2.29.2