diff mbox series

net/tap: Allow all-zero checksum for UDP over IPv4

Message ID 20201109142217.115918-1-michael.pfeiffer@tu-ilmenau.de (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series net/tap: Allow all-zero checksum for UDP over IPv4 |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-testing success Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/travis-robot success Travis build: passed
ci/iol-mellanox-Performance success Performance Testing PASS
ci/Intel-compilation success Compilation OK

Commit Message

Michael Pfeiffer Nov. 9, 2020, 2:22 p.m. UTC 
  Unlike TCP, UDP checksums are optional and may be zero to indicate "not
set" [RFC 768] (except for IPv6, where this prohibited [RFC 8200]). Add
this special case to the checksum offload emulation in net/tap.

Signed-off-by: Michael Pfeiffer <michael.pfeiffer@tu-ilmenau.de>
---
 drivers/net/tap/rte_eth_tap.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

Comments

Ferruh Yigit Nov. 10, 2020, 2:46 p.m. UTC | #1 
On 11/9/2020 2:22 PM, Michael Pfeiffer wrote:
> Unlike TCP, UDP checksums are optional and may be zero to indicate "not
> set" [RFC 768] (except for IPv6, where this prohibited [RFC 8200]). Add
> this special case to the checksum offload emulation in net/tap.
> 
> Signed-off-by: Michael Pfeiffer <michael.pfeiffer@tu-ilmenau.de>
> ---
>   drivers/net/tap/rte_eth_tap.c | 13 +++++++++++--
>   1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index 2f8abb12c..e486b41c5 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -303,6 +303,7 @@ tap_verify_csum(struct rte_mbuf *mbuf)
>   	uint16_t cksum = 0;
>   	void *l3_hdr;
>   	void *l4_hdr;
> +	struct rte_udp_hdr *udp_hdr;
>   
>   	if (l2 == RTE_PTYPE_L2_ETHER_VLAN)
>   		l2_len += 4;
> @@ -349,10 +350,18 @@ 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) {
> +			if (l4 == RTE_PTYPE_L4_UDP) {
> +				udp_hdr = (struct rte_udp_hdr *)l4_hdr;
> +				if (udp_hdr->dgram_cksum == 0) {
> +					mbuf->ol_flags |= PKT_RX_L4_CKSUM_NONE;
> +					return;
> +				}
> +			}
>   			cksum = ~rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
> -		else if (l3 == RTE_PTYPE_L3_IPV6)
> +		} else if (l3 == RTE_PTYPE_L3_IPV6) {
>   			cksum = ~rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
> +		}
>   		mbuf->ol_flags |= cksum ?
>   			PKT_RX_L4_CKSUM_BAD :
>   			PKT_RX_L4_CKSUM_GOOD;
> 

While checking this I stuck with following part:

  cksum = ~rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
  ...
   mbuf->ol_flags |= cksum ?
   	PKT_RX_L4_CKSUM_BAD :
  	PKT_RX_L4_CKSUM_GOOD;


