[PATCH net-next v4 0/4] net: selftest: improve test string formatting and checksum handling

[PATCH net-next v4 0/4] net: selftest: improve test string formatting and checksum handling

[Oleksij Rempel]

This patchset addresses two issues in the current net selftest
framework:

- Truncated test names: Existing test names are prefixed with an index,
  reducing the available space within the ETH_GSTRING_LEN limit.  This
  patch removes the index to allow more descriptive names.

- Inconsistent checksum behavior: On DSA setups and similar
  environments, checksum offloading is not always available or
  appropriate. The previous selftests did not distinguish between software
  and hardware checksum modes, leading to unreliable results. This
  patchset introduces explicit csum_mode handling and adds separate tests
  for both software and hardware checksum validation.

changes v4:
- s/Return /Return:/

changes v3:
- no functional changes
- rebase against latest net-next

Oleksij Rempel (4):
  net: selftests: drop test index from net_selftest_get_strings()
  net: selftests: prepare for detailed error handling in
    net_test_get_skb()
  net: selftests: add checksum mode support and SW checksum handling
  net: selftests: add PHY loopback tests with HW checksum offload

 net/core/selftests.c | 308 ++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 291 insertions(+), 17 deletions(-)

--
2.39.5

[PATCH net-next v4 1/4] net: selftests: drop test index from net_selftest_get_strings()

[Oleksij Rempel]

The test index is redundant and reduces available space for test names,
which are already limited to ETH_GSTRING_LEN (32 bytes). Removing the
index improves readability in tools like `ethtool -t`, especially when
longer test names are used.

Before this change:
  3. PHY internal loopback, enab
  7. PHY internal loopback, disa

After this change:
  PHY internal loopback, enable
  PHY internal loopback, disable

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
Reviewed-by: Simon Horman <horms@kernel.org>
---
changes v2:
- use ethtool_puts instead of ethtool_sprintf
---
 net/core/selftests.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/core/selftests.c b/net/core/selftests.c
index 35f807ea9952..9146e33db25a 100644
--- a/net/core/selftests.c
+++ b/net/core/selftests.c
@@ -408,8 +408,7 @@ void net_selftest_get_strings(u8 *data)
 	int i;
 
 	for (i = 0; i < net_selftest_get_count(); i++)
-		ethtool_sprintf(&data, "%2d. %s", i + 1,
-				net_selftests[i].name);
+		ethtool_puts(&data, net_selftests[i].name);
 }
 EXPORT_SYMBOL_GPL(net_selftest_get_strings);
 
-- 
2.39.5

[PATCH net-next v4 2/4] net: selftests: prepare for detailed error handling in net_test_get_skb()

[Oleksij Rempel]

Replace NULL return with ERR_PTR(-ENOMEM) in net_test_get_skb() and
update the caller to use IS_ERR/PTR_ERR.

This prepares the code for follow-up changes that will return more
specific error codes from net_test_get_skb().

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
Reviewed-by: Simon Horman <horms@kernel.org>
---
 net/core/selftests.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/core/selftests.c b/net/core/selftests.c
