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

[sashiko-bot@kernel.org]

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 <mst@redhat.com>

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/cover.v4.git.mst@redhat.com?part=1