mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH net-next 0/4] more virtio-net csum hardening
@ 2026-10-13 10:58 Michael S. Tsirkin
  2026-10-13 10:58 ` [PATCH net-next 1/4] net: add ip4_parse() Michael S. Tsirkin
                   ` (5 more replies)
  0 siblings, 6 replies; 12+ messages in thread
From: Michael S. Tsirkin @ 2026-10-13 10:58 UTC (permalink / raw)
  To: linux-kernel
  Cc: Willem de Bruijn, Jason Wang, Eugenio Pérez, Xuan Zhuo,
	Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, netdev, virtualization, Max Krasnyansky

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH net-next 1/4] net: add ip4_parse()
  2026-10-13 10:58 [PATCH net-next 0/4] more virtio-net csum hardening Michael S. Tsirkin
@ 2026-10-13 10:58 ` Michael S. Tsirkin
  2026-10-13 10:58 ` [PATCH net-next 2/4] net: use ip4_parse() in receive validation Michael S. Tsirkin
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 12+ messages in thread
From: Michael S. Tsirkin @ 2026-10-13 10:58 UTC (permalink / raw)
  To: linux-kernel
  Cc: Willem de Bruijn, Jason Wang, Eugenio Pérez, Xuan Zhuo,
	Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, netdev, virtualization, Max Krasnyansky,
	Rusty Russell

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH net-next 2/4] net: use ip4_parse() in receive validation
  2026-10-13 10:58 [PATCH net-next 0/4] more virtio-net csum hardening Michael S. Tsirkin
  2026-10-13 10:58 ` [PATCH net-next 1/4] net: add ip4_parse() Michael S. Tsirkin
@ 2026-10-13 10:58 ` Michael S. Tsirkin
  2026-10-13 10:58 ` [PATCH net-next 3/4] net: resolve uncommon partial checksums at ingress Michael S. Tsirkin
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 12+ messages in thread
From: Michael S. Tsirkin @ 2026-10-13 10:58 UTC (permalink / raw)
  To: linux-kernel
  Cc: Willem de Bruijn, Jason Wang, Eugenio Pérez, Xuan Zhuo,
	Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, netdev, virtualization, Max Krasnyansky,
	Rusty Russell, Pablo Neira Ayuso, Florian Westphal, Phil Sutter,
	Nikolay Aleksandrov, Ido Schimmel, Simon Horman, David Ahern,
	netfilter-devel, coreteam, bridge

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH net-next 3/4] net: resolve uncommon partial checksums at ingress
  2026-10-13 10:58 [PATCH net-next 0/4] more virtio-net csum hardening Michael S. Tsirkin
  2026-10-13 10:58 ` [PATCH net-next 1/4] net: add ip4_parse() Michael S. Tsirkin
  2026-10-13 10:58 ` [PATCH net-next 2/4] net: use ip4_parse() in receive validation Michael S. Tsirkin
@ 2026-10-13 10:58 ` Michael S. Tsirkin
  2026-10-13 11:06   ` Michael S. Tsirkin
  2026-10-13 10:58 ` [PATCH net-next 4/4] net: account for ingress-resolved virtio net hdr state Michael S. Tsirkin
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 12+ messages in thread
From: Michael S. Tsirkin @ 2026-10-13 10:58 UTC (permalink / raw)
  To: linux-kernel
  Cc: Willem de Bruijn, Jason Wang, Eugenio Pérez, Xuan Zhuo,
	Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, netdev, virtualization, Max Krasnyansky,
	Rusty Russell

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH net-next 4/4] net: account for ingress-resolved virtio net hdr state
  2026-10-13 10:58 [PATCH net-next 0/4] more virtio-net csum hardening Michael S. Tsirkin
                   ` (2 preceding siblings ...)
  2026-10-13 10:58 ` [PATCH net-next 3/4] net: resolve uncommon partial checksums at ingress Michael S. Tsirkin
@ 2026-10-13 10:58 ` Michael S. Tsirkin
  2026-10-13 17:45   ` Björn Töpel
  2026-10-13 11:05 ` [PATCH net-next 0/4] more virtio-net csum hardening Michael S. Tsirkin
  2026-10-13 14:41 ` Willem de Bruijn
  5 siblings, 1 reply; 12+ messages in thread
From: Michael S. Tsirkin @ 2026-10-13 10:58 UTC (permalink / raw)
  To: linux-kernel
  Cc: Willem de Bruijn, Jason Wang, Eugenio Pérez, Xuan Zhuo,
	Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, netdev, virtualization, Max Krasnyansky,
	Rusty Russell, Simon Horman

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net-next 0/4] more virtio-net csum hardening
  2026-10-13 10:58 [PATCH net-next 0/4] more virtio-net csum hardening Michael S. Tsirkin
                   ` (3 preceding siblings ...)
  2026-10-13 10:58 ` [PATCH net-next 4/4] net: account for ingress-resolved virtio net hdr state Michael S. Tsirkin
@ 2026-10-13 11:05 ` Michael S. Tsirkin
  2026-10-13 14:41 ` Willem de Bruijn
  5 siblings, 0 replies; 12+ messages in thread
