From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 2B9E9A0C40; Tue, 8 Jun 2021 14:39:28 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id A919640689; Tue, 8 Jun 2021 14:39:27 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id CA6354067A for ; Tue, 8 Jun 2021 14:39:26 +0200 (CEST) Received: from [192.168.38.17] (aros.oktetlabs.ru [192.168.38.17]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id 5D87D7F514; Tue, 8 Jun 2021 15:39:26 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 5D87D7F514 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1623155966; bh=PI/RlbbanTMf4/AsAD0XWygezebPg3cbzuy6ztNhI+Q=; h=Subject:To:Cc:References:From:Date:In-Reply-To; b=nnu05pl7W+DWhXC6CXm6oZM1qSuol61kQybQT8ZBeiNpQjpGWWnUXbGtQxLQqW/2P v0DNbj7ZLI/TOwmgoWzh7C9iNlWNiqSUZDIxHH/mxQid01IjQPhz8JPRyRHt6LP0GL ZhD2bNjWp1byEg8xH8Nn1mfIN11/nmSTG6mneft0= To: Olivier Matz Cc: Ferruh Yigit , dev@dpdk.org, Keith Wiles , Hongzhi Guo , =?UTF-8?Q?Morten_Br=c3=b8rup?= , Thomas Monjalon References: <20210427135755.927-1-olivier.matz@6wind.com> <20210427135755.927-4-olivier.matz@6wind.com> <1c377463-9d2d-88ac-ef63-f0452fe8bd13@oktetlabs.ru> From: Andrew Rybchenko Organization: OKTET Labs Message-ID: <875c88f5-e76c-867f-2fbb-16d5d4c662bc@oktetlabs.ru> Date: Tue, 8 Jun 2021 15:39:26 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH 3/4] net: introduce functions to verify L4 checksums X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 >>>> --- >>>> 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). Andrew. > Olivier > >> >>>> } >>>> >>>> /** >>>> @@ -448,6 +484,25 @@ rte_ipv6_phdr_cksum(const struct rte_ipv6_hdr *ipv6_hdr, uint64_t ol_flags) >>>> return __rte_raw_cksum_reduce(sum); >>>> } >>>> >>>> +/** >>>> + * @internal Calculate the non-complemented IPv4 L4 checksum >>>> + */ >>>> +static inline uint16_t >>>> +__rte_ipv6_udptcp_cksum(const struct rte_ipv6_hdr *ipv6_hdr, const void *l4_hdr) >>>> +{ >>>> + uint32_t cksum; >>>> + uint32_t l4_len; >>>> + >>>> + l4_len = rte_be_to_cpu_16(ipv6_hdr->payload_len); >>>> + >>>> + cksum = rte_raw_cksum(l4_hdr, l4_len); >>>> + cksum += rte_ipv6_phdr_cksum(ipv6_hdr, 0); >>>> + >>>> + cksum = ((cksum & 0xffff0000) >> 16) + (cksum & 0xffff); >>>> + >>>> + return (uint16_t)cksum; >>>> +} >>>> + >>>> /** >>>> * Process the IPv6 UDP or TCP checksum. >>>> * >>>> @@ -464,16 +519,10 @@ rte_ipv6_phdr_cksum(const struct rte_ipv6_hdr *ipv6_hdr, uint64_t ol_flags) >>>> static inline uint16_t >>>> rte_ipv6_udptcp_cksum(const struct rte_ipv6_hdr *ipv6_hdr, const void *l4_hdr) >>>> { >>>> - uint32_t cksum; >>>> - uint32_t l4_len; >>>> - >>>> - l4_len = rte_be_to_cpu_16(ipv6_hdr->payload_len); >>>> + uint16_t cksum = __rte_ipv6_udptcp_cksum(ipv6_hdr, l4_hdr); >>>> >>>> - cksum = rte_raw_cksum(l4_hdr, l4_len); >>>> - cksum += rte_ipv6_phdr_cksum(ipv6_hdr, 0); >>>> + cksum = ~cksum; >>>> >>>> - cksum = ((cksum & 0xffff0000) >> 16) + (cksum & 0xffff); >>>> - cksum = (~cksum) & 0xffff; >>>> /* >>>> * Per RFC 768: If the computed checksum is zero for UDP, >>>> * it is transmitted as all ones >>>> @@ -482,7 +531,34 @@ rte_ipv6_udptcp_cksum(const struct rte_ipv6_hdr *ipv6_hdr, const void *l4_hdr) >>>> if (cksum == 0 && ipv6_hdr->proto == IPPROTO_UDP) >>>> cksum = 0xffff; >>>> >>>> - return (uint16_t)cksum; >>>> + return cksum; >>>> +} >>>> + >>>> +/** >>>> + * Validate the IPv6 UDP or TCP checksum. >>>> + * >>>> + * The function accepts a 0 checksum, since it can exceptionally happen. See 8.1 >>>> + * (Upper-Layer Checksums) in RFC 8200. >>>> + * >>>> + * @param ipv6_hdr >>>> + * The pointer to the contiguous IPv6 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_ipv6_udptcp_cksum_verify(const struct rte_ipv6_hdr *ipv6_hdr, >>>> + const void *l4_hdr) >>>> +{ >>>> + uint16_t cksum; >>>> + >>>> + cksum = __rte_ipv6_udptcp_cksum(ipv6_hdr, l4_hdr); >>>> + if (cksum != 0xffff) >>>> + return -1; >>>> + >>>> + return 0; >>>> } >>> >>> Nitpicking but, 'rte_ipv4_udptcp_cksum_verify()' is almost same with this >>> function ('rte_ipv6_udptcp_cksum_verify()') but they have different line >>> spacing, can be good to have similar syntax for both. >>> >>