Is this correct, or am I missing something, can intention be '!' here instead of 
'~' ?
Ferruh Yigit Nov. 10, 2020, 3:56 p.m. UTC | #2 
On 11/10/2020 2:46 PM, Ferruh Yigit wrote:
> On 11/9/2020 2:22 PM, Michael Pfeiffer wrote:
>> Unlike TCP, UDP checksums are optional and may be zero to indicate "not
>> set" [RFC 768] (except for IPv6, where this prohibited [RFC 8200]). Add
>> this special case to the checksum offload emulation in net/tap.
>>
>> Signed-off-by: Michael Pfeiffer <michael.pfeiffer@tu-ilmenau.de>
>> ---
>>   drivers/net/tap/rte_eth_tap.c | 13 +++++++++++--
>>   1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
>> index 2f8abb12c..e486b41c5 100644
>> --- a/drivers/net/tap/rte_eth_tap.c
>> +++ b/drivers/net/tap/rte_eth_tap.c
>> @@ -303,6 +303,7 @@ tap_verify_csum(struct rte_mbuf *mbuf)
>>       uint16_t cksum = 0;
>>       void *l3_hdr;
>>       void *l4_hdr;
>> +    struct rte_udp_hdr *udp_hdr;
>>       if (l2 == RTE_PTYPE_L2_ETHER_VLAN)
>>           l2_len += 4;
>> @@ -349,10 +350,18 @@ 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) {
>> +            if (l4 == RTE_PTYPE_L4_UDP) {
>> +                udp_hdr = (struct rte_udp_hdr *)l4_hdr;
>> +                if (udp_hdr->dgram_cksum == 0) {
>> +                    mbuf->ol_flags |= PKT_RX_L4_CKSUM_NONE;
>> +                    return;
>> +                }
>> +            }
>>               cksum = ~rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
>> -        else if (l3 == RTE_PTYPE_L3_IPV6)
>> +        } else if (l3 == RTE_PTYPE_L3_IPV6) {
>>               cksum = ~rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
>> +        }
>>           mbuf->ol_flags |= cksum ?
>>               PKT_RX_L4_CKSUM_BAD :
>>               PKT_RX_L4_CKSUM_GOOD;
>>
> 
> While checking this I stuck with following part:
> 
>   cksum = ~rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
>   ...
>    mbuf->ol_flags |= cksum ?
>        PKT_RX_L4_CKSUM_BAD :
>       PKT_RX_L4_CKSUM_GOOD;
> 
> 
> Is this correct, or am I missing something, can intention be '!' here instead of 
> '~' ?

This seems correct, this is related to what cksum functions return and how to 
verify cksum, I will proceed with the patch.
Ferruh Yigit Nov. 10, 2020, 3:59 p.m. UTC | #3 
On 11/9/2020 2:22 PM, Michael Pfeiffer wrote:
> Unlike TCP, UDP checksums are optional and may be zero to indicate "not
> set" [RFC 768] (except for IPv6, where this prohibited [RFC 8200]). Add
> this special case to the checksum offload emulation in net/tap.
> 
> Signed-off-by: Michael Pfeiffer <michael.pfeiffer@tu-ilmenau.de>
> ---
>   drivers/net/tap/rte_eth_tap.c | 13 +++++++++++--
>   1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index 2f8abb12c..e486b41c5 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -303,6 +303,7 @@ tap_verify_csum(struct rte_mbuf *mbuf)
>   	uint16_t cksum = 0;
>   	void *l3_hdr;
>   	void *l4_hdr;
> +	struct rte_udp_hdr *udp_hdr;
>   
>   	if (l2 == RTE_PTYPE_L2_ETHER_VLAN)
>   		l2_len += 4;
> @@ -349,10 +350,18 @@ 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) {
> +			if (l4 == RTE_PTYPE_L4_UDP) {
> +				udp_hdr = (struct rte_udp_hdr *)l4_hdr;
> +				if (udp_hdr->dgram_cksum == 0) {

Overall patch looks good to me, but can you please add a comment on top of above 
check to describe why checksum can be zero, as done in the commit log.

> +					mbuf->ol_flags |= PKT_RX_L4_CKSUM_NONE;
> +					return;
> +				}
> +			}
>   			cksum = ~rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
> -		else if (l3 == RTE_PTYPE_L3_IPV6)
> +		} else if (l3 == RTE_PTYPE_L3_IPV6) {
>   			cksum = ~rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
> +		}
>   		mbuf->ol_flags |= cksum ?
>   			PKT_RX_L4_CKSUM_BAD :
>   			PKT_RX_L4_CKSUM_GOOD;
>
Morten Brørup Nov. 10, 2020, 4:01 p.m. UTC | #4 
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ferruh Yigit
> Sent: Tuesday, November 10, 2020 3:47 PM
> 
> On 11/9/2020 2:22 PM, Michael Pfeiffer wrote:
> > Unlike TCP, UDP checksums are optional and may be zero to indicate "not
> > set" [RFC 768] (except for IPv6, where this prohibited [RFC 8200]). Add
> > this special case to the checksum offload emulation in net/tap.
> >
> > Signed-off-by: Michael Pfeiffer <michael.pfeiffer@tu-ilmenau.de>
> > ---
> >   drivers/net/tap/rte_eth_tap.c | 13 +++++++++++--
> >   1 file changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/tap/rte_eth_tap.c
> b/drivers/net/tap/rte_eth_tap.c
> > index 2f8abb12c..e486b41c5 100644
> > --- a/drivers/net/tap/rte_eth_tap.c
> > +++ b/drivers/net/tap/rte_eth_tap.c
> > @@ -303,6 +303,7 @@ tap_verify_csum(struct rte_mbuf *mbuf)
> >   	uint16_t cksum = 0;
> >   	void *l3_hdr;
> >   	void *l4_hdr;
> > +	struct rte_udp_hdr *udp_hdr;
> >
> >   	if (l2 == RTE_PTYPE_L2_ETHER_VLAN)
> >   		l2_len += 4;
> > @@ -349,10 +350,18 @@ 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) {
> > +			if (l4 == RTE_PTYPE_L4_UDP) {
> > +				udp_hdr = (struct rte_udp_hdr *)l4_hdr;
> > +				if (udp_hdr->dgram_cksum == 0) {
> > +					mbuf->ol_flags |= PKT_RX_L4_CKSUM_NONE;
> > +					return;
> > +				}
> > +			}
> >   			cksum = ~rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
> > -		else if (l3 == RTE_PTYPE_L3_IPV6)
> > +		} else if (l3 == RTE_PTYPE_L3_IPV6) {
> >   			cksum = ~rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
> > +		}
> >   		mbuf->ol_flags |= cksum ?
> >   			PKT_RX_L4_CKSUM_BAD :
> >   			PKT_RX_L4_CKSUM_GOOD;
> >
> 
> While checking this I stuck with following part:
> 
>   cksum = ~rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
>   ...
>    mbuf->ol_flags |= cksum ?
>    	PKT_RX_L4_CKSUM_BAD :
>   	PKT_RX_L4_CKSUM_GOOD;
> 
> 
> Is this correct, or am I missing something, can intention be '!' here
> instead of
> '~' ?