From: Michael S. Tsirkin @ 2026-10-13 11:05 UTC (permalink / raw)
  To: linux-kernel
  Cc: Willem de Bruijn, Jason Wang, Eugenio Pérez, Xuan Zhuo,
	Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, netdev, virtualization, Max Krasnyansky,
	Paulos Yibelo, Chris Arges

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
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net-next 3/4] net: resolve uncommon partial checksums at ingress
  2026-10-13 10:58 ` [PATCH net-next 3/4] net: resolve uncommon partial checksums at ingress Michael S. Tsirkin
@ 2026-10-13 11:06   ` Michael S. Tsirkin
  2026-10-13 15:03     ` Willem de Bruijn
  0 siblings, 1 reply; 12+ messages in thread
From: Michael S. Tsirkin @ 2026-10-13 11:06 UTC (permalink / raw)
  To: linux-kernel
  Cc: Willem de Bruijn, Jason Wang, Eugenio Pérez, Xuan Zhuo,
	Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, netdev, virtualization, Max Krasnyansky,
	Rusty Russell

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
> 


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net-next 0/4] more virtio-net csum hardening
  2026-10-13 10:58 [PATCH net-next 0/4] more virtio-net csum hardening Michael S. Tsirkin
                   ` (4 preceding siblings ...)
  2026-10-13 11:05 ` [PATCH net-next 0/4] more virtio-net csum hardening Michael S. Tsirkin
@ 2026-10-13 14:41 ` Willem de Bruijn
  5 siblings, 0 replies; 12+ messages in thread
From: Willem de Bruijn @ 2026-10-13 14:41 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: linux-kernel, Jason Wang, Eugenio Pérez, Xuan Zhuo,
	Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, netdev, virtualization, Max Krasnyansky,
	Paulos Yibelo, Chris Arges

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net-next 3/4] net: resolve uncommon partial checksums at ingress
  2026-10-13 11:06   ` Michael S. Tsirkin
@ 2026-10-13 15:03     ` Willem de Bruijn
  2026-10-13 18:35       ` Michael S. Tsirkin
  0 siblings, 1 reply; 12+ messages in thread
From: Willem de Bruijn @ 2026-10-13 15:03 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: linux-kernel, Jason Wang, Eugenio Pérez, Xuan Zhuo,
	Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, netdev, virtualization, Max Krasnyansky,
	Rusty Russell

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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net-next 4/4] net: account for ingress-resolved virtio net hdr state
  2026-10-13 10:58 ` [PATCH net-next 4/4] net: account for ingress-resolved virtio net hdr state Michael S. Tsirkin
@ 2026-10-13 17:45   ` Björn Töpel
  2026-10-13 18:12     ` Michael S. Tsirkin
  0 siblings, 1 reply; 12+ messages in thread
From: Björn Töpel @ 2026-10-13 17:45 UTC (permalink / raw)
  To: Michael S. Tsirkin, linux-kernel
  Cc: Willem de Bruijn, Jason Wang, Eugenio Pérez, Xuan Zhuo,
	Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, netdev, virtualization, Max Krasnyansky,
	Rusty Russell, Simon Horman

"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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net-next 4/4] net: account for ingress-resolved virtio net hdr state
  2026-10-13 17:45   ` Björn Töpel
@ 2026-10-13 18:12     ` Michael S. Tsirkin
  0 siblings, 0 replies; 12+ messages in thread
From: Michael S. Tsirkin @ 2026-10-13 18:12 UTC (permalink / raw)
  To: Björn Töpel
  Cc: linux-kernel, Willem de Bruijn, Jason Wang, Eugenio Pérez,
	Xuan Zhuo, Andrew Lunn, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, netdev, virtualization,
	Max Krasnyansky, Rusty Russell, Simon Horman

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net-next 3/4] net: resolve uncommon partial checksums at ingress
  2026-10-13 15:03     ` Willem de Bruijn
@ 2026-10-13 18:35       ` Michael S. Tsirkin
  0 siblings, 0 replies; 12+ messages in thread
From: Michael S. Tsirkin @ 2026-10-13 18:35 UTC (permalink / raw)
  To: Willem de Bruijn
  Cc: linux-kernel, Jason Wang, Eugenio Pérez, Xuan Zhuo,
	Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, netdev, virtualization, Max Krasnyansky,
	Rusty Russell

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.


^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2026-10-13 18:36 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-10-13 10:58 [PATCH net-next 0/4] more virtio-net csum hardening Michael S. Tsirkin
2026-10-13 10:58 ` [PATCH net-next 1/4] net: add ip4_parse() Michael S. Tsirkin
2026-10-13 10:58 ` [PATCH net-next 2/4] net: use ip4_parse() in receive validation Michael S. Tsirkin
2026-10-13 10:58 ` [PATCH net-next 3/4] net: resolve uncommon partial checksums at ingress Michael S. Tsirkin
2026-10-13 11:06   ` Michael S. Tsirkin
2026-10-13 15:03     ` Willem de Bruijn
2026-10-13 18:35       ` Michael S. Tsirkin
2026-10-13 10:58 ` [PATCH net-next 4/4] net: account for ingress-resolved virtio net hdr state Michael S. Tsirkin
2026-10-13 17:45   ` Björn Töpel
2026-10-13 18:12     ` Michael S. Tsirkin
2026-10-13 11:05 ` [PATCH net-next 0/4] more virtio-net csum hardening Michael S. Tsirkin
2026-10-13 14:41 ` Willem de Bruijn

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®