[PATCH net-next 0/4] more virtio-net csum hardening

[PATCH net-next 0/4] more virtio-net csum hardening

[Michael S. Tsirkin]

Allowing untrusted hardware to specify csum start/offset for
CHECKSUM_PARTIAL was not a good idea, because, if it edits the packet
outside the actual checksum, and depending on the setup, the networking
stack could be looking at the packet both before and after the edit, and
seeing different results.

Previously, commit 6785011f8b16 ("ipv4: validate checksum_start before
completing checksum") validated all known instances where this
causes the most grief, but the real fix is to make sure
that either only actual checksum fields are ever edited,
or the checksum is completed before the net stack looks at it,
so it is always consistent.

This basically does this: checksum at ingress, except in a narrow list
of specified known good cases.

Fixes: f43798c27684 ("tun: Allow GSO using virtio_net_hdr")

Michael S. Tsirkin (4):
  net: add ip4_parse()
  net: use ip4_parse() in receive validation
  net: resolve uncommon partial checksums at ingress
  net: account for ingress-resolved virtio net hdr state

 drivers/net/tap.c                   |   4 +-
 drivers/net/tun.c                   |  44 +++++++-
 drivers/net/virtio_net.c            |   2 +-
 include/linux/ip.h                  |  24 +++++
 include/linux/virtio_net.h          | 161 ++++++++++++++++++++++++----
 net/bridge/br_netfilter_hooks.c     |  15 +--
 net/ipv4/ip_input.c                 |  13 +--
 net/ipv4/netfilter/nf_reject_ipv4.c |  13 +--
 net/packet/af_packet.c              |   4 +-
 9 files changed, 228 insertions(+), 52 deletions(-)

-- 
MST

[PATCH net-next 1/4] net: add ip4_parse()

[Michael S. Tsirkin]

Multiple places in the net stack open-code the same minimal IPv4 header
validation: pull the fixed header, check version 4 and IHL >= 5 (where 5
is the fixed header size in 32-bit units) - and locate the transport
header at IHL * 4 past the network header.

Add a helper for doing that: it returns the validated header and
the transport header offset, or NULL if truncated or malformed.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Assisted-by: LLM
---
 include/linux/ip.h | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/include/linux/ip.h b/include/linux/ip.h
index d11c25f5030a0..98012f6d0efb5 100644
--- a/include/linux/ip.h
+++ b/include/linux/ip.h
@@ -56,4 +56,28 @@ static inline void iph_set_totlen(struct iphdr *iph, unsigned int len)
 {
 	iph->tot_len = len <= IP_MAX_MTU ? htons(len) : 0;
 }
+
+/*
+ * Pull and minimally validate the IPv4 header at @nhoff.
+ * Returns the header and stores the transport header offset in
+ * @thoff, or NULL if truncated or malformed.  Only the fixed header
+ * is guaranteed linear; callers that need the options or the
+ * transport header must pull further.
+ */
+static inline struct iphdr *ip4_parse(struct sk_buff *skb,
+				      unsigned int nhoff,
+				      unsigned int *thoff)
+{
+	struct iphdr *iph;
+
+	if (!pskb_may_pull(skb, nhoff + sizeof(struct iphdr)))
+		return NULL;
+
+	iph = (struct iphdr *)(skb->data + nhoff);
+	if (iph->version != 4 || iph->ihl < sizeof(struct iphdr) / sizeof(u32))
+		return NULL;
+
+	*thoff = nhoff + iph->ihl * sizeof(u32);
+	return iph;
+}
 #endif	/* _LINUX_IP_H */
-- 
MST

Re: [PATCH net-next 1/4] net: add ip4_parse()

[sashiko-bot]

> Multiple places in the net stack open-code the same minimal IPv4 header
> validation: pull the fixed header, check version 4 and IHL >= 5 (where 5
> is the fixed header size in 32-bit units) - and locate the transport
> header at IHL * 4 past the network header.
> 
> Add a helper for doing that: it returns the validated header and
> the transport header offset, or NULL if truncated or malformed.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1791887415.git.mst@redhat.com?part=1

[PATCH net-next 2/4] net: use ip4_parse() in receive validation

[Michael S. Tsirkin]

Convert the sites that pull and do version and ihl checks on the ipv4
header and validate the packet transport offset to use ip4_parse.

Sites that validate a read-only copy instead of pulling - the nft
and nft_inner header_pointer validators, ip_check_defrag()'s
skb_copy_bits, the flow dissector - stay open-coded for now.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Assisted-by: LLM
---
 net/bridge/br_netfilter_hooks.c     | 15 +++++----------
 net/ipv4/ip_input.c                 | 13 +++++--------
 net/ipv4/netfilter/nf_reject_ipv4.c | 13 +++++--------
 3 files changed, 15 insertions(+), 26 deletions(-)

diff --git a/net/bridge/br_netfilter_hooks.c b/net/bridge/br_netfilter_hooks.c
index 0a394e5f43916..e89da0e415426 100644
--- a/net/bridge/br_netfilter_hooks.c
+++ b/net/bridge/br_netfilter_hooks.c
@@ -206,18 +206,13 @@ static inline void nf_bridge_pull_encap_header_rcsum(struct sk_buff *skb)
 static int br_validate_ipv4(struct net *net, struct sk_buff *skb)
 {
 	const struct iphdr *iph;
-	u32 len;
+	u32 len, thoff;
 
-	if (!pskb_may_pull(skb, sizeof(struct iphdr)))
+	iph = ip4_parse(skb, 0, &thoff);
+	if (!iph)
 		goto inhdr_error;
 
-	iph = ip_hdr(skb);
-
-	/* Basic sanity checks */
-	if (iph->ihl < 5 || iph->version != 4)
-		goto inhdr_error;
-
-	if (!pskb_may_pull(skb, iph->ihl*4))
+	if (!pskb_may_pull(skb, thoff))
 		goto inhdr_error;
 
 	iph = ip_hdr(skb);
@@ -228,7 +223,7 @@ static int br_validate_ipv4(struct net *net, struct sk_buff *skb)
 	if (skb->len < len) {
 		__IP_INC_STATS(net, IPSTATS_MIB_INTRUNCATEDPKTS);
 		goto drop;
-	} else if (len < (iph->ihl*4))
+	} else if (len < thoff)
 		goto inhdr_error;
 
 	if (pskb_trim_rcsum(skb, len)) {
diff --git a/net/ipv4/ip_input.c b/net/ipv4/ip_input.c
index 9860178752b8c..5adf01763e857 100644
--- a/net/ipv4/ip_input.c
+++ b/net/ipv4/ip_input.c
@@ -500,7 +500,7 @@ static struct sk_buff *ip_rcv_core(struct sk_buff *skb, struct net *net)
 {
 	const struct iphdr *iph;
 	int drop_reason;
-	u32 len;
+	u32 len, thoff;
 
 	/* When the interface is in promisc. mode, drop all the crap
 	 * that it receives, do not try to analyse it.
@@ -520,10 +520,6 @@ static struct sk_buff *ip_rcv_core(struct sk_buff *skb, struct net *net)
 	}
 
 	drop_reason = SKB_DROP_REASON_NOT_SPECIFIED;
-	if (!pskb_may_pull(skb, sizeof(struct iphdr)))
-		goto inhdr_error;
-
-	iph = ip_hdr(skb);
 
 	/*
 	 *	RFC1122: 3.2.1.2 MUST silently discard any IP frame that fails the checksum.
@@ -536,7 +532,8 @@ static struct sk_buff *ip_rcv_core(struct sk_buff *skb, struct net *net)
 	 *	4.	Doesn't have a bogus length
 	 */
 
-	if (iph->ihl < 5 || iph->version != 4)
+	iph = ip4_parse(skb, 0, &thoff);
+	if (!iph)
 		goto inhdr_error;
 
 	BUILD_BUG_ON(IPSTATS_MIB_ECT1PKTS != IPSTATS_MIB_NOECTPKTS + INET_ECN_ECT_1);
@@ -546,7 +543,7 @@ static struct sk_buff *ip_rcv_core(struct sk_buff *skb, struct net *net)
 		       IPSTATS_MIB_NOECTPKTS + (iph->tos & INET_ECN_MASK),
 		       max_t(unsigned short, 1, skb_shinfo(skb)->gso_segs));
 
-	if (!pskb_may_pull(skb, iph->ihl*4))
+	if (!pskb_may_pull(skb, thoff))
 		goto inhdr_error;
 
 	iph = ip_hdr(skb);
@@ -559,7 +556,7 @@ static struct sk_buff *ip_rcv_core(struct sk_buff *skb, struct net *net)
 		drop_reason = SKB_DROP_REASON_PKT_TOO_SMALL;
 		__IP_INC_STATS(net, IPSTATS_MIB_INTRUNCATEDPKTS);
 		goto drop;
-	} else if (len < (iph->ihl*4))
+	} else if (len < thoff)
 		goto inhdr_error;
 
 	/* Our transport medium may have padded the buffer out. Now we know it
diff --git a/net/ipv4/netfilter/nf_reject_ipv4.c b/net/ipv4/netfilter/nf_reject_ipv4.c
index 59ec465a9df98..42d639854ddf1 100644
--- a/net/ipv4/netfilter/nf_reject_ipv4.c
+++ b/net/ipv4/netfilter/nf_reject_ipv4.c
@@ -25,22 +25,19 @@ nf_reject_ip_tcphdr_get(struct sk_buff *oldskb,
 static int nf_reject_iphdr_validate(struct sk_buff *skb)
 {
 	struct iphdr *iph;
-	u32 len;
+	u32 len, thoff;
 
-	if (!pskb_may_pull(skb, sizeof(struct iphdr)))
-		return 0;
-
-	iph = ip_hdr(skb);
-	if (iph->ihl < 5 || iph->version != 4)
+	iph = ip4_parse(skb, 0, &thoff);
+	if (!iph)
 		return 0;
 
 	len = ntohs(iph->tot_len);
 	if (skb->len < len)
 		return 0;
-	else if (len < (iph->ihl*4))
+	else if (len < thoff)
 		return 0;
 
-	if (!pskb_may_pull(skb, iph->ihl*4))
+	if (!pskb_may_pull(skb, thoff))
 		return 0;
 
 	return 1;
-- 
MST

Re: [PATCH net-next 2/4] net: use ip4_parse() in receive validation

[sashiko-bot]

> Convert the sites that pull and do version and ihl checks on the ipv4
> header and validate the packet transport offset to use ip4_parse.
> 
> Sites that validate a read-only copy instead of pulling - the nft
> and nft_inner header_pointer validators, ip_check_defrag()'s
> skb_copy_bits, the flow dissector - stay open-coded for now.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1791887415.git.mst@redhat.com?part=2

[PATCH net-next 3/4] net: resolve uncommon partial checksums at ingress

[Michael S. Tsirkin]

A virtio net header can place CHECKSUM_PARTIAL at any in-packet
offset.  If that state is retained, later checksum completion can
modify the packet after protocol processing has made decisions from it.

Let's fix that: let's validate csum_start and csum_offset and only keep
CHECKSUM_PARTIAL when csum_start and csum_offset match a parsed TCP or
UDP header, while handling VLAN headers, IPv4 options and IPv6 extension
headers.  Complete other non-GSO checksums immediately, before any
protocol processing runs.

We can't easily "complete the checksum" for GSO we don't recognize - GSO
assumes knowledge of the protocol. We could segment ignoring the
checksum then do whatever csum_start and csum_offset say, but that
seems like an overkill for something that should happen.

UDP tunnel headers get the same handling on the inner header.

Packets without VIRTIO_NET_HDR_F_NEEDS_CSUM are unaffected.

Suggested-by: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Assisted-by: LLM
---
 include/linux/virtio_net.h | 151 ++++++++++++++++++++++++++++++++-----
 1 file changed, 133 insertions(+), 18 deletions(-)

diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
index d6466f96cdd00..b181183118b3a 100644
--- a/include/linux/virtio_net.h
+++ b/include/linux/virtio_net.h
@@ -5,7 +5,11 @@
 #include <linux/if_vlan.h>
 #include <linux/ip.h>
 #include <linux/ipv6.h>
+#include <linux/netdevice.h>
 #include <linux/udp.h>
+#include <net/gso.h>
+#include <net/ip.h>
+#include <net/ipv6.h>
 #include <net/tcp.h>
 #include <uapi/linux/tcp.h>
 #include <uapi/linux/virtio_net.h>
@@ -48,15 +52,111 @@ static inline int virtio_net_hdr_set_proto(struct sk_buff *skb,
 	return 0;
 }
 
+/* Does checksum metadata match a known TCP or UDP header? */
+static inline bool
+virtio_net_csum_partial_ok_at_offset(struct sk_buff *skb, u8 hdr_gso_type,
+				     unsigned int nhoff, __be16 proto)
+{
+	unsigned int thoff;
+	int ip_proto;
+
+	if (proto == htons(ETH_P_IP)) {
+		const struct iphdr *iph = ip4_parse(skb, nhoff, &thoff);
+
+		if (!iph || ip_is_fragment(iph))
+			return false;
+		ip_proto = iph->protocol;
+	} else if (proto == htons(ETH_P_IPV6)) {
+		const struct ipv6hdr *iph;
+		int flags = 0;
+
+		if (!pskb_may_pull(skb, nhoff + sizeof(*iph)))
+			return false;
+		iph = (const void *)(skb->data + nhoff);
+		if (iph->version != 6)
+			return false;
+		thoff = nhoff + sizeof(*iph);
+		ip_proto = iph->nexthdr;
+		if (ip_proto != IPPROTO_TCP && ip_proto != IPPROTO_UDP) {
+			thoff = nhoff;
+			ip_proto = ipv6_find_hdr(skb, &thoff, -1, NULL,
+						 &flags);
+			if (ip_proto < 0 || flags & IP6_FH_F_FRAG)
+				return false;
+		}
+	} else {
+		return false;
+	}
+
+	if (skb_checksum_start_offset(skb) != thoff)
+		return false;
+
+	switch (ip_proto) {
+	case IPPROTO_TCP:
+		if (skb->csum_offset != offsetof(struct tcphdr, check) ||
+		    !pskb_may_pull(skb, thoff + sizeof(struct tcphdr)))
+			return false;
+		break;
+	case IPPROTO_UDP:
+		if (skb->csum_offset != offsetof(struct udphdr, check) ||
+		    !pskb_may_pull(skb, thoff + sizeof(struct udphdr)))
+			return false;
+		break;
+	default:
+		return false;
+	}
+
+	switch (hdr_gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {
+	case VIRTIO_NET_HDR_GSO_NONE:
+		return true;
+	case VIRTIO_NET_HDR_GSO_TCPV4:
+		return proto == htons(ETH_P_IP) && ip_proto == IPPROTO_TCP;
+	case VIRTIO_NET_HDR_GSO_TCPV6:
+		return proto == htons(ETH_P_IPV6) && ip_proto == IPPROTO_TCP;
+	case VIRTIO_NET_HDR_GSO_UDP:
+	case VIRTIO_NET_HDR_GSO_UDP_L4:
+		return ip_proto == IPPROTO_UDP;
+	default:
+		return false;
+	}
+}
+
+static inline bool
+virtio_net_csum_partial_ok(struct sk_buff *skb, u8 hdr_gso_type)
+{
+	unsigned int nhoff;
+	__be16 proto;
+
+	if (skb->dev->type == ARPHRD_ETHER) {
+		if (!pskb_may_pull(skb, ETH_HLEN))
+			return false;
+		nhoff = ETH_HLEN;
+		proto = eth_hdr(skb)->h_proto;
+		if (eth_type_vlan(proto)) {
+			proto = __vlan_get_protocol(skb, proto, &nhoff);
+			if (!proto)
+				return false;
+		}
+	} else {
+		nhoff = skb_network_offset(skb);
+		proto = skb->protocol;
+	}
+
+	return virtio_net_csum_partial_ok_at_offset(skb, hdr_gso_type, nhoff,
+						   proto);
+}
+
 static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,
 					  const struct virtio_net_hdr *hdr,
-					  bool little_endian, u8 hdr_gso_type)
+					  bool little_endian, u8 hdr_gso_type,
+					  bool need_csum_check)
 {
-	unsigned int nh_min_len = sizeof(struct iphdr);
+	typeof(skb->transport_header) old_transport = skb->transport_header;
 	unsigned int gso_type = 0;
 	unsigned int thlen = 0;
 	unsigned int p_off = 0;
 	unsigned int ip_proto;
+	bool csum_partial_ok = true;
 
 	if (hdr_gso_type != VIRTIO_NET_HDR_GSO_NONE) {
 		switch (hdr_gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {
@@ -69,7 +169,6 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,
 			gso_type = SKB_GSO_TCPV6;
 			ip_proto = IPPROTO_TCP;
 			thlen = sizeof(struct tcphdr);
-			nh_min_len = sizeof(struct ipv6hdr);
 			break;
 		case VIRTIO_NET_HDR_GSO_UDP:
 			gso_type = SKB_GSO_UDP;
@@ -104,11 +203,11 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,
 
 		if (!skb_partial_csum_set(skb, start, off))
 			return -EINVAL;
-		if (skb_transport_offset(skb) < nh_min_len)
-			return -EINVAL;
+		if (need_csum_check)
+			csum_partial_ok =
+				virtio_net_csum_partial_ok(skb, hdr_gso_type);
 
-		nh_min_len = skb_transport_offset(skb);
-		p_off = nh_min_len + thlen;
+		p_off = skb_transport_offset(skb) + thlen;
 		if (!pskb_may_pull(skb, p_off))
 			return -EINVAL;
 	} else if (gso_type) {
@@ -158,8 +257,6 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,
 		case SKB_GSO_UDP_L4:
 			if (!(hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM))
 				return -EINVAL;
-			if (skb->csum_offset != offsetof(struct udphdr, check))
-				return -EINVAL;
 			if (skb->len - p_off > gso_size * UDP_MAX_SEGMENTS)
 				return -EINVAL;
 			if (gso_type != SKB_GSO_UDP_L4)
@@ -167,10 +264,6 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,
 			break;
 		case SKB_GSO_TCPV4:
 		case SKB_GSO_TCPV6:
-			if (skb->ip_summed == CHECKSUM_PARTIAL &&
-			    skb->csum_offset != offsetof(struct tcphdr, check))
-				return -EINVAL;
-
 			BUILD_BUG_ON(TCP_MIN_GSO_SIZE * GSO_MAX_SEGS < GSO_MAX_SIZE);
 			gso_size = max(gso_size, TCP_MIN_GSO_SIZE);
 			break;
@@ -191,6 +284,17 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,
 		}
 	}
 
+	if ((hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) && !csum_partial_ok) {
+		int err;
+
+		if (skb_is_gso(skb))
+			return -EINVAL;
+
+		err = skb_checksum_help(skb);
+		skb->transport_header = old_transport;
+		return err;
+	}
+
 	return 0;
 }
 
@@ -198,7 +302,8 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
 					const struct virtio_net_hdr *hdr,
 					bool little_endian)
 {
-	return __virtio_net_hdr_to_skb(skb, hdr, little_endian, hdr->gso_type);
+	return __virtio_net_hdr_to_skb(skb, hdr, little_endian,
+				       hdr->gso_type, true);
 }
 
 /* This function must be called after virtio_net_hdr_from_skb(). */