It is correct. The packet's checksum is calculated by rte_ipv6_udptcp_cksum(), and it should be 0xFFFF. The '~' operation makes cksum 0 iff the calculated checksum is 0xFFFF.
Ferruh Yigit Nov. 10, 2020, 5:42 p.m. UTC | #5 
On 11/10/2020 4:01 PM, Morten Brørup wrote:
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ferruh Yigit
>> Sent: Tuesday, November 10, 2020 3:47 PM
>>
>> On 11/9/2020 2:22 PM, Michael Pfeiffer wrote:
>>> Unlike TCP, UDP checksums are optional and may be zero to indicate "not
>>> set" [RFC 768] (except for IPv6, where this prohibited [RFC 8200]). Add
>>> this special case to the checksum offload emulation in net/tap.
>>>
>>> Signed-off-by: Michael Pfeiffer <michael.pfeiffer@tu-ilmenau.de>
>>> ---
>>>    drivers/net/tap/rte_eth_tap.c | 13 +++++++++++--
>>>    1 file changed, 11 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/tap/rte_eth_tap.c
>> b/drivers/net/tap/rte_eth_tap.c
>>> index 2f8abb12c..e486b41c5 100644
>>> --- a/drivers/net/tap/rte_eth_tap.c
>>> +++ b/drivers/net/tap/rte_eth_tap.c
>>> @@ -303,6 +303,7 @@ tap_verify_csum(struct rte_mbuf *mbuf)
>>>    	uint16_t cksum = 0;
>>>    	void *l3_hdr;
>>>    	void *l4_hdr;
>>> +	struct rte_udp_hdr *udp_hdr;
>>>
>>>    	if (l2 == RTE_PTYPE_L2_ETHER_VLAN)
>>>    		l2_len += 4;
>>> @@ -349,10 +350,18 @@ 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) {
>>> +			if (l4 == RTE_PTYPE_L4_UDP) {
>>> +				udp_hdr = (struct rte_udp_hdr *)l4_hdr;
>>> +				if (udp_hdr->dgram_cksum == 0) {
>>> +					mbuf->ol_flags |= PKT_RX_L4_CKSUM_NONE;
>>> +					return;
>>> +				}
>>> +			}
>>>    			cksum = ~rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
>>> -		else if (l3 == RTE_PTYPE_L3_IPV6)
>>> +		} else if (l3 == RTE_PTYPE_L3_IPV6) {
>>>    			cksum = ~rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
>>> +		}
>>>    		mbuf->ol_flags |= cksum ?
>>>    			PKT_RX_L4_CKSUM_BAD :
>>>    			PKT_RX_L4_CKSUM_GOOD;
>>>
>>
>> While checking this I stuck with following part:
>>
>>    cksum = ~rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
>>    ...
>>     mbuf->ol_flags |= cksum ?
>>     	PKT_RX_L4_CKSUM_BAD :
>>    	PKT_RX_L4_CKSUM_GOOD;
>>
>>
>> Is this correct, or am I missing something, can intention be '!' here
>> instead of
>> '~' ?
> 
> It is correct. The packet's checksum is calculated by rte_ipv6_udptcp_cksum(), and it should be 0xFFFF. The '~' operation makes cksum 0 iff the calculated checksum is 0xFFFF.
> 

