[dpdk-dev] [PATCH 0/4] net/tap: fix Rx cksum

[dpdk-dev] [PATCH 0/4] net/tap: fix Rx cksum

[Olivier Matz]

This patchset fixes the Rx checksum flags in net/tap
driver. The first two patches are the effective fixes.

The last 2 patches introduce a new checksum API to
verify a L4 checksum and its unt test, in order to
simplify the net/tap code, or any other code that has
the same needs.

The last 2 patches may be postponed to 20.08 if required.

Olivier Matz (4):
  net/tap: fix Rx cksum flags on IP options packets
  net/tap: fix Rx cksum flags on TCP packets
  net: introduce functions to verify L4 checksums
  test/cksum: new test for L3/L4 checksum API

 MAINTAINERS                   |   1 +
 app/test/autotest_data.py     |   6 +
 app/test/meson.build          |   2 +
 app/test/test_cksum.c         | 271 ++++++++++++++++++++++++++++++++++
 drivers/net/tap/rte_eth_tap.c |  17 ++-
 lib/net/rte_ip.h              | 124 +++++++++++++---
 6 files changed, 390 insertions(+), 31 deletions(-)
 create mode 100644 app/test/test_cksum.c

-- 
2.29.2

[dpdk-dev] [PATCH 1/4] net/tap: fix Rx cksum flags on IP options packets

[Olivier Matz]

When packet type is IPV4_EXT, the checksum is always marked as good in
the mbuf offload flags.

Since we know the header lengths, we can easily call
rte_ipv4_udptcp_cksum() in this case too.

Fixes: 8ae3023387e9 ("net/tap: add Rx/Tx checksum offload support")
Cc: stable@dpdk.org

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 drivers/net/tap/rte_eth_tap.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 68baa18523..e7b185a4b5 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -350,7 +350,7 @@ tap_verify_csum(struct rte_mbuf *mbuf)
 		/* Don't verify checksum for multi-segment packets. */
 		if (mbuf->nb_segs > 1)
 			return;
-		if (l3 == RTE_PTYPE_L3_IPV4) {
+		if (l3 == RTE_PTYPE_L3_IPV4 || l3 == RTE_PTYPE_L3_IPV4_EXT) {
 			if (l4 == RTE_PTYPE_L4_UDP) {
 				udp_hdr = (struct rte_udp_hdr *)l4_hdr;
 				if (udp_hdr->dgram_cksum == 0) {
@@ -364,7 +364,7 @@ tap_verify_csum(struct rte_mbuf *mbuf)
 				}
 			}
 			cksum = ~rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
-		} else if (l3 == RTE_PTYPE_L3_IPV6) {
+		} else { /* l3 == RTE_PTYPE_L3_IPV6, checked above */
 			cksum = ~rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
 		}
 		mbuf->ol_flags |= cksum ?
-- 
2.29.2

[dpdk-dev] [PATCH 2/4] net/tap: fix Rx cksum flags on TCP packets

[Olivier Matz]

Since commit d5df2ae0428a ("net: fix unneeded replacement of TCP
checksum 0"), the functions rte_ipv4_udptcp_cksum() or
rte_ipv6_udptcp_cksum() can return either 0x0000 or 0xffff when used to
verify a packet containing a valid checksum.

This new behavior broke the checksum verification in tap driver for TCP
packets: these packets are marked with PKT_RX_L4_CKSUM_BAD.

Fix this by checking the 2 possible values. A next commit will introduce
a checksum verification helper to simplify this a bit.

Fixes: d5df2ae0428a ("net: fix unneeded replacement of TCP checksum 0")
Cc: stable@dpdk.org

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 drivers/net/tap/rte_eth_tap.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index e7b185a4b5..71282e8065 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -346,6 +346,8 @@ tap_verify_csum(struct rte_mbuf *mbuf)
 		return;
 	}
 	if (l4 == RTE_PTYPE_L4_UDP || l4 == RTE_PTYPE_L4_TCP) {
+		int cksum_ok;
+
 		l4_hdr = rte_pktmbuf_mtod_offset(mbuf, void *, l2_len + l3_len);
 		/* Don't verify checksum for multi-segment packets. */
 		if (mbuf->nb_segs > 1)
@@ -363,13 +365,13 @@ tap_verify_csum(struct rte_mbuf *mbuf)
 					return;
 				}
 			}
-			cksum = ~rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
+			cksum = rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
 		} else { /* l3 == RTE_PTYPE_L3_IPV6, checked above */
-			cksum = ~rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
+			cksum = rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
 		}
-		mbuf->ol_flags |= cksum ?
-			PKT_RX_L4_CKSUM_BAD :
-			PKT_RX_L4_CKSUM_GOOD;
+		cksum_ok = (cksum == 0) || (cksum == 0xffff);
+		mbuf->ol_flags |= cksum_ok ?
+			PKT_RX_L4_CKSUM_GOOD : PKT_RX_L4_CKSUM_BAD;
 	}
 }
 
-- 
2.29.2

[dpdk-dev] [PATCH 3/4] net: introduce functions to verify L4 checksums

[Olivier Matz]

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 <olivier.matz@6wind.com>
---
 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;
 }
 
 /**
@@ -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;
 }
 
 /** IPv6 fragment extension header. */
-- 
2.29.2

[dpdk-dev] [PATCH 4/4] test/cksum: new test for L3/L4 checksum API

[Olivier Matz]

Add a simple unit test for checksum API.

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 MAINTAINERS               |   1 +
 app/test/autotest_data.py |   6 +
 app/test/meson.build      |   2 +
 app/test/test_cksum.c     | 271 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 280 insertions(+)
 create mode 100644 app/test/test_cksum.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 44f3d322ed..9fe7c92eac 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1309,6 +1309,7 @@ Packet processing
 Network headers
 M: Olivier Matz <olivier.matz@6wind.com>
 F: lib/net/
+F: app/test/test_cksum.c
 
 Packet CRC
 M: Jasvinder Singh <jasvinder.singh@intel.com>
diff --git a/app/test/autotest_data.py b/app/test/autotest_data.py
index 097638941f..2871ed8994 100644
--- a/app/test/autotest_data.py
+++ b/app/test/autotest_data.py
@@ -585,6 +585,12 @@
         "Func":    default_autotest,
         "Report":  None,
     },