@@ -293,6 +398,7 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
 	unsigned int inner_nh, outer_th, inner_th;
 	unsigned int inner_l3min, outer_l3min;
 	u8 gso_inner_type, gso_tunnel_type;
+	__be16 inner_proto;
 	bool outer_isv6, inner_isv6;
 	int ret;
 
@@ -334,7 +440,7 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
 
 	/* Let the basic parsing deal with plain GSO features. */
 	ret = __virtio_net_hdr_to_skb(skb, hdr, true,
-				      hdr->gso_type & ~gso_tunnel_type);
+				      hdr->gso_type & ~gso_tunnel_type, false);
 	if (ret)
 		return ret;
 
@@ -352,8 +458,8 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
 			return -EINVAL;
 	}
 
-	skb_set_inner_protocol(skb, inner_isv6 ? htons(ETH_P_IPV6) :
-						 htons(ETH_P_IP));
+	inner_proto = inner_isv6 ? htons(ETH_P_IPV6) : htons(ETH_P_IP);
+	skb_set_inner_protocol(skb, inner_proto);
 	if (hdr->flags & VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM) {
 		if (!tnl_csum_negotiated)
 			return -EINVAL;
@@ -368,7 +474,16 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
 	skb->inner_mac_header = inner_nh + skb_headroom(skb);
 	skb->transport_header = outer_th + skb_headroom(skb);
 	skb->encapsulation = 1;
-	return 0;
+
+	if (virtio_net_csum_partial_ok_at_offset(skb, gso_inner_type, inner_nh,
+					       inner_proto))
+		return 0;
+
+	if (skb_is_gso(skb))
+		return -EINVAL;
+
+	ret = skb_checksum_help(skb);
+	return ret ?: 1;
 }
 
 /* Checksum-related fields validation for the driver */
-- 
MST

Re: [PATCH net-next 3/4] net: resolve uncommon partial checksums at ingress

[Michael S. Tsirkin]

On Tue, Oct 13, 2026 at 06:58:41AM -0400, Michael S. Tsirkin wrote:
> A virtio net header can place CHECKSUM_PARTIAL at any in-packet
> offset.  If that state is retained, later checksum completion can
> modify the packet after protocol processing has made decisions from it.
> 
> Let's fix that: let's validate csum_start and csum_offset and only keep
> CHECKSUM_PARTIAL when csum_start and csum_offset match a parsed TCP or
> UDP header, while handling VLAN headers, IPv4 options and IPv6 extension
> headers.  Complete other non-GSO checksums immediately, before any
> protocol processing runs.
> 
> We can't easily "complete the checksum" for GSO we don't recognize - GSO
> assumes knowledge of the protocol. We could segment ignoring the
> checksum then do whatever csum_start and csum_offset say, but that
> seems like an overkill for something that should happen.
> 
> UDP tunnel headers get the same handling on the inner header.
> 
> Packets without VIRTIO_NET_HDR_F_NEEDS_CSUM are unaffected.
> 
> Suggested-by: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Assisted-by: LLM


I forgot reported-by tags:

Reported-by: Paulos Yibelo <habte.yibelo@gmail.com>
Reported-by: Chris Arges <carges@cloudflare.com>