Yep, figure that out late,
as far as I understand when the checksum value is zero, 
'rte_ipv6_udptcp_cksum()' will return the checksum value and when checksum is 
correct in the packet, function will return 0xFFFF, this is based on checksum 
calculation, is this right?
Morten Brørup Nov. 11, 2020, 7:06 a.m. UTC | #6 
> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
> Sent: Tuesday, November 10, 2020 6:43 PM
> 
> On 11/10/2020 4:01 PM, Morten Brørup wrote:
> >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ferruh Yigit
> >> Sent: Tuesday, November 10, 2020 3:47 PM
> >>
> >> On 11/9/2020 2:22 PM, Michael Pfeiffer wrote:
> >>> Unlike TCP, UDP checksums are optional and may be zero to indicate
> "not
> >>> set" [RFC 768] (except for IPv6, where this prohibited [RFC 8200]).
> Add
> >>> this special case to the checksum offload emulation in net/tap.
> >>>
> >>> Signed-off-by: Michael Pfeiffer <michael.pfeiffer@tu-ilmenau.de>
> >>> ---
> >>>    drivers/net/tap/rte_eth_tap.c | 13 +++++++++++--
> >>>    1 file changed, 11 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/net/tap/rte_eth_tap.c
> >> b/drivers/net/tap/rte_eth_tap.c
> >>> index 2f8abb12c..e486b41c5 100644
> >>> --- a/drivers/net/tap/rte_eth_tap.c
> >>> +++ b/drivers/net/tap/rte_eth_tap.c
> >>> @@ -303,6 +303,7 @@ tap_verify_csum(struct rte_mbuf *mbuf)
> >>>    	uint16_t cksum = 0;
> >>>    	void *l3_hdr;
> >>>    	void *l4_hdr;
> >>> +	struct rte_udp_hdr *udp_hdr;
> >>>
> >>>    	if (l2 == RTE_PTYPE_L2_ETHER_VLAN)
> >>>    		l2_len += 4;
> >>> @@ -349,10 +350,18 @@ 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) {
> >>> +			if (l4 == RTE_PTYPE_L4_UDP) {
> >>> +				udp_hdr = (struct rte_udp_hdr *)l4_hdr;
> >>> +				if (udp_hdr->dgram_cksum == 0) {
> >>> +					mbuf->ol_flags |= PKT_RX_L4_CKSUM_NONE;
> >>> +					return;
> >>> +				}
> >>> +			}
> >>>    			cksum = ~rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
> >>> -		else if (l3 == RTE_PTYPE_L3_IPV6)
> >>> +		} else if (l3 == RTE_PTYPE_L3_IPV6) {
> >>>    			cksum = ~rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
> >>> +		}
> >>>    		mbuf->ol_flags |= cksum ?
> >>>    			PKT_RX_L4_CKSUM_BAD :
> >>>    			PKT_RX_L4_CKSUM_GOOD;
> >>>
> >>
> >> While checking this I stuck with following part:
> >>
> >>    cksum = ~rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
> >>    ...
> >>     mbuf->ol_flags |= cksum ?
> >>     	PKT_RX_L4_CKSUM_BAD :
> >>    	PKT_RX_L4_CKSUM_GOOD;
> >>
> >>
> >> Is this correct, or am I missing something, can intention be '!'
> here
> >> instead of
> >> '~' ?
> >
> > It is correct. The packet's checksum is calculated by
> rte_ipv6_udptcp_cksum(), and it should be 0xFFFF. The '~' operation
> makes cksum 0 iff the calculated checksum is 0xFFFF.
> >
> 
> Yep, figure that out late,
> as far as I understand when the checksum value is zero,
> 'rte_ipv6_udptcp_cksum()' will return the checksum value and when
> checksum is
> correct in the packet, function will return 0xFFFF, this is based on
> checksum
> calculation, is this right?