+    {
+        "Name":    "Checksum autotest",
+        "Command": "cksum_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
     #
     #Please always keep all dump tests at the end and together!
     #
diff --git a/app/test/meson.build b/app/test/meson.build
index 08c82d3d23..28d8a9a111 100644
--- a/app/test/meson.build
+++ b/app/test/meson.build
@@ -17,6 +17,7 @@ test_sources = files(
         'test_bitmap.c',
         'test_bpf.c',
         'test_byteorder.c',
+        'test_cksum.c',
         'test_cmdline.c',
         'test_cmdline_cirbuf.c',
         'test_cmdline_etheraddr.c',
@@ -189,6 +190,7 @@ fast_tests = [
         ['atomic_autotest', false],
         ['bitops_autotest', true],
         ['byteorder_autotest', true],
+        ['cksum_autotest', true],
         ['cmdline_autotest', true],
         ['common_autotest', true],
         ['cpuflags_autotest', true],
diff --git a/app/test/test_cksum.c b/app/test/test_cksum.c
new file mode 100644
index 0000000000..cd983d7c01
--- /dev/null
+++ b/app/test/test_cksum.c
@@ -0,0 +1,271 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright 2021 6WIND S.A.
+ */
+
+#include <string.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <stdint.h>
+
+#include <rte_net.h>
+#include <rte_mbuf.h>
+#include <rte_ip.h>
+
+#include "test.h"
+
+#define MEMPOOL_CACHE_SIZE      0
+#define MBUF_DATA_SIZE          256
+#define NB_MBUF                 128
+
+/*
+ * Test L3/L4 checksum API.
+ */
+
+#define GOTO_FAIL(str, ...) do {					\
+		printf("cksum test FAILED (l.%d): <" str ">\n",		\
+		       __LINE__,  ##__VA_ARGS__);			\
+		goto fail;						\
+	} while (0)
+
+/* generated in scapy with Ether()/IP()/TCP())) */
+static const char test_cksum_ipv4_tcp[] = {
+	0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x08, 0x00, 0x45, 0x00,
+	0x00, 0x28, 0x00, 0x01, 0x00, 0x00, 0x40, 0x06,
+	0x7c, 0xcd, 0x7f, 0x00, 0x00, 0x01, 0x7f, 0x00,
+	0x00, 0x01, 0x00, 0x14, 0x00, 0x50, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x50, 0x02,
+	0x20, 0x00, 0x91, 0x7c, 0x00, 0x00,
+
+};
+
+/* generated in scapy with Ether()/IPv6()/TCP()) */
+static const char test_cksum_ipv6_tcp[] = {
+	0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x08, 0x00, 0x60, 0x00,
+	0x00, 0x00, 0x00, 0x14, 0x06, 0x40, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x14,
+	0x00, 0x50, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x50, 0x02, 0x20, 0x00, 0x8f, 0x7d,
+	0x00, 0x00,
+};
+
+/* generated in scapy with Ether()/IP()/UDP()/Raw('x')) */
+static const char test_cksum_ipv4_udp[] = {
+	0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x08, 0x00, 0x45, 0x00,
+	0x00, 0x1d, 0x00, 0x01, 0x00, 0x00, 0x40, 0x11,
+	0x7c, 0xcd, 0x7f, 0x00, 0x00, 0x01, 0x7f, 0x00,
+	0x00, 0x01, 0x00, 0x35, 0x00, 0x35, 0x00, 0x09,
+	0x89, 0x6f, 0x78,
+};
+
+/* generated in scapy with Ether()/IPv6()/UDP()/Raw('x')) */
+static const char test_cksum_ipv6_udp[] = {
+	0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x86, 0xdd, 0x60, 0x00,
+	0x00, 0x00, 0x00, 0x09, 0x11, 0x40, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x35,
+	0x00, 0x35, 0x00, 0x09, 0x87, 0x70, 0x78,
+};
+
+/* generated in scapy with Ether()/IP(options='\x00')/UDP()/Raw('x')) */
+static const char test_cksum_ipv4_opts_udp[] = {
+	0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x08, 0x00, 0x46, 0x00,
+	0x00, 0x21, 0x00, 0x01, 0x00, 0x00, 0x40, 0x11,
+	0x7b, 0xc9, 0x7f, 0x00, 0x00, 0x01, 0x7f, 0x00,
+	0x00, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x35,
+	0x00, 0x35, 0x00, 0x09, 0x89, 0x6f, 0x78,
+};
+
+/* test l3/l4 checksum api */
+static int
+test_l4_cksum(struct rte_mempool *pktmbuf_pool, const char *pktdata, size_t len)
+{
+	struct rte_net_hdr_lens hdr_lens;
+	struct rte_mbuf *m = NULL;
+	uint32_t packet_type;
+	uint16_t prev_cksum;
+	void *l3_hdr;
+	void *l4_hdr;
+	uint32_t l3;
+	uint32_t l4;
+	char *data;
+
+	m = rte_pktmbuf_alloc(pktmbuf_pool);
+	if (m == NULL)
+		GOTO_FAIL("Cannot allocate mbuf");
+
+	data = rte_pktmbuf_append(m, len);
+	if (data == NULL)
+		GOTO_FAIL("Cannot append data");
+
+	memcpy(data, pktdata, len);
+
+	packet_type = rte_net_get_ptype(m, &hdr_lens, RTE_PTYPE_ALL_MASK);
+	l3 = packet_type & RTE_PTYPE_L3_MASK;
+	l4 = packet_type & RTE_PTYPE_L4_MASK;
+
+	l3_hdr = rte_pktmbuf_mtod_offset(m, void *, hdr_lens.l2_len);
+	l4_hdr = rte_pktmbuf_mtod_offset(m, void *,
+					 hdr_lens.l2_len + hdr_lens.l3_len);
+
+	if (l3 == RTE_PTYPE_L3_IPV4 || l3 == RTE_PTYPE_L3_IPV4_EXT) {
+		struct rte_ipv4_hdr *ip = l3_hdr;
+
+		/* verify IPv4 checksum */
+		if (rte_ipv4_cksum(l3_hdr) != 0)
+			GOTO_FAIL("invalid IPv4 checksum verification");
+
+		/* verify bad IPv4 checksum */
+		ip->hdr_checksum++;
+		if (rte_ipv4_cksum(l3_hdr) == 0)
+			GOTO_FAIL("invalid IPv4 bad checksum verification");
+		ip->hdr_checksum--;
+
+		/* recalculate IPv4 checksum */
+		prev_cksum = ip->hdr_checksum;
+		ip->hdr_checksum = 0;
+		ip->hdr_checksum = rte_ipv4_cksum(ip);
+		if (ip->hdr_checksum != prev_cksum)
+			GOTO_FAIL("invalid IPv4 checksum calculation");
+
+		/* verify L4 checksum */
+		if (rte_ipv4_udptcp_cksum_verify(l3_hdr, l4_hdr) != 0)
+			GOTO_FAIL("invalid L4 checksum verification");
+
+		if (l4 == RTE_PTYPE_L4_TCP) {
+			struct rte_tcp_hdr *tcp = l4_hdr;
+
+			/* verify bad TCP checksum */
+			tcp->cksum++;
+			if (rte_ipv4_udptcp_cksum_verify(l3_hdr, l4_hdr) == 0)
+				GOTO_FAIL("invalid bad TCP checksum verification");
+			tcp->cksum--;
+
+			/* recalculate TCP checksum */
+			prev_cksum = tcp->cksum;
+			tcp->cksum = 0;
+			tcp->cksum = rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
+			if (tcp->cksum != prev_cksum)
+				GOTO_FAIL("invalid TCP checksum calculation");
+
+		} else if (l4 == RTE_PTYPE_L4_UDP) {
+			struct rte_udp_hdr *udp = l4_hdr;
+
+			/* verify bad UDP checksum */
+			udp->dgram_cksum++;
+			if (rte_ipv4_udptcp_cksum_verify(l3_hdr, l4_hdr) == 0)
+				GOTO_FAIL("invalid bad UDP checksum verification");
+			udp->dgram_cksum--;
+
+			/* recalculate UDP checksum */
+			prev_cksum = udp->dgram_cksum;
+			udp->dgram_cksum = 0;
+			udp->dgram_cksum = rte_ipv4_udptcp_cksum(l3_hdr,
+								 l4_hdr);
+			if (udp->dgram_cksum != prev_cksum)
+				GOTO_FAIL("invalid TCP checksum calculation");
+		}
+	} else if (l3 == RTE_PTYPE_L3_IPV6 || l3 == RTE_PTYPE_L3_IPV6_EXT) {
+		if (rte_ipv6_udptcp_cksum_verify(l3_hdr, l4_hdr) != 0)
+			GOTO_FAIL("invalid L4 checksum verification");
+
+		if (l4 == RTE_PTYPE_L4_TCP) {
+			struct rte_tcp_hdr *tcp = l4_hdr;
+
+			/* verify bad TCP checksum */
+			tcp->cksum++;
+			if (rte_ipv6_udptcp_cksum_verify(l3_hdr, l4_hdr) == 0)
+				GOTO_FAIL("invalid bad TCP checksum verification");
+			tcp->cksum--;
+
+			/* recalculate TCP checksum */
+			prev_cksum = tcp->cksum;
+			tcp->cksum = 0;
+			tcp->cksum = rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
+			if (tcp->cksum != prev_cksum)
+				GOTO_FAIL("invalid TCP checksum calculation");
+
+		} else if (l4 == RTE_PTYPE_L4_UDP) {
+			struct rte_udp_hdr *udp = l4_hdr;
+
+			/* verify bad UDP checksum */
+			udp->dgram_cksum++;
+			if (rte_ipv6_udptcp_cksum_verify(l3_hdr, l4_hdr) == 0)
+				GOTO_FAIL("invalid bad UDP checksum verification");
+			udp->dgram_cksum--;
+
+			/* recalculate UDP checksum */
+			prev_cksum = udp->dgram_cksum;
+			udp->dgram_cksum = 0;
+			udp->dgram_cksum = rte_ipv6_udptcp_cksum(l3_hdr,
+								 l4_hdr);
+			if (udp->dgram_cksum != prev_cksum)
+				GOTO_FAIL("invalid TCP checksum calculation");
+		}
+	}
+
+	rte_pktmbuf_free(m);
+
+	return 0;
+
+fail:
+	if (m)
+		rte_pktmbuf_free(m);
+
+	return -1;
+}
+
+static int
+test_cksum(void)
+{
+	struct rte_mempool *pktmbuf_pool = NULL;
+
+	/* create pktmbuf pool if it does not exist */
+	pktmbuf_pool = rte_pktmbuf_pool_create("test_cksum_mbuf_pool",
+			NB_MBUF, MEMPOOL_CACHE_SIZE, 0, MBUF_DATA_SIZE,
+			SOCKET_ID_ANY);
+
+	if (pktmbuf_pool == NULL)
+		GOTO_FAIL("cannot allocate mbuf pool");
+
+	if (test_l4_cksum(pktmbuf_pool, test_cksum_ipv4_tcp,
+			  sizeof(test_cksum_ipv4_tcp)) < 0)
+		GOTO_FAIL("checksum error on ipv4_tcp");
+
+	if (test_l4_cksum(pktmbuf_pool, test_cksum_ipv6_tcp,
+			  sizeof(test_cksum_ipv6_tcp)) < 0)
+		GOTO_FAIL("checksum error on ipv6_tcp");
+
+	if (test_l4_cksum(pktmbuf_pool, test_cksum_ipv4_udp,
+			  sizeof(test_cksum_ipv4_udp)) < 0)
+		GOTO_FAIL("checksum error on ipv4_udp");
+
+	if (test_l4_cksum(pktmbuf_pool, test_cksum_ipv6_udp,
+			  sizeof(test_cksum_ipv6_udp)) < 0)
+		GOTO_FAIL("checksum error on ipv6_udp");
+
+	if (test_l4_cksum(pktmbuf_pool, test_cksum_ipv4_opts_udp,
+			  sizeof(test_cksum_ipv4_opts_udp)) < 0)
+		GOTO_FAIL("checksum error on ipv4_opts_udp");
+
+	rte_mempool_free(pktmbuf_pool);
+
+	return 0;
+
+fail:
+	rte_mempool_free(pktmbuf_pool);
+
+	return -1;
+}
+#undef GOTO_FAIL
+
+REGISTER_TEST_COMMAND(cksum_autotest, test_cksum);
-- 
2.29.2

Re: [dpdk-dev] [PATCH 3/4] net: introduce functions to verify L4 checksums

[Morten Brørup]

> From: Olivier Matz [mailto:olivier.matz@6wind.com]
> Sent: Tuesday, April 27, 2021 3:58 PM
> 
> 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 <olivier.matz@6wind.com>
> ---
>  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;
> +}

The GCC static branch predictor treats the above comparison as likely. Playing around with Godbolt, I came up with this alternative:

	if (likely(cksum != 0)) return cksum;
	if (ipv4_hdr->next_proto_id == IPPROTO_UDP) return 0xffff;
	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;

The GCC static branch predictor treats the above comparison as likely, so I would prefer unlikely() around it.

> +
> +	return 0;
>  }
> 
>  /**
> @@ -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;

Same comment about GCC static branch prediction as above.

> 
> -	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;

Same comment about GCC static branch prediction as above.

> +
> +	return 0;
>  }
> 
>  /** IPv6 fragment extension header. */
> --
> 2.29.2
> 

With or without my suggested modifications:

Acked-by: Morten Brørup <mb@smartsharesystems.com>

Without my suggested modifications:

Reviewed-by: Morten Brørup <mb@smartsharesystems.com>

Re: [dpdk-dev] [PATCH 3/4] net: introduce functions to verify L4 checksums

[Morten Brørup]

> From: Olivier Matz [mailto:olivier.matz@6wind.com]
> Sent: Tuesday, April 27, 2021 3:58 PM
> 
> 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 <olivier.matz@6wind.com>
> ---
>  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;
> +}

