Re: [dpdk-dev] [PATCH 3/4] net: introduce functions to verify L4 checksums

[Ferruh Yigit <ferruh.yigit@linux.intel.com>]

On 6/8/2021 1:39 PM, Andrew Rybchenko wrote:
> On 6/8/21 3:29 PM, Olivier Matz wrote:
>> Hi Ferruh, Andrew,
>>
>> On Tue, Jun 08, 2021 at 01:23:33PM +0300, Andrew Rybchenko wrote:
>>> On 4/30/21 6:42 PM, Ferruh Yigit wrote:
>>>> On 4/27/2021 2:57 PM, Olivier Matz wrote:
>>>>> Since commit d5df2ae0428a ("net: fix unneeded replacement of TCP
>>>>> checksum 0"), the functions rte_ipv4_udptcp_cksum() and
>>>>> rte_ipv6_udptcp_cksum() can return either 0x0000 or 0xffff when used to
>>>>> verify a packet containing a valid checksum.
>>>>>
>>>>> Since these functions should be used to calculate the checksum to set in
>>>>> a packet, introduce 2 new helpers for checksum verification. They return
>>>>> 0 if the checksum is valid in the packet.
>>>>>
>>>>> Use this new helper in net/tap driver.
>>>>>
>>>>> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
>>>>> ---
>>>>>  drivers/net/tap/rte_eth_tap.c |   7 +-
>>>>>  lib/net/rte_ip.h              | 124 +++++++++++++++++++++++++++-------
>>>>>  2 files changed, 104 insertions(+), 27 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
>>>>> index 71282e8065..b14d5a1d55 100644
>>>>> --- a/drivers/net/tap/rte_eth_tap.c
>>>>> +++ b/drivers/net/tap/rte_eth_tap.c
>>>>> @@ -365,11 +365,12 @@ tap_verify_csum(struct rte_mbuf *mbuf)
>>>>>  					return;
>>>>>  				}
>>>>>  			}
>>>>> -			cksum = rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
>>>>> +			cksum_ok = !rte_ipv4_udptcp_cksum_verify(l3_hdr,
>>>>> +								 l4_hdr);
>>>>>  		} else { /* l3 == RTE_PTYPE_L3_IPV6, checked above */
>>>>> -			cksum = rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
>>>>> +			cksum_ok = !rte_ipv6_udptcp_cksum_verify(l3_hdr,
>>>>> +								 l4_hdr);
>>>>>  		}
>>>>> -		cksum_ok = (cksum == 0) || (cksum == 0xffff);
>>>>>  		mbuf->ol_flags |= cksum_ok ?
>>>>>  			PKT_RX_L4_CKSUM_GOOD : PKT_RX_L4_CKSUM_BAD;
>>>>>  	}
>>>>> diff --git a/lib/net/rte_ip.h b/lib/net/rte_ip.h
>>>>> index 8c189009b0..ef84bcc5bf 100644
>>>>> --- a/lib/net/rte_ip.h
>>>>> +++ b/lib/net/rte_ip.h
>>>>> @@ -344,20 +344,10 @@ rte_ipv4_phdr_cksum(const struct rte_ipv4_hdr *ipv4_hdr, uint64_t ol_flags)
>>>>>  }
>>>>>  
>>>>>  /**
>>>>> - * Process the IPv4 UDP or TCP checksum.
>>>>> - *
>>>>> - * The IP and layer 4 checksum must be set to 0 in the packet by
>>>>> - * the caller.
>>>>> - *
>>>>> - * @param ipv4_hdr
>>>>> - *   The pointer to the contiguous IPv4 header.
>>>>> - * @param l4_hdr
>>>>> - *   The pointer to the beginning of the L4 header.
>>>>> - * @return
>>>>> - *   The complemented checksum to set in the IP packet.
>>>>> + * @internal Calculate the non-complemented IPv4 L4 checksum
>>>>>   */
>>>>>  static inline uint16_t
>>>>> -rte_ipv4_udptcp_cksum(const struct rte_ipv4_hdr *ipv4_hdr, const void *l4_hdr)
>>>>> +__rte_ipv4_udptcp_cksum(const struct rte_ipv4_hdr *ipv4_hdr, const void *l4_hdr)
>>>>>  {
>>>>>  	uint32_t cksum;
>>>>>  	uint32_t l3_len, l4_len;
>>>>> @@ -374,16 +364,62 @@ rte_ipv4_udptcp_cksum(const struct rte_ipv4_hdr *ipv4_hdr, const void *l4_hdr)
>>>>>  	cksum += rte_ipv4_phdr_cksum(ipv4_hdr, 0);
>>>>>  
>>>>>  	cksum = ((cksum & 0xffff0000) >> 16) + (cksum & 0xffff);
>>>>> -	cksum = (~cksum) & 0xffff;
>>>>> +
>>>>> +	return (uint16_t)cksum;
>>>>> +}
>>>>> +
>>>>> +/**
>>>>> + * Process the IPv4 UDP or TCP checksum.
>>>>> + *
>>>>> + * The IP and layer 4 checksum must be set to 0 in the packet by
>>>>> + * the caller.
>>>>> + *
>>>>> + * @param ipv4_hdr
>>>>> + *   The pointer to the contiguous IPv4 header.
>>>>> + * @param l4_hdr
>>>>> + *   The pointer to the beginning of the L4 header.
>>>>> + * @return
>>>>> + *   The complemented checksum to set in the IP packet.
>>>>> + */
>>>>> +static inline uint16_t
>>>>> +rte_ipv4_udptcp_cksum(const struct rte_ipv4_hdr *ipv4_hdr, const void *l4_hdr)
>>>>> +{
>>>>> +	uint16_t cksum = __rte_ipv4_udptcp_cksum(ipv4_hdr, l4_hdr);
>>>>> +
>>>>> +	cksum = ~cksum;
>>>>> +
>>>>>  	/*
>>>>> -	 * Per RFC 768:If the computed checksum is zero for UDP,
>>>>> +	 * Per RFC 768: If the computed checksum is zero for UDP,
>>>>>  	 * it is transmitted as all ones
>>>>>  	 * (the equivalent in one's complement arithmetic).
>>>>>  	 */
>>>>>  	if (cksum == 0 && ipv4_hdr->next_proto_id == IPPROTO_UDP)
>>>>>  		cksum = 0xffff;
>>>>>  
>>>>> -	return (uint16_t)cksum;
>>>>> +	return cksum;
>>>>> +}
>>>>> +
>>>>> +/**
>>>>> + * Validate the IPv4 UDP or TCP checksum.
>>>>> + *
>>>>> + * @param ipv4_hdr
>>>>> + *   The pointer to the contiguous IPv4 header.
>>>>> + * @param l4_hdr
>>>>> + *   The pointer to the beginning of the L4 header.
>>>>> + * @return
>>>>> + *   Return 0 if the checksum is correct, else -1.
>>>>> + */
>>>>> +__rte_experimental
>>>>> +static inline int
>>>>> +rte_ipv4_udptcp_cksum_verify(const struct rte_ipv4_hdr *ipv4_hdr,
>>>>> +			     const void *l4_hdr)
>>>>> +{
>>>>> +	uint16_t cksum = __rte_ipv4_udptcp_cksum(ipv4_hdr, l4_hdr);
>>>>> +
>>>>> +	if (cksum != 0xffff)
>>>>> +		return -1;
>>>>> +
>>>>> +	return 0;
>>>>
>>>> There is behavior change in tap verify, I am asking just to verify if expected,
>>>>
>>>> Previously for UDP, if calculated checksum is '0', the 'rte_ipv4_udptcp_cksum()'
>>>> returns 0xFFFF.
>>>> And 0xFFFF is taken as good checksum by tap PMD.
>>
>> rte_ipv4_udptcp_cksum() cannot return 0xFFFF: this is only possible if
>> all data is 0. Before verifying a udp packet, the user must check that
>> it is not 0 (which means no checksum). In tcp, "Data offset" is never
>> 0. Moreover, port=0 is a reserved value for both udp and tcp.
>>
>>>> With new 'rte_ipv4_udptcp_cksum_verify()', if calculated checksum is '0' it will
>>>> be taken as bad checksum.
>>>>
>>>> I don't know if calculated checksum with valid checksum in place can return 0.
>>>>
>>>>
>>>> Also for TCP, 'rte_ipv4_udptcp_cksum_verify()' doesn't have inversion (cksum =
>>>> ~cksum;) seems changing pass/fail status of the checksum, unless I am not
>>>> missing anything here.
>>>
>>> Yes, it looks suspicious to me as well.
>>>
>>> Olivier, could you clarify, please.
>>
>> Before commit d5df2ae0428a ("net: fix unneeded replacement of TCP checksum 0"),
>> the behavior was:
>>
>>   // rte_ipv4_udptcp_cksum() is 0xffff if checksum is valid
>>   // so cksum is 0 if checksum is valid
>>   cksum = ~rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
>>   // ol_flags is set to PKT_RX_L4_CKSUM_GOOD if checksum is valid
>>   mbuf->ol_flags |= cksum ? PKT_RX_L4_CKSUM_BAD : PKT_RX_L4_CKSUM_GOOD;
>>
>> After commit d5df2ae0428a ("net: fix unneeded replacement of TCP checksum 0"),
>> it is broken:
>>
>>   // rte_ipv4_udptcp_cksum() is 0 (tcp) or 0xffff (udp) if checksum is valid
>>   // so cksum is 0xffff (tcp) or 0 (udp) if checksum is valid
>>   cksum = ~rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
>>   // ol_flags is set to BAD (tcp) or GOOD (udp) if checksum is valid
>>   mbuf->ol_flags |= cksum ? PKT_RX_L4_CKSUM_BAD : PKT_RX_L4_CKSUM_GOOD;
>>
>> After patch 2/4 ("net/tap: fix Rx cksum flags on TCP packets"), the
>> correct behavior is restored:
>>
>>   // cksum is 0 (tcp) or 0xffff (udp) if checksum is valid
>>   // note: 0xffff for tcp cannot happen (there is at least 1 bit set in the header)
>>   // note: 0 for udp cannot happen (it is replaced by in rte_ipv4_udptcp_cksum())
>>   cksum = rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
>>   // cksum_ok is 1 if checksum is valid
>>   cksum_ok = (cksum == 0) || (cksum == 0xffff);
>>   // ol_flags is set to GOOD if checksum is valid
>>   mbuf->ol_flags |= cksum_ok ? PKT_RX_L4_CKSUM_GOOD : PKT_RX_L4_CKSUM_BAD;
>>
>> After this patch [3/4] ("net: introduce functions to verify L4 checksums"),
>> it is simplified by using rte_ipv4_udptcp_cksum_verify():
>>
>>   // cksum_ok is 1 if checksum is valid
>>   cksum_ok = !rte_ipv4_udptcp_cksum_verify(l3_hdr, l4_hdr);
>>   // ol_flags is set to GOOD if checksum is valid
>>   mbuf->ol_flags |= cksum_ok ? PKT_RX_L4_CKSUM_GOOD : PKT_RX_L4_CKSUM_BAD;
>>
> 
> Many thanks for the detailed explanations. It replies to all my
> questions (even not asked, but kept in my head).
> 

Thanks for clarification, after checking again with help of description above,
it looks good to me.