> ---
>  include/linux/virtio_net.h | 151 ++++++++++++++++++++++++++++++++-----
>  1 file changed, 133 insertions(+), 18 deletions(-)
> 
> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index d6466f96cdd00..b181183118b3a 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h
> @@ -5,7 +5,11 @@
>  #include <linux/if_vlan.h>
>  #include <linux/ip.h>
>  #include <linux/ipv6.h>
> +#include <linux/netdevice.h>
>  #include <linux/udp.h>
> +#include <net/gso.h>
> +#include <net/ip.h>
> +#include <net/ipv6.h>
>  #include <net/tcp.h>
>  #include <uapi/linux/tcp.h>
>  #include <uapi/linux/virtio_net.h>
> @@ -48,15 +52,111 @@ static inline int virtio_net_hdr_set_proto(struct sk_buff *skb,
>  	return 0;
>  }
>  
> +/* Does checksum metadata match a known TCP or UDP header? */
> +static inline bool
> +virtio_net_csum_partial_ok_at_offset(struct sk_buff *skb, u8 hdr_gso_type,
> +				     unsigned int nhoff, __be16 proto)
> +{
> +	unsigned int thoff;
> +	int ip_proto;
> +
> +	if (proto == htons(ETH_P_IP)) {
> +		const struct iphdr *iph = ip4_parse(skb, nhoff, &thoff);
> +
> +		if (!iph || ip_is_fragment(iph))
> +			return false;
> +		ip_proto = iph->protocol;
> +	} else if (proto == htons(ETH_P_IPV6)) {
> +		const struct ipv6hdr *iph;
> +		int flags = 0;
> +
> +		if (!pskb_may_pull(skb, nhoff + sizeof(*iph)))
> +			return false;
> +		iph = (const void *)(skb->data + nhoff);
> +		if (iph->version != 6)
> +			return false;
> +		thoff = nhoff + sizeof(*iph);
> +		ip_proto = iph->nexthdr;
> +		if (ip_proto != IPPROTO_TCP && ip_proto != IPPROTO_UDP) {
> +			thoff = nhoff;
> +			ip_proto = ipv6_find_hdr(skb, &thoff, -1, NULL,
> +						 &flags);
> +			if (ip_proto < 0 || flags & IP6_FH_F_FRAG)
> +				return false;
> +		}
> +	} else {
> +		return false;
> +	}
> +
> +	if (skb_checksum_start_offset(skb) != thoff)
> +		return false;
> +
> +	switch (ip_proto) {
> +	case IPPROTO_TCP:
> +		if (skb->csum_offset != offsetof(struct tcphdr, check) ||
> +		    !pskb_may_pull(skb, thoff + sizeof(struct tcphdr)))
> +			return false;
> +		break;
> +	case IPPROTO_UDP:
> +		if (skb->csum_offset != offsetof(struct udphdr, check) ||
> +		    !pskb_may_pull(skb, thoff + sizeof(struct udphdr)))
> +			return false;
> +		break;
> +	default:
> +		return false;
> +	}
> +
> +	switch (hdr_gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {
> +	case VIRTIO_NET_HDR_GSO_NONE:
> +		return true;
> +	case VIRTIO_NET_HDR_GSO_TCPV4:
> +		return proto == htons(ETH_P_IP) && ip_proto == IPPROTO_TCP;
> +	case VIRTIO_NET_HDR_GSO_TCPV6:
> +		return proto == htons(ETH_P_IPV6) && ip_proto == IPPROTO_TCP;
> +	case VIRTIO_NET_HDR_GSO_UDP:
> +	case VIRTIO_NET_HDR_GSO_UDP_L4:
> +		return ip_proto == IPPROTO_UDP;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static inline bool
> +virtio_net_csum_partial_ok(struct sk_buff *skb, u8 hdr_gso_type)
> +{
> +	unsigned int nhoff;
> +	__be16 proto;
> +
> +	if (skb->dev->type == ARPHRD_ETHER) {
> +		if (!pskb_may_pull(skb, ETH_HLEN))
> +			return false;
> +		nhoff = ETH_HLEN;
> +		proto = eth_hdr(skb)->h_proto;
> +		if (eth_type_vlan(proto)) {
> +			proto = __vlan_get_protocol(skb, proto, &nhoff);
> +			if (!proto)
> +				return false;
> +		}
> +	} else {
> +		nhoff = skb_network_offset(skb);
> +		proto = skb->protocol;
> +	}
> +
> +	return virtio_net_csum_partial_ok_at_offset(skb, hdr_gso_type, nhoff,
> +						   proto);
> +}
> +
>  static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,
>  					  const struct virtio_net_hdr *hdr,
> -					  bool little_endian, u8 hdr_gso_type)
> +					  bool little_endian, u8 hdr_gso_type,
> +					  bool need_csum_check)
>  {
> -	unsigned int nh_min_len = sizeof(struct iphdr);
> +	typeof(skb->transport_header) old_transport = skb->transport_header;
>  	unsigned int gso_type = 0;
>  	unsigned int thlen = 0;
>  	unsigned int p_off = 0;
>  	unsigned int ip_proto;
> +	bool csum_partial_ok = true;
>  
>  	if (hdr_gso_type != VIRTIO_NET_HDR_GSO_NONE) {
>  		switch (hdr_gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {
> @@ -69,7 +169,6 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,
>  			gso_type = SKB_GSO_TCPV6;
>  			ip_proto = IPPROTO_TCP;
>  			thlen = sizeof(struct tcphdr);
> -			nh_min_len = sizeof(struct ipv6hdr);
>  			break;
>  		case VIRTIO_NET_HDR_GSO_UDP:
>  			gso_type = SKB_GSO_UDP;
> @@ -104,11 +203,11 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,
>  
>  		if (!skb_partial_csum_set(skb, start, off))
>  			return -EINVAL;
> -		if (skb_transport_offset(skb) < nh_min_len)
> -			return -EINVAL;
> +		if (need_csum_check)
> +			csum_partial_ok =
> +				virtio_net_csum_partial_ok(skb, hdr_gso_type);
>  
> -		nh_min_len = skb_transport_offset(skb);
> -		p_off = nh_min_len + thlen;
> +		p_off = skb_transport_offset(skb) + thlen;
>  		if (!pskb_may_pull(skb, p_off))
>  			return -EINVAL;
>  	} else if (gso_type) {
> @@ -158,8 +257,6 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,
>  		case SKB_GSO_UDP_L4:
>  			if (!(hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM))
>  				return -EINVAL;
> -			if (skb->csum_offset != offsetof(struct udphdr, check))
> -				return -EINVAL;
>  			if (skb->len - p_off > gso_size * UDP_MAX_SEGMENTS)
>  				return -EINVAL;
>  			if (gso_type != SKB_GSO_UDP_L4)
> @@ -167,10 +264,6 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,
>  			break;
>  		case SKB_GSO_TCPV4:
>  		case SKB_GSO_TCPV6:
> -			if (skb->ip_summed == CHECKSUM_PARTIAL &&
> -			    skb->csum_offset != offsetof(struct tcphdr, check))
> -				return -EINVAL;
> -
>  			BUILD_BUG_ON(TCP_MIN_GSO_SIZE * GSO_MAX_SEGS < GSO_MAX_SIZE);
>  			gso_size = max(gso_size, TCP_MIN_GSO_SIZE);
>  			break;
> @@ -191,6 +284,17 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,
>  		}
>  	}
>  
> +	if ((hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) && !csum_partial_ok) {
> +		int err;
> +
> +		if (skb_is_gso(skb))
> +			return -EINVAL;
> +
> +		err = skb_checksum_help(skb);
> +		skb->transport_header = old_transport;
> +		return err;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -198,7 +302,8 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
>  					const struct virtio_net_hdr *hdr,
>  					bool little_endian)
>  {
> -	return __virtio_net_hdr_to_skb(skb, hdr, little_endian, hdr->gso_type);
> +	return __virtio_net_hdr_to_skb(skb, hdr, little_endian,
> +				       hdr->gso_type, true);
>  }
>  
>  /* This function must be called after virtio_net_hdr_from_skb(). */
> @@ -293,6 +398,7 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
>  	unsigned int inner_nh, outer_th, inner_th;
>  	unsigned int inner_l3min, outer_l3min;
>  	u8 gso_inner_type, gso_tunnel_type;
> +	__be16 inner_proto;
>  	bool outer_isv6, inner_isv6;
>  	int ret;
>  
> @@ -334,7 +440,7 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
>  
>  	/* Let the basic parsing deal with plain GSO features. */
>  	ret = __virtio_net_hdr_to_skb(skb, hdr, true,
> -				      hdr->gso_type & ~gso_tunnel_type);
> +				      hdr->gso_type & ~gso_tunnel_type, false);
>  	if (ret)
>  		return ret;
>  
> @@ -352,8 +458,8 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
>  			return -EINVAL;
>  	}
>  
> -	skb_set_inner_protocol(skb, inner_isv6 ? htons(ETH_P_IPV6) :
> -						 htons(ETH_P_IP));
> +	inner_proto = inner_isv6 ? htons(ETH_P_IPV6) : htons(ETH_P_IP);
> +	skb_set_inner_protocol(skb, inner_proto);
>  	if (hdr->flags & VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM) {
>  		if (!tnl_csum_negotiated)
>  			return -EINVAL;
> @@ -368,7 +474,16 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
>  	skb->inner_mac_header = inner_nh + skb_headroom(skb);
>  	skb->transport_header = outer_th + skb_headroom(skb);
>  	skb->encapsulation = 1;
> -	return 0;
> +
> +	if (virtio_net_csum_partial_ok_at_offset(skb, gso_inner_type, inner_nh,
> +					       inner_proto))
> +		return 0;
> +
> +	if (skb_is_gso(skb))
> +		return -EINVAL;
> +
> +	ret = skb_checksum_help(skb);
> +	return ret ?: 1;
>  }
>  
>  /* Checksum-related fields validation for the driver */
> -- 
> MST
> 

Re: [PATCH net-next 3/4] net: resolve uncommon partial checksums at ingress

[Willem de Bruijn]

On Tue, Oct 13, 2026 at 5:06 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, Oct 13, 2026 at 06:58:41AM -0400, Michael S. Tsirkin wrote:
> > A virtio net header can place CHECKSUM_PARTIAL at any in-packet
> > offset.  If that state is retained, later checksum completion can
> > modify the packet after protocol processing has made decisions from it.
> >
> > Let's fix that: let's validate csum_start and csum_offset and only keep
> > CHECKSUM_PARTIAL when csum_start and csum_offset match a parsed TCP or
> > UDP header, while handling VLAN headers, IPv4 options and IPv6 extension
> > headers.  Complete other non-GSO checksums immediately, before any
> > protocol processing runs.
> >
> > We can't easily "complete the checksum" for GSO we don't recognize - GSO
> > assumes knowledge of the protocol. We could segment ignoring the
> > checksum then do whatever csum_start and csum_offset say, but that
> > seems like an overkill for something that should happen.
> >
> > UDP tunnel headers get the same handling on the inner header.
> >
> > Packets without VIRTIO_NET_HDR_F_NEEDS_CSUM are unaffected.
> >
> > Suggested-by: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > Assisted-by: LLM
>
>
> I forgot reported-by tags:
>
> Reported-by: Paulos Yibelo <habte.yibelo@gmail.com>
> Reported-by: Chris Arges <carges@cloudflare.com>
>
>
> > ---
> >  include/linux/virtio_net.h | 151 ++++++++++++++++++++++++++++++++-----
> >  1 file changed, 133 insertions(+), 18 deletions(-)
> >
> > diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> > index d6466f96cdd00..b181183118b3a 100644
> > --- a/include/linux/virtio_net.h
> > +++ b/include/linux/virtio_net.h
> > @@ -5,7 +5,11 @@
> >  #include <linux/if_vlan.h>
> >  #include <linux/ip.h>
> >  #include <linux/ipv6.h>
> > +#include <linux/netdevice.h>
> >  #include <linux/udp.h>
> > +#include <net/gso.h>
> > +#include <net/ip.h>
> > +#include <net/ipv6.h>
> >  #include <net/tcp.h>
> >  #include <uapi/linux/tcp.h>
> >  #include <uapi/linux/virtio_net.h>
> > @@ -48,15 +52,111 @@ static inline int virtio_net_hdr_set_proto(struct sk_buff *skb,
> >       return 0;
> >  }
> >
> > +/* Does checksum metadata match a known TCP or UDP header? */
> > +static inline bool
> > +virtio_net_csum_partial_ok_at_offset(struct sk_buff *skb, u8 hdr_gso_type,
> > +                                  unsigned int nhoff, __be16 proto)
> > +{
> > +     unsigned int thoff;
> > +     int ip_proto;
> > +
> > +     if (proto == htons(ETH_P_IP)) {
> > +             const struct iphdr *iph = ip4_parse(skb, nhoff, &thoff);
> > +
> > +             if (!iph || ip_is_fragment(iph))
> > +                     return false;
> > +             ip_proto = iph->protocol;
> > +     } else if (proto == htons(ETH_P_IPV6)) {
> > +             const struct ipv6hdr *iph;
> > +             int flags = 0;
> > +
> > +             if (!pskb_may_pull(skb, nhoff + sizeof(*iph)))
> > +                     return false;
> > +             iph = (const void *)(skb->data + nhoff);
> > +             if (iph->version != 6)
> > +                     return false;
> > +             thoff = nhoff + sizeof(*iph);
> > +             ip_proto = iph->nexthdr;
> > +             if (ip_proto != IPPROTO_TCP && ip_proto != IPPROTO_UDP) {
> > +                     thoff = nhoff;
> > +                     ip_proto = ipv6_find_hdr(skb, &thoff, -1, NULL,
> > +                                              &flags);

Skip this rare case of extension headers?

> > +                     if (ip_proto < 0 || flags & IP6_FH_F_FRAG)
> > +                             return false;
> > +             }
> > +     } else {
> > +             return false;
> > +     }
> > +

> >  static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,
> >                                         const struct virtio_net_hdr *hdr,
> > -                                       bool little_endian, u8 hdr_gso_type)
> > +                                       bool little_endian, u8 hdr_gso_type,
> > +                                       bool need_csum_check)
> >  {
> > -     unsigned int nh_min_len = sizeof(struct iphdr);
> > +     typeof(skb->transport_header) old_transport = skb->transport_header;
> >       unsigned int gso_type = 0;
> >       unsigned int thlen = 0;
> >       unsigned int p_off = 0;
> >       unsigned int ip_proto;
> > +     bool csum_partial_ok = true;
> >
> >       if (hdr_gso_type != VIRTIO_NET_HDR_GSO_NONE) {
> >               switch (hdr_gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {
> > @@ -69,7 +169,6 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,
> >                       gso_type = SKB_GSO_TCPV6;
> >                       ip_proto = IPPROTO_TCP;
> >                       thlen = sizeof(struct tcphdr);
> > -                     nh_min_len = sizeof(struct ipv6hdr);
> >                       break;
> >               case VIRTIO_NET_HDR_GSO_UDP:
> >                       gso_type = SKB_GSO_UDP;
> > @@ -104,11 +203,11 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,
> >
> >               if (!skb_partial_csum_set(skb, start, off))
> >                       return -EINVAL;
> > -             if (skb_transport_offset(skb) < nh_min_len)
> > -                     return -EINVAL;
> > +             if (need_csum_check)
> > +                     csum_partial_ok =
> > +                             virtio_net_csum_partial_ok(skb, hdr_gso_type);
> >
> > -             nh_min_len = skb_transport_offset(skb);
> > -             p_off = nh_min_len + thlen;
> > +             p_off = skb_transport_offset(skb) + thlen;
> >               if (!pskb_may_pull(skb, p_off))
> >                       return -EINVAL;
> >       } else if (gso_type) {
> > @@ -158,8 +257,6 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,
> >               case SKB_GSO_UDP_L4:
> >                       if (!(hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM))
> >                               return -EINVAL;
> > -                     if (skb->csum_offset != offsetof(struct udphdr, check))
> > -                             return -EINVAL;
> >                       if (skb->len - p_off > gso_size * UDP_MAX_SEGMENTS)
> >                               return -EINVAL;
> >                       if (gso_type != SKB_GSO_UDP_L4)
> > @@ -167,10 +264,6 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,
> >                       break;
> >               case SKB_GSO_TCPV4:
> >               case SKB_GSO_TCPV6:
> > -                     if (skb->ip_summed == CHECKSUM_PARTIAL &&
> > -                         skb->csum_offset != offsetof(struct tcphdr, check))
> > -                             return -EINVAL;
> > -
> >                       BUILD_BUG_ON(TCP_MIN_GSO_SIZE * GSO_MAX_SEGS < GSO_MAX_SIZE);
> >                       gso_size = max(gso_size, TCP_MIN_GSO_SIZE);
> >                       break;
> > @@ -191,6 +284,17 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,
> >               }
> >       }
> >
> > +     if ((hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) && !csum_partial_ok) {
> > +             int err;
> > +
> > +             if (skb_is_gso(skb))
> > +                     return -EINVAL;
> > +
> > +             err = skb_checksum_help(skb);

Instead of this variable csum_partial_ok and this branch, could
virtio_net_csum_partial_ok just call skb_checksum_help directly and
drop VIRTIO_NET_HDR_F_NEEDS_CSUM?

> > +             skb->transport_header = old_transport;
> > +             return err;
> > +     }
> > +
> >       return 0;
> >  }
> >
> > @@ -198,7 +302,8 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
> >                                       const struct virtio_net_hdr *hdr,
> >                                       bool little_endian)
> >  {
> > -     return __virtio_net_hdr_to_skb(skb, hdr, little_endian, hdr->gso_type);
> > +     return __virtio_net_hdr_to_skb(skb, hdr, little_endian,
> > +                                    hdr->gso_type, true);

Would be great if we can avoid the extra arg and having to update the callers.

> >  }
> >
> >  /* This function must be called after virtio_net_hdr_from_skb(). */
> > @@ -293,6 +398,7 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
> >       unsigned int inner_nh, outer_th, inner_th;
> >       unsigned int inner_l3min, outer_l3min;
> >       u8 gso_inner_type, gso_tunnel_type;
> > +     __be16 inner_proto;
> >       bool outer_isv6, inner_isv6;
> >       int ret;
> >
> > @@ -334,7 +440,7 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
> >
> >       /* Let the basic parsing deal with plain GSO features. */
> >       ret = __virtio_net_hdr_to_skb(skb, hdr, true,
> > -                                   hdr->gso_type & ~gso_tunnel_type);
> > +                                   hdr->gso_type & ~gso_tunnel_type, false);
> >       if (ret)
> >               return ret;
> >
> > @@ -352,8 +458,8 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
> >                       return -EINVAL;
> >       }
> >
> > -     skb_set_inner_protocol(skb, inner_isv6 ? htons(ETH_P_IPV6) :
> > -                                              htons(ETH_P_IP));
> > +     inner_proto = inner_isv6 ? htons(ETH_P_IPV6) : htons(ETH_P_IP);
> > +     skb_set_inner_protocol(skb, inner_proto);

Can drop

