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 <[email protected]>
> > ---
> > 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
> >
>
