Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] virtio_net: duplicate rx_ prefix for rx_csum_sw in virtnet_rq_stats_desc - [Medium] net: live ethtool_ops overwritten in ipvtap_newlink() and macvtap_newlink() --
commit aec871fb755137e08ae8b9205b5e122143828b6d Author: Michael S. Tsirkin <[email protected]> net: account for ingress-resolved virtio net hdr state This commit updates callers of virtio_net_hdr_to_skb() and virtio_net_hdr_tnl_to_skb() to check only for negative return values as errors, and adds an rx_csum_sw ethtool statistic to tun, tap (ipvtap and macvtap), and virtio_net to count packets requiring software checksum resolution at ingress. > diff --git a/drivers/net/ipvlan/ipvtap.c b/drivers/net/ipvlan/ipvtap.c > index 66c949d942610..9e2f1c7073d2a 100644 > --- a/drivers/net/ipvlan/ipvtap.c > +++ b/drivers/net/ipvlan/ipvtap.c [ ... ] > +/* 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; [ ... ] > diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c > index cc975dfb73805..3288cc6ae6c20 100644 > --- a/drivers/net/macvtap.c > +++ b/drivers/net/macvtap.c [ ... ] > +/* 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; [Severity: Medium] Can overwriting the shared global ipvtap_ethtool_ops in drivers/net/ipvlan/ipvtap.c:ipvtap_newlink() and macvtap_ethtool_ops in drivers/net/macvtap.c:macvtap_newlink() race with concurrent readers on already-registered devices? When the first ipvtap or macvtap interface is created and registered, its dev->ethtool_ops points to the shared static ipvtap_ethtool_ops or macvtap_ethtool_ops structure. When a second ipvtap or macvtap interface is created via rtnl_newlink_create(), ipvtap_setup() or macvtap_setup() first sets the new device's dev->ethtool_ops to &ipvlan_ethtool_ops or &macvlan_ethtool_ops (where get_sset_count, get_strings, and get_ethtool_stats are null). Then ipvtap_newlink() or macvtap_newlink() copies *dev->ethtool_ops over the shared global ipvtap_ethtool_ops or macvtap_ethtool_ops structure in-place before restoring the three statistics callbacks. Meanwhile, several paths read dev->ethtool_ops on the first registered interface without holding rtnl_lock(): net/ethtool/common.c:__ethtool_get_ts_info() { ... const struct ethtool_ops *ops = dev->ethtool_ops; struct phy_device *phydev = dev->phydev; ... } else if (ops->get_ts_info) { err = ops->get_ts_info(dev, info); ... } which is called under rcu_read_lock() from sock_timestamping_bind_phc()->ethtool_get_phc_vclocks(), as well as net/ethtool/netlink.c:ethnl_default_set_doit() calling ops->set_validate() before acquiring rtnl_lock(), and net/core/net-sysfs.c:speed_show() and duplex_show() reading netdev->ethtool_ops->get_link_ksettings before sysfs_rtnl_lock(). Could ipvtap_ethtool_ops and macvtap_ethtool_ops be initialized once at module init time (for example in ipvtap_init() and macvtap_init()) instead of being mutated on each newlink call? [ ... ] > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index daab43ac92ce4..b0441bf8e8750 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), [Severity: Medium] Will defining the descriptor string as "rx_csum_sw" in drivers/net/virtio_net.c:virtnet_rq_stats_desc cause ethtool -S to report a duplicated "rx_" prefix? When userspace queries ethtool -S strings via virtnet_get_strings()->virtnet_get_stats_string()->virtnet_stats_sprintf(), virtnet_get_stats_string() automatically prepends "rx_" and "rx%u_" to every entry in virtnet_rq_stats_desc: drivers/net/virtio_net.c:virtnet_get_stats_string() { ... if (type == VIRTNET_Q_TYPE_RX) { fmt = "rx%u_%s"; noq_fmt = "rx_%s"; desc = &virtnet_rq_stats_desc[0]; num = ARRAY_SIZE(virtnet_rq_stats_desc); virtnet_stats_sprintf(&p, fmt, noq_fmt, num, qid, desc); ... } Because of this formatting, "rx_csum_sw" becomes "rx_rx_csum_sw" for the device total and "rx0_rx_csum_sw" for the per-queue counters. Should the descriptor string in virtnet_rq_stats_desc be "csum_sw" to match the other entries and produce "rx_csum_sw"? [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