Exactly.
Michael Pfeiffer Nov. 11, 2020, 7:23 a.m. UTC | #7 
Hi,

On Tue, 2020-11-10 at 15:59 +0000, Ferruh Yigit wrote:
> On 11/9/2020 2:22 PM, Michael Pfeiffer wrote:
> > Unlike TCP, UDP checksums are optional and may be zero to indicate "not
> > set" [RFC 768] (except for IPv6, where this prohibited [RFC 8200]). Add
> > this special case to the checksum offload emulation in net/tap.
> > 
> > Signed-off-by: Michael Pfeiffer <michael.pfeiffer@tu-ilmenau.de>
> > ---
> >   drivers/net/tap/rte_eth_tap.c | 13 +++++++++++--
> >   1 file changed, 11 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> > index 2f8abb12c..e486b41c5 100644
> > --- a/drivers/net/tap/rte_eth_tap.c
> > +++ b/drivers/net/tap/rte_eth_tap.c
> > @@ -303,6 +303,7 @@ tap_verify_csum(struct rte_mbuf *mbuf)
> >         uint16_t cksum = 0;
> >         void *l3_hdr;
> >         void *l4_hdr;
> > +       struct rte_udp_hdr *udp_hdr;
> >   
> >         if (l2 == RTE_PTYPE_L2_ETHER_VLAN)
> >                 l2_len += 4;
> > @@ -349,10 +350,18 @@ 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) {
> > +                       if (l4 == RTE_PTYPE_L4_UDP) {
> > +                               udp_hdr = (struct rte_udp_hdr *)l4_hdr;
> > +                               if (udp_hdr->dgram_cksum == 0) {
> 
> Overall patch looks good to me, but can you please add a comment on top of
> above 
> check to describe why checksum can be zero, as done in the commit log.

Sure, I will update the patch. I am also not completely sure whether
PKT_RX_L4_CKSUM_NONE is the right flag for this case (rather than _UNKNOWN).
From rte_core_mbuf.h:

 * - PKT_RX_L4_CKSUM_UNKNOWN: no information about the RX L4 checksum
 * - PKT_RX_L4_CKSUM_NONE: the L4 checksum is not correct in the packet
 *   data, but the integrity of the L4 data is verified.

The second part after the "but" is not really the case here. I don't know how
relevant the distinction is, as most application side code will probably only
do something like

if ((mbuf->ol_flags & PKT_RX_L4_CKSUM_MASK) == PKT_RX_L4_CKSUM_BAD)
	rte_pktmbuf_free(mbuf);

anyway. Do you have any opinions on that?

> > +                                       mbuf->ol_flags |=
> > PKT_RX_L4_CKSUM_NONE;
> > +                                       return;
> > +                               }
> > +                       }
> >                         cksum = ~rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
> > -               else if (l3 == RTE_PTYPE_L3_IPV6)
> > +               } else if (l3 == RTE_PTYPE_L3_IPV6) {
> >                         cksum = ~rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
> > +               }
> >                 mbuf->ol_flags |= cksum ?
> >                         PKT_RX_L4_CKSUM_BAD :
> >                         PKT_RX_L4_CKSUM_GOOD;
> > 
> 

