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 B2C1AA034F; Tue, 8 Jun 2021 12:23:35 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 784B7410F0; Tue, 8 Jun 2021 12:23:34 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id 97AD1410EF for ; Tue, 8 Jun 2021 12:23:33 +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 41E0B7F66F; Tue, 8 Jun 2021 13:23:33 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 41E0B7F66F DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1623147813; bh=uLynbDCQ1AckSpGvA1d2gTi+QP2THp4pf7RQD5zXJ+M=; h=Subject:To:Cc:References:From:Date:In-Reply-To; b=U9h+sCppGSkqMjV9wEoyxdTVq87x/BpD5S37eo3QBAZZZeU3WhxQHw4wDJ3y37Yc6 sFfeNJ24Iqh5dcuYYuZASaz5DbJULjQkVbzuz37VRR2tscOZSskOTHIb+6fRs/mAOg oMz3qCtMvBP9wclyaTi/1R8lu9s0Iz2KcRI48BkY= To: Ferruh Yigit , Olivier Matz , dev@dpdk.org Cc: 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> From: Andrew Rybchenko Organization: OKTET Labs Message-ID: <1c377463-9d2d-88ac-ef63-f0452fe8bd13@oktetlabs.ru> Date: Tue, 8 Jun 2021 13:23:33 +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 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. > > 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. >> } >> >> /** >> @@ -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. >