> >       if (hdr->flags & VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM) {
> >               if (!tnl_csum_negotiated)
> >                       return -EINVAL;
> > @@ -368,7 +474,16 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
> >       skb->inner_mac_header = inner_nh + skb_headroom(skb);
> >       skb->transport_header = outer_th + skb_headroom(skb);
> >       skb->encapsulation = 1;
> > -     return 0;
> > +
> > +     if (virtio_net_csum_partial_ok_at_offset(skb, gso_inner_type, inner_nh,
> > +                                            inner_proto))
> > +             return 0;
> > +
> > +     if (skb_is_gso(skb))
> > +             return -EINVAL;

This is a risk the commit message will have to call out.

I don't see a simple way around it. We discussed segmenting here too,
but that requires intrusive changes to all callers too.

Alternatively,

1. call segmentation, but only to see it remove the SKB_GSO_DODGY and
thus verify that the TSO/USO packet is valid. Still drop otherwise.
This also does not add overhead, because it would be called later
anyway.

2. a failsafe off-by-default sysctl + static_branch that skips the
drop when set. Messy, but so is the situation we're dealing with.

Re: [PATCH net-next 3/4] net: resolve uncommon partial checksums at ingress

[Michael S. Tsirkin]

On Tue, Oct 13, 2026 at 09:03:04AM -0600, Willem de Bruijn wrote:
> On Tue, Oct 13, 2026 at 5:06 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Tue, Oct 13, 2026 at 06:58:41AM -0400, Michael S. Tsirkin wrote:
> > > A virtio net header can place CHECKSUM_PARTIAL at any in-packet
> > > offset.  If that state is retained, later checksum completion can
> > > modify the packet after protocol processing has made decisions from it.
> > >
> > > Let's fix that: let's validate csum_start and csum_offset and only keep
> > > CHECKSUM_PARTIAL when csum_start and csum_offset match a parsed TCP or
> > > UDP header, while handling VLAN headers, IPv4 options and IPv6 extension
> > > headers.  Complete other non-GSO checksums immediately, before any
> > > protocol processing runs.
> > >
> > > We can't easily "complete the checksum" for GSO we don't recognize - GSO
> > > assumes knowledge of the protocol. We could segment ignoring the
> > > checksum then do whatever csum_start and csum_offset say, but that
> > > seems like an overkill for something that should happen.
> > >
> > > UDP tunnel headers get the same handling on the inner header.
> > >
> > > Packets without VIRTIO_NET_HDR_F_NEEDS_CSUM are unaffected.
> > >
> > > Suggested-by: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > Assisted-by: LLM
> >
> >
> > I forgot reported-by tags:
> >
> > Reported-by: Paulos Yibelo <habte.yibelo@gmail.com>
> > Reported-by: Chris Arges <carges@cloudflare.com>
> >
> >
> > > ---
> > >  include/linux/virtio_net.h | 151 ++++++++++++++++++++++++++++++++-----
> > >  1 file changed, 133 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> > > index d6466f96cdd00..b181183118b3a 100644
> > > --- a/include/linux/virtio_net.h
> > > +++ b/include/linux/virtio_net.h
> > > @@ -5,7 +5,11 @@
> > >  #include <linux/if_vlan.h>
> > >  #include <linux/ip.h>
> > >  #include <linux/ipv6.h>
> > > +#include <linux/netdevice.h>
> > >  #include <linux/udp.h>
> > > +#include <net/gso.h>
> > > +#include <net/ip.h>
> > > +#include <net/ipv6.h>
> > >  #include <net/tcp.h>
> > >  #include <uapi/linux/tcp.h>
> > >  #include <uapi/linux/virtio_net.h>
> > > @@ -48,15 +52,111 @@ static inline int virtio_net_hdr_set_proto(struct sk_buff *skb,
> > >       return 0;
> > >  }
> > >
> > > +/* Does checksum metadata match a known TCP or UDP header? */
> > > +static inline bool
> > > +virtio_net_csum_partial_ok_at_offset(struct sk_buff *skb, u8 hdr_gso_type,
> > > +                                  unsigned int nhoff, __be16 proto)
> > > +{
> > > +     unsigned int thoff;
> > > +     int ip_proto;
> > > +
> > > +     if (proto == htons(ETH_P_IP)) {
> > > +             const struct iphdr *iph = ip4_parse(skb, nhoff, &thoff);
> > > +
> > > +             if (!iph || ip_is_fragment(iph))
> > > +                     return false;
> > > +             ip_proto = iph->protocol;
> > > +     } else if (proto == htons(ETH_P_IPV6)) {
> > > +             const struct ipv6hdr *iph;
> > > +             int flags = 0;
> > > +
> > > +             if (!pskb_may_pull(skb, nhoff + sizeof(*iph)))
> > > +                     return false;
> > > +             iph = (const void *)(skb->data + nhoff);
> > > +             if (iph->version != 6)
> > > +                     return false;
> > > +             thoff = nhoff + sizeof(*iph);
> > > +             ip_proto = iph->nexthdr;
> > > +             if (ip_proto != IPPROTO_TCP && ip_proto != IPPROTO_UDP) {
> > > +                     thoff = nhoff;
> > > +                     ip_proto = ipv6_find_hdr(skb, &thoff, -1, NULL,
> > > +                                              &flags);
> 
> Skip this rare case of extension headers?
> 
> > > +                     if (ip_proto < 0 || flags & IP6_FH_F_FRAG)
> > > +                             return false;
> > > +             }
> > > +     } else {
> > > +             return false;
> > > +     }
> > > +
> 
> > >  static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,
> > >                                         const struct virtio_net_hdr *hdr,
> > > -                                       bool little_endian, u8 hdr_gso_type)
> > > +                                       bool little_endian, u8 hdr_gso_type,
> > > +                                       bool need_csum_check)
> > >  {
> > > -     unsigned int nh_min_len = sizeof(struct iphdr);
> > > +     typeof(skb->transport_header) old_transport = skb->transport_header;
> > >       unsigned int gso_type = 0;
> > >       unsigned int thlen = 0;
> > >       unsigned int p_off = 0;
> > >       unsigned int ip_proto;
> > > +     bool csum_partial_ok = true;
> > >
> > >       if (hdr_gso_type != VIRTIO_NET_HDR_GSO_NONE) {
> > >               switch (hdr_gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {
> > > @@ -69,7 +169,6 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,
> > >                       gso_type = SKB_GSO_TCPV6;
> > >                       ip_proto = IPPROTO_TCP;
> > >                       thlen = sizeof(struct tcphdr);
> > > -                     nh_min_len = sizeof(struct ipv6hdr);
> > >                       break;
> > >               case VIRTIO_NET_HDR_GSO_UDP:
> > >                       gso_type = SKB_GSO_UDP;
> > > @@ -104,11 +203,11 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,
> > >
> > >               if (!skb_partial_csum_set(skb, start, off))
> > >                       return -EINVAL;
> > > -             if (skb_transport_offset(skb) < nh_min_len)
> > > -                     return -EINVAL;
> > > +             if (need_csum_check)
> > > +                     csum_partial_ok =
> > > +                             virtio_net_csum_partial_ok(skb, hdr_gso_type);
> > >
> > > -             nh_min_len = skb_transport_offset(skb);
> > > -             p_off = nh_min_len + thlen;
> > > +             p_off = skb_transport_offset(skb) + thlen;
> > >               if (!pskb_may_pull(skb, p_off))
> > >                       return -EINVAL;
> > >       } else if (gso_type) {
> > > @@ -158,8 +257,6 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,
> > >               case SKB_GSO_UDP_L4:
> > >                       if (!(hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM))
> > >                               return -EINVAL;
> > > -                     if (skb->csum_offset != offsetof(struct udphdr, check))
> > > -                             return -EINVAL;
> > >                       if (skb->len - p_off > gso_size * UDP_MAX_SEGMENTS)
> > >                               return -EINVAL;
> > >                       if (gso_type != SKB_GSO_UDP_L4)
> > > @@ -167,10 +264,6 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,
> > >                       break;
> > >               case SKB_GSO_TCPV4:
> > >               case SKB_GSO_TCPV6:
> > > -                     if (skb->ip_summed == CHECKSUM_PARTIAL &&
> > > -                         skb->csum_offset != offsetof(struct tcphdr, check))
> > > -                             return -EINVAL;
> > > -
> > >                       BUILD_BUG_ON(TCP_MIN_GSO_SIZE * GSO_MAX_SEGS < GSO_MAX_SIZE);
> > >                       gso_size = max(gso_size, TCP_MIN_GSO_SIZE);
> > >                       break;
> > > @@ -191,6 +284,17 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,
> > >               }
> > >       }
> > >
> > > +     if ((hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) && !csum_partial_ok) {
> > > +             int err;
> > > +
> > > +             if (skb_is_gso(skb))
> > > +                     return -EINVAL;
> > > +
> > > +             err = skb_checksum_help(skb);
> 
> Instead of this variable csum_partial_ok and this branch, could
> virtio_net_csum_partial_ok just call skb_checksum_help directly and
> drop VIRTIO_NET_HDR_F_NEEDS_CSUM?
> 
> > > +             skb->transport_header = old_transport;
> > > +             return err;
> > > +     }
> > > +
> > >       return 0;
> > >  }
> > >
> > > @@ -198,7 +302,8 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
> > >                                       const struct virtio_net_hdr *hdr,
> > >                                       bool little_endian)
> > >  {
> > > -     return __virtio_net_hdr_to_skb(skb, hdr, little_endian, hdr->gso_type);
> > > +     return __virtio_net_hdr_to_skb(skb, hdr, little_endian,
> > > +                                    hdr->gso_type, true);
> 
> Would be great if we can avoid the extra arg and having to update the callers.
> 
> > >  }
> > >
> > >  /* This function must be called after virtio_net_hdr_from_skb(). */
> > > @@ -293,6 +398,7 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
> > >       unsigned int inner_nh, outer_th, inner_th;
> > >       unsigned int inner_l3min, outer_l3min;
> > >       u8 gso_inner_type, gso_tunnel_type;
> > > +     __be16 inner_proto;
> > >       bool outer_isv6, inner_isv6;
> > >       int ret;
> > >
> > > @@ -334,7 +440,7 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
> > >
> > >       /* Let the basic parsing deal with plain GSO features. */
> > >       ret = __virtio_net_hdr_to_skb(skb, hdr, true,
> > > -                                   hdr->gso_type & ~gso_tunnel_type);
> > > +                                   hdr->gso_type & ~gso_tunnel_type, false);
> > >       if (ret)
> > >               return ret;
> > >
> > > @@ -352,8 +458,8 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
> > >                       return -EINVAL;
> > >       }
> > >
> > > -     skb_set_inner_protocol(skb, inner_isv6 ? htons(ETH_P_IPV6) :
> > > -                                              htons(ETH_P_IP));
> > > +     inner_proto = inner_isv6 ? htons(ETH_P_IPV6) : htons(ETH_P_IP);
> > > +     skb_set_inner_protocol(skb, inner_proto);
> 
> Can drop
> 
> > >       if (hdr->flags & VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM) {
> > >               if (!tnl_csum_negotiated)
> > >                       return -EINVAL;
> > > @@ -368,7 +474,16 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
> > >       skb->inner_mac_header = inner_nh + skb_headroom(skb);
> > >       skb->transport_header = outer_th + skb_headroom(skb);
> > >       skb->encapsulation = 1;
> > > -     return 0;
> > > +
> > > +     if (virtio_net_csum_partial_ok_at_offset(skb, gso_inner_type, inner_nh,
> > > +                                            inner_proto))
> > > +             return 0;
> > > +
> > > +     if (skb_is_gso(skb))
> > > +             return -EINVAL;
> 
> This is a risk the commit message will have to call out.

I tried with this:

	> > > We can't easily "complete the checksum" for GSO we don't recognize - GSO
	> > > assumes knowledge of the protocol. We could segment ignoring the
	> > > checksum then do whatever csum_start and csum_offset say, but that
	> > > seems like an overkill for something that should happen.

I guess I can add a more detailed explanation about what the risk is?


> I don't see a simple way around it. We discussed segmenting here too,
> but that requires intrusive changes to all callers too.
> Alternatively,
> 
> 1. call segmentation, but only to see it remove the SKB_GSO_DODGY and
> thus verify that the TSO/USO packet is valid. Still drop otherwise.
> This also does not add overhead, because it would be called later
> anyway.
> 
> 2. a failsafe off-by-default sysctl + static_branch that skips the
> drop when set. Messy, but so is the situation we're dealing with.

Tack this on top or do you feel it's needed straight away.

Re: [PATCH net-next 3/4] net: resolve uncommon partial checksums at ingress

[Willem de Bruijn]

> > > > +
> > > > +     if (virtio_net_csum_partial_ok_at_offset(skb, gso_inner_type, inner_nh,
> > > > +                                            inner_proto))
> > > > +             return 0;
> > > > +
> > > > +     if (skb_is_gso(skb))
> > > > +             return -EINVAL;
> >
> > This is a risk the commit message will have to call out.
>
> I tried with this:
>
>         > > > We can't easily "complete the checksum" for GSO we don't recognize - GSO
>         > > > assumes knowledge of the protocol. We could segment ignoring the
>         > > > checksum then do whatever csum_start and csum_offset say, but that
>         > > > seems like an overkill for something that should happen.
>
> I guess I can add a more detailed explanation about what the risk is?

Let's just explicitly mention the consequence: non-standard GSO
packets that previous would be sent may now fail.

>
> > I don't see a simple way around it. We discussed segmenting here too,
> > but that requires intrusive changes to all callers too.
> > Alternatively,
> >
> > 1. call segmentation, but only to see it remove the SKB_GSO_DODGY and
> > thus verify that the TSO/USO packet is valid. Still drop otherwise.
> > This also does not add overhead, because it would be called later
> > anyway.
> >
> > 2. a failsafe off-by-default sysctl + static_branch that skips the
> > drop when set. Messy, but so is the situation we're dealing with.
>
> Tack this on top or do you feel it's needed straight away.

Not urgent. Can even wait until a report appears. But worth taking a
look sooner if either of us gets around to it.

Re: [PATCH net-next 3/4] net: resolve uncommon partial checksums at ingress

[Michael S. Tsirkin]

On Tue, Oct 13, 2026 at 02:32:26PM -0600, Willem de Bruijn wrote:
> > > > > +
> > > > > +     if (virtio_net_csum_partial_ok_at_offset(skb, gso_inner_type, inner_nh,
> > > > > +                                            inner_proto))
> > > > > +             return 0;
> > > > > +
> > > > > +     if (skb_is_gso(skb))
> > > > > +             return -EINVAL;
> > >
> > > This is a risk the commit message will have to call out.
> >
> > I tried with this:
> >
> >         > > > We can't easily "complete the checksum" for GSO we don't recognize - GSO
> >         > > > assumes knowledge of the protocol. We could segment ignoring the
> >         > > > checksum then do whatever csum_start and csum_offset say, but that
> >         > > > seems like an overkill for something that should happen.
> >
> > I guess I can add a more detailed explanation about what the risk is?
> 
> Let's just explicitly mention the consequence: non-standard GSO
> packets that previous would be sent may now fail.
> 
> >
> > > I don't see a simple way around it. We discussed segmenting here too,
> > > but that requires intrusive changes to all callers too.
> > > Alternatively,
> > >
> > > 1. call segmentation, but only to see it remove the SKB_GSO_DODGY and
> > > thus verify that the TSO/USO packet is valid. Still drop otherwise.
> > > This also does not add overhead, because it would be called later
> > > anyway.
> > >
> > > 2. a failsafe off-by-default sysctl + static_branch that skips the
> > > drop when set. Messy, but so is the situation we're dealing with.
> >
> > Tack this on top or do you feel it's needed straight away.
> 
> Not urgent. Can even wait until a report appears. But worth taking a
> look sooner if either of us gets around to it.

Maybe if people intend all this for stable, I'll feel safer with this in
there.

Just to make sure, the branch would just short-circuit all checks, yes?

-- 
MST

Re: [PATCH net-next 3/4] net: resolve uncommon partial checksums at ingress

[Willem de Bruijn]

On Tue, Oct 13, 2026 at 2:38 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, Oct 13, 2026 at 02:32:26PM -0600, Willem de Bruijn wrote:
> > > > > > +
> > > > > > +     if (virtio_net_csum_partial_ok_at_offset(skb, gso_inner_type, inner_nh,
> > > > > > +                                            inner_proto))
> > > > > > +             return 0;
> > > > > > +
> > > > > > +     if (skb_is_gso(skb))
> > > > > > +             return -EINVAL;
> > > >
> > > > This is a risk the commit message will have to call out.
> > >
> > > I tried with this:
> > >
> > >         > > > We can't easily "complete the checksum" for GSO we don't recognize - GSO
> > >         > > > assumes knowledge of the protocol. We could segment ignoring the
> > >         > > > checksum then do whatever csum_start and csum_offset say, but that
> > >         > > > seems like an overkill for something that should happen.
> > >
> > > I guess I can add a more detailed explanation about what the risk is?
> >
> > Let's just explicitly mention the consequence: non-standard GSO
> > packets that previous would be sent may now fail.
> >
> > >
> > > > I don't see a simple way around it. We discussed segmenting here too,
> > > > but that requires intrusive changes to all callers too.
> > > > Alternatively,
> > > >
> > > > 1. call segmentation, but only to see it remove the SKB_GSO_DODGY and
> > > > thus verify that the TSO/USO packet is valid. Still drop otherwise.
> > > > This also does not add overhead, because it would be called later
> > > > anyway.
> > > >
> > > > 2. a failsafe off-by-default sysctl + static_branch that skips the
> > > > drop when set. Messy, but so is the situation we're dealing with.
> > >
> > > Tack this on top or do you feel it's needed straight away.
> >
> > Not urgent. Can even wait until a report appears. But worth taking a
> > look sooner if either of us gets around to it.
>
> Maybe if people intend all this for stable, I'll feel safer with this in
> there.
>
> Just to make sure, the branch would just short-circuit all checks, yes?

Ah option 2, the failsafe? At least any new failures (GSO) that could
cause regression reports. Operating entirely as before this series
sounds fine too.

Re: [PATCH net-next 3/4] net: resolve uncommon partial checksums at ingress

[Michael S. Tsirkin]

On Tue, Oct 13, 2026 at 02:45:07PM -0600, Willem de Bruijn wrote:
> On Tue, Oct 13, 2026 at 2:38 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Tue, Oct 13, 2026 at 02:32:26PM -0600, Willem de Bruijn wrote:
> > > > > > > +
> > > > > > > +     if (virtio_net_csum_partial_ok_at_offset(skb, gso_inner_type, inner_nh,
> > > > > > > +                                            inner_proto))
> > > > > > > +             return 0;
> > > > > > > +
> > > > > > > +     if (skb_is_gso(skb))
> > > > > > > +             return -EINVAL;
> > > > >
> > > > > This is a risk the commit message will have to call out.
> > > >
> > > > I tried with this:
> > > >
> > > >         > > > We can't easily "complete the checksum" for GSO we don't recognize - GSO
> > > >         > > > assumes knowledge of the protocol. We could segment ignoring the
> > > >         > > > checksum then do whatever csum_start and csum_offset say, but that
> > > >         > > > seems like an overkill for something that should happen.
> > > >
> > > > I guess I can add a more detailed explanation about what the risk is?
> > >
> > > Let's just explicitly mention the consequence: non-standard GSO
> > > packets that previous would be sent may now fail.
> > >
> > > >
> > > > > I don't see a simple way around it. We discussed segmenting here too,
> > > > > but that requires intrusive changes to all callers too.
> > > > > Alternatively,
> > > > >
> > > > > 1. call segmentation, but only to see it remove the SKB_GSO_DODGY and
> > > > > thus verify that the TSO/USO packet is valid. Still drop otherwise.
> > > > > This also does not add overhead, because it would be called later
> > > > > anyway.
> > > > >
> > > > > 2. a failsafe off-by-default sysctl + static_branch that skips the
> > > > > drop when set. Messy, but so is the situation we're dealing with.
> > > >
> > > > Tack this on top or do you feel it's needed straight away.
> > >
> > > Not urgent. Can even wait until a report appears. But worth taking a
> > > look sooner if either of us gets around to it.
> >
> > Maybe if people intend all this for stable, I'll feel safer with this in
> > there.
> >
> > Just to make sure, the branch would just short-circuit all checks, yes?
> 
> Ah option 2, the failsafe? At least any new failures (GSO) that could
> cause regression reports. Operating entirely as before this series
> sounds fine too.

Right. I feel 1 is just as likely to cause regressions. At least 2 gives
us exactly legacy behaviour back.

-- 
MST

Re: [PATCH net-next 3/4] net: resolve uncommon partial checksums at ingress

[Michael S. Tsirkin]

On Tue, Oct 13, 2026 at 09:03:04AM -0600, Willem de Bruijn wrote:
> On Tue, Oct 13, 2026 at 5:06 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Tue, Oct 13, 2026 at 06:58:41AM -0400, Michael S. Tsirkin wrote:
> > > A virtio net header can place CHECKSUM_PARTIAL at any in-packet
> > > offset.  If that state is retained, later checksum completion can
> > > modify the packet after protocol processing has made decisions from it.
> > >
> > > Let's fix that: let's validate csum_start and csum_offset and only keep
> > > CHECKSUM_PARTIAL when csum_start and csum_offset match a parsed TCP or
> > > UDP header, while handling VLAN headers, IPv4 options and IPv6 extension
> > > headers.  Complete other non-GSO checksums immediately, before any
> > > protocol processing runs.
> > >
> > > We can't easily "complete the checksum" for GSO we don't recognize - GSO
> > > assumes knowledge of the protocol. We could segment ignoring the
> > > checksum then do whatever csum_start and csum_offset say, but that
> > > seems like an overkill for something that should happen.
> > >
> > > UDP tunnel headers get the same handling on the inner header.
> > >
> > > Packets without VIRTIO_NET_HDR_F_NEEDS_CSUM are unaffected.
> > >
> > > Suggested-by: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > Assisted-by: LLM
> >
> >
> > I forgot reported-by tags:
> >
> > Reported-by: Paulos Yibelo <habte.yibelo@gmail.com>
> > Reported-by: Chris Arges <carges@cloudflare.com>
> >
> >
> > > ---
> > >  include/linux/virtio_net.h | 151 ++++++++++++++++++++++++++++++++-----
> > >  1 file changed, 133 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> > > index d6466f96cdd00..b181183118b3a 100644
> > > --- a/include/linux/virtio_net.h
> > > +++ b/include/linux/virtio_net.h
> > > @@ -5,7 +5,11 @@
> > >  #include <linux/if_vlan.h>
> > >  #include <linux/ip.h>
> > >  #include <linux/ipv6.h>
> > > +#include <linux/netdevice.h>
> > >  #include <linux/udp.h>
> > > +#include <net/gso.h>
> > > +#include <net/ip.h>
> > > +#include <net/ipv6.h>
> > >  #include <net/tcp.h>
> > >  #include <uapi/linux/tcp.h>
> > >  #include <uapi/linux/virtio_net.h>
> > > @@ -48,15 +52,111 @@ static inline int virtio_net_hdr_set_proto(struct sk_buff *skb,
> > >       return 0;
> > >  }
> > >
> > > +/* Does checksum metadata match a known TCP or UDP header? */
> > > +static inline bool
> > > +virtio_net_csum_partial_ok_at_offset(struct sk_buff *skb, u8 hdr_gso_type,
> > > +                                  unsigned int nhoff, __be16 proto)
> > > +{
> > > +     unsigned int thoff;
> > > +     int ip_proto;
> > > +
> > > +     if (proto == htons(ETH_P_IP)) {
> > > +             const struct iphdr *iph = ip4_parse(skb, nhoff, &thoff);
> > > +
> > > +             if (!iph || ip_is_fragment(iph))
> > > +                     return false;
> > > +             ip_proto = iph->protocol;
> > > +     } else if (proto == htons(ETH_P_IPV6)) {
> > > +             const struct ipv6hdr *iph;
> > > +             int flags = 0;
> > > +
> > > +             if (!pskb_may_pull(skb, nhoff + sizeof(*iph)))
> > > +                     return false;
> > > +             iph = (const void *)(skb->data + nhoff);
> > > +             if (iph->version != 6)
> > > +                     return false;
> > > +             thoff = nhoff + sizeof(*iph);
> > > +             ip_proto = iph->nexthdr;
> > > +             if (ip_proto != IPPROTO_TCP && ip_proto != IPPROTO_UDP) {
> > > +                     thoff = nhoff;
> > > +                     ip_proto = ipv6_find_hdr(skb, &thoff, -1, NULL,
> > > +                                              &flags);
> 
> Skip this rare case of extension headers?

but then it will be dropped if it's gso. so we can't.

> > > +                     if (ip_proto < 0 || flags & IP6_FH_F_FRAG)
> > > +                             return false;
> > > +             }
> > > +     } else {
> > > +             return false;
> > > +     }
> > > +
> 
> > >  static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,
> > >                                         const struct virtio_net_hdr *hdr,
> > > -                                       bool little_endian, u8 hdr_gso_type)
> > > +                                       bool little_endian, u8 hdr_gso_type,
> > > +                                       bool need_csum_check)
> > >  {
> > > -     unsigned int nh_min_len = sizeof(struct iphdr);
> > > +     typeof(skb->transport_header) old_transport = skb->transport_header;
> > >       unsigned int gso_type = 0;
> > >       unsigned int thlen = 0;
> > >       unsigned int p_off = 0;
> > >       unsigned int ip_proto;
> > > +     bool csum_partial_ok = true;
> > >
> > >       if (hdr_gso_type != VIRTIO_NET_HDR_GSO_NONE) {
> > >               switch (hdr_gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {
> > > @@ -69,7 +169,6 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,
> > >                       gso_type = SKB_GSO_TCPV6;
> > >                       ip_proto = IPPROTO_TCP;
> > >                       thlen = sizeof(struct tcphdr);
> > > -                     nh_min_len = sizeof(struct ipv6hdr);
> > >                       break;
> > >               case VIRTIO_NET_HDR_GSO_UDP:
> > >                       gso_type = SKB_GSO_UDP;
> > > @@ -104,11 +203,11 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,
> > >
> > >               if (!skb_partial_csum_set(skb, start, off))
> > >                       return -EINVAL;
> > > -             if (skb_transport_offset(skb) < nh_min_len)
> > > -                     return -EINVAL;
> > > +             if (need_csum_check)
> > > +                     csum_partial_ok =
> > > +                             virtio_net_csum_partial_ok(skb, hdr_gso_type);
> > >
> > > -             nh_min_len = skb_transport_offset(skb);
> > > -             p_off = nh_min_len + thlen;
> > > +             p_off = skb_transport_offset(skb) + thlen;
> > >               if (!pskb_may_pull(skb, p_off))
> > >                       return -EINVAL;
> > >       } else if (gso_type) {
> > > @@ -158,8 +257,6 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,
> > >               case SKB_GSO_UDP_L4:
> > >                       if (!(hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM))
> > >                               return -EINVAL;
> > > -                     if (skb->csum_offset != offsetof(struct udphdr, check))
> > > -                             return -EINVAL;
> > >                       if (skb->len - p_off > gso_size * UDP_MAX_SEGMENTS)
> > >                               return -EINVAL;
> > >                       if (gso_type != SKB_GSO_UDP_L4)
> > > @@ -167,10 +264,6 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,
> > >                       break;
> > >               case SKB_GSO_TCPV4:
> > >               case SKB_GSO_TCPV6:
> > > -                     if (skb->ip_summed == CHECKSUM_PARTIAL &&
> > > -                         skb->csum_offset != offsetof(struct tcphdr, check))
> > > -                             return -EINVAL;
> > > -
> > >                       BUILD_BUG_ON(TCP_MIN_GSO_SIZE * GSO_MAX_SEGS < GSO_MAX_SIZE);
> > >                       gso_size = max(gso_size, TCP_MIN_GSO_SIZE);
> > >                       break;
> > > @@ -191,6 +284,17 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,
> > >               }
> > >       }
> > >
> > > +     if ((hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) && !csum_partial_ok) {
> > > +             int err;
> > > +
> > > +             if (skb_is_gso(skb))
> > > +                     return -EINVAL;
> > > +
> > > +             err = skb_checksum_help(skb);
> 
> Instead of this variable csum_partial_ok and this branch, could
> virtio_net_csum_partial_ok just call skb_checksum_help directly and
> drop VIRTIO_NET_HDR_F_NEEDS_CSUM?

no, hdr is const. and I think I like it that way. we could
copy flags I guess - I'll play with it a bit.


> > > +             skb->transport_header = old_transport;
> > > +             return err;
> > > +     }
> > > +
> > >       return 0;
> > >  }
> > >
> > > @@ -198,7 +302,8 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
> > >                                       const struct virtio_net_hdr *hdr,
> > >                                       bool little_endian)
> > >  {
> > > -     return __virtio_net_hdr_to_skb(skb, hdr, little_endian, hdr->gso_type);
> > > +     return __virtio_net_hdr_to_skb(skb, hdr, little_endian,
> > > +                                    hdr->gso_type, true);
> 
> Would be great if we can avoid the extra arg and having to update the callers.

There are 2 callers why is this a concern?

But yes the extra bool is kinda ugly. i'll look into refactoring
this but i do not think the diff will be smaller - likely larger.

> > >  }
> > >
> > >  /* This function must be called after virtio_net_hdr_from_skb(). */
> > > @@ -293,6 +398,7 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
> > >       unsigned int inner_nh, outer_th, inner_th;
> > >       unsigned int inner_l3min, outer_l3min;
> > >       u8 gso_inner_type, gso_tunnel_type;
> > > +     __be16 inner_proto;
> > >       bool outer_isv6, inner_isv6;
> > >       int ret;
> > >
> > > @@ -334,7 +440,7 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
> > >
> > >       /* Let the basic parsing deal with plain GSO features. */
> > >       ret = __virtio_net_hdr_to_skb(skb, hdr, true,
> > > -                                   hdr->gso_type & ~gso_tunnel_type);
> > > +                                   hdr->gso_type & ~gso_tunnel_type, false);
> > >       if (ret)
> > >               return ret;
> > >
> > > @@ -352,8 +458,8 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
> > >                       return -EINVAL;
> > >       }
> > >
> > > -     skb_set_inner_protocol(skb, inner_isv6 ? htons(ETH_P_IPV6) :
> > > -                                              htons(ETH_P_IP));
> > > +     inner_proto = inner_isv6 ? htons(ETH_P_IPV6) : htons(ETH_P_IP);
> > > +     skb_set_inner_protocol(skb, inner_proto);
> 
> Can drop