The GCC static branch predictor treats the above comparison as likely. Playing around with Godbolt, I came up with this alternative:

	if (likely(cksum != 0)) return cksum;
	if (ipv4_hdr->next_proto_id == IPPROTO_UDP) return 0xffff;
	return 0;

> +
> +/**
> + * 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;

The GCC static branch predictor treats the above comparison as likely, so I would prefer unlikely() around it.

> +
> +	return 0;
>  }
> 
>  /**
> @@ -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;

Same comment about GCC static branch prediction as above.

> 
> -	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;

Same comment about GCC static branch prediction as above.

> +
> +	return 0;
>  }
> 
>  /** IPv6 fragment extension header. */
> --
> 2.29.2
> 

With or without my suggested modifications:

Acked-by: Morten Brørup <mb@smartsharesystems.com>

Without my suggested modifications:

Reviewed-by: Morten Brørup <mb@smartsharesystems.com>

Re: [dpdk-dev] [PATCH 3/4] net: introduce functions to verify L4 checksums

[Olivier Matz]

Hi Morten,

Thank you for the review.

<...>

On Tue, Apr 27, 2021 at 05:07:04PM +0200, Morten Brørup wrote:
> > +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;
> > +}
> 
> The GCC static branch predictor treats the above comparison as likely. Playing around with Godbolt, I came up with this alternative:
> 
> 	if (likely(cksum != 0)) return cksum;
> 	if (ipv4_hdr->next_proto_id == IPPROTO_UDP) return 0xffff;
> 	return 0;

Good idea, this is indeed an unlikely branch.
However this code was already present before this patch,
so I suggest to add it as a specific optimization patch.