Regards
Michael
Ferruh Yigit Nov. 11, 2020, 9:31 a.m. UTC | #8 
On 11/11/2020 7:23 AM, Michael Pfeiffer wrote:
> Hi,
> 
> On Tue, 2020-11-10 at 15:59 +0000, Ferruh Yigit wrote:
>> On 11/9/2020 2:22 PM, Michael Pfeiffer wrote:
>>> Unlike TCP, UDP checksums are optional and may be zero to indicate "not
>>> set" [RFC 768] (except for IPv6, where this prohibited [RFC 8200]). Add
>>> this special case to the checksum offload emulation in net/tap.
>>>
>>> Signed-off-by: Michael Pfeiffer <michael.pfeiffer@tu-ilmenau.de>
>>> ---
>>>    drivers/net/tap/rte_eth_tap.c | 13 +++++++++++--
>>>    1 file changed, 11 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
>>> index 2f8abb12c..e486b41c5 100644
>>> --- a/drivers/net/tap/rte_eth_tap.c
>>> +++ b/drivers/net/tap/rte_eth_tap.c
>>> @@ -303,6 +303,7 @@ tap_verify_csum(struct rte_mbuf *mbuf)
>>>          uint16_t cksum = 0;
>>>          void *l3_hdr;
>>>          void *l4_hdr;
>>> +       struct rte_udp_hdr *udp_hdr;
>>>    
>>>          if (l2 == RTE_PTYPE_L2_ETHER_VLAN)
>>>                  l2_len += 4;
>>> @@ -349,10 +350,18 @@ 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) {
>>> +                       if (l4 == RTE_PTYPE_L4_UDP) {
>>> +                               udp_hdr = (struct rte_udp_hdr *)l4_hdr;
>>> +                               if (udp_hdr->dgram_cksum == 0) {
>>
>> Overall patch looks good to me, but can you please add a comment on top of
>> above
>> check to describe why checksum can be zero, as done in the commit log.
> 
> Sure, I will update the patch. I am also not completely sure whether
> PKT_RX_L4_CKSUM_NONE is the right flag for this case (rather than _UNKNOWN).
>  From rte_core_mbuf.h:
> 
>   * - PKT_RX_L4_CKSUM_UNKNOWN: no information about the RX L4 checksum
>   * - PKT_RX_L4_CKSUM_NONE: the L4 checksum is not correct in the packet
>   *   data, but the integrity of the L4 data is verified.
> 
> The second part after the "but" is not really the case here. I don't know how
> relevant the distinction is, as most application side code will probably only
> do something like
> 
> if ((mbuf->ol_flags & PKT_RX_L4_CKSUM_MASK) == PKT_RX_L4_CKSUM_BAD)
> 	rte_pktmbuf_free(mbuf);
> 
> anyway. Do you have any opinions on that?
> 

I also checked for that and wasn't sure about it :) cc'ed Olivier too for comment.

I think it is NOT 'PKT_RX_L4_CKSUM_UNKNOWN', since we know that checksum value 
is 0x0000 which means it is not provided.

'PKT_RX_L4_CKSUM_NONE' suits better but not sure about the expectation on 
"integrity of the L4 data is verified" part, I assume that explanation is just 
to differentiate between 'CKSUM_BAD'.
Ferruh Yigit Nov. 13, 2020, 1:02 p.m. UTC | #9 
On 11/11/2020 9:31 AM, Ferruh Yigit wrote:
> On 11/11/2020 7:23 AM, Michael Pfeiffer wrote:
>> Hi,
>>
>> On Tue, 2020-11-10 at 15:59 +0000, Ferruh Yigit wrote:
>>> On 11/9/2020 2:22 PM, Michael Pfeiffer wrote:
>>>> Unlike TCP, UDP checksums are optional and may be zero to indicate "not
>>>> set" [RFC 768] (except for IPv6, where this prohibited [RFC 8200]). Add
>>>> this special case to the checksum offload emulation in net/tap.
>>>>
>>>> Signed-off-by: Michael Pfeiffer <michael.pfeiffer@tu-ilmenau.de>
>>>> ---
>>>>    drivers/net/tap/rte_eth_tap.c | 13 +++++++++++--
>>>>    1 file changed, 11 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
>>>> index 2f8abb12c..e486b41c5 100644
>>>> --- a/drivers/net/tap/rte_eth_tap.c
>>>> +++ b/drivers/net/tap/rte_eth_tap.c
>>>> @@ -303,6 +303,7 @@ tap_verify_csum(struct rte_mbuf *mbuf)
>>>>          uint16_t cksum = 0;
>>>>          void *l3_hdr;
>>>>          void *l4_hdr;
>>>> +       struct rte_udp_hdr *udp_hdr;
>>>>          if (l2 == RTE_PTYPE_L2_ETHER_VLAN)
>>>>                  l2_len += 4;
>>>> @@ -349,10 +350,18 @@ 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) {
>>>> +                       if (l4 == RTE_PTYPE_L4_UDP) {
>>>> +                               udp_hdr = (struct rte_udp_hdr *)l4_hdr;
>>>> +                               if (udp_hdr->dgram_cksum == 0) {
>>>
>>> Overall patch looks good to me, but can you please add a comment on top of
>>> above
>>> check to describe why checksum can be zero, as done in the commit log.
>>
>> Sure, I will update the patch. I am also not completely sure whether
>> PKT_RX_L4_CKSUM_NONE is the right flag for this case (rather than _UNKNOWN).
>>  From rte_core_mbuf.h:
>>
>>   * - PKT_RX_L4_CKSUM_UNKNOWN: no information about the RX L4 checksum
>>   * - PKT_RX_L4_CKSUM_NONE: the L4 checksum is not correct in the packet
>>   *   data, but the integrity of the L4 data is verified.
>>
>> The second part after the "but" is not really the case here. I don't know how
>> relevant the distinction is, as most application side code will probably only
>> do something like
>>
>> if ((mbuf->ol_flags & PKT_RX_L4_CKSUM_MASK) == PKT_RX_L4_CKSUM_BAD)
>>     rte_pktmbuf_free(mbuf);
>>
>> anyway. Do you have any opinions on that?
>>
> 
> I also checked for that and wasn't sure about it :) cc'ed Olivier too for comment.
> 
> I think it is NOT 'PKT_RX_L4_CKSUM_UNKNOWN', since we know that checksum value 
> is 0x0000 which means it is not provided.
> 
> 'PKT_RX_L4_CKSUM_NONE' suits better but not sure about the expectation on 
> "integrity of the L4 data is verified" part, I assume that explanation is just 
> to differentiate between 'CKSUM_BAD'.

I suggest to continue with 'PKT_RX_L4_CKSUM_NONE'.

Can it be possible to get the new version today, so we can include this to the -rc4?

Thanks
diff mbox series

Patch

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 2f8abb12c..e486b41c5 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -303,6 +303,7 @@  tap_verify_csum(struct rte_mbuf *mbuf)
 	uint16_t cksum = 0;
 	void *l3_hdr;
 	void *l4_hdr;
+	struct rte_udp_hdr *udp_hdr;
 
 	if (l2 == RTE_PTYPE_L2_ETHER_VLAN)
 		l2_len += 4;
@@ -349,10 +350,18 @@  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) {
+			if (l4 == RTE_PTYPE_L4_UDP) {
+				udp_hdr = (struct rte_udp_hdr *)l4_hdr;
+				if (udp_hdr->dgram_cksum == 0) {
+					mbuf->ol_flags |= PKT_RX_L4_CKSUM_NONE;
+					return;
+				}
+			}
 			cksum = ~rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
-		else if (l3 == RTE_PTYPE_L3_IPV6)
+		} else if (l3 == RTE_PTYPE_L3_IPV6) {
 			cksum = ~rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
+		}
 		mbuf->ol_flags |= cksum ?
 			PKT_RX_L4_CKSUM_BAD :
 			PKT_RX_L4_CKSUM_GOOD;