It is here because inner_proto is used below, seems silly to re-compute
when we have it.


> > >       if (hdr->flags & VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM) {
> > >               if (!tnl_csum_negotiated)
> > >                       return -EINVAL;
> > > @@ -368,7 +474,16 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
> > >       skb->inner_mac_header = inner_nh + skb_headroom(skb);
> > >       skb->transport_header = outer_th + skb_headroom(skb);
> > >       skb->encapsulation = 1;
> > > -     return 0;
> > > +
> > > +     if (virtio_net_csum_partial_ok_at_offset(skb, gso_inner_type, inner_nh,
> > > +                                            inner_proto))
> > > +             return 0;
> > > +
> > > +     if (skb_is_gso(skb))
> > > +             return -EINVAL;
> 
> This is a risk the commit message will have to call out.

I tried but good point, not enough.  Will do.

> I don't see a simple way around it. We discussed segmenting here too,
> but that requires intrusive changes to all callers too.
> 
> Alternatively,
> 
> 1. call segmentation, but only to see it remove the SKB_GSO_DODGY and
> thus verify that the TSO/USO packet is valid. Still drop otherwise.
> This also does not add overhead, because it would be called later
> anyway.

Yes, I implemented this originally. It's an even more messy code but the
big issue is that it's not clear it is worth it. So we get gso with
a bad checksum. Currently it goes out on the wire corrupted. Ideally
we would do exactly that, but that would mean much more work that
SKB_GSO_DODGY play. Conversely we could just ignore the
checksum hints. Then the packet would go on wire because we
"fixed" it but it just has a chance to cause even more problems.
So... shrug. I opted for the simpler way.


So... shrug. I opted for the simpler way.  If a real workload shows
up, the counters in the last patch will tell us and we can revisit.

I will add this to the cover letter I feel, not commit log material,
right?

> 2. a failsafe off-by-default sysctl + static_branch that skips the
> drop when set. Messy, but so is the situation we're dealing with.

good idea, I'll add that.

-- 
MST

Re: [PATCH net-next 3/4] net: resolve uncommon partial checksums at ingress

[sashiko-bot]

> A virtio net header can place CHECKSUM_PARTIAL at any in-packet
> offset.  If that state is retained, later checksum completion can
> modify the packet after protocol processing has made decisions from it.
> 
> Let's fix that: let's validate csum_start and csum_offset and only keep
> CHECKSUM_PARTIAL when csum_start and csum_offset match a parsed TCP or
> UDP header, while handling VLAN headers, IPv4 options and IPv6 extension
> headers.  Complete other non-GSO checksums immediately, before any
> protocol processing runs.
> 
> We can't easily "complete the checksum" for GSO we don't recognize - GSO
> assumes knowledge of the protocol. We could segment ignoring the
> checksum then do whatever csum_start and csum_offset say, but that
> seems like an overkill for something that should happen.
> 
> UDP tunnel headers get the same handling on the inner header.
> 
> Packets without VIRTIO_NET_HDR_F_NEEDS_CSUM are unaffected.
> 
> Suggested-by: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1791887415.git.mst@redhat.com?part=3

[PATCH net-next 4/4] net: account for ingress-resolved virtio net hdr state

[Michael S. Tsirkin]

Ingress resolution of uncommon virtio net hdr checksum metadata
(software checksum completion at ingress) adds performance overhead but
is currently invisible: no counter tracks it, so a hypothetical workload
relying on uncommon metadata cannot be distinguished from none at all,
and packets quietly pay the host-CPU checksum overhead instead of offload.

Have virtio_net_hdr_to_skb() return 1 when it completes an uncommon
checksum in software, and flip every converter call site to test for
negative errno.  Have tun count these events per device as
rx_csum_sw and expose the counter via ethtool -S.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Assisted-by: LLM
Fixes: f43798c27684 ("tun: Allow GSO using virtio_net_hdr")

---
 drivers/net/tap.c          |  4 ++--
 drivers/net/tun.c          | 44 ++++++++++++++++++++++++++++++++++++--
 drivers/net/virtio_net.c   |  2 +-
 include/linux/virtio_net.h | 12 +++++++++--
 net/packet/af_packet.c     |  4 ++--
 5 files changed, 57 insertions(+), 9 deletions(-)

diff --git a/drivers/net/tap.c b/drivers/net/tap.c
index ff67d99deb39e..73e8833944463 100644
--- a/drivers/net/tap.c
+++ b/drivers/net/tap.c
@@ -649,7 +649,7 @@ static ssize_t tap_get_user(struct tap_queue *q, void *msg_control,
 
 	if (vnet_hdr_len) {
 		err = tun_vnet_hdr_to_skb(q->flags, skb, &vnet_hdr);
-		if (err) {
+		if (err < 0) {
 			rcu_read_unlock();
 			drop_reason = SKB_DROP_REASON_DEV_HDR;
 			goto err_kfree;
@@ -1084,7 +1084,7 @@ static int tap_get_user_xdp(struct tap_queue *q, struct xdp_buff *xdp)
 
 	if (vnet_hdr_len) {
 		err = tun_vnet_hdr_to_skb(q->flags, skb, gso);
-		if (err) {
+		if (err < 0) {
 			rcu_read_unlock();
 			goto err_kfree;
 		}
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 242899f7fd071..138ca8987dfc6 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -210,6 +210,7 @@ struct tun_struct {
 	u32 flow_count;
 	u32 rx_batched;
 	atomic_long_t rx_frame_errors;
+	atomic_long_t rx_csum_sw;
 	struct bpf_prog __rcu *xdp_prog;
 	struct tun_prog __rcu *steering_prog;
 	struct tun_prog __rcu *filter_prog;
@@ -1935,11 +1936,19 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 		break;
 	}
 
-	if (tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, &hdr)) {
+	err = tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, &hdr);
+	if (err < 0) {
 		atomic_long_inc(&tun->rx_frame_errors);
 		err = -EINVAL;
 		goto free_skb;
 	}
+	if (err > 0)
+		atomic_long_inc(&tun->rx_csum_sw);
+	/* The status is consumed: keep err clear so later exit paths
+	 * (XDP drop via unlock_frags) do not return it to write() as
+	 * a short count.
+	 */
+	err = 0;
 
 	if ((tun->flags & TUN_TYPE_MASK) == IFF_TAP)
 		skb->protocol = eth_type_trans(skb, tun->dev);
@@ -2605,12 +2614,15 @@ static int tun_xdp_one(struct tun_struct *tun,
 	tnl_hdr = (struct virtio_net_hdr_v1_hash_tunnel *)gso;
 	skb->dev = tun->dev;
 	skb_set_network_header(skb, ETH_HLEN);
-	if (tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, tnl_hdr)) {
+	ret = tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, tnl_hdr);
+	if (ret < 0) {
 		atomic_long_inc(&tun->rx_frame_errors);
 		kfree_skb(skb);
 		ret = -EINVAL;
 		goto out;
 	}
+	if (ret > 0)
+		atomic_long_inc(&tun->rx_csum_sw);
 
 	skb->protocol = eth_type_trans(skb, tun->dev);
 	skb_reset_network_header(skb);
@@ -3739,7 +3751,35 @@ static void tun_get_channels(struct net_device *dev,
 	channels->max_combined = tun->flags & IFF_MULTI_QUEUE ? MAX_TAP_QUEUES : 1;
 }
 
+#define TUN_STATS_LEN	1
+static const char tun_ethtool_strings[TUN_STATS_LEN][ETH_GSTRING_LEN] = {
+	"rx_csum_sw",
+};
+
+static int tun_get_sset_count(struct net_device *dev, int sset)
+{
+	return sset == ETH_SS_STATS ? TUN_STATS_LEN : -EOPNOTSUPP;
+}
+
+static void tun_get_strings(struct net_device *dev, u32 stringset, u8 *data)
+{
+	if (stringset == ETH_SS_STATS)
+		memcpy(data, tun_ethtool_strings,
+		       sizeof(tun_ethtool_strings));
+}
+
+static void tun_get_ethtool_stats(struct net_device *dev,
+				  struct ethtool_stats *stats, u64 *data)
+{
+	struct tun_struct *tun = netdev_priv(dev);
+
+	data[0] = atomic_long_read(&tun->rx_csum_sw);
+}
+
 static const struct ethtool_ops tun_ethtool_ops = {
+	.get_sset_count		= tun_get_sset_count,
+	.get_strings		= tun_get_strings,
+	.get_ethtool_stats	= tun_get_ethtool_stats,
 	.supported_coalesce_params = ETHTOOL_COALESCE_RX_MAX_FRAMES,
 	.get_drvinfo	= tun_get_drvinfo,
 	.get_msglevel	= tun_get_msglevel,
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index daab43ac92ce4..2e6fd81df9c41 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2519,7 +2519,7 @@ static void virtnet_receive_done(struct virtnet_info *vi, struct receive_queue *
 	skb_set_network_header(skb, ETH_HLEN);
 	if (virtio_net_hdr_tnl_to_skb(skb, &hdr->tnl_hdr, vi->rx_tnl,
 				      vi->rx_tnl_csum,
-				      virtio_is_little_endian(vi->vdev))) {
+				      virtio_is_little_endian(vi->vdev)) < 0) {
 		net_warn_ratelimited("%s: bad gso: type: %x, size: %u, flags %x tunnel %d tnl csum %d\n",
 				     dev->name, hdr->hdr.gso_type,
 				     hdr->hdr.gso_size, hdr->hdr.flags,
diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
index b181183118b3a..d8447db58ac83 100644
--- a/include/linux/virtio_net.h
+++ b/include/linux/virtio_net.h
@@ -14,6 +14,14 @@
 #include <uapi/linux/tcp.h>
 #include <uapi/linux/virtio_net.h>
 
+/*
+ * virtio_net_hdr_to_skb() and virtio_net_hdr_tnl_to_skb() return 0 on
+ * success and a negative errno on failure.  On success they may also
+ * return 1, indicating that an uncommon checksum was completed in
+ * software at ingress rather than deferred to the egress path;
+ * drivers can use this to account for such packets.
+ */
+
 static inline bool virtio_net_hdr_match_proto(__be16 protocol, __u8 gso_type)
 {
 	switch (gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {
@@ -292,7 +300,7 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,
 
 		err = skb_checksum_help(skb);
 		skb->transport_header = old_transport;
-		return err;
+		return err ?: 1;
 	}
 
 	return 0;
@@ -441,7 +449,7 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
 	/* Let the basic parsing deal with plain GSO features. */
 	ret = __virtio_net_hdr_to_skb(skb, hdr, true,
 				      hdr->gso_type & ~gso_tunnel_type, false);
-	if (ret)
+	if (ret < 0)
 		return ret;
 
 	/* In case of USO, the inner protocol is still unknown and
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 52ae3d875e971..e6aec9ecfa994 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -2913,7 +2913,7 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
 		}
 
 		if (has_vnet_hdr) {
-			if (virtio_net_hdr_to_skb(skb, &vnet_hdr, vio_le())) {
+			if (virtio_net_hdr_to_skb(skb, &vnet_hdr, vio_le()) < 0) {
 				tp_len = -EINVAL;
 				goto tpacket_error;
 			}
@@ -3136,7 +3136,7 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
 
 	if (vnet_hdr_sz) {
 		err = virtio_net_hdr_to_skb(skb, &vnet_hdr, vio_le());
-		if (err)
+		if (err < 0)
 			goto out_free;
 		len += vnet_hdr_sz;
 	}
-- 
MST

Re: [PATCH net-next 4/4] net: account for ingress-resolved virtio net hdr state

[sashiko-bot]

> Ingress resolution of uncommon virtio net hdr checksum metadata
> (software checksum completion at ingress) adds performance overhead but
> is currently invisible: no counter tracks it, so a hypothetical workload
> relying on uncommon metadata cannot be distinguished from none at all,
> and packets quietly pay the host-CPU checksum overhead instead of offload.
> 
> Have virtio_net_hdr_to_skb() return 1 when it completes an uncommon
> checksum in software, and flip every converter call site to test for
> negative errno.  Have tun count these events per device as
> rx_csum_sw and expose the counter via ethtool -S.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1791887415.git.mst@redhat.com?part=4

Re: [PATCH net-next 4/4] net: account for ingress-resolved virtio net hdr state

[Björn Töpel]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> Ingress resolution of uncommon virtio net hdr checksum metadata
> (software checksum completion at ingress) adds performance overhead but
> is currently invisible: no counter tracks it, so a hypothetical workload
> relying on uncommon metadata cannot be distinguished from none at all,
> and packets quietly pay the host-CPU checksum overhead instead of offload.
>
> Have virtio_net_hdr_to_skb() return 1 when it completes an uncommon
> checksum in software, and flip every converter call site to test for
> negative errno.  Have tun count these events per device as
> rx_csum_sw and expose the counter via ethtool -S.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Assisted-by: LLM
> Fixes: f43798c27684 ("tun: Allow GSO using virtio_net_hdr")
>
> ---
>  drivers/net/tap.c          |  4 ++--
>  drivers/net/tun.c          | 44 ++++++++++++++++++++++++++++++++++++--
>  drivers/net/virtio_net.c   |  2 +-
>  include/linux/virtio_net.h | 12 +++++++++--
>  net/packet/af_packet.c     |  4 ++--
>  5 files changed, 57 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/tap.c b/drivers/net/tap.c
> index ff67d99deb39e..73e8833944463 100644
> --- a/drivers/net/tap.c
> +++ b/drivers/net/tap.c
> @@ -649,7 +649,7 @@ static ssize_t tap_get_user(struct tap_queue *q, void *msg_control,
>  
>  	if (vnet_hdr_len) {
>  		err = tun_vnet_hdr_to_skb(q->flags, skb, &vnet_hdr);
> -		if (err) {
> +		if (err < 0) {
>  			rcu_read_unlock();
>  			drop_reason = SKB_DROP_REASON_DEV_HDR;
>  			goto err_kfree;
> @@ -1084,7 +1084,7 @@ static int tap_get_user_xdp(struct tap_queue *q, struct xdp_buff *xdp)
>  
>  	if (vnet_hdr_len) {
>  		err = tun_vnet_hdr_to_skb(q->flags, skb, gso);
> -		if (err) {
> +		if (err < 0) {
>  			rcu_read_unlock();
>  			goto err_kfree;
>  		}
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 242899f7fd071..138ca8987dfc6 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -210,6 +210,7 @@ struct tun_struct {
>  	u32 flow_count;
>  	u32 rx_batched;
>  	atomic_long_t rx_frame_errors;
> +	atomic_long_t rx_csum_sw;
>  	struct bpf_prog __rcu *xdp_prog;
>  	struct tun_prog __rcu *steering_prog;
>  	struct tun_prog __rcu *filter_prog;
> @@ -1935,11 +1936,19 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
>  		break;
>  	}
>  
> -	if (tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, &hdr)) {
> +	err = tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, &hdr);
> +	if (err < 0) {
>  		atomic_long_inc(&tun->rx_frame_errors);
>  		err = -EINVAL;
>  		goto free_skb;
>  	}
> +	if (err > 0)
> +		atomic_long_inc(&tun->rx_csum_sw);
> +	/* The status is consumed: keep err clear so later exit paths
> +	 * (XDP drop via unlock_frags) do not return it to write() as
> +	 * a short count.
> +	 */
> +	err = 0;
>  
>  	if ((tun->flags & TUN_TYPE_MASK) == IFF_TAP)
>  		skb->protocol = eth_type_trans(skb, tun->dev);
> @@ -2605,12 +2614,15 @@ static int tun_xdp_one(struct tun_struct *tun,
>  	tnl_hdr = (struct virtio_net_hdr_v1_hash_tunnel *)gso;
>  	skb->dev = tun->dev;
>  	skb_set_network_header(skb, ETH_HLEN);
> -	if (tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, tnl_hdr)) {
> +	ret = tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, tnl_hdr);
> +	if (ret < 0) {
>  		atomic_long_inc(&tun->rx_frame_errors);
>  		kfree_skb(skb);
>  		ret = -EINVAL;
>  		goto out;
>  	}
> +	if (ret > 0)
> +		atomic_long_inc(&tun->rx_csum_sw);
>  
>  	skb->protocol = eth_type_trans(skb, tun->dev);
>  	skb_reset_network_header(skb);
> @@ -3739,7 +3751,35 @@ static void tun_get_channels(struct net_device *dev,
>  	channels->max_combined = tun->flags & IFF_MULTI_QUEUE ? MAX_TAP_QUEUES : 1;
>  }
>  
> +#define TUN_STATS_LEN	1
> +static const char tun_ethtool_strings[TUN_STATS_LEN][ETH_GSTRING_LEN] = {
> +	"rx_csum_sw",
> +};
> +
> +static int tun_get_sset_count(struct net_device *dev, int sset)
> +{
> +	return sset == ETH_SS_STATS ? TUN_STATS_LEN : -EOPNOTSUPP;
> +}
> +
> +static void tun_get_strings(struct net_device *dev, u32 stringset, u8 *data)
> +{
> +	if (stringset == ETH_SS_STATS)
> +		memcpy(data, tun_ethtool_strings,
> +		       sizeof(tun_ethtool_strings));
> +}
> +
> +static void tun_get_ethtool_stats(struct net_device *dev,
> +				  struct ethtool_stats *stats, u64 *data)
> +{
> +	struct tun_struct *tun = netdev_priv(dev);
> +
> +	data[0] = atomic_long_read(&tun->rx_csum_sw);
> +}
> +
>  static const struct ethtool_ops tun_ethtool_ops = {
> +	.get_sset_count		= tun_get_sset_count,
> +	.get_strings		= tun_get_strings,
> +	.get_ethtool_stats	= tun_get_ethtool_stats,
>  	.supported_coalesce_params = ETHTOOL_COALESCE_RX_MAX_FRAMES,
>  	.get_drvinfo	= tun_get_drvinfo,
>  	.get_msglevel	= tun_get_msglevel,
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index daab43ac92ce4..2e6fd81df9c41 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2519,7 +2519,7 @@ static void virtnet_receive_done(struct virtnet_info *vi, struct receive_queue *
>  	skb_set_network_header(skb, ETH_HLEN);
>  	if (virtio_net_hdr_tnl_to_skb(skb, &hdr->tnl_hdr, vi->rx_tnl,
>  				      vi->rx_tnl_csum,
> -				      virtio_is_little_endian(vi->vdev))) {
> +				      virtio_is_little_endian(vi->vdev)) < 0) {
>  		net_warn_ratelimited("%s: bad gso: type: %x, size: %u, flags %x tunnel %d tnl csum %d\n",
>  				     dev->name, hdr->hdr.gso_type,
>  				     hdr->hdr.gso_size, hdr->hdr.flags,
> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index b181183118b3a..d8447db58ac83 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h
> @@ -14,6 +14,14 @@
>  #include <uapi/linux/tcp.h>
>  #include <uapi/linux/virtio_net.h>
>  
> +/*
> + * virtio_net_hdr_to_skb() and virtio_net_hdr_tnl_to_skb() return 0 on
> + * success and a negative errno on failure.  On success they may also
> + * return 1, indicating that an uncommon checksum was completed in
> + * software at ingress rather than deferred to the egress path;
> + * drivers can use this to account for such packets.
> + */
> +
>  static inline bool virtio_net_hdr_match_proto(__be16 protocol, __u8 gso_type)
>  {
>  	switch (gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {
> @@ -292,7 +300,7 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,
>  
>  		err = skb_checksum_help(skb);
>  		skb->transport_header = old_transport;
> -		return err;
> +		return err ?: 1;
>  	}
>  
>  	return 0;
> @@ -441,7 +449,7 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
>  	/* Let the basic parsing deal with plain GSO features. */
>  	ret = __virtio_net_hdr_to_skb(skb, hdr, true,
>  				      hdr->gso_type & ~gso_tunnel_type, false);
> -	if (ret)
> +	if (ret < 0)
>  		return ret;
>  
>  	/* In case of USO, the inner protocol is still unknown and
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 52ae3d875e971..e6aec9ecfa994 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -2913,7 +2913,7 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
>  		}
>  
>  		if (has_vnet_hdr) {
> -			if (virtio_net_hdr_to_skb(skb, &vnet_hdr, vio_le())) {
> +			if (virtio_net_hdr_to_skb(skb, &vnet_hdr, vio_le()) < 0) {

nit: I don't know if we care, but this sort of breaks bisectability.
Previous patch does return ret ?: 1, but it's this patch that does the
if-statement fixup. (Same for tun/tap... ^^^)


Björn

Re: [PATCH net-next 4/4] net: account for ingress-resolved virtio net hdr state

[Michael S. Tsirkin]

On Tue, Oct 13, 2026 at 07:45:17PM +0200, Björn Töpel wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > Ingress resolution of uncommon virtio net hdr checksum metadata
> > (software checksum completion at ingress) adds performance overhead but
> > is currently invisible: no counter tracks it, so a hypothetical workload
> > relying on uncommon metadata cannot be distinguished from none at all,
> > and packets quietly pay the host-CPU checksum overhead instead of offload.
> >
> > Have virtio_net_hdr_to_skb() return 1 when it completes an uncommon
> > checksum in software, and flip every converter call site to test for
> > negative errno.  Have tun count these events per device as
> > rx_csum_sw and expose the counter via ethtool -S.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > Assisted-by: LLM
> > Fixes: f43798c27684 ("tun: Allow GSO using virtio_net_hdr")
> >
> > ---
> >  drivers/net/tap.c          |  4 ++--
> >  drivers/net/tun.c          | 44 ++++++++++++++++++++++++++++++++++++--
> >  drivers/net/virtio_net.c   |  2 +-
> >  include/linux/virtio_net.h | 12 +++++++++--
> >  net/packet/af_packet.c     |  4 ++--
> >  5 files changed, 57 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/net/tap.c b/drivers/net/tap.c
> > index ff67d99deb39e..73e8833944463 100644
> > --- a/drivers/net/tap.c
> > +++ b/drivers/net/tap.c
> > @@ -649,7 +649,7 @@ static ssize_t tap_get_user(struct tap_queue *q, void *msg_control,
> >  
> >  	if (vnet_hdr_len) {
> >  		err = tun_vnet_hdr_to_skb(q->flags, skb, &vnet_hdr);
> > -		if (err) {
> > +		if (err < 0) {
> >  			rcu_read_unlock();
> >  			drop_reason = SKB_DROP_REASON_DEV_HDR;
> >  			goto err_kfree;
> > @@ -1084,7 +1084,7 @@ static int tap_get_user_xdp(struct tap_queue *q, struct xdp_buff *xdp)
> >  
> >  	if (vnet_hdr_len) {
> >  		err = tun_vnet_hdr_to_skb(q->flags, skb, gso);
> > -		if (err) {
> > +		if (err < 0) {
> >  			rcu_read_unlock();
> >  			goto err_kfree;
> >  		}
> > diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> > index 242899f7fd071..138ca8987dfc6 100644
> > --- a/drivers/net/tun.c
> > +++ b/drivers/net/tun.c
> > @@ -210,6 +210,7 @@ struct tun_struct {
> >  	u32 flow_count;
> >  	u32 rx_batched;
> >  	atomic_long_t rx_frame_errors;
> > +	atomic_long_t rx_csum_sw;
> >  	struct bpf_prog __rcu *xdp_prog;
> >  	struct tun_prog __rcu *steering_prog;
> >  	struct tun_prog __rcu *filter_prog;
> > @@ -1935,11 +1936,19 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
> >  		break;
> >  	}
> >  
> > -	if (tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, &hdr)) {
> > +	err = tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, &hdr);
> > +	if (err < 0) {
> >  		atomic_long_inc(&tun->rx_frame_errors);
> >  		err = -EINVAL;
> >  		goto free_skb;
> >  	}
> > +	if (err > 0)
> > +		atomic_long_inc(&tun->rx_csum_sw);
> > +	/* The status is consumed: keep err clear so later exit paths
> > +	 * (XDP drop via unlock_frags) do not return it to write() as
> > +	 * a short count.
> > +	 */
> > +	err = 0;
> >  
> >  	if ((tun->flags & TUN_TYPE_MASK) == IFF_TAP)
> >  		skb->protocol = eth_type_trans(skb, tun->dev);
> > @@ -2605,12 +2614,15 @@ static int tun_xdp_one(struct tun_struct *tun,
> >  	tnl_hdr = (struct virtio_net_hdr_v1_hash_tunnel *)gso;
> >  	skb->dev = tun->dev;
> >  	skb_set_network_header(skb, ETH_HLEN);
> > -	if (tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, tnl_hdr)) {
> > +	ret = tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, tnl_hdr);
> > +	if (ret < 0) {
> >  		atomic_long_inc(&tun->rx_frame_errors);
> >  		kfree_skb(skb);
> >  		ret = -EINVAL;
> >  		goto out;
> >  	}
> > +	if (ret > 0)
> > +		atomic_long_inc(&tun->rx_csum_sw);
> >  
> >  	skb->protocol = eth_type_trans(skb, tun->dev);
> >  	skb_reset_network_header(skb);
> > @@ -3739,7 +3751,35 @@ static void tun_get_channels(struct net_device *dev,
> >  	channels->max_combined = tun->flags & IFF_MULTI_QUEUE ? MAX_TAP_QUEUES : 1;
> >  }
> >  
> > +#define TUN_STATS_LEN	1
> > +static const char tun_ethtool_strings[TUN_STATS_LEN][ETH_GSTRING_LEN] = {
> > +	"rx_csum_sw",
> > +};
> > +
> > +static int tun_get_sset_count(struct net_device *dev, int sset)
> > +{
> > +	return sset == ETH_SS_STATS ? TUN_STATS_LEN : -EOPNOTSUPP;
> > +}
> > +
> > +static void tun_get_strings(struct net_device *dev, u32 stringset, u8 *data)
> > +{
> > +	if (stringset == ETH_SS_STATS)
> > +		memcpy(data, tun_ethtool_strings,
> > +		       sizeof(tun_ethtool_strings));
> > +}
> > +
> > +static void tun_get_ethtool_stats(struct net_device *dev,
> > +				  struct ethtool_stats *stats, u64 *data)
> > +{
> > +	struct tun_struct *tun = netdev_priv(dev);
> > +
> > +	data[0] = atomic_long_read(&tun->rx_csum_sw);
> > +}
> > +
> >  static const struct ethtool_ops tun_ethtool_ops = {
> > +	.get_sset_count		= tun_get_sset_count,
> > +	.get_strings		= tun_get_strings,
> > +	.get_ethtool_stats	= tun_get_ethtool_stats,
> >  	.supported_coalesce_params = ETHTOOL_COALESCE_RX_MAX_FRAMES,
> >  	.get_drvinfo	= tun_get_drvinfo,
> >  	.get_msglevel	= tun_get_msglevel,
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index daab43ac92ce4..2e6fd81df9c41 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -2519,7 +2519,7 @@ static void virtnet_receive_done(struct virtnet_info *vi, struct receive_queue *
> >  	skb_set_network_header(skb, ETH_HLEN);
> >  	if (virtio_net_hdr_tnl_to_skb(skb, &hdr->tnl_hdr, vi->rx_tnl,
> >  				      vi->rx_tnl_csum,
> > -				      virtio_is_little_endian(vi->vdev))) {
> > +				      virtio_is_little_endian(vi->vdev)) < 0) {
> >  		net_warn_ratelimited("%s: bad gso: type: %x, size: %u, flags %x tunnel %d tnl csum %d\n",
> >  				     dev->name, hdr->hdr.gso_type,
> >  				     hdr->hdr.gso_size, hdr->hdr.flags,
> > diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> > index b181183118b3a..d8447db58ac83 100644
> > --- a/include/linux/virtio_net.h
> > +++ b/include/linux/virtio_net.h
> > @@ -14,6 +14,14 @@
> >  #include <uapi/linux/tcp.h>
> >  #include <uapi/linux/virtio_net.h>
> >  
> > +/*
> > + * virtio_net_hdr_to_skb() and virtio_net_hdr_tnl_to_skb() return 0 on
> > + * success and a negative errno on failure.  On success they may also
> > + * return 1, indicating that an uncommon checksum was completed in
> > + * software at ingress rather than deferred to the egress path;
> > + * drivers can use this to account for such packets.
> > + */
> > +
> >  static inline bool virtio_net_hdr_match_proto(__be16 protocol, __u8 gso_type)
> >  {
> >  	switch (gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {
> > @@ -292,7 +300,7 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,
> >  
> >  		err = skb_checksum_help(skb);
> >  		skb->transport_header = old_transport;
> > -		return err;
> > +		return err ?: 1;
> >  	}
> >  
> >  	return 0;
> > @@ -441,7 +449,7 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
> >  	/* Let the basic parsing deal with plain GSO features. */
> >  	ret = __virtio_net_hdr_to_skb(skb, hdr, true,
> >  				      hdr->gso_type & ~gso_tunnel_type, false);
> > -	if (ret)
> > +	if (ret < 0)
> >  		return ret;
> >  
> >  	/* In case of USO, the inner protocol is still unknown and
> > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> > index 52ae3d875e971..e6aec9ecfa994 100644
> > --- a/net/packet/af_packet.c
> > +++ b/net/packet/af_packet.c
> > @@ -2913,7 +2913,7 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
> >  		}
> >  
> >  		if (has_vnet_hdr) {
> > -			if (virtio_net_hdr_to_skb(skb, &vnet_hdr, vio_le())) {
> > +			if (virtio_net_hdr_to_skb(skb, &vnet_hdr, vio_le()) < 0) {
> 
> nit: I don't know if we care, but this sort of breaks bisectability.
> Previous patch does return ret ?: 1, but it's this patch that does the
> if-statement fixup. (Same for tun/tap... ^^^)
> 

Oh ugh. You are right, I kept rebasing to tweak things and moved it
by chance. I'll fix it up in v2.


> Björn

Re: [PATCH net-next 0/4] more virtio-net csum hardening

[Michael S. Tsirkin]

On Tue, Oct 13, 2026 at 06:58:28AM -0400, Michael S. Tsirkin wrote:
> Allowing untrusted hardware to specify csum start/offset for
> CHECKSUM_PARTIAL was not a good idea, because, if it edits the packet
> outside the actual checksum, and depending on the setup, the networking
> stack could be looking at the packet both before and after the edit, and
> seeing different results.
> 
> Previously, commit 6785011f8b16 ("ipv4: validate checksum_start before
> completing checksum") validated all known instances where this
> causes the most grief, but the real fix is to make sure
> that either only actual checksum fields are ever edited,
> or the checksum is completed before the net stack looks at it,
> so it is always consistent.
> 
> This basically does this: checksum at ingress, except in a narrow list
> of specified known good cases.
> 
> Fixes: f43798c27684 ("tun: Allow GSO using virtio_net_hdr")


I forgot.

Reported-by: Paulos Yibelo <habte.yibelo@gmail.com>
Reported-by: Chris Arges <carges@cloudflare.com>


> Michael S. Tsirkin (4):
>   net: add ip4_parse()
>   net: use ip4_parse() in receive validation
>   net: resolve uncommon partial checksums at ingress
>   net: account for ingress-resolved virtio net hdr state
> 
>  drivers/net/tap.c                   |   4 +-
>  drivers/net/tun.c                   |  44 +++++++-
>  drivers/net/virtio_net.c            |   2 +-
>  include/linux/ip.h                  |  24 +++++
>  include/linux/virtio_net.h          | 161 ++++++++++++++++++++++++----
>  net/bridge/br_netfilter_hooks.c     |  15 +--
>  net/ipv4/ip_input.c                 |  13 +--
>  net/ipv4/netfilter/nf_reject_ipv4.c |  13 +--
>  net/packet/af_packet.c              |   4 +-
>  9 files changed, 228 insertions(+), 52 deletions(-)
> 
> -- 
> MST
> 

Re: [PATCH net-next 0/4] more virtio-net csum hardening

[Chris Arges]

On 2026-10-13 07:05:34, Michael S. Tsirkin wrote:
> On Tue, Oct 13, 2026 at 06:58:28AM -0400, Michael S. Tsirkin wrote:
> > Allowing untrusted hardware to specify csum start/offset for
> > CHECKSUM_PARTIAL was not a good idea, because, if it edits the packet
> > outside the actual checksum, and depending on the setup, the networking
> > stack could be looking at the packet both before and after the edit, and
> > seeing different results.
> > 
> > Previously, commit 6785011f8b16 ("ipv4: validate checksum_start before
> > completing checksum") validated all known instances where this
> > causes the most grief, but the real fix is to make sure
> > that either only actual checksum fields are ever edited,
> > or the checksum is completed before the net stack looks at it,
> > so it is always consistent.
> > 
> > This basically does this: checksum at ingress, except in a narrow list
> > of specified known good cases.
> > 
> > Fixes: f43798c27684 ("tun: Allow GSO using virtio_net_hdr")
> 
> 
> I forgot.
> 
> Reported-by: Paulos Yibelo <habte.yibelo@gmail.com>
> Reported-by: Chris Arges <carges@cloudflare.com>
> 
>
I did some testing of this patch-set and it mitigates my cases in VM
testing.

Tested-by: Chris Arges <carges@cloudflare.com>

> > Michael S. Tsirkin (4):
> >   net: add ip4_parse()
> >   net: use ip4_parse() in receive validation
> >   net: resolve uncommon partial checksums at ingress
> >   net: account for ingress-resolved virtio net hdr state
> > 
> >  drivers/net/tap.c                   |   4 +-
> >  drivers/net/tun.c                   |  44 +++++++-
> >  drivers/net/virtio_net.c            |   2 +-
> >  include/linux/ip.h                  |  24 +++++
> >  include/linux/virtio_net.h          | 161 ++++++++++++++++++++++++----
> >  net/bridge/br_netfilter_hooks.c     |  15 +--
> >  net/ipv4/ip_input.c                 |  13 +--
> >  net/ipv4/netfilter/nf_reject_ipv4.c |  13 +--
> >  net/packet/af_packet.c              |   4 +-
> >  9 files changed, 228 insertions(+), 52 deletions(-)
> > 
> > -- 
> > MST
> > 
> 

Re: [PATCH net-next 0/4] more virtio-net csum hardening

[Willem de Bruijn]

On Tue, Oct 13, 2026 at 4:58 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> Allowing untrusted hardware to specify csum start/offset for
> CHECKSUM_PARTIAL was not a good idea, because, if it edits the packet
> outside the actual checksum, and depending on the setup, the networking
> stack could be looking at the packet both before and after the edit, and
> seeing different results.
>
> Previously, commit 6785011f8b16 ("ipv4: validate checksum_start before
> completing checksum") validated all known instances where this
> causes the most grief, but the real fix is to make sure
> that either only actual checksum fields are ever edited,
> or the checksum is completed before the net stack looks at it,
> so it is always consistent.

+1 on the general approach of performing the checksum offload at
input, unless the input can easily be validated to be one of a few
common protocols. That greatly reduces the risk vs my previous attempt
at trying to drop.

Since we do not know the full extent of grief, I think this too should
go to net and stable. That would mean initially a single patch that
touches as few files as possible. The helper function in
include/linux/ip.h and conversion of existing open coded parsers to
use that can follow up later in net-next.


> This basically does this: checksum at ingress, except in a narrow list
> of specified known good cases.
>
> Fixes: f43798c27684 ("tun: Allow GSO using virtio_net_hdr")
>
> Michael S. Tsirkin (4):
>   net: add ip4_parse()
>   net: use ip4_parse() in receive validation
>   net: resolve uncommon partial checksums at ingress
>   net: account for ingress-resolved virtio net hdr state

Re: [PATCH net-next 0/4] more virtio-net csum hardening

[Michael S. Tsirkin]

On Tue, Oct 13, 2026 at 08:41:19AM -0600, Willem de Bruijn wrote:
> On Tue, Oct 13, 2026 at 4:58 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > Allowing untrusted hardware to specify csum start/offset for
> > CHECKSUM_PARTIAL was not a good idea, because, if it edits the packet
> > outside the actual checksum, and depending on the setup, the networking
> > stack could be looking at the packet both before and after the edit, and
> > seeing different results.
> >
> > Previously, commit 6785011f8b16 ("ipv4: validate checksum_start before
> > completing checksum") validated all known instances where this
> > causes the most grief, but the real fix is to make sure
> > that either only actual checksum fields are ever edited,
> > or the checksum is completed before the net stack looks at it,
> > so it is always consistent.
> 
> +1 on the general approach of performing the checksum offload at
> input, unless the input can easily be validated to be one of a few
> common protocols. That greatly reduces the risk vs my previous attempt
> at trying to drop.
> 
> Since we do not know the full extent of grief, I think this too should
> go to net and stable.

I think it needs a bit more testing to be frank, I do not see it
wait months out of the tree. So if this lands in next before the
merge window, we are good I think. If we miss that I will target net.


> That would mean initially a single patch that
> touches as few files as possible. The helper function in
> include/linux/ip.h and conversion of existing open coded parsers to
> use that can follow up later in net-next.

I don't see the value to be frank.
What moving code between files accomplishes, I donnu.
Squashing things surely makes review harder?
Does your backporting system really struggle with trivial
dependencies like this?

I can drop patch 2 and will if I am targeting net.

> 
> > This basically does this: checksum at ingress, except in a narrow list
> > of specified known good cases.
> >
> > Fixes: f43798c27684 ("tun: Allow GSO using virtio_net_hdr")
> >
> > Michael S. Tsirkin (4):
> >   net: add ip4_parse()
> >   net: use ip4_parse() in receive validation
> >   net: resolve uncommon partial checksums at ingress
> >   net: account for ingress-resolved virtio net hdr state

Re: [PATCH net-next 0/4] more virtio-net csum hardening

[Willem de Bruijn]

On Tue, Oct 13, 2026 at 12:41 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, Oct 13, 2026 at 08:41:19AM -0600, Willem de Bruijn wrote:
> > On Tue, Oct 13, 2026 at 4:58 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > Allowing untrusted hardware to specify csum start/offset for
> > > CHECKSUM_PARTIAL was not a good idea, because, if it edits the packet
> > > outside the actual checksum, and depending on the setup, the networking
> > > stack could be looking at the packet both before and after the edit, and
> > > seeing different results.
> > >
> > > Previously, commit 6785011f8b16 ("ipv4: validate checksum_start before
> > > completing checksum") validated all known instances where this
> > > causes the most grief, but the real fix is to make sure
> > > that either only actual checksum fields are ever edited,
> > > or the checksum is completed before the net stack looks at it,
> > > so it is always consistent.
> >
> > +1 on the general approach of performing the checksum offload at
> > input, unless the input can easily be validated to be one of a few
> > common protocols. That greatly reduces the risk vs my previous attempt
> > at trying to drop.
> >
> > Since we do not know the full extent of grief, I think this too should
> > go to net and stable.
>
> I think it needs a bit more testing to be frank, I do not see it
> wait months out of the tree. So if this lands in next before the
> merge window, we are good I think. If we miss that I will target net.
>
>
> > That would mean initially a single patch that
> > touches as few files as possible. The helper function in
> > include/linux/ip.h and conversion of existing open coded parsers to
> > use that can follow up later in net-next.
>
> I don't see the value to be frank.
> What moving code between files accomplishes, I donnu.
> Squashing things surely makes review harder?
> Does your backporting system really struggle with trivial
> dependencies like this?
>
> I can drop patch 2 and will if I am targeting net.

Good. This is not about "my" backporting, but upstream stable.
Refactor patches do not belong there.

Since patch 3 might have to be backported in the future if a serious
abuse surfaces, even if you now land it in net-next, it is better to
not depend on other patches.

> >
> > > This basically does this: checksum at ingress, except in a narrow list
> > > of specified known good cases.
> > >
> > > Fixes: f43798c27684 ("tun: Allow GSO using virtio_net_hdr")
> > >
> > > Michael S. Tsirkin (4):
> > >   net: add ip4_parse()
> > >   net: use ip4_parse() in receive validation
> > >   net: resolve uncommon partial checksums at ingress
> > >   net: account for ingress-resolved virtio net hdr state
>

Re: [PATCH net-next 0/4] more virtio-net csum hardening

[Michael S. Tsirkin]

On Tue, Oct 13, 2026 at 02:29:53PM -0600, Willem de Bruijn wrote:
> On Tue, Oct 13, 2026 at 12:41 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Tue, Oct 13, 2026 at 08:41:19AM -0600, Willem de Bruijn wrote:
> > > On Tue, Oct 13, 2026 at 4:58 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > >
> > > > Allowing untrusted hardware to specify csum start/offset for
> > > > CHECKSUM_PARTIAL was not a good idea, because, if it edits the packet
> > > > outside the actual checksum, and depending on the setup, the networking
> > > > stack could be looking at the packet both before and after the edit, and
> > > > seeing different results.
> > > >
> > > > Previously, commit 6785011f8b16 ("ipv4: validate checksum_start before
> > > > completing checksum") validated all known instances where this
> > > > causes the most grief, but the real fix is to make sure
> > > > that either only actual checksum fields are ever edited,
> > > > or the checksum is completed before the net stack looks at it,
> > > > so it is always consistent.
> > >
> > > +1 on the general approach of performing the checksum offload at
> > > input, unless the input can easily be validated to be one of a few
> > > common protocols. That greatly reduces the risk vs my previous attempt
> > > at trying to drop.
> > >
> > > Since we do not know the full extent of grief, I think this too should
> > > go to net and stable.
> >
> > I think it needs a bit more testing to be frank, I do not see it
> > wait months out of the tree. So if this lands in next before the
> > merge window, we are good I think. If we miss that I will target net.
> >
> >
> > > That would mean initially a single patch that
> > > touches as few files as possible. The helper function in
> > > include/linux/ip.h and conversion of existing open coded parsers to
> > > use that can follow up later in net-next.
> >
> > I don't see the value to be frank.
> > What moving code between files accomplishes, I donnu.
> > Squashing things surely makes review harder?
> > Does your backporting system really struggle with trivial
> > dependencies like this?
> >
> > I can drop patch 2 and will if I am targeting net.
> 
> Good. This is not about "my" backporting, but upstream stable.
> Refactor patches do not belong there.
>
> Since patch 3 might have to be backported in the future if a serious
> abuse surfaces, even if you now land it in net-next, it is better to
> not depend on other patches.

Right. While this is targeting next, I will reorder it last in series so
it's obvious for people it can be dropped.

Thanks a lot for the review!

> > >
> > > > This basically does this: checksum at ingress, except in a narrow list
> > > > of specified known good cases.
> > > >
> > > > Fixes: f43798c27684 ("tun: Allow GSO using virtio_net_hdr")
> > > >
> > > > Michael S. Tsirkin (4):
> > > >   net: add ip4_parse()
> > > >   net: use ip4_parse() in receive validation
> > > >   net: resolve uncommon partial checksums at ingress
> > > >   net: account for ingress-resolved virtio net hdr state
> >

Re: [PATCH net-next 0/4] more virtio-net csum hardening

[Willem de Bruijn]

On Tue, Oct 13, 2026 at 2:36 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, Oct 13, 2026 at 02:29:53PM -0600, Willem de Bruijn wrote:
> > On Tue, Oct 13, 2026 at 12:41 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Tue, Oct 13, 2026 at 08:41:19AM -0600, Willem de Bruijn wrote:
> > > > On Tue, Oct 13, 2026 at 4:58 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > >
> > > > > Allowing untrusted hardware to specify csum start/offset for
> > > > > CHECKSUM_PARTIAL was not a good idea, because, if it edits the packet
> > > > > outside the actual checksum, and depending on the setup, the networking
> > > > > stack could be looking at the packet both before and after the edit, and
> > > > > seeing different results.
> > > > >
> > > > > Previously, commit 6785011f8b16 ("ipv4: validate checksum_start before
> > > > > completing checksum") validated all known instances where this
> > > > > causes the most grief, but the real fix is to make sure
> > > > > that either only actual checksum fields are ever edited,
> > > > > or the checksum is completed before the net stack looks at it,
> > > > > so it is always consistent.
> > > >
> > > > +1 on the general approach of performing the checksum offload at
> > > > input, unless the input can easily be validated to be one of a few
> > > > common protocols. That greatly reduces the risk vs my previous attempt
> > > > at trying to drop.
> > > >
> > > > Since we do not know the full extent of grief, I think this too should
> > > > go to net and stable.
> > >
> > > I think it needs a bit more testing to be frank, I do not see it
> > > wait months out of the tree. So if this lands in next before the
> > > merge window, we are good I think. If we miss that I will target net.
> > >
> > >
> > > > That would mean initially a single patch that
> > > > touches as few files as possible. The helper function in
> > > > include/linux/ip.h and conversion of existing open coded parsers to
> > > > use that can follow up later in net-next.
> > >
> > > I don't see the value to be frank.
> > > What moving code between files accomplishes, I donnu.
> > > Squashing things surely makes review harder?
> > > Does your backporting system really struggle with trivial
> > > dependencies like this?
> > >
> > > I can drop patch 2 and will if I am targeting net.
> >
> > Good. This is not about "my" backporting, but upstream stable.
> > Refactor patches do not belong there.
> >
> > Since patch 3 might have to be backported in the future if a serious
> > abuse surfaces, even if you now land it in net-next, it is better to
> > not depend on other patches.
>
> Right. While this is targeting next, I will reorder it last in series so
> it's obvious for people it can be dropped.
>
> Thanks a lot for the review!

Of course. Thanks for the fixes!

I would open-code ip4_parse directly in "net: resolve uncommon partial
checksums at ingress". It's not a lot of code. Then that patch stands
on its own.

And then optionally squash patches 1 and 2 and place them at the end
of this series as one dedup patch, or send that separately later.
Personally I would drop that entirely and leave the existing code as
is. But either way.

>
> > > >
> > > > > This basically does this: checksum at ingress, except in a narrow list
> > > > > of specified known good cases.
> > > > >
> > > > > Fixes: f43798c27684 ("tun: Allow GSO using virtio_net_hdr")
> > > > >
> > > > > Michael S. Tsirkin (4):
> > > > >   net: add ip4_parse()
> > > > >   net: use ip4_parse() in receive validation
> > > > >   net: resolve uncommon partial checksums at ingress
> > > > >   net: account for ingress-resolved virtio net hdr state
> > >
>

Re: [PATCH net-next 0/4] more virtio-net csum hardening

[Michael S. Tsirkin]

On Tue, Oct 13, 2026 at 02:54:24PM -0600, Willem de Bruijn wrote:
> On Tue, Oct 13, 2026 at 2:36 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Tue, Oct 13, 2026 at 02:29:53PM -0600, Willem de Bruijn wrote:
> > > On Tue, Oct 13, 2026 at 12:41 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > >
> > > > On Tue, Oct 13, 2026 at 08:41:19AM -0600, Willem de Bruijn wrote:
> > > > > On Tue, Oct 13, 2026 at 4:58 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > >
> > > > > > Allowing untrusted hardware to specify csum start/offset for
> > > > > > CHECKSUM_PARTIAL was not a good idea, because, if it edits the packet
> > > > > > outside the actual checksum, and depending on the setup, the networking
> > > > > > stack could be looking at the packet both before and after the edit, and
> > > > > > seeing different results.
> > > > > >
> > > > > > Previously, commit 6785011f8b16 ("ipv4: validate checksum_start before
> > > > > > completing checksum") validated all known instances where this
> > > > > > causes the most grief, but the real fix is to make sure
> > > > > > that either only actual checksum fields are ever edited,
> > > > > > or the checksum is completed before the net stack looks at it,
> > > > > > so it is always consistent.
> > > > >
> > > > > +1 on the general approach of performing the checksum offload at
> > > > > input, unless the input can easily be validated to be one of a few
> > > > > common protocols. That greatly reduces the risk vs my previous attempt
> > > > > at trying to drop.
> > > > >
> > > > > Since we do not know the full extent of grief, I think this too should
> > > > > go to net and stable.
> > > >
> > > > I think it needs a bit more testing to be frank, I do not see it
> > > > wait months out of the tree. So if this lands in next before the
> > > > merge window, we are good I think. If we miss that I will target net.
> > > >
> > > >
> > > > > That would mean initially a single patch that
> > > > > touches as few files as possible. The helper function in
> > > > > include/linux/ip.h and conversion of existing open coded parsers to
> > > > > use that can follow up later in net-next.
> > > >
> > > > I don't see the value to be frank.
> > > > What moving code between files accomplishes, I donnu.
> > > > Squashing things surely makes review harder?
> > > > Does your backporting system really struggle with trivial
> > > > dependencies like this?
> > > >
> > > > I can drop patch 2 and will if I am targeting net.
> > >
> > > Good. This is not about "my" backporting, but upstream stable.
> > > Refactor patches do not belong there.
> > >
> > > Since patch 3 might have to be backported in the future if a serious
> > > abuse surfaces, even if you now land it in net-next, it is better to
> > > not depend on other patches.
> >
> > Right. While this is targeting next, I will reorder it last in series so
> > it's obvious for people it can be dropped.
> >
> > Thanks a lot for the review!
> 
> Of course. Thanks for the fixes!
> 
> I would open-code ip4_parse directly in "net: resolve uncommon partial
> checksums at ingress". It's not a lot of code. Then that patch stands
> on its own.

Ok if you prefer, except I'll make it a small inline in the same file.
Easier to then dedup at the end.

> And then optionally squash patches 1 and 2 and place them at the end
> of this series as one dedup patch, or send that separately later.
> Personally I would drop that entirely and leave the existing code as
> is. But either way.
> 
> >
> > > > >
> > > > > > This basically does this: checksum at ingress, except in a narrow list
> > > > > > of specified known good cases.
> > > > > >
> > > > > > Fixes: f43798c27684 ("tun: Allow GSO using virtio_net_hdr")
> > > > > >
> > > > > > Michael S. Tsirkin (4):
> > > > > >   net: add ip4_parse()
> > > > > >   net: use ip4_parse() in receive validation
> > > > > >   net: resolve uncommon partial checksums at ingress
> > > > > >   net: account for ingress-resolved virtio net hdr state
> > > >
> >