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

On Tue, Oct 13, 2026 at 5:06 AM Michael S. Tsirkin [off-list ref] wrote:

On Tue, Oct 13, 2026 at 06:58:41AM -0400, Michael S. Tsirkin wrote:

quoted

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 <redacted>
Reported-by: Chris Arges <redacted>

quoted

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

quoted

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

quoted

 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?

quoted

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

quoted

 }

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

quoted

      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.