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 30FC6A0547; Fri, 25 Jun 2021 17:38:32 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id AB14A40698; Fri, 25 Jun 2021 17:38:31 +0200 (CEST) Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by mails.dpdk.org (Postfix) with ESMTP id 8044B4068A for ; Fri, 25 Jun 2021 17:38:29 +0200 (CEST) IronPort-SDR: CbzGsatpLacb4WwCH7JgTjspVYwUL+9S2gwTJq9/vDRW01q9X37aFk1rmjIR7oti/0Y6jgu0Dy IYxZyzlgdqCQ== X-IronPort-AV: E=McAfee;i="6200,9189,10026"; a="207509570" X-IronPort-AV: E=Sophos;i="5.83,299,1616482800"; d="scan'208";a="207509570" Received: from orsmga008.jf.intel.com ([10.7.209.65]) by fmsmga103.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 25 Jun 2021 08:38:28 -0700 IronPort-SDR: xLuGk0ISZZ127ebcsLc4QVpS44X11RKgu/0iLzlNmpbGu8xHqrQJue/fqdws5sjnFrAitUOlzA oBSh6Rwg6EWw== X-IronPort-AV: E=Sophos;i="5.83,299,1616482800"; d="scan'208";a="453851296" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.213.204.105]) ([10.213.204.105]) by orsmga008-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 25 Jun 2021 08:38:26 -0700 To: Andrew Rybchenko , 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> <875c88f5-e76c-867f-2fbb-16d5d4c662bc@oktetlabs.ru> From: Ferruh Yigit Message-ID: Date: Fri, 25 Jun 2021 16:38:22 +0100 MIME-Version: 1.0 In-Reply-To: <875c88f5-e76c-867f-2fbb-16d5d4c662bc@oktetlabs.ru> 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/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 >>>>> --- >>>>> 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.