index 9146e33db25a..3f82a5d14cd4 100644
--- a/net/core/selftests.c
+++ b/net/core/selftests.c
@@ -74,7 +74,7 @@ static struct sk_buff *net_test_get_skb(struct net_device *ndev,
 
 	skb = netdev_alloc_skb(ndev, size);
 	if (!skb)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 
 	prefetchw(skb->data);
 
@@ -267,8 +267,8 @@ static int __net_test_loopback(struct net_device *ndev,
 	dev_add_pack(&tpriv->pt);
 
 	skb = net_test_get_skb(ndev, attr);
-	if (!skb) {
-		ret = -ENOMEM;
+	if (IS_ERR(skb)) {
+		ret = PTR_ERR(skb);
 		goto cleanup;
 	}
 
-- 
2.39.5

[PATCH net-next v4 3/4] net: selftests: add checksum mode support and SW checksum handling

[Oleksij Rempel]

Introduce `enum net_test_checksum_mode` to support both CHECKSUM_COMPLETE
and CHECKSUM_PARTIAL modes in selftest packet generation.

Add helpers to calculate and apply software checksums for TCP/UDP in
CHECKSUM_COMPLETE mode, and refactor checksum handling into a dedicated
function `net_test_set_checksum()`.

Update PHY loopback tests to use CHECKSUM_COMPLETE by default to avoid
hardware offload dependencies and improve reliability.

Also rename loopback test names to clarify checksum type and transport.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
changes v4:
- s/Returns /Return: /
changes v2:
- Rebased on latest net-next
- Fixed Sparse warnings in net_test_setup_sw_csum():
  * Use __sum16 instead of __be16 for final_csum
---
 net/core/selftests.c | 219 ++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 207 insertions(+), 12 deletions(-)

diff --git a/net/core/selftests.c b/net/core/selftests.c
index 3f82a5d14cd4..d533246f0a26 100644
--- a/net/core/selftests.c
+++ b/net/core/selftests.c
@@ -10,10 +10,16 @@
  */
 
 #include <linux/phy.h>
+#include <net/checksum.h>
 #include <net/selftests.h>
 #include <net/tcp.h>
 #include <net/udp.h>
 
+enum net_test_checksum_mode {
+	NET_TEST_CHECKSUM_COMPLETE,
+	NET_TEST_CHECKSUM_PARTIAL,
+};
+
 struct net_packet_attrs {
 	const unsigned char *src;
 	const unsigned char *dst;
@@ -27,6 +33,7 @@ struct net_packet_attrs {
 	int max_size;
 	u8 id;
 	u16 queue_mapping;
+	enum net_test_checksum_mode csum_mode;
 };
 
 struct net_test_priv {
@@ -51,6 +58,133 @@ static u8 net_test_next_id;
 #define NET_TEST_PKT_MAGIC	0xdeadcafecafedeadULL
 #define NET_LB_TIMEOUT		msecs_to_jiffies(200)
 
+/**
+ * net_test_setup_sw_csum - Compute and apply software checksum
+ *                          (CHECKSUM_COMPLETE)
+ * @skb: Socket buffer with transport header set
+ * @iph: Pointer to IPv4 header inside skb
+ *
+ * This function computes and fills the transport layer checksum (TCP or UDP),
+ * and sets skb->ip_summed = CHECKSUM_COMPLETE.
+ *
+ * Return: 0 on success, or negative error code on failure.
+ */
+static int net_test_setup_sw_csum(struct sk_buff *skb,
+				  struct iphdr *iph)
+{
+	int transport_offset = skb_transport_offset(skb);
+	int transport_len = skb->len - transport_offset;
+	__sum16 final_csum;
+	__wsum csum;
+
+	switch (iph->protocol) {
+	case IPPROTO_TCP:
+		if (!pskb_may_pull(skb,
+				   transport_offset + sizeof(struct tcphdr)))
+			return -EFAULT;
+
+		tcp_hdr(skb)->check = 0;
+		break;
+	case IPPROTO_UDP:
+		if (!pskb_may_pull(skb,
+				   transport_offset + sizeof(struct udphdr)))
+			return -EFAULT;
+
+		udp_hdr(skb)->check = 0;
+		break;
+	default:
+		pr_err("net_selftest: unsupported proto for sw csum: %u\n",
+		       iph->protocol);
+		return -EINVAL;
+	}
+
+	csum = skb_checksum(skb, transport_offset, transport_len, 0);
+	final_csum = csum_tcpudp_magic(iph->saddr, iph->daddr, transport_len,
+				       iph->protocol, csum);
+
+	if (iph->protocol == IPPROTO_UDP && final_csum == 0)
+		final_csum = CSUM_MANGLED_0;
+
+	if (iph->protocol == IPPROTO_TCP)
+		tcp_hdr(skb)->check = final_csum;
+	else
+		udp_hdr(skb)->check = final_csum;
+
+	skb->ip_summed = CHECKSUM_COMPLETE;
+
+	return 0;
+}
+
+/**
+ * net_test_setup_hw_csum - Setup skb for hardware checksum offload
+ *			    (CHECKSUM_PARTIAL)
+ * @skb: Socket buffer to prepare
+ * @iph: Pointer to IPv4 header inside skb
+ *
+ * This function sets skb fields and clears transport checksum field
+ * so that the NIC or driver can compute the checksum during transmit.
+ *
+ * Return: 0 on success, or negative error code on failure.
+ */
+static int net_test_setup_hw_csum(struct sk_buff *skb, struct iphdr *iph)
+{
+	u16 csum_offset;
+
+	skb->ip_summed = CHECKSUM_PARTIAL;
+	skb->csum = 0;
+
+	switch (iph->protocol) {
+	case IPPROTO_TCP:
+		if (!tcp_hdr(skb))
+			return -EINVAL;
+		tcp_hdr(skb)->check = 0;
+		csum_offset = offsetof(struct tcphdr, check);
+		break;
+	case IPPROTO_UDP:
+		if (!udp_hdr(skb))
+			return -EINVAL;
+		udp_hdr(skb)->check = 0;
+		csum_offset = offsetof(struct udphdr, check);
+		break;
+	default:
+		pr_err("net_selftest: unsupported proto for hw csum: %u\n",
+		       iph->protocol);
+		return -EINVAL;
+	}
+
+	skb->csum_start = skb_transport_header(skb) - skb->head;
+	skb->csum_offset = csum_offset;
+
+	return 0;
+}
+
+/**
+ * net_test_set_checksum - Apply requested checksum mode to skb
+ * @skb: Socket buffer containing the packet
+ * @attr: Packet attributes including desired checksum mode
+ * @iph: Pointer to the IP header within skb
+ *
+ * This function sets up the skb's checksum handling based on
+ * attr->csum_mode by calling the appropriate helper.
+ *
+ * Return: 0 on success, or negative error code on failure.
+ */
+static int net_test_set_checksum(struct sk_buff *skb,
+				 struct net_packet_attrs *attr,
+				 struct iphdr *iph)
+{
+	switch (attr->csum_mode) {
+	case NET_TEST_CHECKSUM_COMPLETE:
+		return net_test_setup_sw_csum(skb, iph);
+	case NET_TEST_CHECKSUM_PARTIAL:
+		return net_test_setup_hw_csum(skb, iph);
+	default:
+		pr_err("net_selftest: invalid checksum mode: %d\n",
+		       attr->csum_mode);
+		return -EINVAL;
+	}
+}
+
 static struct sk_buff *net_test_get_skb(struct net_device *ndev,
 					struct net_packet_attrs *attr)
 {
@@ -61,6 +195,7 @@ static struct sk_buff *net_test_get_skb(struct net_device *ndev,
 	struct ethhdr *ehdr;
 	struct iphdr *ihdr;
 	int iplen, size;
+	int ret;
 
 	size = attr->size + NET_TEST_PKT_SIZE;
 
@@ -157,15 +292,10 @@ static struct sk_buff *net_test_get_skb(struct net_device *ndev,
 		memset(pad, 0, pad_len);
 	}
 
-	skb->csum = 0;
-	skb->ip_summed = CHECKSUM_PARTIAL;
-	if (attr->tcp) {
-		thdr->check = ~tcp_v4_check(skb->len, ihdr->saddr,
-					    ihdr->daddr, 0);
-		skb->csum_start = skb_transport_header(skb) - skb->head;
-		skb->csum_offset = offsetof(struct tcphdr, check);
-	} else {
-		udp4_hwcsum(skb, ihdr->saddr, ihdr->daddr);
+	ret = net_test_set_checksum(skb, attr, ihdr);
+	if (ret < 0) {
+		kfree_skb(skb);
+		return ERR_PTR(ret);
 	}
 
 	skb->protocol = htons(ETH_P_IP);
@@ -318,29 +448,94 @@ static int net_test_phy_loopback_disable(struct net_device *ndev)
 	return phy_loopback(ndev->phydev, false, 0);
 }
 
+/**
+ * net_test_phy_loopback_udp - Basic PHY loopback test using UDP with SW
+ *                             checksum
+ * @ndev: The network device to test
+ *
+ * Sends and receives a minimal UDP packet through the device's internal
+ * PHY loopback path. The transport checksum is computed in software
+ * (CHECKSUM_COMPLETE), ensuring test validity regardless of hardware
+ * checksum offload support.
+ *
+ * Expected packet path:
+ *   Test code → MAC driver → MAC HW → xMII → PHY →
+ *   internal PHY loopback → xMII → MAC HW → MAC driver → test code
+ *
+ * The test frame includes Ethernet (14B), IPv4 (20B), UDP (8B), and a
+ * minimal payload (13B), totaling 55 bytes before padding/FCS. Most
+ * MACs will pad this to 60 bytes before appending the FCS.
+ *
+ * Return: 0 on success, or negative error code on failure.
+ */
 static int net_test_phy_loopback_udp(struct net_device *ndev)
 {
 	struct net_packet_attrs attr = { };
 
 	attr.dst = ndev->dev_addr;
+	attr.tcp = false;
+	attr.csum_mode = NET_TEST_CHECKSUM_COMPLETE;
+
 	return __net_test_loopback(ndev, &attr);
 }
 
+/**
+ * net_test_phy_loopback_udp_mtu - PHY loopback test using UDP MTU-sized frame
+ *                                 with SW checksum
+ * @ndev: The network device to test
+ *
+ * Sends and receives a UDP packet through the device's internal PHY loopback
+ * path. The packet uses software checksum calculation (CHECKSUM_COMPLETE),
+ * and the total L2 frame size is padded to match the device MTU.
+ *
+ * This tests the loopback path with larger frames and ensures checksum
+ * correctness regardless of hardware offload support.
+ *
+ * Expected packet path:
+ *   Test code → MAC driver → MAC HW → xMII → PHY →
+ *   internal PHY loopback → xMII → MAC HW → MAC driver → test code
+ *
+ * Return: 0 on success, or negative error code on failure.
+ */
 static int net_test_phy_loopback_udp_mtu(struct net_device *ndev)
 {
 	struct net_packet_attrs attr = { };
 
 	attr.dst = ndev->dev_addr;
 	attr.max_size = ndev->mtu;
+	attr.tcp = false;
+	attr.csum_mode = NET_TEST_CHECKSUM_COMPLETE;
+
 	return __net_test_loopback(ndev, &attr);
 }
 
+/**
+ * net_test_phy_loopback_tcp - PHY loopback test using TCP with SW checksum
+ * @ndev: The network device to test
+ *
+ * Sends and receives a minimal TCP packet through the device's internal
+ * PHY loopback path. The checksum is computed in software
+ * (CHECKSUM_COMPLETE), avoiding reliance on hardware checksum offload,
+ * which may behave inconsistently with TCP in some loopback setups.
+ *
+ * Expected packet path:
+ *   Test code → MAC driver → MAC HW → xMII → PHY →
+ *   internal PHY loopback → xMII → MAC HW → MAC driver → test code
+ *
+ * The generated test frame includes Ethernet (14B), IPv4 (20B), TCP (20B),
+ * and a small payload (13B), totaling 67 bytes before FCS. Since the total
+ * exceeds the Ethernet minimum, MAC padding is typically not applied.
+ *
+ * Return: 0 on success, or negative error code on failure.
+ */
 static int net_test_phy_loopback_tcp(struct net_device *ndev)
 {
 	struct net_packet_attrs attr = { };
 
 	attr.dst = ndev->dev_addr;
 	attr.tcp = true;
+	attr.csum_mode = NET_TEST_CHECKSUM_COMPLETE;
+
 	return __net_test_loopback(ndev, &attr);
 }
 
@@ -359,13 +554,13 @@ static const struct net_test {
 		.name = "PHY internal loopback, enable ",
 		.fn = net_test_phy_loopback_enable,
 	}, {
-		.name = "PHY internal loopback, UDP    ",
+		.name = "PHY loopback UDP (SW csum)    ",
 		.fn = net_test_phy_loopback_udp,
 	}, {
-		.name = "PHY internal loopback, MTU    ",
+		.name = "PHY loopback UDP MTU (SW csum)",
 		.fn = net_test_phy_loopback_udp_mtu,
 	}, {
-		.name = "PHY internal loopback, TCP    ",
+		.name = "PHY loopback TCP (SW csum)    ",
 		.fn = net_test_phy_loopback_tcp,
 	}, {
 		/* This test should be done after all PHY loopback test */
-- 
2.39.5

Re: [PATCH net-next v4 3/4] net: selftests: add checksum mode support and SW checksum handling

[Simon Horman]

On Thu, May 15, 2025 at 10:30:59AM +0200, Oleksij Rempel wrote:
> Introduce `enum net_test_checksum_mode` to support both CHECKSUM_COMPLETE
> and CHECKSUM_PARTIAL modes in selftest packet generation.
> 
> Add helpers to calculate and apply software checksums for TCP/UDP in
> CHECKSUM_COMPLETE mode, and refactor checksum handling into a dedicated
> function `net_test_set_checksum()`.
> 
> Update PHY loopback tests to use CHECKSUM_COMPLETE by default to avoid
> hardware offload dependencies and improve reliability.
> 
> Also rename loopback test names to clarify checksum type and transport.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net-next v4 3/4] net: selftests: add checksum mode support and SW checksum handling

[Jakub Kicinski]

On Thu, 15 May 2025 10:30:59 +0200 Oleksij Rempel wrote:
> +enum net_test_checksum_mode {
> +	NET_TEST_CHECKSUM_COMPLETE,

Why COMPLETE? that means skb has checksum of the complete data.
If packet requires no checksumming you should probably use CHECKSUM_NONE

> +	switch (iph->protocol) {
> +	case IPPROTO_TCP:
> +		if (!pskb_may_pull(skb,
> +				   transport_offset + sizeof(struct tcphdr)))

Why are you so diligently checking if you can pull for the SW sum but
not for the HW sum? Both of them update the same header fields :)

> +static int net_test_setup_hw_csum(struct sk_buff *skb, struct iphdr *iph)
> +{
> +	u16 csum_offset;
> +
> +	skb->ip_summed = CHECKSUM_PARTIAL;
> +	skb->csum = 0;
> +
> +	switch (iph->protocol) {
> +	case IPPROTO_TCP:
> +		if (!tcp_hdr(skb))
> +			return -EINVAL;
> +		tcp_hdr(skb)->check = 0;

this filed should be filled with pseudo header checksum for HW offload,
not with 0, like the existing code did
-- 
pw-bot: cr

[PATCH net-next v4 4/4] net: selftests: add PHY loopback tests with HW checksum offload

[Oleksij Rempel]

Introduce two new PHY loopback tests that validate hardware checksum
offload functionality using UDP and TCP packets. These tests set
csum_mode = CHECKSUM_PARTIAL, allowing the NIC to compute transport
checksums.

Tests are only executed if the device advertises NETIF_F_HW_CSUM
support. If not, they are skipped with -EOPNOTSUPP.

Also register the tests under descriptive names in the test list.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
Reviewed-by: Simon Horman <horms@kernel.org>
---
changes v4:
- s/Returns /Return: /
---
 net/core/selftests.c | 80 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 80 insertions(+)

diff --git a/net/core/selftests.c b/net/core/selftests.c
index d533246f0a26..ade6aeae7b59 100644
--- a/net/core/selftests.c
+++ b/net/core/selftests.c
@@ -539,6 +539,79 @@ static int net_test_phy_loopback_tcp(struct net_device *ndev)
 	return __net_test_loopback(ndev, &attr);
 }
 
+/**
+ * net_test_phy_loopback_udp_hwcsum - PHY loopback test using UDP with HW
+ *                                    checksum
+ * @ndev: The network device to test
+ *
+ * Sends and receives a UDP packet through the device's internal PHY loopback
+ * path. The packet is configured for hardware checksum offload
+ * (CHECKSUM_PARTIAL), allowing the NIC to compute the transport checksum.
+ *
+ * Expected packet path:
+ *   Test code → MAC driver → MAC HW → xMII → PHY →
+ *   internal PHY loopback → xMII → MAC HW → MAC driver → test code
+ *
+ * The test frame includes Ethernet (14B), IPv4 (20B), UDP (8B), and a
+ * small payload (13B), totaling 55 bytes before MAC padding/FCS. Most
+ * MACs pad this to the minimum Ethernet payload (60 bytes before FCS).
+ *
+ * If the device does not support NETIF_F_HW_CSUM, the test is skipped
+ * and -EOPNOTSUPP is returned.
+ *
+ * Return: 0 on success, or negative error code on failure.
+ */
+static int net_test_phy_loopback_udp_hwcsum(struct net_device *ndev)
+{
+	struct net_packet_attrs attr = { };
+
+	if (!(ndev->features & NETIF_F_HW_CSUM))
+		return -EOPNOTSUPP;
+
+	attr.dst = ndev->dev_addr;
+	attr.tcp = false;
+	attr.csum_mode = NET_TEST_CHECKSUM_PARTIAL;
+
+	return __net_test_loopback(ndev, &attr);
+}
+
+/**
+ * net_test_phy_loopback_tcp_hwcsum - PHY loopback test using TCP with HW
+ *                                    checksum
+ * @ndev: The network device to test
+ *
+ * Sends and receives a TCP packet through the device's internal PHY loopback
+ * path. The packet is configured for hardware checksum offload
+ * (CHECKSUM_PARTIAL), allowing the NIC to compute the transport checksum.
+ *
+ * Expected packet path:
+ *   Test code → MAC driver → MAC HW → xMII → PHY →
+ *   internal PHY loopback → xMII → MAC HW → MAC driver → test code
+ *   (via packet_type handler)
+ *
+ * The test frame includes Ethernet (14B), IPv4 (20B), TCP (20B),
+ * and a small payload (13B), totaling 67 bytes before FCS.
+ * No additional padding is required.
+ *
+ * If the device does not support NETIF_F_HW_CSUM, the test is skipped
+ * and -EOPNOTSUPP is returned.
+ *
+ * Return: 0 on success, or negative error code on failure.
+ */
+static int net_test_phy_loopback_tcp_hwcsum(struct net_device *ndev)
+{
+	struct net_packet_attrs attr = { };
+
+	if (!(ndev->features & NETIF_F_HW_CSUM))
+		return -EOPNOTSUPP;
+
+	attr.dst = ndev->dev_addr;
+	attr.tcp = true;
+	attr.csum_mode = NET_TEST_CHECKSUM_PARTIAL;
+
+	return __net_test_loopback(ndev, &attr);
+}
+
 static const struct net_test {
 	char name[ETH_GSTRING_LEN];
 	int (*fn)(struct net_device *ndev);
@@ -562,6 +635,13 @@ static const struct net_test {
 	}, {
 		.name = "PHY loopback TCP (SW csum)    ",
 		.fn = net_test_phy_loopback_tcp,
+	}, {
+		/* Conditional HW checksum tests */
+		.name = "PHY loopback UDP (HW csum)    ",
+		.fn = net_test_phy_loopback_udp_hwcsum,
+	}, {
+		.name = "PHY loopback TCP (HW csum)    ",
+		.fn = net_test_phy_loopback_tcp_hwcsum,
 	}, {
 		/* This test should be done after all PHY loopback test */
 		.name = "PHY internal loopback, disable",
-- 
2.39.5

Re: [PATCH net-next v4 0/4] net: selftest: improve test string formatting and checksum handling

[Jakub Kicinski]

On Thu, 15 May 2025 10:30:56 +0200 Oleksij Rempel wrote:
> - Inconsistent checksum behavior: On DSA setups and similar
>   environments, checksum offloading is not always available or
>   appropriate. The previous selftests did not distinguish between software
>   and hardware checksum modes, leading to unreliable results. This
>   patchset introduces explicit csum_mode handling and adds separate tests
>   for both software and hardware checksum validation.

What device are you talking about? How is this a problem with 
the selftest and not with the stack? If the test is flaky I'd 
think real traffic will suffer too. We pass these selftest packets
thru xmit validation AFAICT, so the stack should compute checksum
for the if the device can't.

Re: [PATCH net-next v4 0/4] net: selftest: improve test string formatting and checksum handling

[Oleksij Rempel]

Hi Jakub,

Sorry for the delay in getting back to you.

On Fri, May 16, 2025 at 06:45:10PM -0700, Jakub Kicinski wrote:
> On Thu, 15 May 2025 10:30:56 +0200 Oleksij Rempel wrote:
> > - Inconsistent checksum behavior: On DSA setups and similar
> >   environments, checksum offloading is not always available or
> >   appropriate. The previous selftests did not distinguish between software
> >   and hardware checksum modes, leading to unreliable results. This
> >   patchset introduces explicit csum_mode handling and adds separate tests
> >   for both software and hardware checksum validation.
> 
> What device are you talking about? How is this a problem with 
> the selftest and not with the stack? If the test is flaky I'd 
> think real traffic will suffer too. We pass these selftest packets
> thru xmit validation AFAICT, so the stack should compute checksum
> for the if the device can't.
> 

Let me first describe the setup where this issue was observed and my findings.
The problem occurs on a system utilizing a Microchip DSA driver with an STMMAC
Ethernet controller attached to the CPU port.

In the current selftest implementation, the TCP checksum validation fails,
while the UDP test passes. The existing code prepares the skb for hardware
checksum offload by setting skb->ip_summed = CHECKSUM_PARTIAL. For TCP, it sets
the thdr->check field to the complement of the pseudo-header checksum, and for
UDP, it uses udp4_hwcsum. If I understand it correct, this configuration tells
the kernel that the hardware should perform the checksum calculation.

However, during testing, I noticed that "rx-checksumming" is enabled by default
on the CPU port, and this leads to the TCP test failure.  Only after disabling
"rx-checksumming" on the CPU port did the selftest pass. This suggests that the
issue is specifically related to the hardware checksum offload mechanism in
this particular setup. The behavior indicates that something on the path
recalculated the checksum incorrectly.

When examining the loopbacked frames, I observed that the TCP checksum was
incorrect. Upon further investigation, the xmit helper in net/dsa/tag_ksz.c
includes the following:

if (skb->ip_summed == CHECKSUM_PARTIAL && skb_checksum_help(skb))
    return NULL;

I assume skb_checksum_help() is intended to calculate the proper checksum when
CHECKSUM_PARTIAL is set, indicating that the software should complete the
checksum before handing it to the hardware. My understanding is that the STMMAC
hardware then calculates the checksum for egress frames if CHECKSUM_PARTIAL is
used. Since these egress frames are passed from the DSA framework with a
tailtag, the checksum calculated by the hardware would then be incorrect for
the original packet. The STMMAC then seems to drop ingress packets if they have
an incorrect checksum.

I'm still trying to grasp the full picture of checksumming in such complex
environments. I would be grateful for your guidance on how this problem should
be addressed properly.

Regarding the current patch series, do these tests and the csum_mode
implementation make sense to you in this context? I believe it would be good
practice to have selftests that can detect these kinds of checksum
inconsistencies in drivers.

Best Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next v4 0/4] net: selftest: improve test string formatting and checksum handling

[Jakub Kicinski]

On Fri, 20 Jun 2025 12:53:23 +0200 Oleksij Rempel wrote:
> > What device are you talking about? How is this a problem with 
> > the selftest and not with the stack? If the test is flaky I'd 
> > think real traffic will suffer too. We pass these selftest packets
> > thru xmit validation AFAICT, so the stack should compute checksum
> > for the if the device can't.
> >   
> 
> Let me first describe the setup where this issue was observed and my findings.
> The problem occurs on a system utilizing a Microchip DSA driver with an STMMAC
> Ethernet controller attached to the CPU port.
> 
> In the current selftest implementation, the TCP checksum validation fails,
> while the UDP test passes. The existing code prepares the skb for hardware
> checksum offload by setting skb->ip_summed = CHECKSUM_PARTIAL. For TCP, it sets
> the thdr->check field to the complement of the pseudo-header checksum, and for
> UDP, it uses udp4_hwcsum. If I understand it correct, this configuration tells
> the kernel that the hardware should perform the checksum calculation.
> 
> However, during testing, I noticed that "rx-checksumming" is enabled by default
> on the CPU port, and this leads to the TCP test failure.  Only after disabling
> "rx-checksumming" on the CPU port did the selftest pass. This suggests that the
> issue is specifically related to the hardware checksum offload mechanism in
> this particular setup. The behavior indicates that something on the path
> recalculated the checksum incorrectly.

Interesting, that sounds like the smoking gun. When rx-checksumming 
is enabled the packet still reaches the stack right?
If so does the frame enter the stack with CHECKSUM_COMPLETE or
UNNECESSARY?

> When examining the loopbacked frames, I observed that the TCP checksum was
> incorrect. Upon further investigation, the xmit helper in net/dsa/tag_ksz.c
> includes the following:
> 
> if (skb->ip_summed == CHECKSUM_PARTIAL && skb_checksum_help(skb))
>     return NULL;
> 
> I assume skb_checksum_help() is intended to calculate the proper checksum when
> CHECKSUM_PARTIAL is set, indicating that the software should complete the
> checksum before handing it to the hardware. My understanding is that the STMMAC
> hardware then calculates the checksum for egress frames if CHECKSUM_PARTIAL is
> used.

stmmac shouldn't touch the frame, note that skb_checksum_help() sets
skb->ip_summed = CHECKSUM_NONE; so the skb should no longer be considered
for csum offload.

> Since these egress frames are passed from the DSA framework with a
> tailtag, the checksum calculated by the hardware would then be incorrect for
> the original packet. The STMMAC then seems to drop ingress packets if they have
> an incorrect checksum.
> 
> I'm still trying to grasp the full picture of checksumming in such complex
> environments. I would be grateful for your guidance on how this problem should
> be addressed properly.
> 
> Regarding the current patch series, do these tests and the csum_mode
> implementation make sense to you in this context? I believe it would be good
> practice to have selftests that can detect these kinds of checksum
> inconsistencies in drivers.

Not yet, at least. Once we figure out the problem you're seeing we can
decide whether we should adjust the tests or the tests are failing
because they are doing their job.

Re: [PATCH net-next v4 0/4] net: selftest: improve test string formatting and checksum handling

[Oleksij Rempel]

On Sat, Jun 21, 2025 at 06:46:00AM -0700, Jakub Kicinski wrote:
> On Fri, 20 Jun 2025 12:53:23 +0200 Oleksij Rempel wrote:
> > > What device are you talking about? How is this a problem with 
> > > the selftest and not with the stack? If the test is flaky I'd 
> > > think real traffic will suffer too. We pass these selftest packets
> > > thru xmit validation AFAICT, so the stack should compute checksum
> > > for the if the device can't.
> > >   
> > 
> > Let me first describe the setup where this issue was observed and my findings.
> > The problem occurs on a system utilizing a Microchip DSA driver with an STMMAC
> > Ethernet controller attached to the CPU port.
> > 
> > In the current selftest implementation, the TCP checksum validation fails,
> > while the UDP test passes. The existing code prepares the skb for hardware
> > checksum offload by setting skb->ip_summed = CHECKSUM_PARTIAL. For TCP, it sets
> > the thdr->check field to the complement of the pseudo-header checksum, and for
> > UDP, it uses udp4_hwcsum. If I understand it correct, this configuration tells
> > the kernel that the hardware should perform the checksum calculation.
> > 
> > However, during testing, I noticed that "rx-checksumming" is enabled by default
> > on the CPU port, and this leads to the TCP test failure.  Only after disabling
> > "rx-checksumming" on the CPU port did the selftest pass. This suggests that the
> > issue is specifically related to the hardware checksum offload mechanism in
> > this particular setup. The behavior indicates that something on the path
> > recalculated the checksum incorrectly.
> 
> Interesting, that sounds like the smoking gun. When rx-checksumming 
> is enabled the packet still reaches the stack right?

No. It looks like this packets are just silently dropped, before they was
seen by the stack. The only counter which confirms presence of this
frames is HW specific mmc_rx_tcp_err. But it will be increasing even if
rx-checksumming is disabled and packets are forwarded to the stack.

> If so does the frame enter the stack with CHECKSUM_COMPLETE or
> UNNECESSARY?

If rx-checksumming is enabled and packet has supported ethertype,
then CHECKSUM_UNNECESSARY will be set. Otherwise CHECKSUM_NONE.

> > When examining the loopbacked frames, I observed that the TCP checksum was
> > incorrect. Upon further investigation, the xmit helper in net/dsa/tag_ksz.c
> > includes the following:
> > 
> > if (skb->ip_summed == CHECKSUM_PARTIAL && skb_checksum_help(skb))
> >     return NULL;
> > 
> > I assume skb_checksum_help() is intended to calculate the proper checksum when
> > CHECKSUM_PARTIAL is set, indicating that the software should complete the
> > checksum before handing it to the hardware. My understanding is that the STMMAC
> > hardware then calculates the checksum for egress frames if CHECKSUM_PARTIAL is
> > used.
> 
> stmmac shouldn't touch the frame, note that skb_checksum_help() sets
> skb->ip_summed = CHECKSUM_NONE; so the skb should no longer be considered
> for csum offload.

It looks like skb_checksum_help(), which is used in tag_ksz.c, generates
a TCP checksum without accounting for the IP pseudo-header. The
resulting checksum is then incorrect and is filtered out by the STMMAC
HW on ingress

If I generate the checksum manually by combining the result of
skb_checksum() with the csum_tcpudp_magic() function - I get a different
checksum from the skb_checksum_help() result, which is then not dropped
by STMMAC on ingress.

Should tag_ksz.c use a different helper function instead of
skb_checksum_help()?

Best Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next v4 0/4] net: selftest: improve test string formatting and checksum handling

[Jakub Kicinski]

On Mon, 23 Jun 2025 13:45:41 +0200 Oleksij Rempel wrote:
> On Sat, Jun 21, 2025 at 06:46:00AM -0700, Jakub Kicinski wrote:
> > On Fri, 20 Jun 2025 12:53:23 +0200 Oleksij Rempel wrote:
> > > Let me first describe the setup where this issue was observed and my findings.
> > > The problem occurs on a system utilizing a Microchip DSA driver with an STMMAC
> > > Ethernet controller attached to the CPU port.
> > > 
> > > In the current selftest implementation, the TCP checksum validation fails,
> > > while the UDP test passes. The existing code prepares the skb for hardware
> > > checksum offload by setting skb->ip_summed = CHECKSUM_PARTIAL. For TCP, it sets
> > > the thdr->check field to the complement of the pseudo-header checksum, and for
> > > UDP, it uses udp4_hwcsum. If I understand it correct, this configuration tells
> > > the kernel that the hardware should perform the checksum calculation.
> > > 
> > > However, during testing, I noticed that "rx-checksumming" is enabled by default
> > > on the CPU port, and this leads to the TCP test failure.  Only after disabling
> > > "rx-checksumming" on the CPU port did the selftest pass. This suggests that the
> > > issue is specifically related to the hardware checksum offload mechanism in
> > > this particular setup. The behavior indicates that something on the path
> > > recalculated the checksum incorrectly.  
> > 
> > Interesting, that sounds like the smoking gun. When rx-checksumming 
> > is enabled the packet still reaches the stack right?  
> 
> No. It looks like this packets are just silently dropped, before they was
> seen by the stack. The only counter which confirms presence of this
> frames is HW specific mmc_rx_tcp_err. But it will be increasing even if
> rx-checksumming is disabled and packets are forwarded to the stack.

If you happen to have the docs for the STMMAC instantiation in the SoC
it'd be good to check if discarding frames with bad csum can be
disabled. Various monitoring systems will expect the L4 checksum errors
to appear in nstat, not some obscure ethtool -S counter.

> > If so does the frame enter the stack with CHECKSUM_COMPLETE or
> > UNNECESSARY?  
> 
> If rx-checksumming is enabled and packet has supported ethertype,
> then CHECKSUM_UNNECESSARY will be set. Otherwise CHECKSUM_NONE.
> 
> > > When examining the loopbacked frames, I observed that the TCP checksum was
> > > incorrect. Upon further investigation, the xmit helper in net/dsa/tag_ksz.c
> > > includes the following:
> > > 
> > > if (skb->ip_summed == CHECKSUM_PARTIAL && skb_checksum_help(skb))
> > >     return NULL;
> > > 
> > > I assume skb_checksum_help() is intended to calculate the proper checksum when
> > > CHECKSUM_PARTIAL is set, indicating that the software should complete the
> > > checksum before handing it to the hardware. My understanding is that the STMMAC
> > > hardware then calculates the checksum for egress frames if CHECKSUM_PARTIAL is
> > > used.  
> > 
> > stmmac shouldn't touch the frame, note that skb_checksum_help() sets
> > skb->ip_summed = CHECKSUM_NONE; so the skb should no longer be considered
> > for csum offload.  
> 
> It looks like skb_checksum_help(), which is used in tag_ksz.c, generates
> a TCP checksum without accounting for the IP pseudo-header. The
> resulting checksum is then incorrect and is filtered out by the STMMAC
> HW on ingress

The pseudo-header csum is filled in net_test_get_skb(), where it calls
tcp_v4_check(). But I think you're right, it's incorrect. Could you try:

diff --git a/net/core/selftests.c b/net/core/selftests.c
index 35f807ea9952..1166dd1ddb07 100644
--- a/net/core/selftests.c
+++ b/net/core/selftests.c
@@ -160,8 +160,10 @@ static struct sk_buff *net_test_get_skb(struct net_device *ndev,
        skb->csum = 0;
        skb->ip_summed = CHECKSUM_PARTIAL;
        if (attr->tcp) {
-               thdr->check = ~tcp_v4_check(skb->len, ihdr->saddr,
-                                           ihdr->daddr, 0);
+               int l4len;
+
+               l4len = skb->tail - skb_transport_header(skb);
+               thdr->check = ~tcp_v4_check(l4len, ihdr->saddr, ihdr->daddr, 0);
                skb->csum_start = skb_transport_header(skb) - skb->head;
                skb->csum_offset = offsetof(struct tcphdr, check);
        } else {

Or some such?

> If I generate the checksum manually by combining the result of
> skb_checksum() with the csum_tcpudp_magic() function - I get a different
> checksum from the skb_checksum_help() result, which is then not dropped
> by STMMAC on ingress.
> 
> Should tag_ksz.c use a different helper function instead of
> skb_checksum_help()?

Re: [PATCH net-next v4 0/4] net: selftest: improve test string formatting and checksum handling

[Oleksij Rempel]

On Mon, Jun 23, 2025 at 10:19:20AM -0700, Jakub Kicinski wrote:
> On Mon, 23 Jun 2025 13:45:41 +0200 Oleksij Rempel wrote:
> > On Sat, Jun 21, 2025 at 06:46:00AM -0700, Jakub Kicinski wrote:
> > > On Fri, 20 Jun 2025 12:53:23 +0200 Oleksij Rempel wrote:
> > > > Let me first describe the setup where this issue was observed and my findings.
> > > > The problem occurs on a system utilizing a Microchip DSA driver with an STMMAC
> > > > Ethernet controller attached to the CPU port.
> > > > 
> > > > In the current selftest implementation, the TCP checksum validation fails,
> > > > while the UDP test passes. The existing code prepares the skb for hardware
> > > > checksum offload by setting skb->ip_summed = CHECKSUM_PARTIAL. For TCP, it sets
> > > > the thdr->check field to the complement of the pseudo-header checksum, and for
> > > > UDP, it uses udp4_hwcsum. If I understand it correct, this configuration tells
> > > > the kernel that the hardware should perform the checksum calculation.
> > > > 
> > > > However, during testing, I noticed that "rx-checksumming" is enabled by default
> > > > on the CPU port, and this leads to the TCP test failure.  Only after disabling
> > > > "rx-checksumming" on the CPU port did the selftest pass. This suggests that the
> > > > issue is specifically related to the hardware checksum offload mechanism in
> > > > this particular setup. The behavior indicates that something on the path
> > > > recalculated the checksum incorrectly.  
> > > 
> > > Interesting, that sounds like the smoking gun. When rx-checksumming 
> > > is enabled the packet still reaches the stack right?  
> > 
> > No. It looks like this packets are just silently dropped, before they was
> > seen by the stack. The only counter which confirms presence of this
> > frames is HW specific mmc_rx_tcp_err. But it will be increasing even if
> > rx-checksumming is disabled and packets are forwarded to the stack.
> 
> If you happen to have the docs for the STMMAC instantiation in the SoC
> it'd be good to check if discarding frames with bad csum can be
> disabled. Various monitoring systems will expect the L4 checksum errors
> to appear in nstat, not some obscure ethtool -S counter.

Ack. I will it add to my todo.


For proper understanding of STMMAC and other drivers, here is how I currently
understand the expected behavior on the receive path, with some open questions:

Receive Path Checksum Scenarios

* No Hardware Verification
    * The hardware is not configured for RX checksum offload
      or does not support the packet type, passing the packet to the driver
      as-is.
    * Expected driver behavior: The driver should set the packet's state to
      `CHECKSUM_NONE`, signaling to the kernel that a software checksum
      validation is required.

* Hardware Verifies and Reports All Frames (Ideal Linux Behavior)
    * The hardware is configured not to drop packets with bad checksums.
      It verifies the checksum of each packet and reports the result (good
      or bad) in a status field on the DMA descriptor.
    * Expected driver behavior: The driver must read the status for every
      packet.
        * If the hardware reports the checksum is good, the driver should set
          the packet's state to `CHECKSUM_UNNECESSARY`.
        * If the hardware reports the checksum is bad, the driver should set
          the packet's state to `CHECKSUM_NONE` and still pass it to the
          kernel.
    * Open Questions:
        * When the hardware reports a bad checksum in this mode, should the
          driver increment `rx_crc_errors` immediately? Or should it only set
          the packet's state to `CHECKSUM_NONE` and let the kernel stack find
          the error and increment the counter, in order to avoid
          double-counting the same error?

* Hardware Verifies and Drops on Error
    * The hardware's RX checksum engine is active and configured to
      automatically discard any packet with an incorrect checksum before it is
      delivered to the driver.
    * Open Questions:

        * When reporting these hardware-level drops, what is the most
          appropriate existing standard `net_device_stats` counter to use
          (e.g., `rx_crc_errors`, `rx_errors`)?
        * If no existing standard counter is a good semantic fit, add new
          standard counters?
        * If the "drop on error" feature cannot be disabled independently,
          and reporting the error via a standard counter is not feasible,
          does this imply that the entire RX checksum offload feature must be
          disabled to ensure error visibility?

* Hardware Provides Full Packet Checksum (`CHECKSUM_COMPLETE`)
    * The hardware calculates a single checksum over the entire packet and
      provides this value to the driver, without needing to parse the
      L3/L4 headers.
    * Expected driver behavior: The driver should place the checksum provided
      by the hardware into the `skb->csum` field and set the packet's state
      to `CHECKSUM_COMPLETE`.

Anything I forgot?

> > > If so does the frame enter the stack with CHECKSUM_COMPLETE or
> > > UNNECESSARY?  
> > 
> > If rx-checksumming is enabled and packet has supported ethertype,
> > then CHECKSUM_UNNECESSARY will be set. Otherwise CHECKSUM_NONE.
> > 
> > > > When examining the loopbacked frames, I observed that the TCP checksum was
> > > > incorrect. Upon further investigation, the xmit helper in net/dsa/tag_ksz.c
> > > > includes the following:
> > > > 
> > > > if (skb->ip_summed == CHECKSUM_PARTIAL && skb_checksum_help(skb))
> > > >     return NULL;
> > > > 
> > > > I assume skb_checksum_help() is intended to calculate the proper checksum when
> > > > CHECKSUM_PARTIAL is set, indicating that the software should complete the
> > > > checksum before handing it to the hardware. My understanding is that the STMMAC
> > > > hardware then calculates the checksum for egress frames if CHECKSUM_PARTIAL is
> > > > used.  
> > > 
> > > stmmac shouldn't touch the frame, note that skb_checksum_help() sets
> > > skb->ip_summed = CHECKSUM_NONE; so the skb should no longer be considered
> > > for csum offload.  
> > 
> > It looks like skb_checksum_help(), which is used in tag_ksz.c, generates
> > a TCP checksum without accounting for the IP pseudo-header. The
> > resulting checksum is then incorrect and is filtered out by the STMMAC
> > HW on ingress
> 
> The pseudo-header csum is filled in net_test_get_skb(), where it calls
> tcp_v4_check(). But I think you're right, it's incorrect. Could you try:
> 
> diff --git a/net/core/selftests.c b/net/core/selftests.c
> index 35f807ea9952..1166dd1ddb07 100644
> --- a/net/core/selftests.c
> +++ b/net/core/selftests.c
> @@ -160,8 +160,10 @@ static struct sk_buff *net_test_get_skb(struct net_device *ndev,
>         skb->csum = 0;
>         skb->ip_summed = CHECKSUM_PARTIAL;
>         if (attr->tcp) {
> -               thdr->check = ~tcp_v4_check(skb->len, ihdr->saddr,
> -                                           ihdr->daddr, 0);
> +               int l4len;
> +
> +               l4len = skb->tail - skb_transport_header(skb);
> +               thdr->check = ~tcp_v4_check(l4len, ihdr->saddr, ihdr->daddr, 0);
>                 skb->csum_start = skb_transport_header(skb) - skb->head;
>                 skb->csum_offset = offsetof(struct tcphdr, check);
>         } else {
> 
> Or some such?

Ah, it works now!

So, for my understanding:
- does skb_checksum_help() rely on a precalculated and integrated
  pseudo-header csum?
- And is this how typical HW-accelerated checksumming works?
- Is this why it is called CHECKSUM_PARTIAL, because only one part of the
  checksum is pre-calculated?

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next v4 0/4] net: selftest: improve test string formatting and checksum handling

[Jakub Kicinski]

On Tue, 24 Jun 2025 10:26:02 +0200 Oleksij Rempel wrote:
> On Mon, Jun 23, 2025 at 10:19:20AM -0700, Jakub Kicinski wrote:
> > On Mon, 23 Jun 2025 13:45:41 +0200 Oleksij Rempel wrote:  
> > > On Sat, Jun 21, 2025 at 06:46:00AM -0700, Jakub Kicinski wrote:  
> > > > On Fri, 20 Jun 2025 12:53:23 +0200 Oleksij Rempel wrote:  
> > > > > Let me first describe the setup where this issue was observed and my findings.
> > > > > The problem occurs on a system utilizing a Microchip DSA driver with an STMMAC
> > > > > Ethernet controller attached to the CPU port.
> > > > > 
> > > > > In the current selftest implementation, the TCP checksum validation fails,
> > > > > while the UDP test passes. The existing code prepares the skb for hardware
> > > > > checksum offload by setting skb->ip_summed = CHECKSUM_PARTIAL. For TCP, it sets
> > > > > the thdr->check field to the complement of the pseudo-header checksum, and for
> > > > > UDP, it uses udp4_hwcsum. If I understand it correct, this configuration tells
> > > > > the kernel that the hardware should perform the checksum calculation.
> > > > > 
> > > > > However, during testing, I noticed that "rx-checksumming" is enabled by default
> > > > > on the CPU port, and this leads to the TCP test failure.  Only after disabling
> > > > > "rx-checksumming" on the CPU port did the selftest pass. This suggests that the
> > > > > issue is specifically related to the hardware checksum offload mechanism in
> > > > > this particular setup. The behavior indicates that something on the path
> > > > > recalculated the checksum incorrectly.    
> > > > 
> > > > Interesting, that sounds like the smoking gun. When rx-checksumming 
> > > > is enabled the packet still reaches the stack right?    
> > > 
> > > No. It looks like this packets are just silently dropped, before they was
> > > seen by the stack. The only counter which confirms presence of this
> > > frames is HW specific mmc_rx_tcp_err. But it will be increasing even if
> > > rx-checksumming is disabled and packets are forwarded to the stack.  
> > 
> > If you happen to have the docs for the STMMAC instantiation in the SoC
> > it'd be good to check if discarding frames with bad csum can be
> > disabled. Various monitoring systems will expect the L4 checksum errors
> > to appear in nstat, not some obscure ethtool -S counter.  
> 
> Ack. I will it add to my todo.
> 
> For proper understanding of STMMAC and other drivers, here is how I currently
> understand the expected behavior on the receive path, with some open questions:
> 
> Receive Path Checksum Scenarios
> 
> * No Hardware Verification
>     * The hardware is not configured for RX checksum offload
>       or does not support the packet type, passing the packet to the driver
>       as-is.
>     * Expected driver behavior: The driver should set the packet's state to
>       `CHECKSUM_NONE`, signaling to the kernel that a software checksum
>       validation is required.
> 
> * Hardware Verifies and Reports All Frames (Ideal Linux Behavior)
>     * The hardware is configured not to drop packets with bad checksums.
>       It verifies the checksum of each packet and reports the result (good
>       or bad) in a status field on the DMA descriptor.
>     * Expected driver behavior: The driver must read the status for every
>       packet.
>         * If the hardware reports the checksum is good, the driver should set
>           the packet's state to `CHECKSUM_UNNECESSARY`.
>         * If the hardware reports the checksum is bad, the driver should set
>           the packet's state to `CHECKSUM_NONE` and still pass it to the
>           kernel.
>     * Open Questions:
>         * When the hardware reports a bad checksum in this mode, should the
>           driver increment `rx_crc_errors` immediately? Or should it only set
>           the packet's state to `CHECKSUM_NONE` and let the kernel stack find
>           the error and increment the counter, in order to avoid
>           double-counting the same error?

Driver can increment its local counter. It doesn't matter much.

But one important distinction, we're talking about layer 3 and up
checksums. IPv4 checksum, and TCP/UDP checksums. Those are not CRC.
The HW _should_ discard packets with bad CRC / Layer 2 checksum
unless the NETIF_F_RXALL feature is enabled.

> * Hardware Verifies and Drops on Error
>     * The hardware's RX checksum engine is active and configured to
>       automatically discard any packet with an incorrect checksum before it is
>       delivered to the driver.
>     * Open Questions:
> 
>         * When reporting these hardware-level drops, what is the most
>           appropriate existing standard `net_device_stats` counter to use
>           (e.g., `rx_crc_errors`, `rx_errors`)?

I'd say rx_errors, most likely to be noticed.

>         * If no existing standard counter is a good semantic fit, add new
>           standard counters?

Given this is behavior we don't want to encourage I think adding a
standard stat would send the wrong signal.

>         * If the "drop on error" feature cannot be disabled independently,
>           and reporting the error via a standard counter is not feasible,
>           does this imply that the entire RX checksum offload feature must be
>           disabled to ensure error visibility?

Probably not, users should also monitor rx_errors.

> * Hardware Provides Full Packet Checksum (`CHECKSUM_COMPLETE`)
>     * The hardware calculates a single checksum over the entire packet and
>       provides this value to the driver, without needing to parse the
>       L3/L4 headers.

Not entire, it skips the base Ethernet header (first 14 bytes)

>     * Expected driver behavior: The driver should place the checksum provided
>       by the hardware into the `skb->csum` field and set the packet's state
>       to `CHECKSUM_COMPLETE`.

Correct.

> > > > If so does the frame enter the stack with CHECKSUM_COMPLETE or
> > > > UNNECESSARY?    
> > > 
> > > If rx-checksumming is enabled and packet has supported ethertype,
> > > then CHECKSUM_UNNECESSARY will be set. Otherwise CHECKSUM_NONE.
> > >   
> > > > > When examining the loopbacked frames, I observed that the TCP checksum was
> > > > > incorrect. Upon further investigation, the xmit helper in net/dsa/tag_ksz.c
> > > > > includes the following:
> > > > > 
> > > > > if (skb->ip_summed == CHECKSUM_PARTIAL && skb_checksum_help(skb))
> > > > >     return NULL;
> > > > > 
> > > > > I assume skb_checksum_help() is intended to calculate the proper checksum when
> > > > > CHECKSUM_PARTIAL is set, indicating that the software should complete the
> > > > > checksum before handing it to the hardware. My understanding is that the STMMAC
> > > > > hardware then calculates the checksum for egress frames if CHECKSUM_PARTIAL is
> > > > > used.    
> > > > 
> > > > stmmac shouldn't touch the frame, note that skb_checksum_help() sets
> > > > skb->ip_summed = CHECKSUM_NONE; so the skb should no longer be considered
> > > > for csum offload.    
> > > 
> > > It looks like skb_checksum_help(), which is used in tag_ksz.c, generates
> > > a TCP checksum without accounting for the IP pseudo-header. The
> > > resulting checksum is then incorrect and is filtered out by the STMMAC
> > > HW on ingress  
> > 
> > The pseudo-header csum is filled in net_test_get_skb(), where it calls
> > tcp_v4_check(). But I think you're right, it's incorrect. Could you try:
> > 
> > diff --git a/net/core/selftests.c b/net/core/selftests.c
> > index 35f807ea9952..1166dd1ddb07 100644
> > --- a/net/core/selftests.c
> > +++ b/net/core/selftests.c
> > @@ -160,8 +160,10 @@ static struct sk_buff *net_test_get_skb(struct net_device *ndev,
> >         skb->csum = 0;
> >         skb->ip_summed = CHECKSUM_PARTIAL;
> >         if (attr->tcp) {
> > -               thdr->check = ~tcp_v4_check(skb->len, ihdr->saddr,
> > -                                           ihdr->daddr, 0);
> > +               int l4len;
> > +
> > +               l4len = skb->tail - skb_transport_header(skb);
> > +               thdr->check = ~tcp_v4_check(l4len, ihdr->saddr, ihdr->daddr, 0);
> >                 skb->csum_start = skb_transport_header(skb) - skb->head;
> >                 skb->csum_offset = offsetof(struct tcphdr, check);
> >         } else {
> > 
> > Or some such?  
> 
> Ah, it works now!
> 
> So, for my understanding:
> - does skb_checksum_help() rely on a precalculated and integrated
>   pseudo-header csum?
> - And is this how typical HW-accelerated checksumming works?
> - Is this why it is called CHECKSUM_PARTIAL, because only one part of the
>   checksum is pre-calculated?

IDK why it's called PARTIAL, your guess seems reasonable :)
And yes on the other questions. PARTIAL means the HW is supposed to do
a csum over a linear buffer and write it where pointed without header
parsing. Because of how UDP and TCP define the csum for them that means
the csum field has to be set to the pseudo header csum.

Let me send the fix officially.

Re: [PATCH net-next v4 0/4] net: selftest: improve test string formatting and checksum handling

[Oleksij Rempel]

On Tue, Jun 24, 2025 at 09:09:53AM -0700, Jakub Kicinski wrote:
> On Tue, 24 Jun 2025 10:26:02 +0200 Oleksij Rempel wrote:
> > On Mon, Jun 23, 2025 at 10:19:20AM -0700, Jakub Kicinski wrote:
> > > On Mon, 23 Jun 2025 13:45:41 +0200 Oleksij Rempel wrote:  
> > > > On Sat, Jun 21, 2025 at 06:46:00AM -0700, Jakub Kicinski wrote:  
> > > > > On Fri, 20 Jun 2025 12:53:23 +0200 Oleksij Rempel wrote:  
> > > > > > Let me first describe the setup where this issue was observed and my findings.
> > > > > > The problem occurs on a system utilizing a Microchip DSA driver with an STMMAC
> > > > > > Ethernet controller attached to the CPU port.
> > > > > > 
> > > > > > In the current selftest implementation, the TCP checksum validation fails,
> > > > > > while the UDP test passes. The existing code prepares the skb for hardware
> > > > > > checksum offload by setting skb->ip_summed = CHECKSUM_PARTIAL. For TCP, it sets
> > > > > > the thdr->check field to the complement of the pseudo-header checksum, and for
> > > > > > UDP, it uses udp4_hwcsum. If I understand it correct, this configuration tells
> > > > > > the kernel that the hardware should perform the checksum calculation.
> > > > > > 
> > > > > > However, during testing, I noticed that "rx-checksumming" is enabled by default
> > > > > > on the CPU port, and this leads to the TCP test failure.  Only after disabling
> > > > > > "rx-checksumming" on the CPU port did the selftest pass. This suggests that the
> > > > > > issue is specifically related to the hardware checksum offload mechanism in
> > > > > > this particular setup. The behavior indicates that something on the path
> > > > > > recalculated the checksum incorrectly.    
> > > > > 
> > > > > Interesting, that sounds like the smoking gun. When rx-checksumming 
> > > > > is enabled the packet still reaches the stack right?    
> > > > 
> > > > No. It looks like this packets are just silently dropped, before they was
> > > > seen by the stack. The only counter which confirms presence of this
> > > > frames is HW specific mmc_rx_tcp_err. But it will be increasing even if
> > > > rx-checksumming is disabled and packets are forwarded to the stack.  
> > > 
> > > If you happen to have the docs for the STMMAC instantiation in the SoC
> > > it'd be good to check if discarding frames with bad csum can be
> > > disabled. Various monitoring systems will expect the L4 checksum errors
> > > to appear in nstat, not some obscure ethtool -S counter.  
> > 
> > Ack. I will it add to my todo.
> > 
> > For proper understanding of STMMAC and other drivers, here is how I currently
> > understand the expected behavior on the receive path, with some open questions:
> > 
> > Receive Path Checksum Scenarios
> > 
> > * No Hardware Verification
> >     * The hardware is not configured for RX checksum offload
> >       or does not support the packet type, passing the packet to the driver
> >       as-is.
> >     * Expected driver behavior: The driver should set the packet's state to
> >       `CHECKSUM_NONE`, signaling to the kernel that a software checksum
> >       validation is required.
> > 
> > * Hardware Verifies and Reports All Frames (Ideal Linux Behavior)
> >     * The hardware is configured not to drop packets with bad checksums.
> >       It verifies the checksum of each packet and reports the result (good
> >       or bad) in a status field on the DMA descriptor.
> >     * Expected driver behavior: The driver must read the status for every
> >       packet.
> >         * If the hardware reports the checksum is good, the driver should set
> >           the packet's state to `CHECKSUM_UNNECESSARY`.
> >         * If the hardware reports the checksum is bad, the driver should set
> >           the packet's state to `CHECKSUM_NONE` and still pass it to the
> >           kernel.
> >     * Open Questions:
> >         * When the hardware reports a bad checksum in this mode, should the
> >           driver increment `rx_crc_errors` immediately? Or should it only set
> >           the packet's state to `CHECKSUM_NONE` and let the kernel stack find
> >           the error and increment the counter, in order to avoid
> >           double-counting the same error?
> 
> Driver can increment its local counter. It doesn't matter much.
> 
> But one important distinction, we're talking about layer 3 and up
> checksums. IPv4 checksum, and TCP/UDP checksums. Those are not CRC.
> The HW _should_ discard packets with bad CRC / Layer 2 checksum
> unless the NETIF_F_RXALL feature is enabled.
> 
> > * Hardware Verifies and Drops on Error
> >     * The hardware's RX checksum engine is active and configured to
> >       automatically discard any packet with an incorrect checksum before it is
> >       delivered to the driver.
> >     * Open Questions:
> > 
> >         * When reporting these hardware-level drops, what is the most
> >           appropriate existing standard `net_device_stats` counter to use
> >           (e.g., `rx_crc_errors`, `rx_errors`)?
> 
> I'd say rx_errors, most likely to be noticed.
> 
> >         * If no existing standard counter is a good semantic fit, add new
> >           standard counters?
> 
> Given this is behavior we don't want to encourage I think adding a
> standard stat would send the wrong signal.
> 
> >         * If the "drop on error" feature cannot be disabled independently,
> >           and reporting the error via a standard counter is not feasible,
> >           does this imply that the entire RX checksum offload feature must be
> >           disabled to ensure error visibility?
> 
> Probably not, users should also monitor rx_errors.
> 
> > * Hardware Provides Full Packet Checksum (`CHECKSUM_COMPLETE`)
> >     * The hardware calculates a single checksum over the entire packet and
> >       provides this value to the driver, without needing to parse the
> >       L3/L4 headers.
> 
> Not entire, it skips the base Ethernet header (first 14 bytes)
> 
> >     * Expected driver behavior: The driver should place the checksum provided
> >       by the hardware into the `skb->csum` field and set the packet's state
> >       to `CHECKSUM_COMPLETE`.
> 
> Correct.

Hm... at least part of this behavior can be verified with self-tests:

- Send a TCP packet with an intentionally incorrect checksum,
  ensuring its state is CHECKSUM_NONE so the transmit path doesn't change it.
- Test if we receive this packet back via the PHY loopback.
   - If received: The test checks the ip_summed status of the
     received packet.
      - A status of CHECKSUM_NONE indicates the hardware correctly passed
        the packet up without validating it.
      - A status of CHECKSUM_UNNECESSARY indicates a failure, as the hardware
        or driver incorrectly marked a bad checksum as good.
   - If not received (after a timeout): The test then checks the device's
     error statistics.
      - If the rx_errors counter has incremented
      - If the counter has not incremented, the packet was lost for an unknown
        reason, and the test fails.

What do you think?

Best Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH net-next v4 0/4] net: selftest: improve test string formatting and checksum handling

[Jakub Kicinski]

On Wed, 25 Jun 2025 07:07:42 +0200 Oleksij Rempel wrote:
> Hm... at least part of this behavior can be verified with self-tests:
> 
> - Send a TCP packet with an intentionally incorrect checksum,
>   ensuring its state is CHECKSUM_NONE so the transmit path doesn't change it.
> - Test if we receive this packet back via the PHY loopback.
>    - If received: The test checks the ip_summed status of the
>      received packet.
>       - A status of CHECKSUM_NONE indicates the hardware correctly passed
>         the packet up without validating it.

_NONE or _COMPLETE are both fine in this case.

>       - A status of CHECKSUM_UNNECESSARY indicates a failure, as the hardware
>         or driver incorrectly marked a bad checksum as good.
>    - If not received (after a timeout): The test then checks the device's
>      error statistics.
>       - If the rx_errors counter has incremented
>       - If the counter has not incremented, the packet was lost for an unknown
>         reason, and the test fails.
> 
> What do you think?

Sounds like a good idea! Not sure if I'd bother with the rx_error
handling. Hopefully the drivers can be configured to pass the packet
thru.

Re: [PATCH net-next v4 0/4] net: selftest: improve test string formatting and checksum handling

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1778 bytes --]

On 24.06.2025 09:09:53, Jakub Kicinski wrote:
> > Receive Path Checksum Scenarios

[...]

> > * Hardware Verifies and Reports All Frames (Ideal Linux Behavior)
> >     * The hardware is configured not to drop packets with bad checksums.
> >       It verifies the checksum of each packet and reports the result (good
> >       or bad) in a status field on the DMA descriptor.
> >     * Expected driver behavior: The driver must read the status for every
> >       packet.
> >         * If the hardware reports the checksum is good, the driver should set
> >           the packet's state to `CHECKSUM_UNNECESSARY`.
> >         * If the hardware reports the checksum is bad, the driver should set
> >           the packet's state to `CHECKSUM_NONE` and still pass it to the
> >           kernel.

While discussing things internally, one question came up:

Is passing packets with known bad checksums to the networking stack with
CHECKSUM_NONE, so that the checksum is recalculated in software a
potential DoS vector?

Our reasoning is as follows: Consider a system that is designed for a
certain bandwidth of network traffic and relies on the hardware to do
the checksum calculation. How much does the CPU load rise if all
checksum calculation can be force to take place on the CPU by sending
packets with broken checksums?

Is there a way to tell the network stack that the hardware/driver has
already performed the checksum calculation and that it is incorrect?

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH net-next v4 0/4] net: selftest: improve test string formatting and checksum handling

[Jakub Kicinski]

On Fri, 11 Jul 2025 10:42:52 +0200 Marc Kleine-Budde wrote:
> Is passing packets with known bad checksums to the networking stack with
> CHECKSUM_NONE, so that the checksum is recalculated in software a
> potential DoS vector?

I'm not aware of it being a DoS vector. We're talking about a basic
arithmetic sum here, not a CRC or anything math-intensive. So if we
assume the CPU is fast enough to copy this data via the socket API
to user space it is definitely fast enough to add it up and drop 
a packet.