Re: [PATCH net v4 1/7] net: account for ingress-resolved virtio net hdr state

["Michael S. Tsirkin" <mst@redhat.com>]

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