On Tue, Oct 20, 2026 at 04:02:26PM -0400, Willem de Bruijn wrote:
> Michael S. Tsirkin wrote:
> > On Tue, Oct 20, 2026 at 03:27:46PM -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, and have tun count these events per device as rx_csum_sw, exposed
> > > > via ethtool -S.
> > > >
> > > > Assisted-by: LLM
> > > > Signed-off-by: Michael S. Tsirkin <[email protected]>
> > > > ---
> > > > drivers/net/tap.c | 4 ++--
> > > > drivers/net/tun.c | 41 ++++++++++++++++++++++++++++++++++++++--
> > > > drivers/net/virtio_net.c | 2 +-
> > > > net/packet/af_packet.c | 4 ++--
> > > > 4 files changed, 44 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/drivers/net/tap.c b/drivers/net/tap.c
> > > > index ff67d99deb39..73e883394446 100644
> > > > --- a/drivers/net/tap.c
> > > > +++ b/drivers/net/tap.c
> > > > @@ -649,7 +649,7 @@ static ssize_t tap_get_user(struct tap_queue *q,
> > > > void *msg_control,
> > > >
> > > > if (vnet_hdr_len) {
> > > > err = tun_vnet_hdr_to_skb(q->flags, skb, &vnet_hdr);
> > > > - if (err) {
> > > > + if (err < 0) {
> > > > rcu_read_unlock();
> > > > drop_reason = SKB_DROP_REASON_DEV_HDR;
> > > > goto err_kfree;
> > > > @@ -1084,7 +1084,7 @@ static int tap_get_user_xdp(struct tap_queue *q,
> > > > struct xdp_buff *xdp)
> > > >
> > > > if (vnet_hdr_len) {
> > > > err = tun_vnet_hdr_to_skb(q->flags, skb, gso);
> > > > - if (err) {
> > > > + if (err < 0) {
> > > > rcu_read_unlock();
> > > > goto err_kfree;
> > > > }
> > >
> > > similar to tun, should tap count this event?
> > >
> > > > 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;
> > >
> > > is a normal (and cheaper on 32b platforms) atomic_t or atomic_long_t
> > > sufficient?
> >
> > well it can overflow easily. i don't know that we care that a slow
> > path will be even a bit slower on these ancient platforms. correct
> > reporting seems more important.
>
> Ok. I would hope that monitoring catches this before it hits 4B, but
> agreed that as slow path it doesn't matter.
>
> >
> > > > 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) {
> > >
> > > } else 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)
> > >
> > > same
> > >
> > > > + atomic64_inc(&tun->rx_csum_sw);
> > >
> > > maybe reset ret = 0 in all these branches
> > >
> > > >
> > > > 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..2e6fd81df9c4 100644
> > > > --- a/drivers/net/virtio_net.c
> > > > +++ b/drivers/net/virtio_net.c
> > > > @@ -2519,7 +2519,7 @@ static void virtnet_receive_done(struct
> > > > virtnet_info *vi, struct receive_queue *
> > > > skb_set_network_header(skb, ETH_HLEN);
> > > > if (virtio_net_hdr_tnl_to_skb(skb, &hdr->tnl_hdr, vi->rx_tnl,
> > > > vi->rx_tnl_csum,
> > > > -
> > > > virtio_is_little_endian(vi->vdev))) {
> > > > +
> > > > virtio_is_little_endian(vi->vdev)) < 0) {
> > > > net_warn_ratelimited("%s: bad gso: type: %x, size: %u,
> > > > flags %x tunnel %d tnl csum %d\n",
> > > > dev->name, hdr->hdr.gso_type,
> > > > hdr->hdr.gso_size, hdr->hdr.flags,
> > >
> > > should virtio_net count these too? Or do we assume that devices are
> > > more trustworthy, so unlikely to cause this issue?
> >
> > With memory encryption, from an untrusted host?
> > i can add that.
>
> I honestly don't know if it's worth the effort. Just curious whether
> you had purposely skipped this and tap.
more like it slipped my mind.
> >
> > > > 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
> > > >
> > >
> >
>