> > +
> > +/**
> > + * 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;
> 
> The GCC static branch predictor treats the above comparison as likely, so I would prefer unlikely() around it.

For this one, I'm less convinced: should we decide here whether
the good or the bad checksum is more likely than the other?

Given it's a static inline function, wouldn't it be better to let
the application call it this way:
  if (likely(rte_ipv4_udptcp_cksum_verify(...) == 0))  ?


Regards,
Olivier

Re: [dpdk-dev] [PATCH 3/4] net: introduce functions to verify L4 checksums

[Morten Brørup]

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier Matz
> Sent: Wednesday, April 28, 2021 2:22 PM
> 
> Hi Morten,
> 
> Thank you for the review.
> 
> <...>
> 
> On Tue, Apr 27, 2021 at 05:07:04PM +0200, Morten Brørup wrote:
> > > +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;
> > > +}
> >
> > The GCC static branch predictor treats the above comparison as
> likely. Playing around with Godbolt, I came up with this alternative:
> >
> > 	if (likely(cksum != 0)) return cksum;
> > 	if (ipv4_hdr->next_proto_id == IPPROTO_UDP) return 0xffff;
> > 	return 0;
> 
> Good idea, this is indeed an unlikely branch.
> However this code was already present before this patch,
> so I suggest to add it as a specific optimization patch.

Please do.

> 
> > > +
> > > +/**
> > > + * 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;
> >
> > The GCC static branch predictor treats the above comparison as
> likely, so I would prefer unlikely() around it.
> 
> For this one, I'm less convinced: should we decide here whether
> the good or the bad checksum is more likely than the other?

You are right... this may be a question of personal preference - or application specific preference.

> 
> Given it's a static inline function, wouldn't it be better to let
> the application call it this way:
>   if (likely(rte_ipv4_udptcp_cksum_verify(...) == 0))  ?
> 

Good point. Double checking on Godbolt confirms the validity of your suggestion.

> 
> Regards,
> Olivier

Re: [dpdk-dev] [dpdk-stable] [PATCH 1/4] net/tap: fix Rx cksum flags on IP options packets

[Ferruh Yigit]

On 4/27/2021 2:57 PM, Olivier Matz wrote:
> When packet type is IPV4_EXT, the checksum is always marked as good in
> the mbuf offload flags.
> 
> Since we know the header lengths, we can easily call
> rte_ipv4_udptcp_cksum() in this case too.
> 
> Fixes: 8ae3023387e9 ("net/tap: add Rx/Tx checksum offload support")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> ---
>  drivers/net/tap/rte_eth_tap.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index 68baa18523..e7b185a4b5 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -350,7 +350,7 @@ tap_verify_csum(struct rte_mbuf *mbuf)
>  		/* Don't verify checksum for multi-segment packets. */
>  		if (mbuf->nb_segs > 1)
>  			return;
> -		if (l3 == RTE_PTYPE_L3_IPV4) {
> +		if (l3 == RTE_PTYPE_L3_IPV4 || l3 == RTE_PTYPE_L3_IPV4_EXT) {

Should we take 'RTE_PTYPE_L3_IPV4_EXT_UNKNOWN' into account?

>  			if (l4 == RTE_PTYPE_L4_UDP) {
>  				udp_hdr = (struct rte_udp_hdr *)l4_hdr;
>  				if (udp_hdr->dgram_cksum == 0) {
> @@ -364,7 +364,7 @@ tap_verify_csum(struct rte_mbuf *mbuf)
>  				}
>  			}
>  			cksum = ~rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
> -		} else if (l3 == RTE_PTYPE_L3_IPV6) {
> +		} else { /* l3 == RTE_PTYPE_L3_IPV6, checked above */
>  			cksum = ~rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
>  		}
>  		mbuf->ol_flags |= cksum ?
> 

Re: [dpdk-dev] [PATCH 3/4] net: introduce functions to verify L4 checksums

[Ferruh Yigit]

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 <olivier.matz@6wind.com>
> ---
>  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.

Re: [dpdk-dev] [dpdk-stable] [PATCH 1/4] net/tap: fix Rx cksum flags on IP options packets

[Andrew Rybchenko]

On 4/30/21 5:48 PM, Ferruh Yigit wrote:
> On 4/27/2021 2:57 PM, Olivier Matz wrote:
>> When packet type is IPV4_EXT, the checksum is always marked as good in
>> the mbuf offload flags.
>>
>> Since we know the header lengths, we can easily call
>> rte_ipv4_udptcp_cksum() in this case too.
>>
>> Fixes: 8ae3023387e9 ("net/tap: add Rx/Tx checksum offload support")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
>> ---
>>  drivers/net/tap/rte_eth_tap.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
>> index 68baa18523..e7b185a4b5 100644
>> --- a/drivers/net/tap/rte_eth_tap.c
>> +++ b/drivers/net/tap/rte_eth_tap.c
>> @@ -350,7 +350,7 @@ tap_verify_csum(struct rte_mbuf *mbuf)
>>  		/* Don't verify checksum for multi-segment packets. */
>>  		if (mbuf->nb_segs > 1)
>>  			return;
>> -		if (l3 == RTE_PTYPE_L3_IPV4) {
>> +		if (l3 == RTE_PTYPE_L3_IPV4 || l3 == RTE_PTYPE_L3_IPV4_EXT) {
> 
> Should we take 'RTE_PTYPE_L3_IPV4_EXT_UNKNOWN' into account?

I think we should.

> 
>>  			if (l4 == RTE_PTYPE_L4_UDP) {
>>  				udp_hdr = (struct rte_udp_hdr *)l4_hdr;
>>  				if (udp_hdr->dgram_cksum == 0) {
>> @@ -364,7 +364,7 @@ tap_verify_csum(struct rte_mbuf *mbuf)
>>  				}
>>  			}
>>  			cksum = ~rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
>> -		} else if (l3 == RTE_PTYPE_L3_IPV6) {
>> +		} else { /* l3 == RTE_PTYPE_L3_IPV6, checked above */
>>  			cksum = ~rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
>>  		}
>>  		mbuf->ol_flags |= cksum ?
>>

Re: [dpdk-dev] [PATCH 2/4] net/tap: fix Rx cksum flags on TCP packets

[Andrew Rybchenko]

On 4/27/21 4:57 PM, Olivier Matz wrote:
> Since commit d5df2ae0428a ("net: fix unneeded replacement of TCP
> checksum 0"), the functions rte_ipv4_udptcp_cksum() or
> rte_ipv6_udptcp_cksum() can return either 0x0000 or 0xffff when used to
> verify a packet containing a valid checksum.
> 
> This new behavior broke the checksum verification in tap driver for TCP
> packets: these packets are marked with PKT_RX_L4_CKSUM_BAD.
> 
> Fix this by checking the 2 possible values. A next commit will introduce
> a checksum verification helper to simplify this a bit.
> 
> Fixes: d5df2ae0428a ("net: fix unneeded replacement of TCP checksum 0")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>

Acked-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>

Re: [dpdk-dev] [PATCH 3/4] net: introduce functions to verify L4 checksums

[Andrew Rybchenko]

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 <olivier.matz@6wind.com>
>> ---
>>  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.
> 

Re: [dpdk-dev] [dpdk-stable] [PATCH 1/4] net/tap: fix Rx cksum flags on IP options packets

[Olivier Matz]

Hi Ferruh, Andrew,

On Tue, Jun 08, 2021 at 01:13:59PM +0300, Andrew Rybchenko wrote:
> On 4/30/21 5:48 PM, Ferruh Yigit wrote:
> > On 4/27/2021 2:57 PM, Olivier Matz wrote:
> >> When packet type is IPV4_EXT, the checksum is always marked as good in
> >> the mbuf offload flags.
> >>
> >> Since we know the header lengths, we can easily call
> >> rte_ipv4_udptcp_cksum() in this case too.
> >>
> >> Fixes: 8ae3023387e9 ("net/tap: add Rx/Tx checksum offload support")
> >> Cc: stable@dpdk.org
> >>
> >> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> >> ---
> >>  drivers/net/tap/rte_eth_tap.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> >> index 68baa18523..e7b185a4b5 100644
> >> --- a/drivers/net/tap/rte_eth_tap.c
> >> +++ b/drivers/net/tap/rte_eth_tap.c
> >> @@ -350,7 +350,7 @@ tap_verify_csum(struct rte_mbuf *mbuf)
> >>  		/* Don't verify checksum for multi-segment packets. */
> >>  		if (mbuf->nb_segs > 1)
> >>  			return;
> >> -		if (l3 == RTE_PTYPE_L3_IPV4) {
> >> +		if (l3 == RTE_PTYPE_L3_IPV4 || l3 == RTE_PTYPE_L3_IPV4_EXT) {
> > 
> > Should we take 'RTE_PTYPE_L3_IPV4_EXT_UNKNOWN' into account?
> 
> I think we should.

I think 'RTE_PTYPE_L3_IPV4_EXT_UNKNOWN' cannot happen here:

- mbuf->packet_type is generated by rte_net_get_ptype(), which cannot
  return 'RTE_PTYPE_L3_IPV4_EXT_UNKNOWN'
- right above this code, we already returned if l3 is not in
  (RTE_PTYPE_L3_IPV4, RTE_PTYPE_L3_IPV4_EXT, RTE_PTYPE_L3_IPV6)

> > 
> >>  			if (l4 == RTE_PTYPE_L4_UDP) {
> >>  				udp_hdr = (struct rte_udp_hdr *)l4_hdr;
> >>  				if (udp_hdr->dgram_cksum == 0) {
> >> @@ -364,7 +364,7 @@ tap_verify_csum(struct rte_mbuf *mbuf)
> >>  				}
> >>  			}
> >>  			cksum = ~rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
> >> -		} else if (l3 == RTE_PTYPE_L3_IPV6) {
> >> +		} else { /* l3 == RTE_PTYPE_L3_IPV6, checked above */
> >>  			cksum = ~rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
> >>  		}
> >>  		mbuf->ol_flags |= cksum ?
> >>
> 

Re: [dpdk-dev] [PATCH 3/4] net: introduce functions to verify L4 checksums

[Olivier Matz]

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 <olivier.matz@6wind.com>
> >> ---
> >>  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.
> > 
> 

Re: [dpdk-dev] [dpdk-stable] [PATCH 1/4] net/tap: fix Rx cksum flags on IP options packets

[Andrew Rybchenko]

On 6/8/21 3:29 PM, Olivier Matz wrote:
> Hi Ferruh, Andrew,
> 
> On Tue, Jun 08, 2021 at 01:13:59PM +0300, Andrew Rybchenko wrote:
>> On 4/30/21 5:48 PM, Ferruh Yigit wrote:
>>> On 4/27/2021 2:57 PM, Olivier Matz wrote:
>>>> When packet type is IPV4_EXT, the checksum is always marked as good in
>>>> the mbuf offload flags.
>>>>
>>>> Since we know the header lengths, we can easily call
>>>> rte_ipv4_udptcp_cksum() in this case too.
>>>>
>>>> Fixes: 8ae3023387e9 ("net/tap: add Rx/Tx checksum offload support")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
>>>> ---
>>>>  drivers/net/tap/rte_eth_tap.c | 4 ++--
>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
>>>> index 68baa18523..e7b185a4b5 100644
>>>> --- a/drivers/net/tap/rte_eth_tap.c
>>>> +++ b/drivers/net/tap/rte_eth_tap.c
>>>> @@ -350,7 +350,7 @@ tap_verify_csum(struct rte_mbuf *mbuf)
>>>>  		/* Don't verify checksum for multi-segment packets. */
>>>>  		if (mbuf->nb_segs > 1)
>>>>  			return;
>>>> -		if (l3 == RTE_PTYPE_L3_IPV4) {
>>>> +		if (l3 == RTE_PTYPE_L3_IPV4 || l3 == RTE_PTYPE_L3_IPV4_EXT) {
>>>
>>> Should we take 'RTE_PTYPE_L3_IPV4_EXT_UNKNOWN' into account?
>>
>> I think we should.
> 
> I think 'RTE_PTYPE_L3_IPV4_EXT_UNKNOWN' cannot happen here:
> 
> - mbuf->packet_type is generated by 

(), which cannot
>   return 'RTE_PTYPE_L3_IPV4_EXT_UNKNOWN'

My question if it is guaranteed and the only possible branch.
Can application set packet_type itself and do not call
rte_net_get_ptype(). Yes, typically application knows
if it has IPv4 options in the header or not, but theoretically
could be unaware as well.

> - right above this code, we already returned if l3 is not in
>   (RTE_PTYPE_L3_IPV4, RTE_PTYPE_L3_IPV4_EXT, RTE_PTYPE_L3_IPV6)

If so, it sounds like it should be allowed above as well.

> 
>>>
>>>>  			if (l4 == RTE_PTYPE_L4_UDP) {
>>>>  				udp_hdr = (struct rte_udp_hdr *)l4_hdr;
>>>>  				if (udp_hdr->dgram_cksum == 0) {
>>>> @@ -364,7 +364,7 @@ tap_verify_csum(struct rte_mbuf *mbuf)
>>>>  				}
>>>>  			}
>>>>  			cksum = ~rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
>>>> -		} else if (l3 == RTE_PTYPE_L3_IPV6) {
>>>> +		} else { /* l3 == RTE_PTYPE_L3_IPV6, checked above */
>>>>  			cksum = ~rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
>>>>  		}
>>>>  		mbuf->ol_flags |= cksum ?
>>>>
>>

Re: [dpdk-dev] [PATCH 3/4] net: introduce functions to verify L4 checksums

[Andrew Rybchenko]

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 <olivier.matz@6wind.com>
>>>> ---
>>>>  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).

Andrew.

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

Re: [dpdk-dev] [dpdk-stable] [PATCH 1/4] net/tap: fix Rx cksum flags on IP options packets

[Olivier Matz]

On Tue, Jun 08, 2021 at 03:34:36PM +0300, Andrew Rybchenko wrote:
> On 6/8/21 3:29 PM, Olivier Matz wrote:
> > Hi Ferruh, Andrew,
> > 
> > On Tue, Jun 08, 2021 at 01:13:59PM +0300, Andrew Rybchenko wrote:
> >> On 4/30/21 5:48 PM, Ferruh Yigit wrote:
> >>> On 4/27/2021 2:57 PM, Olivier Matz wrote:
> >>>> When packet type is IPV4_EXT, the checksum is always marked as good in
> >>>> the mbuf offload flags.
> >>>>
> >>>> Since we know the header lengths, we can easily call
> >>>> rte_ipv4_udptcp_cksum() in this case too.
> >>>>
> >>>> Fixes: 8ae3023387e9 ("net/tap: add Rx/Tx checksum offload support")
> >>>> Cc: stable@dpdk.org
> >>>>
> >>>> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> >>>> ---
> >>>>  drivers/net/tap/rte_eth_tap.c | 4 ++--
> >>>>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> >>>> index 68baa18523..e7b185a4b5 100644
> >>>> --- a/drivers/net/tap/rte_eth_tap.c
> >>>> +++ b/drivers/net/tap/rte_eth_tap.c
> >>>> @@ -350,7 +350,7 @@ tap_verify_csum(struct rte_mbuf *mbuf)
> >>>>  		/* Don't verify checksum for multi-segment packets. */
> >>>>  		if (mbuf->nb_segs > 1)
> >>>>  			return;
> >>>> -		if (l3 == RTE_PTYPE_L3_IPV4) {
> >>>> +		if (l3 == RTE_PTYPE_L3_IPV4 || l3 == RTE_PTYPE_L3_IPV4_EXT) {
> >>>
> >>> Should we take 'RTE_PTYPE_L3_IPV4_EXT_UNKNOWN' into account?
> >>
> >> I think we should.
> > 
> > I think 'RTE_PTYPE_L3_IPV4_EXT_UNKNOWN' cannot happen here:
> > 
> > - mbuf->packet_type is generated by 
> 
> (), which cannot
> >   return 'RTE_PTYPE_L3_IPV4_EXT_UNKNOWN'
> 
> My question if it is guaranteed and the only possible branch.
> Can application set packet_type itself and do not call
> rte_net_get_ptype(). Yes, typically application knows
> if it has IPv4 options in the header or not, but theoretically
> could be unaware as well.

This function is called on the Rx path from pmd_rx_burst(), so
the application does not have access to the mbuf.

The software parser that sets the packet type returns either
RTE_PTYPE_L3_IPV4 if there is no option, or RTE_PTYPE_L3_IPV4_EXT
else. The value RTE_PTYPE_L3_IPV4_EXT_UNKNOWN is used by PMDs that don't
know if there are options.

> > - right above this code, we already returned if l3 is not in
> >   (RTE_PTYPE_L3_IPV4, RTE_PTYPE_L3_IPV4_EXT, RTE_PTYPE_L3_IPV6)
> 
> If so, it sounds like it should be allowed above as well.
> 
> > 
> >>>
> >>>>  			if (l4 == RTE_PTYPE_L4_UDP) {
> >>>>  				udp_hdr = (struct rte_udp_hdr *)l4_hdr;
> >>>>  				if (udp_hdr->dgram_cksum == 0) {
> >>>> @@ -364,7 +364,7 @@ tap_verify_csum(struct rte_mbuf *mbuf)
> >>>>  				}
> >>>>  			}
> >>>>  			cksum = ~rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
> >>>> -		} else if (l3 == RTE_PTYPE_L3_IPV6) {
> >>>> +		} else { /* l3 == RTE_PTYPE_L3_IPV6, checked above */
> >>>>  			cksum = ~rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
> >>>>  		}
> >>>>  		mbuf->ol_flags |= cksum ?
> >>>>
> >>
> 

Re: [dpdk-dev] [dpdk-stable] [PATCH 1/4] net/tap: fix Rx cksum flags on IP options packets

[Andrew Rybchenko]

On 6/8/21 3:49 PM, Olivier Matz wrote:
> On Tue, Jun 08, 2021 at 03:34:36PM +0300, Andrew Rybchenko wrote:
>> On 6/8/21 3:29 PM, Olivier Matz wrote:
>>> Hi Ferruh, Andrew,
>>>
>>> On Tue, Jun 08, 2021 at 01:13:59PM +0300, Andrew Rybchenko wrote:
>>>> On 4/30/21 5:48 PM, Ferruh Yigit wrote:
>>>>> On 4/27/2021 2:57 PM, Olivier Matz wrote:
>>>>>> When packet type is IPV4_EXT, the checksum is always marked as good in
>>>>>> the mbuf offload flags.
>>>>>>
>>>>>> Since we know the header lengths, we can easily call
>>>>>> rte_ipv4_udptcp_cksum() in this case too.
>>>>>>
>>>>>> Fixes: 8ae3023387e9 ("net/tap: add Rx/Tx checksum offload support")
>>>>>> Cc: stable@dpdk.org
>>>>>>
>>>>>> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
>>>>>> ---
>>>>>>  drivers/net/tap/rte_eth_tap.c | 4 ++--
>>>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
>>>>>> index 68baa18523..e7b185a4b5 100644
>>>>>> --- a/drivers/net/tap/rte_eth_tap.c
>>>>>> +++ b/drivers/net/tap/rte_eth_tap.c
>>>>>> @@ -350,7 +350,7 @@ tap_verify_csum(struct rte_mbuf *mbuf)
>>>>>>  		/* Don't verify checksum for multi-segment packets. */
>>>>>>  		if (mbuf->nb_segs > 1)
>>>>>>  			return;
>>>>>> -		if (l3 == RTE_PTYPE_L3_IPV4) {
>>>>>> +		if (l3 == RTE_PTYPE_L3_IPV4 || l3 == RTE_PTYPE_L3_IPV4_EXT) {
>>>>>
>>>>> Should we take 'RTE_PTYPE_L3_IPV4_EXT_UNKNOWN' into account?
>>>>
>>>> I think we should.
>>>
>>> I think 'RTE_PTYPE_L3_IPV4_EXT_UNKNOWN' cannot happen here:
>>>
>>> - mbuf->packet_type is generated by 
>>
>> (), which cannot
>>>   return 'RTE_PTYPE_L3_IPV4_EXT_UNKNOWN'
>>
>> My question if it is guaranteed and the only possible branch.
>> Can application set packet_type itself and do not call
>> rte_net_get_ptype(). Yes, typically application knows
>> if it has IPv4 options in the header or not, but theoretically
>> could be unaware as well.
> 
> This function is called on the Rx path from pmd_rx_burst(), so
> the application does not have access to the mbuf.
> 
> The software parser that sets the packet type returns either
> RTE_PTYPE_L3_IPV4 if there is no option, or RTE_PTYPE_L3_IPV4_EXT
> else. The value RTE_PTYPE_L3_IPV4_EXT_UNKNOWN is used by PMDs that don't
> know if there are options.

I see. What I'm trying to say that there are non
obvious assumptions here on rte_net_get_ptype()
behaviour which can be changed. May be it makes
sense to add comments here to highlight it.

> 
>>> - right above this code, we already returned if l3 is not in
>>>   (RTE_PTYPE_L3_IPV4, RTE_PTYPE_L3_IPV4_EXT, RTE_PTYPE_L3_IPV6)
>>
>> If so, it sounds like it should be allowed above as well.
>>
>>>
>>>>>
>>>>>>  			if (l4 == RTE_PTYPE_L4_UDP) {
>>>>>>  				udp_hdr = (struct rte_udp_hdr *)l4_hdr;
>>>>>>  				if (udp_hdr->dgram_cksum == 0) {
>>>>>> @@ -364,7 +364,7 @@ tap_verify_csum(struct rte_mbuf *mbuf)
>>>>>>  				}
>>>>>>  			}
>>>>>>  			cksum = ~rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
>>>>>> -		} else if (l3 == RTE_PTYPE_L3_IPV6) {
>>>>>> +		} else { /* l3 == RTE_PTYPE_L3_IPV6, checked above */
>>>>>>  			cksum = ~rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
>>>>>>  		}
>>>>>>  		mbuf->ol_flags |= cksum ?
>>>>>>
>>>>
>>

Re: [dpdk-dev] [dpdk-stable] [PATCH 1/4] net/tap: fix Rx cksum flags on IP options packets

[Olivier Matz]

On Tue, Jun 08, 2021 at 04:57:00PM +0300, Andrew Rybchenko wrote:
> On 6/8/21 3:49 PM, Olivier Matz wrote:
> > On Tue, Jun 08, 2021 at 03:34:36PM +0300, Andrew Rybchenko wrote:
> >> On 6/8/21 3:29 PM, Olivier Matz wrote:
> >>> Hi Ferruh, Andrew,
> >>>
> >>> On Tue, Jun 08, 2021 at 01:13:59PM +0300, Andrew Rybchenko wrote:
> >>>> On 4/30/21 5:48 PM, Ferruh Yigit wrote:
> >>>>> On 4/27/2021 2:57 PM, Olivier Matz wrote:
> >>>>>> When packet type is IPV4_EXT, the checksum is always marked as good in
> >>>>>> the mbuf offload flags.
> >>>>>>
> >>>>>> Since we know the header lengths, we can easily call
> >>>>>> rte_ipv4_udptcp_cksum() in this case too.
> >>>>>>
> >>>>>> Fixes: 8ae3023387e9 ("net/tap: add Rx/Tx checksum offload support")
> >>>>>> Cc: stable@dpdk.org
> >>>>>>
> >>>>>> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> >>>>>> ---
> >>>>>>  drivers/net/tap/rte_eth_tap.c | 4 ++--
> >>>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> >>>>>> index 68baa18523..e7b185a4b5 100644
> >>>>>> --- a/drivers/net/tap/rte_eth_tap.c
> >>>>>> +++ b/drivers/net/tap/rte_eth_tap.c
> >>>>>> @@ -350,7 +350,7 @@ tap_verify_csum(struct rte_mbuf *mbuf)
> >>>>>>  		/* Don't verify checksum for multi-segment packets. */
> >>>>>>  		if (mbuf->nb_segs > 1)
> >>>>>>  			return;
> >>>>>> -		if (l3 == RTE_PTYPE_L3_IPV4) {
> >>>>>> +		if (l3 == RTE_PTYPE_L3_IPV4 || l3 == RTE_PTYPE_L3_IPV4_EXT) {
> >>>>>
> >>>>> Should we take 'RTE_PTYPE_L3_IPV4_EXT_UNKNOWN' into account?
> >>>>
> >>>> I think we should.
> >>>
> >>> I think 'RTE_PTYPE_L3_IPV4_EXT_UNKNOWN' cannot happen here:
> >>>
> >>> - mbuf->packet_type is generated by 
> >>
> >> (), which cannot
> >>>   return 'RTE_PTYPE_L3_IPV4_EXT_UNKNOWN'
> >>
> >> My question if it is guaranteed and the only possible branch.
> >> Can application set packet_type itself and do not call
> >> rte_net_get_ptype(). Yes, typically application knows
> >> if it has IPv4 options in the header or not, but theoretically
> >> could be unaware as well.
> > 
> > This function is called on the Rx path from pmd_rx_burst(), so
> > the application does not have access to the mbuf.
> > 
> > The software parser that sets the packet type returns either
> > RTE_PTYPE_L3_IPV4 if there is no option, or RTE_PTYPE_L3_IPV4_EXT
> > else. The value RTE_PTYPE_L3_IPV4_EXT_UNKNOWN is used by PMDs that don't
> > know if there are options.
> 
> I see. What I'm trying to say that there are non
> obvious assumptions here on rte_net_get_ptype()
> behaviour which can be changed. May be it makes
> sense to add comments here to highlight it.

Ok, I'll add some words about it.

Thanks!

> 
> > 
> >>> - right above this code, we already returned if l3 is not in
> >>>   (RTE_PTYPE_L3_IPV4, RTE_PTYPE_L3_IPV4_EXT, RTE_PTYPE_L3_IPV6)
> >>
> >> If so, it sounds like it should be allowed above as well.
> >>
> >>>
> >>>>>
> >>>>>>  			if (l4 == RTE_PTYPE_L4_UDP) {
> >>>>>>  				udp_hdr = (struct rte_udp_hdr *)l4_hdr;
> >>>>>>  				if (udp_hdr->dgram_cksum == 0) {
> >>>>>> @@ -364,7 +364,7 @@ tap_verify_csum(struct rte_mbuf *mbuf)
> >>>>>>  				}
> >>>>>>  			}
> >>>>>>  			cksum = ~rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
> >>>>>> -		} else if (l3 == RTE_PTYPE_L3_IPV6) {
> >>>>>> +		} else { /* l3 == RTE_PTYPE_L3_IPV6, checked above */
> >>>>>>  			cksum = ~rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
> >>>>>>  		}
> >>>>>>  		mbuf->ol_flags |= cksum ?
> >>>>>>
> >>>>
> >>
> 

Re: [dpdk-dev] [PATCH 3/4] net: introduce functions to verify L4 checksums

[Ferruh Yigit]

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 <olivier.matz@6wind.com>
>>>>> ---
>>>>>  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.

[dpdk-dev] [PATCH v2 0/4] net/tap: fix Rx cksum

[Olivier Matz]

This patchset fixes the Rx checksum flags in net/tap
driver. The first two patches are the effective fixes.

The last 2 patches introduce a new checksum API to
verify a L4 checksum and its unt test, in order to
simplify the net/tap code, or any other code that has
the same needs.

v2:

* clarify why RTE_PTYPE_L3_IPV4_EXT_UNKNOWN cannot happen in
  tap_verify_csum() (patch 1)
* align style of rte_ipv6_udptcp_cksum_verify() to
  rte_ipv4_udptcp_cksum_verify() (patch 3)
* clarify comment above rte_ipv4_udptcp_cksum_verify() and
  rte_ipv6_udptcp_cksum_verify() (patch 3)


Olivier Matz (4):
  net/tap: fix Rx cksum flags on IP options packets
  net/tap: fix Rx cksum flags on TCP packets
  net: introduce functions to verify L4 checksums
  test/cksum: new test for L3/L4 checksum API

 MAINTAINERS                   |   1 +
 app/test/autotest_data.py     |   6 +
 app/test/meson.build          |   2 +
 app/test/test_cksum.c         | 271 ++++++++++++++++++++++++++++++++++
 drivers/net/tap/rte_eth_tap.c |  23 ++-
 lib/net/rte_ip.h              | 127 +++++++++++++---
 6 files changed, 398 insertions(+), 32 deletions(-)
 create mode 100644 app/test/test_cksum.c

-- 
2.29.2

[dpdk-dev] [PATCH v2 1/4] net/tap: fix Rx cksum flags on IP options packets

[Olivier Matz]

When packet type is IPV4_EXT, the checksum is always marked as good in
the mbuf offload flags.

Since we know the header lengths, we can easily call
rte_ipv4_udptcp_cksum() in this case too.

Fixes: 8ae3023387e9 ("net/tap: add Rx/Tx checksum offload support")
Cc: stable@dpdk.org

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 drivers/net/tap/rte_eth_tap.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 5735988e7c..5513cfd2d7 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -342,7 +342,11 @@ tap_verify_csum(struct rte_mbuf *mbuf)
 				rte_pktmbuf_data_len(mbuf))
 			return;
 	} else {
-		/* IPv6 extensions are not supported */
+		/* - RTE_PTYPE_L3_IPV4_EXT_UNKNOWN cannot happen because
+		 *   mbuf->packet_type is filled by rte_net_get_ptype() which
+		 *   never returns this value.
+		 * - IPv6 extensions are not supported.
+		 */
 		return;
 	}
 	if (l4 == RTE_PTYPE_L4_UDP || l4 == RTE_PTYPE_L4_TCP) {
@@ -350,7 +354,7 @@ tap_verify_csum(struct rte_mbuf *mbuf)
 		/* Don't verify checksum for multi-segment packets. */
 		if (mbuf->nb_segs > 1)
 			return;
-		if (l3 == RTE_PTYPE_L3_IPV4) {
+		if (l3 == RTE_PTYPE_L3_IPV4 || l3 == RTE_PTYPE_L3_IPV4_EXT) {
 			if (l4 == RTE_PTYPE_L4_UDP) {
 				udp_hdr = (struct rte_udp_hdr *)l4_hdr;
 				if (udp_hdr->dgram_cksum == 0) {
@@ -364,7 +368,7 @@ tap_verify_csum(struct rte_mbuf *mbuf)
 				}
 			}
 			cksum = ~rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
-		} else if (l3 == RTE_PTYPE_L3_IPV6) {
+		} else { /* l3 == RTE_PTYPE_L3_IPV6, checked above */
 			cksum = ~rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
 		}
 		mbuf->ol_flags |= cksum ?
-- 
2.29.2

[dpdk-dev] [PATCH v2 2/4] net/tap: fix Rx cksum flags on TCP packets

[Olivier Matz]

Since commit d5df2ae0428a ("net: fix unneeded replacement of TCP
checksum 0"), the functions rte_ipv4_udptcp_cksum() or
rte_ipv6_udptcp_cksum() can return either 0x0000 or 0xffff when used to
verify a packet containing a valid checksum.

This new behavior broke the checksum verification in tap driver for TCP
packets: these packets are marked with PKT_RX_L4_CKSUM_BAD.

Fix this by checking the 2 possible values. A next commit will introduce
a checksum verification helper to simplify this a bit.

Fixes: d5df2ae0428a ("net: fix unneeded replacement of TCP checksum 0")
Cc: stable@dpdk.org

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
Acked-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
---
 drivers/net/tap/rte_eth_tap.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 5513cfd2d7..5429f611c1 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -350,6 +350,8 @@ tap_verify_csum(struct rte_mbuf *mbuf)
 		return;
 	}
 	if (l4 == RTE_PTYPE_L4_UDP || l4 == RTE_PTYPE_L4_TCP) {
+		int cksum_ok;
+
 		l4_hdr = rte_pktmbuf_mtod_offset(mbuf, void *, l2_len + l3_len);
 		/* Don't verify checksum for multi-segment packets. */
 		if (mbuf->nb_segs > 1)
@@ -367,13 +369,13 @@ tap_verify_csum(struct rte_mbuf *mbuf)
 					return;
 				}
 			}
-			cksum = ~rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
+			cksum = rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
 		} else { /* l3 == RTE_PTYPE_L3_IPV6, checked above */
-			cksum = ~rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
+			cksum = rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
 		}
-		mbuf->ol_flags |= cksum ?
-			PKT_RX_L4_CKSUM_BAD :
-			PKT_RX_L4_CKSUM_GOOD;
+		cksum_ok = (cksum == 0) || (cksum == 0xffff);
+		mbuf->ol_flags |= cksum_ok ?
+			PKT_RX_L4_CKSUM_GOOD : PKT_RX_L4_CKSUM_BAD;
 	}
 }
 
-- 
2.29.2

[dpdk-dev] [PATCH v2 3/4] net: introduce functions to verify L4 checksums

[Olivier Matz]

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 <olivier.matz@6wind.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
---
 drivers/net/tap/rte_eth_tap.c |   7 +-
 lib/net/rte_ip.h              | 127 +++++++++++++++++++++++++++-------
 2 files changed, 107 insertions(+), 27 deletions(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 5429f611c1..2229eef059 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -369,11 +369,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 4b728969c1..05948b69b7 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,65 @@ 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.
+ *
+ * In case of UDP, the caller must first check if udp_hdr->dgram_cksum is 0
+ * (i.e. no 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;
 }
 
 /**
@@ -448,6 +487,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 +522,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;
+	uint16_t cksum = __rte_ipv6_udptcp_cksum(ipv6_hdr, l4_hdr);
 
-	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;
 
-	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 +534,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.
+ *
+ * In case of UDP, the caller must first check if udp_hdr->dgram_cksum is 0:
+ * this is either invalid or means no checksum in some situations. 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 = __rte_ipv6_udptcp_cksum(ipv6_hdr, l4_hdr);
+
+	if (cksum != 0xffff)
+		return -1;
+
+	return 0;
 }
 
 /** IPv6 fragment extension header. */
-- 
2.29.2

[dpdk-dev] [PATCH v2 4/4] test/cksum: new test for L3/L4 checksum API

[Olivier Matz]

Add a simple unit test for checksum API.

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 MAINTAINERS               |   1 +
 app/test/autotest_data.py |   6 +
 app/test/meson.build      |   2 +
 app/test/test_cksum.c     | 271 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 280 insertions(+)
 create mode 100644 app/test/test_cksum.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 5877a16971..4347555ebc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1314,6 +1314,7 @@ Packet processing
 Network headers
 M: Olivier Matz <olivier.matz@6wind.com>
 F: lib/net/
+F: app/test/test_cksum.c
 
 Packet CRC
 M: Jasvinder Singh <jasvinder.singh@intel.com>
diff --git a/app/test/autotest_data.py b/app/test/autotest_data.py
index 11f9c8640c..302d6374c1 100644
--- a/app/test/autotest_data.py
+++ b/app/test/autotest_data.py
@@ -567,6 +567,12 @@
         "Func":    default_autotest,
         "Report":  None,
     },
+    {
+        "Name":    "Checksum autotest",
+        "Command": "cksum_autotest",
+        "Func":    default_autotest,
+        "Report":  None,
+    },
     #
     #Please always keep all dump tests at the end and together!
     #
diff --git a/app/test/meson.build b/app/test/meson.build
index 0a5f425578..ef90b16f16 100644
--- a/app/test/meson.build
+++ b/app/test/meson.build
@@ -17,6 +17,7 @@ test_sources = files(
         'test_bitmap.c',
         'test_bpf.c',
         'test_byteorder.c',
+        'test_cksum.c',
         'test_cmdline.c',
         'test_cmdline_cirbuf.c',
         'test_cmdline_etheraddr.c',
@@ -188,6 +189,7 @@ fast_tests = [
         ['atomic_autotest', false],
         ['bitops_autotest', true],
         ['byteorder_autotest', true],
+        ['cksum_autotest', true],
         ['cmdline_autotest', true],
         ['common_autotest', true],
         ['cpuflags_autotest', true],
diff --git a/app/test/test_cksum.c b/app/test/test_cksum.c
new file mode 100644
index 0000000000..cd983d7c01
--- /dev/null
+++ b/app/test/test_cksum.c
@@ -0,0 +1,271 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright 2021 6WIND S.A.
+ */
+
+#include <string.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <stdint.h>
+
+#include <rte_net.h>
+#include <rte_mbuf.h>
+#include <rte_ip.h>
+
+#include "test.h"
+
+#define MEMPOOL_CACHE_SIZE      0
+#define MBUF_DATA_SIZE          256
+#define NB_MBUF                 128
+
+/*
+ * Test L3/L4 checksum API.
+ */
+
+#define GOTO_FAIL(str, ...) do {					\
+		printf("cksum test FAILED (l.%d): <" str ">\n",		\
+		       __LINE__,  ##__VA_ARGS__);			\
+		goto fail;						\
+	} while (0)
+
+/* generated in scapy with Ether()/IP()/TCP())) */
+static const char test_cksum_ipv4_tcp[] = {
+	0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x08, 0x00, 0x45, 0x00,
+	0x00, 0x28, 0x00, 0x01, 0x00, 0x00, 0x40, 0x06,
+	0x7c, 0xcd, 0x7f, 0x00, 0x00, 0x01, 0x7f, 0x00,
+	0x00, 0x01, 0x00, 0x14, 0x00, 0x50, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x50, 0x02,
+	0x20, 0x00, 0x91, 0x7c, 0x00, 0x00,
+
+};
+
+/* generated in scapy with Ether()/IPv6()/TCP()) */
+static const char test_cksum_ipv6_tcp[] = {
+	0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x08, 0x00, 0x60, 0x00,
+	0x00, 0x00, 0x00, 0x14, 0x06, 0x40, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x14,
+	0x00, 0x50, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x50, 0x02, 0x20, 0x00, 0x8f, 0x7d,
+	0x00, 0x00,
+};
+
+/* generated in scapy with Ether()/IP()/UDP()/Raw('x')) */
+static const char test_cksum_ipv4_udp[] = {
+	0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x08, 0x00, 0x45, 0x00,
+	0x00, 0x1d, 0x00, 0x01, 0x00, 0x00, 0x40, 0x11,
+	0x7c, 0xcd, 0x7f, 0x00, 0x00, 0x01, 0x7f, 0x00,
+	0x00, 0x01, 0x00, 0x35, 0x00, 0x35, 0x00, 0x09,
+	0x89, 0x6f, 0x78,
+};
+
+/* generated in scapy with Ether()/IPv6()/UDP()/Raw('x')) */
+static const char test_cksum_ipv6_udp[] = {
+	0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x86, 0xdd, 0x60, 0x00,
+	0x00, 0x00, 0x00, 0x09, 0x11, 0x40, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x35,
+	0x00, 0x35, 0x00, 0x09, 0x87, 0x70, 0x78,
+};
+
+/* generated in scapy with Ether()/IP(options='\x00')/UDP()/Raw('x')) */
+static const char test_cksum_ipv4_opts_udp[] = {
+	0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x00, 0x00,
+	0x00, 0x00, 0x00, 0x00, 0x08, 0x00, 0x46, 0x00,
+	0x00, 0x21, 0x00, 0x01, 0x00, 0x00, 0x40, 0x11,
+	0x7b, 0xc9, 0x7f, 0x00, 0x00, 0x01, 0x7f, 0x00,
+	0x00, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x35,
+	0x00, 0x35, 0x00, 0x09, 0x89, 0x6f, 0x78,
+};
+
+/* test l3/l4 checksum api */
+static int
+test_l4_cksum(struct rte_mempool *pktmbuf_pool, const char *pktdata, size_t len)
+{
+	struct rte_net_hdr_lens hdr_lens;
+	struct rte_mbuf *m = NULL;
+	uint32_t packet_type;
+	uint16_t prev_cksum;
+	void *l3_hdr;
+	void *l4_hdr;
+	uint32_t l3;
+	uint32_t l4;
+	char *data;
+
+	m = rte_pktmbuf_alloc(pktmbuf_pool);
+	if (m == NULL)
+		GOTO_FAIL("Cannot allocate mbuf");
+
+	data = rte_pktmbuf_append(m, len);
+	if (data == NULL)
+		GOTO_FAIL("Cannot append data");
+
+	memcpy(data, pktdata, len);
+
+	packet_type = rte_net_get_ptype(m, &hdr_lens, RTE_PTYPE_ALL_MASK);
+	l3 = packet_type & RTE_PTYPE_L3_MASK;
+	l4 = packet_type & RTE_PTYPE_L4_MASK;
+
+	l3_hdr = rte_pktmbuf_mtod_offset(m, void *, hdr_lens.l2_len);
+	l4_hdr = rte_pktmbuf_mtod_offset(m, void *,
+					 hdr_lens.l2_len + hdr_lens.l3_len);
+
+	if (l3 == RTE_PTYPE_L3_IPV4 || l3 == RTE_PTYPE_L3_IPV4_EXT) {
+		struct rte_ipv4_hdr *ip = l3_hdr;
+
+		/* verify IPv4 checksum */
+		if (rte_ipv4_cksum(l3_hdr) != 0)
+			GOTO_FAIL("invalid IPv4 checksum verification");
+
+		/* verify bad IPv4 checksum */
+		ip->hdr_checksum++;
+		if (rte_ipv4_cksum(l3_hdr) == 0)
+			GOTO_FAIL("invalid IPv4 bad checksum verification");
+		ip->hdr_checksum--;
+
+		/* recalculate IPv4 checksum */
+		prev_cksum = ip->hdr_checksum;
+		ip->hdr_checksum = 0;
+		ip->hdr_checksum = rte_ipv4_cksum(ip);
+		if (ip->hdr_checksum != prev_cksum)
+			GOTO_FAIL("invalid IPv4 checksum calculation");
+
+		/* verify L4 checksum */
+		if (rte_ipv4_udptcp_cksum_verify(l3_hdr, l4_hdr) != 0)
+			GOTO_FAIL("invalid L4 checksum verification");
+
+		if (l4 == RTE_PTYPE_L4_TCP) {
+			struct rte_tcp_hdr *tcp = l4_hdr;
+
+			/* verify bad TCP checksum */
+			tcp->cksum++;
+			if (rte_ipv4_udptcp_cksum_verify(l3_hdr, l4_hdr) == 0)
+				GOTO_FAIL("invalid bad TCP checksum verification");
+			tcp->cksum--;
+
+			/* recalculate TCP checksum */
+			prev_cksum = tcp->cksum;
+			tcp->cksum = 0;
+			tcp->cksum = rte_ipv4_udptcp_cksum(l3_hdr, l4_hdr);
+			if (tcp->cksum != prev_cksum)
+				GOTO_FAIL("invalid TCP checksum calculation");
+
+		} else if (l4 == RTE_PTYPE_L4_UDP) {
+			struct rte_udp_hdr *udp = l4_hdr;
+
+			/* verify bad UDP checksum */
+			udp->dgram_cksum++;
+			if (rte_ipv4_udptcp_cksum_verify(l3_hdr, l4_hdr) == 0)
+				GOTO_FAIL("invalid bad UDP checksum verification");
+			udp->dgram_cksum--;
+
+			/* recalculate UDP checksum */
+			prev_cksum = udp->dgram_cksum;
+			udp->dgram_cksum = 0;
+			udp->dgram_cksum = rte_ipv4_udptcp_cksum(l3_hdr,
+								 l4_hdr);
+			if (udp->dgram_cksum != prev_cksum)
+				GOTO_FAIL("invalid TCP checksum calculation");
+		}
+	} else if (l3 == RTE_PTYPE_L3_IPV6 || l3 == RTE_PTYPE_L3_IPV6_EXT) {
+		if (rte_ipv6_udptcp_cksum_verify(l3_hdr, l4_hdr) != 0)
+			GOTO_FAIL("invalid L4 checksum verification");
+
+		if (l4 == RTE_PTYPE_L4_TCP) {
+			struct rte_tcp_hdr *tcp = l4_hdr;
+
+			/* verify bad TCP checksum */
+			tcp->cksum++;
+			if (rte_ipv6_udptcp_cksum_verify(l3_hdr, l4_hdr) == 0)
+				GOTO_FAIL("invalid bad TCP checksum verification");
+			tcp->cksum--;
+
+			/* recalculate TCP checksum */
+			prev_cksum = tcp->cksum;
+			tcp->cksum = 0;
+			tcp->cksum = rte_ipv6_udptcp_cksum(l3_hdr, l4_hdr);
+			if (tcp->cksum != prev_cksum)
+				GOTO_FAIL("invalid TCP checksum calculation");
+
+		} else if (l4 == RTE_PTYPE_L4_UDP) {
+			struct rte_udp_hdr *udp = l4_hdr;
+
+			/* verify bad UDP checksum */
+			udp->dgram_cksum++;
+			if (rte_ipv6_udptcp_cksum_verify(l3_hdr, l4_hdr) == 0)
+				GOTO_FAIL("invalid bad UDP checksum verification");
+			udp->dgram_cksum--;
+
+			/* recalculate UDP checksum */
+			prev_cksum = udp->dgram_cksum;
+			udp->dgram_cksum = 0;
+			udp->dgram_cksum = rte_ipv6_udptcp_cksum(l3_hdr,
+								 l4_hdr);
+			if (udp->dgram_cksum != prev_cksum)
+				GOTO_FAIL("invalid TCP checksum calculation");
+		}
+	}
+
+	rte_pktmbuf_free(m);
+
+	return 0;
+
+fail:
+	if (m)
+		rte_pktmbuf_free(m);
+
+	return -1;
+}
+
+static int
+test_cksum(void)
+{
+	struct rte_mempool *pktmbuf_pool = NULL;
+
+	/* create pktmbuf pool if it does not exist */
+	pktmbuf_pool = rte_pktmbuf_pool_create("test_cksum_mbuf_pool",
+			NB_MBUF, MEMPOOL_CACHE_SIZE, 0, MBUF_DATA_SIZE,
+			SOCKET_ID_ANY);
+
+	if (pktmbuf_pool == NULL)
+		GOTO_FAIL("cannot allocate mbuf pool");
+
+	if (test_l4_cksum(pktmbuf_pool, test_cksum_ipv4_tcp,
+			  sizeof(test_cksum_ipv4_tcp)) < 0)
+		GOTO_FAIL("checksum error on ipv4_tcp");
+
+	if (test_l4_cksum(pktmbuf_pool, test_cksum_ipv6_tcp,
+			  sizeof(test_cksum_ipv6_tcp)) < 0)
+		GOTO_FAIL("checksum error on ipv6_tcp");
+
+	if (test_l4_cksum(pktmbuf_pool, test_cksum_ipv4_udp,
+			  sizeof(test_cksum_ipv4_udp)) < 0)
+		GOTO_FAIL("checksum error on ipv4_udp");
+
+	if (test_l4_cksum(pktmbuf_pool, test_cksum_ipv6_udp,
+			  sizeof(test_cksum_ipv6_udp)) < 0)
+		GOTO_FAIL("checksum error on ipv6_udp");
+
+	if (test_l4_cksum(pktmbuf_pool, test_cksum_ipv4_opts_udp,
+			  sizeof(test_cksum_ipv4_opts_udp)) < 0)
+		GOTO_FAIL("checksum error on ipv4_opts_udp");
+
+	rte_mempool_free(pktmbuf_pool);
+
+	return 0;
+
+fail:
+	rte_mempool_free(pktmbuf_pool);
+
+	return -1;
+}
+#undef GOTO_FAIL
+
+REGISTER_TEST_COMMAND(cksum_autotest, test_cksum);
-- 
2.29.2

Re: [dpdk-dev] [PATCH v2 0/4] net/tap: fix Rx cksum

[Andrew Rybchenko]

On 6/30/21 4:51 PM, Olivier Matz wrote:
> This patchset fixes the Rx checksum flags in net/tap
> driver. The first two patches are the effective fixes.
> 
> The last 2 patches introduce a new checksum API to
> verify a L4 checksum and its unt test, in order to
> simplify the net/tap code, or any other code that has
> the same needs.
> 
> v2:
> 
> * clarify why RTE_PTYPE_L3_IPV4_EXT_UNKNOWN cannot happen in
>   tap_verify_csum() (patch 1)
> * align style of rte_ipv6_udptcp_cksum_verify() to
>   rte_ipv4_udptcp_cksum_verify() (patch 3)
> * clarify comment above rte_ipv4_udptcp_cksum_verify() and
>   rte_ipv6_udptcp_cksum_verify() (patch 3)
> 
> 
> Olivier Matz (4):
>   net/tap: fix Rx cksum flags on IP options packets
>   net/tap: fix Rx cksum flags on TCP packets
>   net: introduce functions to verify L4 checksums
>   test/cksum: new test for L3/L4 checksum API
> 
>  MAINTAINERS                   |   1 +
>  app/test/autotest_data.py     |   6 +
>  app/test/meson.build          |   2 +
>  app/test/test_cksum.c         | 271 ++++++++++++++++++++++++++++++++++
>  drivers/net/tap/rte_eth_tap.c |  23 ++-
>  lib/net/rte_ip.h              | 127 +++++++++++++---
>  6 files changed, 398 insertions(+), 32 deletions(-)
>  create mode 100644 app/test/test_cksum.c
> 

Series-reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>

Applied, thanks.