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 235EEA0C40; Tue, 8 Jun 2021 14:29:40 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 0874040689; Tue, 8 Jun 2021 14:29:40 +0200 (CEST) Received: from mail-wr1-f51.google.com (mail-wr1-f51.google.com [209.85.221.51]) by mails.dpdk.org (Postfix) with ESMTP id 483FA4067A for ; Tue, 8 Jun 2021 14:29:39 +0200 (CEST) Received: by mail-wr1-f51.google.com with SMTP id i94so16336954wri.4 for ; Tue, 08 Jun 2021 05:29:39 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=o1SOFh12AZlB7nedxMysbtpMUF6vUkhdatsxafpljb4=; b=V5wB6nAwkkNBh/X8zHM4k4ZrmPRp4DRpVILyFicOcgmA/AEKGFEyn+abO7HQFpC8/e fIEwmPaPRTq0tynH9o+Al1VvYgsaz+oH9ovy/WbGtPYphMGIB740FU4mVwP2x6Ryle5B WdU2tibALpBmQ4+EyA/2tywE6isFuFrabI/225uzhulER1HKJor/CunlLGjC3avf+79B APXofk03+6x0Po636le6iBTNx5JgPHM+4IRzgDl/a7vewLLw+UjBHR/F8b8yGk9mu0zJ yT5p6l1wh4+DAKCIIZ1OGipiaDTIYcBQYACWSjwZSd9GTZJUP75v68K333qv0A53mB4m xgVA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=o1SOFh12AZlB7nedxMysbtpMUF6vUkhdatsxafpljb4=; b=SLJI5CVuWONGPWj7di09vD5Bish7Vy4uM/S28kBBaDdzPtAQ7FjegEltjT5Xy2WONC sxaD1morVWaE+QgiHVg/XsE7oOLsK8LqlSREuOliOBcM9plaTT1rSr8YOFLGjtcGJeC9 TPxfstoFCsZOkhTxOKUt58G3ZSuw6AqwMN3FylRtvN5p/zfFXCoU3HJ3bLThcr69C+7M /H3lVgcg3AnU16qs5lBCkkwNwOvQGxEd34BYSGY61LZEL++nxsDG/V5RzxI4tt9BA5VA oqsH1Xqqk9iuOv4F3qR5F9/fR0uqYORbZWKR0zP4iI4i7y07OF4LLDidH+gWTQoi3qQD Zn0g== X-Gm-Message-State: AOAM5333/H+gyH7LANaJqzwmGYaRzkckldRad+g1pFE90/JfMJ9702Yc 1drruEE29Ky3kuwVn+yvvBSkWQ== X-Google-Smtp-Source: ABdhPJy0UxF6ALEihUvBnTSKCCIB3AxgNSDR/albIntJk3VAbbOzO+1d5OA3RWD1Wd0+0JmZQiC0Og== X-Received: by 2002:adf:e4d2:: with SMTP id v18mr22733324wrm.114.1623155378975; Tue, 08 Jun 2021 05:29:38 -0700 (PDT) Received: from 6wind.com ([2a01:e0a:5ac:6460:c065:401d:87eb:9b25]) by smtp.gmail.com with ESMTPSA id o22sm2612724wmc.17.2021.06.08.05.29.38 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 08 Jun 2021 05:29:38 -0700 (PDT) Date: Tue, 8 Jun 2021 14:29:37 +0200 From: Olivier Matz To: Andrew Rybchenko Cc: Ferruh Yigit , dev@dpdk.org, Keith Wiles , Hongzhi Guo , Morten =?iso-8859-1?Q?Br=F8rup?= , Thomas Monjalon Message-ID: References: <20210427135755.927-1-olivier.matz@6wind.com> <20210427135755.927-4-olivier.matz@6wind.com> <1c377463-9d2d-88ac-ef63-f0452fe8bd13@oktetlabs.ru> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1c377463-9d2d-88ac-ef63-f0452fe8bd13@oktetlabs.ru> 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" 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; 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. > > >