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 1541EA0546; Fri, 30 Apr 2021 17:43:01 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id CBE404014F; Fri, 30 Apr 2021 17:43:00 +0200 (CEST) Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by mails.dpdk.org (Postfix) with ESMTP id 5A4304013F for ; Fri, 30 Apr 2021 17:42:59 +0200 (CEST) IronPort-SDR: mqjKom6DYXLhsqz/VTHuAh+6SjI4T5cFik3zkNcXL0e6X37K8ktU9LVKykI6FQTZMK83KW0fW+ JEJwbXGirI+A== X-IronPort-AV: E=McAfee;i="6200,9189,9970"; a="195200361" X-IronPort-AV: E=Sophos;i="5.82,263,1613462400"; d="scan'208";a="195200361" Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by fmsmga104.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 30 Apr 2021 08:42:49 -0700 IronPort-SDR: +FNZFNFkCGqQ/K8FUWaO1DGRvayL5tOTzwYqqbMZoy5lhywVffidxxl1HY3+FpNiX7Xymm5QLL wLoDD3E5oKWA== X-IronPort-AV: E=Sophos;i="5.82,263,1613462400"; d="scan'208";a="425058983" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.213.242.68]) ([10.213.242.68]) by fmsmga008-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 30 Apr 2021 08:42:47 -0700 To: 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: Ferruh Yigit X-User: ferruhy Message-ID: Date: Fri, 30 Apr 2021 16:42:44 +0100 MIME-Version: 1.0 In-Reply-To: <20210427135755.927-4-olivier.matz@6wind.com> 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/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. > } > > /** > @@ -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.