* [PATCH net v4 2/7] um: vector: trim skbs before verifying the header
2026-10-21 7:49 ` [PATCH net v4 1/7] net: account for ingress-resolved virtio net hdr state Michael S. Tsirkin
@ 2026-10-21 7:49 ` Michael S. Tsirkin
2026-10-21 16:08 ` Willem de Bruijn
2026-10-21 7:49 ` [PATCH net v4 3/7] virtio_net: parse tunnel csum_start little-endian Michael S. Tsirkin
` (5 subsequent siblings)
6 siblings, 1 reply; 15+ messages in thread
From: Michael S. Tsirkin @ 2026-10-21 7:49 UTC (permalink / raw)
To: linux-kernel
Cc: Willem de Bruijn, Chris Arges, Björn Töpel,
Phil Sutter, Jason Wang, Eugenio Pérez, Xuan Zhuo,
Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, netdev, virtualization, Max Krasnyansky,
Rusty Russell, Richard Weinberger, Anton Ivanov, Johannes Berg,
linux-um
The vector receive path over-allocates the skb and only trims it
to the received length after the transport header is verified.
A follow up patch will complete the packet checksum in software within
virtio_net_hdr_to_skb so we need the packet trimmed (otherwise the
checksum would be computed incorrectly over the unreceived tail of the
buffer).
Trim first, so the import sees the packet as it was received.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
arch/um/drivers/vector_kern.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/um/drivers/vector_kern.c b/arch/um/drivers/vector_kern.c
index 8a70b3a62553..1afbef78df72 100644
--- a/arch/um/drivers/vector_kern.c
+++ b/arch/um/drivers/vector_kern.c
@@ -865,6 +865,7 @@ static int vector_legacy_rx(struct vector_private *vp)
if (skb != NULL) {
if (pkt_len > vp->header_size) {
+ pskb_trim(skb, pkt_len - vp->rx_header_size);
if (vp->header_size > 0) {
header_check = vp->verify_header(
vp->header_rxbuffer, skb, vp);
@@ -879,7 +880,6 @@ static int vector_legacy_rx(struct vector_private *vp)
skb->ip_summed = CHECKSUM_UNNECESSARY;
}
}
- pskb_trim(skb, pkt_len - vp->rx_header_size);
skb->protocol = eth_type_trans(skb, skb->dev);
vp->dev->stats.rx_bytes += skb->len;
vp->dev->stats.rx_packets++;
@@ -981,6 +981,8 @@ static int vector_mmsg_rx(struct vector_private *vp, int budget)
for (i = 0; i < packet_count; i++) {
skb = (*skbuff_vector);
if (mmsg_vector->msg_len > vp->header_size) {
+ pskb_trim(skb,
+ mmsg_vector->msg_len - vp->rx_header_size);
if (vp->header_size > 0) {
header_check = vp->verify_header(
mmsg_vector->msg_hdr.msg_iov->iov_base,
@@ -1005,8 +1007,6 @@ static int vector_mmsg_rx(struct vector_private *vp, int budget)
skb->ip_summed = CHECKSUM_UNNECESSARY;
}
}
- pskb_trim(skb,
- mmsg_vector->msg_len - vp->rx_header_size);
skb->protocol = eth_type_trans(skb, skb->dev);
/*
* We do not need to lock on updating stats here
--
MST
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH net v4 2/7] um: vector: trim skbs before verifying the header
2026-10-21 7:49 ` [PATCH net v4 2/7] um: vector: trim skbs before verifying the header Michael S. Tsirkin
@ 2026-10-21 16:08 ` Willem de Bruijn
0 siblings, 0 replies; 15+ messages in thread
From: Willem de Bruijn @ 2026-10-21 16:08 UTC (permalink / raw)
To: Michael S. Tsirkin, linux-kernel
Cc: Willem de Bruijn, Chris Arges, Björn Töpel,
Phil Sutter, Jason Wang, Eugenio Pérez, Xuan Zhuo,
Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, netdev, virtualization, Max Krasnyansky,
Rusty Russell, Richard Weinberger, Anton Ivanov, Johannes Berg,
linux-um
Michael S. Tsirkin wrote:
> The vector receive path over-allocates the skb and only trims it
> to the received length after the transport header is verified.
>
> A follow up patch will complete the packet checksum in software within
> virtio_net_hdr_to_skb so we need the packet trimmed (otherwise the
> checksum would be computed incorrectly over the unreceived tail of the
> buffer).
>
> Trim first, so the import sees the packet as it was received.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Willem de Bruijn <willemb@google.com>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH net v4 3/7] virtio_net: parse tunnel csum_start little-endian
2026-10-21 7:49 ` [PATCH net v4 1/7] net: account for ingress-resolved virtio net hdr state Michael S. Tsirkin
2026-10-21 7:49 ` [PATCH net v4 2/7] um: vector: trim skbs before verifying the header Michael S. Tsirkin
@ 2026-10-21 7:49 ` Michael S. Tsirkin
2026-10-21 7:50 ` [PATCH net v4 4/7] net: resolve uncommon partial checksums at ingress Michael S. Tsirkin
` (4 subsequent siblings)
6 siblings, 0 replies; 15+ messages in thread
From: Michael S. Tsirkin @ 2026-10-21 7:49 UTC (permalink / raw)
To: linux-kernel
Cc: Willem de Bruijn, Chris Arges, Björn Töpel,
Phil Sutter, Jason Wang, Eugenio Pérez, Xuan Zhuo,
Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, netdev, virtualization, Max Krasnyansky,
Rusty Russell
All tunnel related virtio features are > 32, so none of this
functionality can be enabled on legacy mixed-endian device.
Accordingly, all tunnel header fields, csum_start included, are
little-endian.
The nested virtio net header import parses csum_start that way, but the
inner transport offset is parsed with the endianness specified by the
interface.
If tun is misconfigured to legacy big-endian mode while the tunnel
features are enabled, the two parses of this field disagree, and the
value stored in skb->inner_transport_header - which is only
bounds-checked from below - can point past the end of the packet, where
GSO segmentation later dereferences it.
To fix, parse the field little-endian in both places.
Fixes: a2fb4bc4e2a6 ("net: implement virtio helpers to handle UDP GSO tunneling.")
Cc: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
include/linux/virtio_net.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
index d6466f96cdd0..013c6c7e2dea 100644
--- a/include/linux/virtio_net.h
+++ b/include/linux/virtio_net.h
@@ -324,7 +324,7 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
inner_l3min = virtio_l3min(inner_isv6);
outer_l3min = ETH_HLEN + virtio_l3min(outer_isv6);
- inner_th = __virtio16_to_cpu(little_endian, hdr->csum_start);
+ inner_th = __virtio16_to_cpu(true, hdr->csum_start);
inner_nh = le16_to_cpu(vhdr->inner_nh_offset);
outer_th = le16_to_cpu(vhdr->outer_th_offset);
if (outer_th < outer_l3min ||
--
MST
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH net v4 4/7] net: resolve uncommon partial checksums at ingress
2026-10-21 7:49 ` [PATCH net v4 1/7] net: account for ingress-resolved virtio net hdr state Michael S. Tsirkin
2026-10-21 7:49 ` [PATCH net v4 2/7] um: vector: trim skbs before verifying the header Michael S. Tsirkin
2026-10-21 7:49 ` [PATCH net v4 3/7] virtio_net: parse tunnel csum_start little-endian Michael S. Tsirkin
@ 2026-10-21 7:50 ` Michael S. Tsirkin
2026-10-21 16:13 ` Willem de Bruijn
2026-10-21 7:50 ` [PATCH net v4 5/7] net: add a sysctl to opt out of virtio net hdr checksum validation Michael S. Tsirkin
` (3 subsequent siblings)
6 siblings, 1 reply; 15+ messages in thread
From: Michael S. Tsirkin @ 2026-10-21 7:50 UTC (permalink / raw)
To: linux-kernel
Cc: Willem de Bruijn, Chris Arges, Björn Töpel,
Phil Sutter, Jason Wang, Eugenio Pérez, Xuan Zhuo,
Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, netdev, virtualization, Max Krasnyansky,
Rusty Russell, Paulos Yibelo
As reported first by Paulos Yibelo <habte.yibelo@gmail.com> (after which
others, credited below, found more ways the bug can affect users) 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 shouldn't happen.
There's some risk here:
- non-gso packets which can't be parsed get checksum calculated in
software, adding performance overhead
- gso packets which can't be parsed get dropped
To mitigate the risks, the following patch will add a sysctl knob to
bypass the resolution. This is reasonable if the setup is such that
there is no protocol processing at all.
UDP tunnel headers get the same handling on the inner header.
Packets without VIRTIO_NET_HDR_F_NEEDS_CSUM are unaffected.
Reported-by: Paulos Yibelo <habte.yibelo@gmail.com>
Reported-by: Chris Arges <carges@cloudflare.com>
Tested-by: Chris Arges <carges@cloudflare.com>
Fixes: f43798c27684 ("tun: Allow GSO using virtio_net_hdr")
Suggested-by: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/net/tap.c | 8 +-
drivers/net/tun.c | 8 +-
drivers/net/virtio_net.c | 3 +-
include/linux/virtio_net.h | 276 ++++++++++++++++++++++++++++++++++++-
4 files changed, 281 insertions(+), 14 deletions(-)
diff --git a/drivers/net/tap.c b/drivers/net/tap.c
index d7520622e809..f2a9bb443980 100644
--- a/drivers/net/tap.c
+++ b/drivers/net/tap.c
@@ -653,9 +653,9 @@ static ssize_t tap_get_user(struct tap_queue *q, void *msg_control,
rcu_read_unlock();
drop_reason = SKB_DROP_REASON_DEV_HDR;
goto err_kfree;
- }
- if (err > 0)
+ } else if (err > 0) {
atomic64_inc(&tap->rx_csum_sw);
+ }
}
skb_probe_transport_header(skb);
@@ -1089,9 +1089,9 @@ static int tap_get_user_xdp(struct tap_queue *q, struct xdp_buff *xdp)
if (err < 0) {
rcu_read_unlock();
goto err_kfree;
- }
- if (err > 0)
+ } else if (err > 0) {
atomic64_inc(&tap->rx_csum_sw);
+ }
}
skb_probe_transport_header(skb);
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 19cb443c89b0..197fb8726f8d 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1941,8 +1941,7 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
atomic_long_inc(&tun->rx_frame_errors);
err = -EINVAL;
goto free_skb;
- }
- if (err > 0) {
+ } else if (err > 0) {
atomic64_inc(&tun->rx_csum_sw);
err = 0;
}
@@ -2617,9 +2616,10 @@ static int tun_xdp_one(struct tun_struct *tun,
kfree_skb(skb);
ret = -EINVAL;
goto out;
- }
- if (ret > 0)
+ } else if (ret > 0) {
atomic64_inc(&tun->rx_csum_sw);
+ ret = 0;
+ }
skb->protocol = eth_type_trans(skb, tun->dev);
skb_reset_network_header(skb);
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index b0441bf8e875..5821ddabde80 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2529,8 +2529,7 @@ static void virtnet_receive_done(struct virtnet_info *vi, struct receive_queue *
hdr->hdr.gso_size, hdr->hdr.flags,
vi->rx_tnl, vi->rx_tnl_csum);
goto frame_err;
- }
- if (ret > 0) {
+ } else if (ret > 0) {
u64_stats_update_begin(&rq->stats.syncp);
u64_stats_inc(&rq->stats.rx_csum_sw);
u64_stats_update_end(&rq->stats.syncp);
diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
index 013c6c7e2dea..ec6f47ce1c97 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,6 +52,202 @@ static inline int virtio_net_hdr_set_proto(struct sk_buff *skb,
return 0;
}
+/* TODO: nothing virtio specific here, move to ip.h */
+
+/*
+ * 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 *virtio_net_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;
+}
+
+/* Where does the L3 header start, and what protocol is it?
+ * Handles Ethernet and VLAN headers; other devices use the
+ * network header offset the caller set up.
+ */
+static inline bool
+virtio_net_l3_start(struct sk_buff *skb, int *nhoff, __be16 *proto)
+{
+ /* skb->dev is only guaranteed to be set since 44378d02c5aa
+ * ("net: always dissect GSO packets in
+ * __virtio_net_hdr_to_skb()"), do not crash when it is not.
+ */
+ if (WARN_ON_ONCE(!skb->dev))
+ return false;
+
+ /* Loopback carries ethernet frames, too:
+ * loopback_xmit() calls eth_type_trans() on them.
+ */
+ if (skb->dev->type == ARPHRD_ETHER ||
+ skb->dev->type == ARPHRD_LOOPBACK) {
+ 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);
+ /* An unset network header shows up as an offset
+ * outside of the packet.
+ */
+ if (*nhoff < 0 || (unsigned int)*nhoff >= skb->len)
+ return false;
+ *proto = skb->protocol;
+ if (!*proto) {
+ const struct iphdr *iph;
+
+ /* A packet socket bound to ETH_P_ALL leaves the
+ * protocol unset: infer it from the IP version
+ * at the network header.
+ */
+ if (!pskb_may_pull(skb, *nhoff + 1))
+ return false;
+ iph = (const struct iphdr *)(skb->data + *nhoff);
+ if (iph->version == 4)
+ *proto = htons(ETH_P_IP);
+ else if (iph->version == 6)
+ *proto = htons(ETH_P_IPV6);
+ }
+ }
+ return true;
+}
+
+/* Walk the L3 header at @nhoff: where does its transport header
+ * start, and which protocol is it? Returns false if the header
+ * is truncated, malformed or fragmented.
+ */
+static inline bool
+virtio_net_l4_start(struct sk_buff *skb, unsigned int nhoff, __be16 proto,
+ unsigned int *thoff, int *ip_proto)
+{
+ if (proto == htons(ETH_P_IP)) {
+ const struct iphdr *iph = virtio_net_ip4_parse(skb, nhoff, thoff);
+
+ if (!iph || ip_is_fragment(iph))
+ return false;
+ *ip_proto = iph->protocol;
+ return true;
+ }
+
+ 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;
+ /* Find the transport header behind any
+ * extension headers, AH included.
+ */
+ *ip_proto = ipv6_find_hdr(skb, thoff, -1, NULL,
+ &flags);
+ if (*ip_proto < 0 || flags & IP6_FH_F_FRAG)
+ return false;
+ }
+ return true;
+ }
+
+ return false;
+}
+
+/* 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 (!virtio_net_l4_start(skb, nhoff, proto, &thoff, &ip_proto))
+ 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)
+{
+ int nhoff;
+ __be16 proto;
+
+ if (!virtio_net_l3_start(skb, &nhoff, &proto))
+ return false;
+
+ return virtio_net_csum_partial_ok_at_offset(skb, hdr_gso_type, nhoff,
+ proto);
+}
+
+/*
+ * Handle a packet whose checksum metadata does not match the packet:
+ * GSO packets are dropped, as segmentation needs one checksum per
+ * segment; others are completed in software.
+ */
+static inline int __virtio_net_resolve_csum_partial_mismatch(struct sk_buff *skb)
+{
+ if (skb_is_gso(skb))
+ return -EINVAL;
+
+ return skb_checksum_help(skb) ?: 1;
+}
+
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)
@@ -194,11 +394,35 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,
return 0;
}
+/*
+ * Return value:
+ * 0 - success
+ * >0 - checksum was completed in software (slow path)
+ * <0 - failure
+ */
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);
+ typeof(skb->transport_header) old_transport = skb->transport_header;
+ int ret = __virtio_net_hdr_to_skb(skb, hdr, little_endian,
+ hdr->gso_type);
+
+ if (ret < 0)
+ return ret;
+
+ if (skb->ip_summed != CHECKSUM_PARTIAL ||
+ virtio_net_csum_partial_ok(skb, hdr->gso_type))
+ return 0;
+
+ /*
+ * Here we do not trust the checksum metadata.
+ * __virtio_net_hdr_to_skb set the transport header to csum_start
+ * from virtio hdr, restore it to a known good value.
+ */
+ skb->transport_header = old_transport;
+
+ return __virtio_net_resolve_csum_partial_mismatch(skb);
}
/* This function must be called after virtio_net_hdr_from_skb(). */
@@ -282,6 +506,35 @@ static inline unsigned int virtio_l3min(bool is_ipv6)
return is_ipv6 ? sizeof(struct ipv6hdr) : sizeof(struct iphdr);
}
+/* Do the tunnel offsets match the outer headers in the frame?
+ * The L3 header the tunnel claims must be the one the frame
+ * carries, with UDP following it at @outer_th.
+ */
+static inline bool
+virtio_net_tnl_offsets_ok(struct sk_buff *skb, bool outer_isv6,
+ unsigned int outer_th)
+{
+ int nhoff;
+ __be16 proto;
+ unsigned int thoff;
+ int ip_proto;
+
+ if (!virtio_net_l3_start(skb, &nhoff, &proto))
+ return false;
+ if (outer_isv6 != (proto == htons(ETH_P_IPV6)))
+ return false;
+ if (!virtio_net_l4_start(skb, nhoff, proto, &thoff, &ip_proto))
+ return false;
+
+ return ip_proto == IPPROTO_UDP && outer_th == thoff;
+}
+
+/*
+ * Return value:
+ * 0 - success
+ * >0 - checksum was completed in software (slow path)
+ * <0 - failure
+ */
static inline int
virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
const struct virtio_net_hdr_v1_hash_tunnel *vhdr,
@@ -293,6 +546,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;
@@ -335,9 +589,12 @@ 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);
- if (ret)
+ if (ret < 0)
return ret;
+ if (!virtio_net_tnl_offsets_ok(skb, outer_isv6, outer_th))
+ return -EINVAL;
+
/* In case of USO, the inner protocol is still unknown and
* `inner_isv6` is just a guess, additional parsing is needed.
* The previous validation ensures that accessing an ipv4 inner
@@ -353,7 +610,7 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
}
skb_set_inner_protocol(skb, inner_isv6 ? htons(ETH_P_IPV6) :
- htons(ETH_P_IP));
+ htons(ETH_P_IP));
if (hdr->flags & VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM) {
if (!tnl_csum_negotiated)
return -EINVAL;
@@ -368,7 +625,18 @@ 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;
+
+ /*
+ * We know VIRTIO_NET_HDR_F_NEEDS_CSUM was set so __virtio_net_hdr_to_skb
+ * set CHECKSUM_PARTIAL.
+ */
+ inner_proto = inner_isv6 ? htons(ETH_P_IPV6) : htons(ETH_P_IP);
+ if (virtio_net_csum_partial_ok_at_offset(skb, gso_inner_type, inner_nh,
+ inner_proto))
+ return 0;
+
+ /* A tunnel header implies GSO, so drop. */
+ return -EINVAL;
}
/* Checksum-related fields validation for the driver */
--
MST
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH net v4 4/7] net: resolve uncommon partial checksums at ingress
2026-10-21 7:50 ` [PATCH net v4 4/7] net: resolve uncommon partial checksums at ingress Michael S. Tsirkin
@ 2026-10-21 16:13 ` Willem de Bruijn
0 siblings, 0 replies; 15+ messages in thread
From: Willem de Bruijn @ 2026-10-21 16:13 UTC (permalink / raw)
To: Michael S. Tsirkin, linux-kernel
Cc: Willem de Bruijn, Chris Arges, Björn Töpel,
Phil Sutter, Jason Wang, Eugenio Pérez, Xuan Zhuo,
Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, netdev, virtualization, Max Krasnyansky,
Rusty Russell, Paulos Yibelo
Michael S. Tsirkin wrote:
> As reported first by Paulos Yibelo <habte.yibelo@gmail.com> (after which
> others, credited below, found more ways the bug can affect users) 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 shouldn't happen.
>
> There's some risk here:
> - non-gso packets which can't be parsed get checksum calculated in
> software, adding performance overhead
> - gso packets which can't be parsed get dropped
>
> To mitigate the risks, the following patch will add a sysctl knob to
> bypass the resolution. This is reasonable if the setup is such that
> there is no protocol processing at all.
>
> UDP tunnel headers get the same handling on the inner header.
>
> Packets without VIRTIO_NET_HDR_F_NEEDS_CSUM are unaffected.
>
> Reported-by: Paulos Yibelo <habte.yibelo@gmail.com>
> Reported-by: Chris Arges <carges@cloudflare.com>
> Tested-by: Chris Arges <carges@cloudflare.com>
> Fixes: f43798c27684 ("tun: Allow GSO using virtio_net_hdr")
> Suggested-by: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> drivers/net/tap.c | 8 +-
> drivers/net/tun.c | 8 +-
> drivers/net/virtio_net.c | 3 +-
> include/linux/virtio_net.h | 276 ++++++++++++++++++++++++++++++++++++-
> 4 files changed, 281 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/net/tap.c b/drivers/net/tap.c
> index d7520622e809..f2a9bb443980 100644
> --- a/drivers/net/tap.c
> +++ b/drivers/net/tap.c
> @@ -653,9 +653,9 @@ static ssize_t tap_get_user(struct tap_queue *q, void *msg_control,
> rcu_read_unlock();
> drop_reason = SKB_DROP_REASON_DEV_HDR;
> goto err_kfree;
> - }
> - if (err > 0)
> + } else if (err > 0) {
> atomic64_inc(&tap->rx_csum_sw);
> + }
> }
>
> skb_probe_transport_header(skb);
> @@ -1089,9 +1089,9 @@ static int tap_get_user_xdp(struct tap_queue *q, struct xdp_buff *xdp)
> if (err < 0) {
> rcu_read_unlock();
> goto err_kfree;
> - }
> - if (err > 0)
> + } else if (err > 0) {
> atomic64_inc(&tap->rx_csum_sw);
> + }
All these belong in patch 1 that introduces these branches
> }
>
> skb_probe_transport_header(skb);
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH net v4 5/7] net: add a sysctl to opt out of virtio net hdr checksum validation
2026-10-21 7:49 ` [PATCH net v4 1/7] net: account for ingress-resolved virtio net hdr state Michael S. Tsirkin
` (2 preceding siblings ...)
2026-10-21 7:50 ` [PATCH net v4 4/7] net: resolve uncommon partial checksums at ingress Michael S. Tsirkin
@ 2026-10-21 7:50 ` Michael S. Tsirkin
2026-10-21 16:14 ` Willem de Bruijn
2026-10-21 7:50 ` [PATCH net v4 6/7] net: add ip4_parse() Michael S. Tsirkin
` (2 subsequent siblings)
6 siblings, 1 reply; 15+ messages in thread
From: Michael S. Tsirkin @ 2026-10-21 7:50 UTC (permalink / raw)
To: linux-kernel
Cc: Willem de Bruijn, Chris Arges, Björn Töpel,
Phil Sutter, Jason Wang, Eugenio Pérez, Xuan Zhuo,
Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, netdev, virtualization, Max Krasnyansky,
Rusty Russell, Jonathan Corbet, Shuah Khan, Randy Dunlap,
Simon Horman, linux-doc
The ingress validation as implemented by the previous
patch is stricter than what it replaced: GSO
packets whose checksum metadata matches no transport header are
dropped, and non-GSO ones get their checksum completed in place.
Such packets used to pass with the metadata unvalidated,
and with no overhead.
I judge that this should not affect any deployments, but there is still
some risk that it does. In case a deployment depends on them anyway,
provide a per-netns switch to get the old behavior back:
echo 0 > /proc/sys/net/core/virtio_net_hdr_validate
The default is validating.
Reviewed-by: Björn Töpel <bjorn@kernel.org>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
Documentation/admin-guide/sysctl/net.rst | 19 +++++++++
include/linux/virtio_net.h | 26 ++++++++++++
include/net/netns/core.h | 1 +
net/core/dev.c | 7 ++++
net/core/net_namespace.c | 1 +
net/core/sysctl_net_core.c | 50 ++++++++++++++++++++++++
6 files changed, 104 insertions(+)
diff --git a/Documentation/admin-guide/sysctl/net.rst b/Documentation/admin-guide/sysctl/net.rst
index e586e17fc7a5..3c0223575438 100644
--- a/Documentation/admin-guide/sysctl/net.rst
+++ b/Documentation/admin-guide/sysctl/net.rst
@@ -246,6 +246,25 @@ processes are dropped unless socket option SOF_TIMESTAMPING_OPT_TSONLY is set.
Default: 1 (on)
+virtio_net_hdr_validate
+-----------------------
+
+When processing packets with a virtio net header (through tun, tap, a
+packet socket or a virtio network device) that request checksum offload
+(VIRTIO_NET_HDR_F_NEEDS_CSUM), validate the packet: complete the checksum
+in software, or drop the packet where the header does not match it.
+
+When disabled, the header is trusted not to corrupt the packet, and the
+checksum is offloaded to hardware (or not verified if it never reaches
+hardware). This is only safe if no protocol is processing the packets.
+
+The setting is per network namespace, but only writable by the host
+root: root inside a network namespace created by an unprivileged user
+cannot disable the validation.
+
+Default: 1 (on)
+
+
wmem_default
------------
diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
index ec6f47ce1c97..b3bbecbd023c 100644
--- a/include/linux/virtio_net.h
+++ b/include/linux/virtio_net.h
@@ -5,6 +5,7 @@
#include <linux/if_vlan.h>
#include <linux/ip.h>
#include <linux/ipv6.h>
+#include <linux/jump_label.h>
#include <linux/netdevice.h>
#include <linux/udp.h>
#include <net/gso.h>
@@ -78,6 +79,19 @@ static inline struct iphdr *virtio_net_ip4_parse(struct sk_buff *skb,
*thoff = nhoff + iph->ihl * sizeof(u32);
return iph;
}
+/* Defined in net/core/dev.c, enabled while any network namespace
+ * has net.core.virtio_net_hdr_validate set to 0.
+ */
+DECLARE_STATIC_KEY_FALSE(virtio_net_hdr_validate_disabled_key);
+
+/* Failsafe: a namespace can disable the check with
+ * net.core.virtio_net_hdr_validate.
+ */
+static inline bool virtio_net_hdr_validate_disabled(struct sk_buff *skb)
+{
+ return static_branch_unlikely(&virtio_net_hdr_validate_disabled_key) &&
+ !READ_ONCE(dev_net(skb->dev)->core.sysctl_virtio_net_hdr_validate);
+}
/* Where does the L3 header start, and what protocol is it?
* Handles Ethernet and VLAN headers; other devices use the
@@ -186,6 +200,12 @@ virtio_net_csum_partial_ok_at_offset(struct sk_buff *skb, u8 hdr_gso_type,
unsigned int thoff;
int ip_proto;
+ /* Failsafe: a namespace opted out of the validation,
+ * trust the checksum metadata as before.
+ */
+ if (virtio_net_hdr_validate_disabled(skb))
+ return true;
+
if (!virtio_net_l4_start(skb, nhoff, proto, &thoff, &ip_proto))
return false;
@@ -228,6 +248,9 @@ virtio_net_csum_partial_ok(struct sk_buff *skb, u8 hdr_gso_type)
int nhoff;
__be16 proto;
+ if (virtio_net_hdr_validate_disabled(skb))
+ return true;
+
if (!virtio_net_l3_start(skb, &nhoff, &proto))
return false;
@@ -519,6 +542,9 @@ virtio_net_tnl_offsets_ok(struct sk_buff *skb, bool outer_isv6,
unsigned int thoff;
int ip_proto;
+ if (virtio_net_hdr_validate_disabled(skb))
+ return true;
+
if (!virtio_net_l3_start(skb, &nhoff, &proto))
return false;
if (outer_isv6 != (proto == htons(ETH_P_IPV6)))
diff --git a/include/net/netns/core.h b/include/net/netns/core.h
index 9ef3d70e5e9c..02ce232fe9a9 100644
--- a/include/net/netns/core.h
+++ b/include/net/netns/core.h
@@ -18,6 +18,7 @@ struct netns_core {
u8 sysctl_txrehash;
u8 sysctl_tstamp_allow_data;
u8 sysctl_bypass_prot_mem;
+ u8 sysctl_virtio_net_hdr_validate;
#ifdef CONFIG_PROC_FS
struct prot_inuse __percpu *prot_inuse;
diff --git a/net/core/dev.c b/net/core/dev.c
index e76762e29360..43b8823c83f6 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -128,6 +128,7 @@
#include <linux/if_vlan.h>
#include <linux/ip.h>
#include <net/ip.h>
+#include <linux/virtio_net.h>
#include <net/mpls.h>
#include <linux/ipv6.h>
#include <linux/in.h>
@@ -2389,6 +2390,12 @@ EXPORT_SYMBOL(tcf_sw_enabled_key);
DEFINE_STATIC_KEY_FALSE(netstamp_needed_key);
EXPORT_SYMBOL(netstamp_needed_key);
+
+/* Enabled while any network namespace has net.core.virtio_net_hdr_validate
+ * set to 0.
+ */
+DEFINE_STATIC_KEY_FALSE(virtio_net_hdr_validate_disabled_key);
+EXPORT_SYMBOL(virtio_net_hdr_validate_disabled_key);
#ifdef CONFIG_JUMP_LABEL
static atomic_t netstamp_needed_deferred;
static atomic_t netstamp_wanted;
diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
index da5f881fbd3b..8a21baaa7e6d 100644
--- a/net/core/net_namespace.c
+++ b/net/core/net_namespace.c
@@ -396,6 +396,7 @@ static __net_init void preinit_net_sysctl(struct net *net)
net->core.sysctl_optmem_max = 128 * 1024;
net->core.sysctl_txrehash = SOCK_TXREHASH_ENABLED;
net->core.sysctl_tstamp_allow_data = 1;
+ net->core.sysctl_virtio_net_hdr_validate = 1;
net->core.sysctl_txq_reselection = msecs_to_jiffies(1000);
}
diff --git a/net/core/sysctl_net_core.c b/net/core/sysctl_net_core.c
index eb35da3556f4..3e4037b82a97 100644
--- a/net/core/sysctl_net_core.c
+++ b/net/core/sysctl_net_core.c
@@ -18,6 +18,8 @@
#include <linux/slab.h>
#include <linux/sched/isolation.h>
#include <linux/hex.h>
+#include <linux/virtio_net.h>
+#include <linux/jump_label.h>
#include <net/ip.h>
#include <net/sock.h>
@@ -678,6 +680,43 @@ static struct ctl_table net_core_table[] = {
},
};
+/* Protects changes to virtio_net_hdr_validate_disabled_key and each
+ * net->core.sysctl_virtio_net_hdr_validate. The packet path reads
+ * the value without a lock.
+ */
+static DEFINE_MUTEX(virtio_net_hdr_validate_mutex);
+
+static int virtio_net_hdr_validate_sysctl(const struct ctl_table *table,
+ int write, void *buffer,
+ size_t *lenp, loff_t *ppos)
+{
+ u8 *value = table->data;
+ u8 old, new;
+ int ret;
+
+ /* Root in a user namespace must not be able to weaken the
+ * validation for the network namespace it controls.
+ */
+ if (write && !capable(CAP_NET_ADMIN))
+ return -EPERM;
+
+ mutex_lock(&virtio_net_hdr_validate_mutex);
+
+ old = READ_ONCE(*value);
+ ret = proc_dou8vec_minmax(table, write, buffer, lenp, ppos);
+ new = READ_ONCE(*value);
+ if (write && !ret && old != new) {
+ if (new)
+ static_branch_dec(&virtio_net_hdr_validate_disabled_key);
+ else
+ static_branch_inc(&virtio_net_hdr_validate_disabled_key);
+ }
+
+ mutex_unlock(&virtio_net_hdr_validate_mutex);
+
+ return ret;
+}
+
static const struct ctl_table netns_core_table[] = {
#if IS_ENABLED(CONFIG_RPS)
{
@@ -737,6 +776,15 @@ static const struct ctl_table netns_core_table[] = {
.extra1 = SYSCTL_ZERO,
.extra2 = SYSCTL_ONE
},
+ {
+ .procname = "virtio_net_hdr_validate",
+ .data = &init_net.core.sysctl_virtio_net_hdr_validate,
+ .maxlen = sizeof(u8),
+ .mode = 0644,
+ .proc_handler = virtio_net_hdr_validate_sysctl,
+ .extra1 = SYSCTL_ZERO,
+ .extra2 = SYSCTL_ONE
+ },
/* sysctl_core_net_init() will set the values after this
* to readonly in network namespaces
*/
@@ -841,6 +889,8 @@ static __net_exit void sysctl_core_net_exit(struct net *net)
tbl = net->core.sysctl_hdr->ctl_table_arg;
unregister_net_sysctl_table(net->core.sysctl_hdr);
BUG_ON(tbl == netns_core_table);
+ if (!net->core.sysctl_virtio_net_hdr_validate)
+ static_branch_dec(&virtio_net_hdr_validate_disabled_key);
#if IS_ENABLED(CONFIG_RPS)
kfree(net->core.rps_default_mask);
#endif
--
MST
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH net v4 5/7] net: add a sysctl to opt out of virtio net hdr checksum validation
2026-10-21 7:50 ` [PATCH net v4 5/7] net: add a sysctl to opt out of virtio net hdr checksum validation Michael S. Tsirkin
@ 2026-10-21 16:14 ` Willem de Bruijn
0 siblings, 0 replies; 15+ messages in thread
From: Willem de Bruijn @ 2026-10-21 16:14 UTC (permalink / raw)
To: Michael S. Tsirkin, linux-kernel
Cc: Willem de Bruijn, Chris Arges, Björn Töpel,
Phil Sutter, Jason Wang, Eugenio Pérez, Xuan Zhuo,
Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, netdev, virtualization, Max Krasnyansky,
Rusty Russell, Jonathan Corbet, Shuah Khan, Randy Dunlap,
Simon Horman, linux-doc
Michael S. Tsirkin wrote:
> The ingress validation as implemented by the previous
> patch is stricter than what it replaced: GSO
> packets whose checksum metadata matches no transport header are
> dropped, and non-GSO ones get their checksum completed in place.
> Such packets used to pass with the metadata unvalidated,
> and with no overhead.
>
> I judge that this should not affect any deployments, but there is still
> some risk that it does. In case a deployment depends on them anyway,
> provide a per-netns switch to get the old behavior back:
>
> echo 0 > /proc/sys/net/core/virtio_net_hdr_validate
>
> The default is validating.
>
> Reviewed-by: Björn Töpel <bjorn@kernel.org>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Willem de Bruijn <willemb@google.com>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH net v4 6/7] net: add ip4_parse()
2026-10-21 7:49 ` [PATCH net v4 1/7] net: account for ingress-resolved virtio net hdr state Michael S. Tsirkin
` (3 preceding siblings ...)
2026-10-21 7:50 ` [PATCH net v4 5/7] net: add a sysctl to opt out of virtio net hdr checksum validation Michael S. Tsirkin
@ 2026-10-21 7:50 ` Michael S. Tsirkin
2026-10-21 16:18 ` Willem de Bruijn
2026-10-21 7:50 ` [PATCH net v4 7/7] virtio_net: cleanup protocol calculation Michael S. Tsirkin
2026-10-21 16:08 ` [PATCH net v4 1/7] net: account for ingress-resolved virtio net hdr state Willem de Bruijn
6 siblings, 1 reply; 15+ messages in thread
From: Michael S. Tsirkin @ 2026-10-21 7:50 UTC (permalink / raw)
To: linux-kernel
Cc: Willem de Bruijn, Chris Arges, Björn Töpel,
Phil Sutter, 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,
Nikolay Aleksandrov, Ido Schimmel, Simon Horman, David Ahern,
netfilter-devel, coreteam, bridge
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.
As noted by Phil Sutter, there are more places that could use this
helper, e.g. ip_mc_check_iphdr(). They will be converted in a follow-up
patch.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Björn Töpel <bjorn@kernel.org>
Reviewed-by: Phil Sutter <phil@nwl.cc>
---
include/linux/ip.h | 30 +++++++++++++++++++++++++++++
include/linux/virtio_net.h | 28 +--------------------------
net/bridge/br_netfilter_hooks.c | 15 +++++----------
net/ipv4/ip_input.c | 13 +++++--------
net/ipv4/netfilter/nf_reject_ipv4.c | 13 +++++--------
5 files changed, 46 insertions(+), 53 deletions(-)
diff --git a/include/linux/ip.h b/include/linux/ip.h
index d11c25f5030a..aba37cd60212 100644
--- a/include/linux/ip.h
+++ b/include/linux/ip.h
@@ -56,4 +56,34 @@ static inline void iph_set_totlen(struct iphdr *iph, unsigned int len)
{
iph->tot_len = len <= IP_MAX_MTU ? htons(len) : 0;
}
+
+/**
+ * ip4_parse - pull and minimally validate an IPv4 header
+ * @skb: buffer to parse
+ * @nhoff: offset of the IPv4 header within the linear data
+ * @thoff: returns the transport header offset
+ *
+ * Only the fixed header is guaranteed linear on return: callers
+ * that need the options or the transport header must pull
+ * further.
+ *
+ * Return: the validated IPv4 header, or NULL if truncated or
+ * malformed.
+ */
+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 */
diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
index b3bbecbd023c..724ab8659501 100644
--- a/include/linux/virtio_net.h
+++ b/include/linux/virtio_net.h
@@ -53,32 +53,6 @@ static inline int virtio_net_hdr_set_proto(struct sk_buff *skb,
return 0;
}
-/* TODO: nothing virtio specific here, move to ip.h */
-
-/*
- * 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 *virtio_net_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;
-}
/* Defined in net/core/dev.c, enabled while any network namespace
* has net.core.virtio_net_hdr_validate set to 0.
*/
@@ -157,7 +131,7 @@ virtio_net_l4_start(struct sk_buff *skb, unsigned int nhoff, __be16 proto,
unsigned int *thoff, int *ip_proto)
{
if (proto == htons(ETH_P_IP)) {
- const struct iphdr *iph = virtio_net_ip4_parse(skb, nhoff, thoff);
+ const struct iphdr *iph = ip4_parse(skb, nhoff, thoff);
if (!iph || ip_is_fragment(iph))
return false;
diff --git a/net/bridge/br_netfilter_hooks.c b/net/bridge/br_netfilter_hooks.c
index 0a394e5f4391..e89da0e41542 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 9860178752b8..5adf01763e85 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 59ec465a9df9..42d639854ddf 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] 15+ messages in thread* Re: [PATCH net v4 6/7] net: add ip4_parse()
2026-10-21 7:50 ` [PATCH net v4 6/7] net: add ip4_parse() Michael S. Tsirkin
@ 2026-10-21 16:18 ` Willem de Bruijn
0 siblings, 0 replies; 15+ messages in thread
From: Willem de Bruijn @ 2026-10-21 16:18 UTC (permalink / raw)
To: Michael S. Tsirkin, linux-kernel
Cc: Willem de Bruijn, Chris Arges, Björn Töpel,
Phil Sutter, 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,
Nikolay Aleksandrov, Ido Schimmel, Simon Horman, David Ahern,
netfilter-devel, coreteam, bridge
Michael S. Tsirkin wrote:
> 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.
>
> As noted by Phil Sutter, there are more places that could use this
> helper, e.g. ip_mc_check_iphdr(). They will be converted in a follow-up
> patch.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Reviewed-by: Björn Töpel <bjorn@kernel.org>
> Reviewed-by: Phil Sutter <phil@nwl.cc>
> ---
> include/linux/ip.h | 30 +++++++++++++++++++++++++++++
> include/linux/virtio_net.h | 28 +--------------------------
> net/bridge/br_netfilter_hooks.c | 15 +++++----------
> net/ipv4/ip_input.c | 13 +++++--------
> net/ipv4/netfilter/nf_reject_ipv4.c | 13 +++++--------
> 5 files changed, 46 insertions(+), 53 deletions(-)
>
> diff --git a/include/linux/ip.h b/include/linux/ip.h
> index d11c25f5030a..aba37cd60212 100644
> --- a/include/linux/ip.h
> +++ b/include/linux/ip.h
> @@ -56,4 +56,34 @@ static inline void iph_set_totlen(struct iphdr *iph, unsigned int len)
> {
> iph->tot_len = len <= IP_MAX_MTU ? htons(len) : 0;
> }
> +
> +/**
> + * ip4_parse - pull and minimally validate an IPv4 header
This only pulls sizeof(struct iphdr).
To match function comment and generally be robust: pull again
if ihl != 0.
Apparently virtio_net_hdr_tnl_to_skb will set
skb->transport_header to the thoff reported here, which may be
beyond the end of the linear packet.
> + * @skb: buffer to parse
> + * @nhoff: offset of the IPv4 header within the linear data
> + * @thoff: returns the transport header offset
> + *
> + * Only the fixed header is guaranteed linear on return: callers
> + * that need the options or the transport header must pull
> + * further.
> + *
> + * Return: the validated IPv4 header, or NULL if truncated or
> + * malformed.
> + */
> +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 */
> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index b3bbecbd023c..724ab8659501 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h
> @@ -53,32 +53,6 @@ static inline int virtio_net_hdr_set_proto(struct sk_buff *skb,
> return 0;
> }
>
> -/* TODO: nothing virtio specific here, move to ip.h */
> -
> -/*
> - * 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 *virtio_net_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;
> -}
> /* Defined in net/core/dev.c, enabled while any network namespace
> * has net.core.virtio_net_hdr_validate set to 0.
> */
> @@ -157,7 +131,7 @@ virtio_net_l4_start(struct sk_buff *skb, unsigned int nhoff, __be16 proto,
> unsigned int *thoff, int *ip_proto)
> {
> if (proto == htons(ETH_P_IP)) {
> - const struct iphdr *iph = virtio_net_ip4_parse(skb, nhoff, thoff);
> + const struct iphdr *iph = ip4_parse(skb, nhoff, thoff);
>
> if (!iph || ip_is_fragment(iph))
> return false;
> diff --git a/net/bridge/br_netfilter_hooks.c b/net/bridge/br_netfilter_hooks.c
> index 0a394e5f4391..e89da0e41542 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 9860178752b8..5adf01763e85 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 59ec465a9df9..42d639854ddf 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] 15+ messages in thread
* [PATCH net v4 7/7] virtio_net: cleanup protocol calculation
2026-10-21 7:49 ` [PATCH net v4 1/7] net: account for ingress-resolved virtio net hdr state Michael S. Tsirkin
` (4 preceding siblings ...)
2026-10-21 7:50 ` [PATCH net v4 6/7] net: add ip4_parse() Michael S. Tsirkin
@ 2026-10-21 7:50 ` Michael S. Tsirkin
2026-10-21 16:08 ` [PATCH net v4 1/7] net: account for ingress-resolved virtio net hdr state Willem de Bruijn
6 siblings, 0 replies; 15+ messages in thread
From: Michael S. Tsirkin @ 2026-10-21 7:50 UTC (permalink / raw)
To: linux-kernel
Cc: Willem de Bruijn, Chris Arges, Björn Töpel,
Phil Sutter, 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 trivial cleanup removing a duplicated code line.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Björn Töpel <bjorn@kernel.org>
---
include/linux/virtio_net.h | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
index 724ab8659501..13f76bc505f0 100644
--- a/include/linux/virtio_net.h
+++ b/include/linux/virtio_net.h
@@ -609,8 +609,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;
@@ -630,7 +630,6 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
* We know VIRTIO_NET_HDR_F_NEEDS_CSUM was set so __virtio_net_hdr_to_skb
* set CHECKSUM_PARTIAL.
*/
- inner_proto = inner_isv6 ? htons(ETH_P_IPV6) : htons(ETH_P_IP);
if (virtio_net_csum_partial_ok_at_offset(skb, gso_inner_type, inner_nh,
inner_proto))
return 0;
--
MST
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH net v4 1/7] net: account for ingress-resolved virtio net hdr state
2026-10-21 7:49 ` [PATCH net v4 1/7] net: account for ingress-resolved virtio net hdr state Michael S. Tsirkin
` (5 preceding siblings ...)
2026-10-21 7:50 ` [PATCH net v4 7/7] virtio_net: cleanup protocol calculation Michael S. Tsirkin
@ 2026-10-21 16:08 ` Willem de Bruijn
2026-10-21 16:28 ` Michael S. Tsirkin
6 siblings, 1 reply; 15+ messages in thread
From: Willem de Bruijn @ 2026-10-21 16:08 UTC (permalink / raw)
To: Michael S. Tsirkin, linux-kernel
Cc: Willem de Bruijn, Chris Arges, Björn Töpel,
Phil Sutter, Jason Wang, Eugenio Pérez, Xuan Zhuo,
Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, netdev, virtualization, Max Krasnyansky,
Rusty Russell, Richard Weinberger, Anton Ivanov, Johannes Berg,
Simon Horman, linux-um
Michael S. Tsirkin wrote:
> The following patch will add ingress resolution of uncommon virtio net
> hdr checksum metadata (software checksum completion at ingress) which
> adds performance overhead to the packet processing.
>
> We should make it visible, so a hypothetical workload relying on
> uncommon metadata can be distinguished from none at all. This way issues
> in the parsing will be reported and not quietly pay the host-CPU
> checksum overhead instead of offload.
>
> Flip every call to virtio_net_hdr_to_skb to test for negative errno
> only. The um vector raw transport even ignored the return value
> altogether: rejected packets went on into the stack with their
> metadata unvalidated, so check it there as well.
>
> Have tun, tap and virtio_net count these events per device as
> rx_csum_sw, exposed via ethtool -S.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> arch/um/drivers/vector_transports.c | 4 ++-
> drivers/net/ipvlan/ipvtap.c | 36 +++++++++++++++++++++++++
> drivers/net/macvtap.c | 36 +++++++++++++++++++++++++
> drivers/net/tap.c | 8 ++++--
> drivers/net/tun.c | 41 +++++++++++++++++++++++++++--
> drivers/net/virtio_net.c | 15 ++++++++---
> include/linux/if_tap.h | 1 +
> net/packet/af_packet.c | 4 +--
> 8 files changed, 135 insertions(+), 10 deletions(-)
>
> diff --git a/arch/um/drivers/vector_transports.c b/arch/um/drivers/vector_transports.c
> index e1fb2a76fdff..8b7548458663 100644
> --- a/arch/um/drivers/vector_transports.c
> +++ b/arch/um/drivers/vector_transports.c
> @@ -210,7 +210,9 @@ static int raw_verify_header(
> return 1;
>
> skb_set_network_header(skb, ETH_HLEN);
> - virtio_net_hdr_to_skb(skb, vheader, virtio_legacy_is_little_endian());
> + if (virtio_net_hdr_to_skb(skb, vheader,
> + virtio_legacy_is_little_endian()) < 0)
> + return -1;
> return 0;
> }
>
> diff --git a/drivers/net/ipvlan/ipvtap.c b/drivers/net/ipvlan/ipvtap.c
> index 66c949d94261..9e2f1c7073d2 100644
> --- a/drivers/net/ipvlan/ipvtap.c
> +++ b/drivers/net/ipvlan/ipvtap.c
> @@ -1,5 +1,6 @@
> // SPDX-License-Identifier: GPL-2.0-only
> #include <linux/etherdevice.h>
> +#include <linux/ethtool.h>
> #include "ipvlan.h"
> #include <linux/if_vlan.h>
> #include <linux/if_tap.h>
> @@ -74,6 +75,32 @@ static void ipvtap_update_features(struct tap_dev *tap,
> netdev_update_features(vlan->dev);
> }
>
> +static int ipvtap_get_sset_count(struct net_device *dev, int sset)
> +{
> + return sset == ETH_SS_STATS ? 1 : -EOPNOTSUPP;
> +}
> +
> +static void ipvtap_get_strings(struct net_device *dev, u32 stringset,
> + u8 *data)
> +{
> + if (stringset == ETH_SS_STATS)
> + ethtool_puts(&data, "rx_csum_sw");
> +}
> +
> +static void ipvtap_get_ethtool_stats(struct net_device *dev,
> + struct ethtool_stats *stats,
> + u64 *data)
> +{
> + struct ipvtap_dev *vlantap = netdev_priv(dev);
> +
> + data[0] = atomic64_read(&vlantap->tap.rx_csum_sw);
> +}
> +
> +/* checkpatch warns but this can't be const: cloned from ipvlan's
> + * ops at newlink time, with the statistics above added.
> + */
> +static struct ethtool_ops ipvtap_ethtool_ops;
> +
> static int ipvtap_newlink(struct net_device *dev,
> struct rtnl_newlink_params *params,
> struct netlink_ext_ack *extack)
> @@ -91,6 +118,15 @@ static int ipvtap_newlink(struct net_device *dev,
> vlantap->tap.update_features = ipvtap_update_features;
> vlantap->tap.count_rx_dropped = ipvtap_count_rx_dropped;
>
> + /* ipvlan's ethtool ops are already installed at this point;
> + * clone them and add the tap statistics on top.
> + */
> + ipvtap_ethtool_ops = *dev->ethtool_ops;
> + ipvtap_ethtool_ops.get_sset_count = ipvtap_get_sset_count;
> + ipvtap_ethtool_ops.get_strings = ipvtap_get_strings;
> + ipvtap_ethtool_ops.get_ethtool_stats = ipvtap_get_ethtool_stats;
> + dev->ethtool_ops = &ipvtap_ethtool_ops;
> +
LLM:
This cloning pattern in ipvtap_newlink (and identically in macvtap_newlink below) has serious concurrency and hardening issues:
1. Concurrent Data Race: ipvtap_ethtool_ops is a single static global struct. Although newlink runs under rtnl_lock(), ethtool readers on
existing devices (e.g. netlink stats_prepare_data() and strset_prepare_data()) run under rcu_read_lock() without RTNL. Whenever a new
ipvtap/macvtap device is created, ipvtap_ethtool_ops = *dev->ethtool_ops overwrites the global ops table in-place, temporarily clearing
get_sset_count and get_strings to NULL before re-populating them. Any concurrent ethtool query on an existing device can observe NULL or
torn function pointers.
2. Writable Function Pointer Table: ethtool_ops contains function pointers and should reside in .rodata (const) to prevent kernel exploit
targets.
3. Hardcoded Set Count: ipvtap_get_sset_count returns 1 unconditionally; if the lower device ever adds ethtool stats in the future, they
are completely masked.
Instead of dynamically mutating a non-const global ops struct on every newlink, either:
• Export the base ops from macvlan/ipvlan so tap can declare a const struct ethtool_ops directly, or
• At the very least, initialize it once (e.g. guarded by a check or at module init) so it is not re-clobbered while readers are active.
Additionally, in general, ipvlan/macvlan/tun/tap changes are preferred
to be individual patches.
I don't think this is as important as the main patches 4 and 5. If you
want to submit quickly, I would drop/postpone these second order
features. Maybe go back to the previous tun-only counter for now, if
you consider that important enough.
> err = netdev_rx_handler_register(dev, tap_handle_frame, &vlantap->tap);
> if (err)
> return err;
> diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
> index cc975dfb7380..3288cc6ae6c2 100644
> --- a/drivers/net/macvtap.c
> +++ b/drivers/net/macvtap.c
> @@ -1,5 +1,6 @@
> // SPDX-License-Identifier: GPL-2.0-only
> #include <linux/etherdevice.h>
> +#include <linux/ethtool.h>
> #include <linux/if_macvlan.h>
> #include <linux/if_tap.h>
> #include <linux/if_vlan.h>
> @@ -78,6 +79,32 @@ static void macvtap_update_features(struct tap_dev *tap,
> netdev_update_features(vlan->dev);
> }
>
> +static int macvtap_get_sset_count(struct net_device *dev, int sset)
> +{
> + return sset == ETH_SS_STATS ? 1 : -EOPNOTSUPP;
> +}
> +
> +static void macvtap_get_strings(struct net_device *dev, u32 stringset,
> + u8 *data)
> +{
> + if (stringset == ETH_SS_STATS)
> + ethtool_puts(&data, "rx_csum_sw");
> +}
> +
> +static void macvtap_get_ethtool_stats(struct net_device *dev,
> + struct ethtool_stats *stats,
> + u64 *data)
> +{
> + struct macvtap_dev *vlantap = netdev_priv(dev);
> +
> + data[0] = atomic64_read(&vlantap->tap.rx_csum_sw);
> +}
> +
> +/* checkpatch warns but this can't be const: cloned from macvlan's
> + * ops at newlink time, with the statistics above added.
> + */
> +static struct ethtool_ops macvtap_ethtool_ops;
> +
> static int macvtap_newlink(struct net_device *dev,
> struct rtnl_newlink_params *params,
> struct netlink_ext_ack *extack)
> @@ -99,6 +126,15 @@ static int macvtap_newlink(struct net_device *dev,
> vlantap->tap.count_rx_dropped = macvtap_count_rx_dropped;
> vlantap->tap.update_features = macvtap_update_features;
>
> + /* macvlan's ethtool ops are already installed at this point;
> + * clone them and add the tap statistics on top.
> + */
> + macvtap_ethtool_ops = *dev->ethtool_ops;
> + macvtap_ethtool_ops.get_sset_count = macvtap_get_sset_count;
> + macvtap_ethtool_ops.get_strings = macvtap_get_strings;
> + macvtap_ethtool_ops.get_ethtool_stats = macvtap_get_ethtool_stats;
> + dev->ethtool_ops = &macvtap_ethtool_ops;
> +
> err = netdev_rx_handler_register(dev, tap_handle_frame, &vlantap->tap);
> if (err)
> return err;
> diff --git a/drivers/net/tap.c b/drivers/net/tap.c
> index ff67d99deb39..d7520622e809 100644
> --- a/drivers/net/tap.c
> +++ b/drivers/net/tap.c
> @@ -649,11 +649,13 @@ 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;
> }
> + if (err > 0)
> + atomic64_inc(&tap->rx_csum_sw);
> }
>
> skb_probe_transport_header(skb);
> @@ -1084,10 +1086,12 @@ 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;
> }
> + if (err > 0)
> + atomic64_inc(&tap->rx_csum_sw);
(repeat) reset err to 0 in all paths.
> }
>
> skb_probe_transport_header(skb);
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 242899f7fd07..19cb443c89b0 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;
> + atomic64_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,16 @@ 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) {
> + atomic64_inc(&tun->rx_csum_sw);
> + err = 0;
> + }
>
> if ((tun->flags & TUN_TYPE_MASK) == IFF_TAP)
> skb->protocol = eth_type_trans(skb, tun->dev);
> @@ -2605,12 +2611,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)
> + atomic64_inc(&tun->rx_csum_sw);
>
> skb->protocol = eth_type_trans(skb, tun->dev);
> skb_reset_network_header(skb);
> @@ -3739,7 +3748,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] = atomic64_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 daab43ac92ce..b0441bf8e875 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -125,6 +125,7 @@ struct virtnet_rq_stats {
> u64_stats_t packets;
> u64_stats_t bytes;
> u64_stats_t drops;
> + u64_stats_t rx_csum_sw;
> u64_stats_t xdp_packets;
> u64_stats_t xdp_tx;
> u64_stats_t xdp_redirects;
> @@ -158,6 +159,7 @@ static const struct virtnet_stat_desc virtnet_sq_stats_desc[] = {
>
> static const struct virtnet_stat_desc virtnet_rq_stats_desc[] = {
> VIRTNET_RQ_STAT("drops", drops),
> + VIRTNET_RQ_STAT("rx_csum_sw", rx_csum_sw),
> VIRTNET_RQ_STAT("xdp_packets", xdp_packets),
> VIRTNET_RQ_STAT("xdp_tx", xdp_tx),
> VIRTNET_RQ_STAT("xdp_redirects", xdp_redirects),
> @@ -2502,6 +2504,7 @@ static void virtnet_receive_done(struct virtnet_info *vi, struct receive_queue *
> {
> struct virtio_net_common_hdr *hdr;
> struct net_device *dev = vi->dev;
> + int ret;
>
> hdr = skb_vnet_common_hdr(skb);
> if (dev->features & NETIF_F_RXHASH && vi->has_rss_hash_report)
> @@ -2517,15 +2520,21 @@ static void virtnet_receive_done(struct virtnet_info *vi, struct receive_queue *
>
> skb->dev = dev;
> 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))) {
> + ret = virtio_net_hdr_tnl_to_skb(skb, &hdr->tnl_hdr, vi->rx_tnl,
> + vi->rx_tnl_csum,
> + virtio_is_little_endian(vi->vdev));
> + if (ret < 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,
> vi->rx_tnl, vi->rx_tnl_csum);
> goto frame_err;
> }
> + if (ret > 0) {
> + u64_stats_update_begin(&rq->stats.syncp);
> + u64_stats_inc(&rq->stats.rx_csum_sw);
> + u64_stats_update_end(&rq->stats.syncp);
> + }
Because rx_csum_sw was added to virtnet_rq_stats_desc[], virtnet_poll() already iterates through virtnet_rq_stats_desc[] and syncs each
field from the local stack stats into rq->stats at the end of the NAPI polling cycle inside a single u64_stats_update_begin/end(&rq-
>stats.syncp).
Calling u64_stats_update_begin/end(&rq->stats.syncp) directly per-packet in virtnet_receive_done() bypasses that batching pattern. While SW
csum is off the fast path, it is inconsistent with how other rq stats are handled.
>
> skb_record_rx_queue(skb, vq2rxq(rq->vq));
> skb->protocol = eth_type_trans(skb, dev);
> diff --git a/include/linux/if_tap.h b/include/linux/if_tap.h
> index 553552fa635c..4eac24bb1df7 100644
> --- a/include/linux/if_tap.h
> +++ b/include/linux/if_tap.h
> @@ -41,6 +41,7 @@ struct tap_dev {
> struct list_head queue_list;
> int numvtaps;
> int numqueues;
> + atomic64_t rx_csum_sw;
> netdev_features_t tap_features;
> int minor;
>
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 52ae3d875e97..e6aec9ecfa99 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] 15+ messages in thread* Re: [PATCH net v4 1/7] net: account for ingress-resolved virtio net hdr state
2026-10-21 16:08 ` [PATCH net v4 1/7] net: account for ingress-resolved virtio net hdr state Willem de Bruijn
@ 2026-10-21 16:28 ` Michael S. Tsirkin
0 siblings, 0 replies; 15+ messages in thread
From: Michael S. Tsirkin @ 2026-10-21 16:28 UTC (permalink / raw)
To: Willem de Bruijn
Cc: linux-kernel, Chris Arges, Björn Töpel, Phil Sutter,
Jason Wang, Eugenio Pérez, Xuan Zhuo, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
netdev, virtualization, Max Krasnyansky, Rusty Russell,
Richard Weinberger, Anton Ivanov, Johannes Berg, Simon Horman,
linux-um
On Wed, Oct 21, 2026 at 12:08:06PM -0400, Willem de Bruijn wrote:
> Michael S. Tsirkin wrote:
> > The following patch will add ingress resolution of uncommon virtio net
> > hdr checksum metadata (software checksum completion at ingress) which
> > adds performance overhead to the packet processing.
> >
> > We should make it visible, so a hypothetical workload relying on
> > uncommon metadata can be distinguished from none at all. This way issues
> > in the parsing will be reported and not quietly pay the host-CPU
> > checksum overhead instead of offload.
> >
> > Flip every call to virtio_net_hdr_to_skb to test for negative errno
> > only. The um vector raw transport even ignored the return value
> > altogether: rejected packets went on into the stack with their
> > metadata unvalidated, so check it there as well.
> >
> > Have tun, tap and virtio_net count these events per device as
> > rx_csum_sw, exposed via ethtool -S.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> > arch/um/drivers/vector_transports.c | 4 ++-
> > drivers/net/ipvlan/ipvtap.c | 36 +++++++++++++++++++++++++
> > drivers/net/macvtap.c | 36 +++++++++++++++++++++++++
> > drivers/net/tap.c | 8 ++++--
> > drivers/net/tun.c | 41 +++++++++++++++++++++++++++--
> > drivers/net/virtio_net.c | 15 ++++++++---
> > include/linux/if_tap.h | 1 +
> > net/packet/af_packet.c | 4 +--
> > 8 files changed, 135 insertions(+), 10 deletions(-)
> >
> > diff --git a/arch/um/drivers/vector_transports.c b/arch/um/drivers/vector_transports.c
> > index e1fb2a76fdff..8b7548458663 100644
> > --- a/arch/um/drivers/vector_transports.c
> > +++ b/arch/um/drivers/vector_transports.c
> > @@ -210,7 +210,9 @@ static int raw_verify_header(
> > return 1;
> >
> > skb_set_network_header(skb, ETH_HLEN);
> > - virtio_net_hdr_to_skb(skb, vheader, virtio_legacy_is_little_endian());
> > + if (virtio_net_hdr_to_skb(skb, vheader,
> > + virtio_legacy_is_little_endian()) < 0)
> > + return -1;
> > return 0;
> > }
> >
> > diff --git a/drivers/net/ipvlan/ipvtap.c b/drivers/net/ipvlan/ipvtap.c
> > index 66c949d94261..9e2f1c7073d2 100644
> > --- a/drivers/net/ipvlan/ipvtap.c
> > +++ b/drivers/net/ipvlan/ipvtap.c
> > @@ -1,5 +1,6 @@
> > // SPDX-License-Identifier: GPL-2.0-only
> > #include <linux/etherdevice.h>
> > +#include <linux/ethtool.h>
> > #include "ipvlan.h"
> > #include <linux/if_vlan.h>
> > #include <linux/if_tap.h>
> > @@ -74,6 +75,32 @@ static void ipvtap_update_features(struct tap_dev *tap,
> > netdev_update_features(vlan->dev);
> > }
> >
> > +static int ipvtap_get_sset_count(struct net_device *dev, int sset)
> > +{
> > + return sset == ETH_SS_STATS ? 1 : -EOPNOTSUPP;
> > +}
> > +
> > +static void ipvtap_get_strings(struct net_device *dev, u32 stringset,
> > + u8 *data)
> > +{
> > + if (stringset == ETH_SS_STATS)
> > + ethtool_puts(&data, "rx_csum_sw");
> > +}
> > +
> > +static void ipvtap_get_ethtool_stats(struct net_device *dev,
> > + struct ethtool_stats *stats,
> > + u64 *data)
> > +{
> > + struct ipvtap_dev *vlantap = netdev_priv(dev);
> > +
> > + data[0] = atomic64_read(&vlantap->tap.rx_csum_sw);
> > +}
> > +
> > +/* checkpatch warns but this can't be const: cloned from ipvlan's
> > + * ops at newlink time, with the statistics above added.
> > + */
> > +static struct ethtool_ops ipvtap_ethtool_ops;
> > +
> > static int ipvtap_newlink(struct net_device *dev,
> > struct rtnl_newlink_params *params,
> > struct netlink_ext_ack *extack)
> > @@ -91,6 +118,15 @@ static int ipvtap_newlink(struct net_device *dev,
> > vlantap->tap.update_features = ipvtap_update_features;
> > vlantap->tap.count_rx_dropped = ipvtap_count_rx_dropped;
> >
> > + /* ipvlan's ethtool ops are already installed at this point;
> > + * clone them and add the tap statistics on top.
> > + */
> > + ipvtap_ethtool_ops = *dev->ethtool_ops;
> > + ipvtap_ethtool_ops.get_sset_count = ipvtap_get_sset_count;
> > + ipvtap_ethtool_ops.get_strings = ipvtap_get_strings;
> > + ipvtap_ethtool_ops.get_ethtool_stats = ipvtap_get_ethtool_stats;
> > + dev->ethtool_ops = &ipvtap_ethtool_ops;
> > +
>
> LLM:
>
> This cloning pattern in ipvtap_newlink (and identically in macvtap_newlink below) has serious concurrency and hardening issues:
> 1. Concurrent Data Race: ipvtap_ethtool_ops is a single static global struct. Although newlink runs under rtnl_lock(), ethtool readers on
> existing devices (e.g. netlink stats_prepare_data() and strset_prepare_data()) run under rcu_read_lock() without RTNL. Whenever a new
> ipvtap/macvtap device is created, ipvtap_ethtool_ops = *dev->ethtool_ops overwrites the global ops table in-place, temporarily clearing
> get_sset_count and get_strings to NULL before re-populating them. Any concurrent ethtool query on an existing device can observe NULL or
> torn function pointers.
> 2. Writable Function Pointer Table: ethtool_ops contains function pointers and should reside in .rodata (const) to prevent kernel exploit
> targets.
> 3. Hardcoded Set Count: ipvtap_get_sset_count returns 1 unconditionally; if the lower device ever adds ethtool stats in the future, they
> are completely masked.
>
> Instead of dynamically mutating a non-const global ops struct on every newlink, either:
> • Export the base ops from macvlan/ipvlan so tap can declare a const struct ethtool_ops directly, or
> • At the very least, initialize it once (e.g. guarded by a check or at module init) so it is not re-clobbered while readers are active.
>
> Additionally, in general, ipvlan/macvlan/tun/tap changes are preferred
> to be individual patches.
>
> I don't think this is as important as the main patches 4 and 5. If you
> want to submit quickly, I would drop/postpone these second order
> features. Maybe go back to the previous tun-only counter for now, if
> you consider that important enough.
ok I will do tun only.
> > err = netdev_rx_handler_register(dev, tap_handle_frame, &vlantap->tap);
> > if (err)
> > return err;
> > diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
> > index cc975dfb7380..3288cc6ae6c2 100644
> > --- a/drivers/net/macvtap.c
> > +++ b/drivers/net/macvtap.c
> > @@ -1,5 +1,6 @@
> > // SPDX-License-Identifier: GPL-2.0-only
> > #include <linux/etherdevice.h>
> > +#include <linux/ethtool.h>
> > #include <linux/if_macvlan.h>
> > #include <linux/if_tap.h>
> > #include <linux/if_vlan.h>
> > @@ -78,6 +79,32 @@ static void macvtap_update_features(struct tap_dev *tap,
> > netdev_update_features(vlan->dev);
> > }
> >
> > +static int macvtap_get_sset_count(struct net_device *dev, int sset)
> > +{
> > + return sset == ETH_SS_STATS ? 1 : -EOPNOTSUPP;
> > +}
> > +
> > +static void macvtap_get_strings(struct net_device *dev, u32 stringset,
> > + u8 *data)
> > +{
> > + if (stringset == ETH_SS_STATS)
> > + ethtool_puts(&data, "rx_csum_sw");
> > +}
> > +
> > +static void macvtap_get_ethtool_stats(struct net_device *dev,
> > + struct ethtool_stats *stats,
> > + u64 *data)
> > +{
> > + struct macvtap_dev *vlantap = netdev_priv(dev);
> > +
> > + data[0] = atomic64_read(&vlantap->tap.rx_csum_sw);
> > +}
> > +
> > +/* checkpatch warns but this can't be const: cloned from macvlan's
> > + * ops at newlink time, with the statistics above added.
> > + */
> > +static struct ethtool_ops macvtap_ethtool_ops;
> > +
> > static int macvtap_newlink(struct net_device *dev,
> > struct rtnl_newlink_params *params,
> > struct netlink_ext_ack *extack)
> > @@ -99,6 +126,15 @@ static int macvtap_newlink(struct net_device *dev,
> > vlantap->tap.count_rx_dropped = macvtap_count_rx_dropped;
> > vlantap->tap.update_features = macvtap_update_features;
> >
> > + /* macvlan's ethtool ops are already installed at this point;
> > + * clone them and add the tap statistics on top.
> > + */
> > + macvtap_ethtool_ops = *dev->ethtool_ops;
> > + macvtap_ethtool_ops.get_sset_count = macvtap_get_sset_count;
> > + macvtap_ethtool_ops.get_strings = macvtap_get_strings;
> > + macvtap_ethtool_ops.get_ethtool_stats = macvtap_get_ethtool_stats;
> > + dev->ethtool_ops = &macvtap_ethtool_ops;
> > +
> > err = netdev_rx_handler_register(dev, tap_handle_frame, &vlantap->tap);
> > if (err)
> > return err;
> > diff --git a/drivers/net/tap.c b/drivers/net/tap.c
> > index ff67d99deb39..d7520622e809 100644
> > --- a/drivers/net/tap.c
> > +++ b/drivers/net/tap.c
> > @@ -649,11 +649,13 @@ 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;
> > }
> > + if (err > 0)
> > + atomic64_inc(&tap->rx_csum_sw);
> > }
> >
> > skb_probe_transport_header(skb);
> > @@ -1084,10 +1086,12 @@ 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;
> > }
> > + if (err > 0)
> > + atomic64_inc(&tap->rx_csum_sw);
>
> (repeat) reset err to 0 in all paths.
>
> > }
> >
> > skb_probe_transport_header(skb);
> > diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> > index 242899f7fd07..19cb443c89b0 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;
> > + atomic64_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,16 @@ 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) {
> > + atomic64_inc(&tun->rx_csum_sw);
> > + err = 0;
> > + }
> >
> > if ((tun->flags & TUN_TYPE_MASK) == IFF_TAP)
> > skb->protocol = eth_type_trans(skb, tun->dev);
> > @@ -2605,12 +2611,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)
> > + atomic64_inc(&tun->rx_csum_sw);
> >
> > skb->protocol = eth_type_trans(skb, tun->dev);
> > skb_reset_network_header(skb);
> > @@ -3739,7 +3748,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] = atomic64_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 daab43ac92ce..b0441bf8e875 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -125,6 +125,7 @@ struct virtnet_rq_stats {
> > u64_stats_t packets;
> > u64_stats_t bytes;
> > u64_stats_t drops;
> > + u64_stats_t rx_csum_sw;
> > u64_stats_t xdp_packets;
> > u64_stats_t xdp_tx;
> > u64_stats_t xdp_redirects;
> > @@ -158,6 +159,7 @@ static const struct virtnet_stat_desc virtnet_sq_stats_desc[] = {
> >
> > static const struct virtnet_stat_desc virtnet_rq_stats_desc[] = {
> > VIRTNET_RQ_STAT("drops", drops),
> > + VIRTNET_RQ_STAT("rx_csum_sw", rx_csum_sw),
> > VIRTNET_RQ_STAT("xdp_packets", xdp_packets),
> > VIRTNET_RQ_STAT("xdp_tx", xdp_tx),
> > VIRTNET_RQ_STAT("xdp_redirects", xdp_redirects),
> > @@ -2502,6 +2504,7 @@ static void virtnet_receive_done(struct virtnet_info *vi, struct receive_queue *
> > {
> > struct virtio_net_common_hdr *hdr;
> > struct net_device *dev = vi->dev;
> > + int ret;
> >
> > hdr = skb_vnet_common_hdr(skb);
> > if (dev->features & NETIF_F_RXHASH && vi->has_rss_hash_report)
> > @@ -2517,15 +2520,21 @@ static void virtnet_receive_done(struct virtnet_info *vi, struct receive_queue *
> >
> > skb->dev = dev;
> > 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))) {
> > + ret = virtio_net_hdr_tnl_to_skb(skb, &hdr->tnl_hdr, vi->rx_tnl,
> > + vi->rx_tnl_csum,
> > + virtio_is_little_endian(vi->vdev));
> > + if (ret < 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,
> > vi->rx_tnl, vi->rx_tnl_csum);
> > goto frame_err;
> > }
> > + if (ret > 0) {
> > + u64_stats_update_begin(&rq->stats.syncp);
> > + u64_stats_inc(&rq->stats.rx_csum_sw);
> > + u64_stats_update_end(&rq->stats.syncp);
> > + }
>
> Because rx_csum_sw was added to virtnet_rq_stats_desc[], virtnet_poll() already iterates through virtnet_rq_stats_desc[] and syncs each
> field from the local stack stats into rq->stats at the end of the NAPI polling cycle inside a single u64_stats_update_begin/end(&rq-
> >stats.syncp).
> Calling u64_stats_update_begin/end(&rq->stats.syncp) directly per-packet in virtnet_receive_done() bypasses that batching pattern. While SW
> csum is off the fast path, it is inconsistent with how other rq stats are handled.
>
> >
> > skb_record_rx_queue(skb, vq2rxq(rq->vq));
> > skb->protocol = eth_type_trans(skb, dev);
> > diff --git a/include/linux/if_tap.h b/include/linux/if_tap.h
> > index 553552fa635c..4eac24bb1df7 100644
> > --- a/include/linux/if_tap.h
> > +++ b/include/linux/if_tap.h
> > @@ -41,6 +41,7 @@ struct tap_dev {
> > struct list_head queue_list;
> > int numvtaps;
> > int numqueues;
> > + atomic64_t rx_csum_sw;
> > netdev_features_t tap_features;
> > int minor;
> >
> > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> > index 52ae3d875e97..e6aec9ecfa99 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] 15+ messages in thread