On 6/8/2021 1:39 PM, Andrew Rybchenko wrote:

On 6/8/21 3:29 PM, Olivier Matz wrote:

quoted

Hi Ferruh, Andrew,

On Tue, Jun 08, 2021 at 01:23:33PM +0300, Andrew Rybchenko wrote:

quoted

On 4/30/21 6:42 PM, Ferruh Yigit wrote:

quoted

On 4/27/2021 2:57 PM, Olivier Matz wrote:

quoted

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 <redacted>
---
 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.

quoted

quoted

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.