Re: [PATCH net-next v6 3/3] net: libwx: support vf per-queue statistics via ethtool -S

[Paolo Abeni <pabeni@redhat.com>]

From: AI Reviewer <ai@example.com>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
Now I have enough information to produce the report.

commit e59c58bb080afb34cac34c91b8c320a1f05134e6
Author: Mengyuan Lou <mengyuanlou@net-swift.com>

net: libwx: support vf per-queue statistics via ethtool -S

The patch wires wx_get_sset_count, wx_get_strings, and wx_get_ethtool_stats
into wx_ethtool_ops_vf, adds a small vf-only global stats table, and starts
calling wx_update_stats() from wxvf_service_task. It also adds NULL checks
for wx->rx_ring[i] / wx->tx_ring[i] inside wx_update_stats.

> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c b/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c
> index 22037f015ded..f72908630ea4 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_ethtool.c

[ ... ]

> @@ -114,7 +133,7 @@ void wx_get_strings(struct net_device *netdev, u32 stringset, u8 *data)
>  			ethtool_sprintf(&p, "tx_queue_%u_packets", i);
>  			ethtool_sprintf(&p, "tx_queue_%u_bytes", i);
>  		}
> -		for (i = 0; i < WX_NUM_RX_QUEUES; i++) {
> +		for (i = 0; i < WX_NUM_RX_QUEUES(netdev); i++) {
>  			ethtool_sprintf(&p, "rx_queue_%u_packets", i);
>  			ethtool_sprintf(&p, "rx_queue_%u_bytes", i);
>  		}

[ ... ]

> @@ -852,6 +878,9 @@ static const struct ethtool_ops wx_ethtool_ops_vf = {
>  	.set_coalesce		= wx_set_coalesce,
>  	.get_ts_info		= ethtool_op_get_ts_info,
>  	.get_link_ksettings	= wx_get_link_ksettings_vf,
> +	.get_sset_count		= wx_get_sset_count,
> +	.get_strings		= wx_get_strings,
> +	.get_ethtool_stats	= wx_get_ethtool_stats,
>  };

[Medium]
Before this change wx_ethtool_ops_vf had no get_sset_count / get_strings /
get_ethtool_stats callbacks, so the VF exposed no ethtool -S output at all.
Wiring these in also picks up the shared per-queue string loop that emits
tx_queue_%u_packets / tx_queue_%u_bytes and rx_queue_%u_packets /
rx_queue_%u_bytes.

Per-queue TX/RX packet and byte counters are a category that already has a
standard uAPI (netdev_stat_ops with queue_stats_rx / queue_stats_tx,
surfaced by netlink and ynl). Documentation/networking/statistics.rst and
the ethtool -S guidance state that new counters covering values that have a
standard uAPI should not be added to ethtool -S even when the driver does
not yet implement the standard interface.

The wangxun tree does not implement netdev_stat_ops (no
get_base_stats / get_queue_stats_rx / get_queue_stats_tx callbacks are
present under drivers/net/ethernet/wangxun/).

Would it be preferable to expose the per-queue counters on the VF via
netdev_stat_ops instead of adding them to ethtool -S here?

>  void wx_set_ethtool_ops_vf(struct net_device *netdev)
> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_hw.c b/drivers/net/ethernet/wangxun/libwx/wx_hw.c
> index 260e14d5d541..349afbe60132 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_hw.c
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_hw.c
> @@ -2886,6 +2886,8 @@ void wx_update_stats(struct wx *wx)
>  	for (i = 0; i < wx->num_rx_queues; i++) {
>  		struct wx_ring *rx_ring = wx->rx_ring[i];
>  
> +		if (!rx_ring)
> +			continue;
>  		non_eop_descs += rx_ring->rx_stats.non_eop_descs;

[ ... ]

> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_vf_common.c b/drivers/net/ethernet/wangxun/libwx/wx_vf_common.c
> index 26de78e9a69e..1d84fe35d7d9 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_vf_common.c
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_vf_common.c
> @@ -409,6 +410,7 @@ static void wxvf_service_task(struct work_struct *work)
>  
>  	wxvf_link_config_subtask(wx);
>  	wxvf_reset_subtask(wx);
> +	wx_update_stats(wx);
>  	wx_service_event_complete(wx);
>  }

[High]
Can this call race with VF PCI suspend and shutdown to produce a
use-after-free on wx->rx_ring[i] / wx->tx_ring[i]?

Both ngbevf and txgbevf use wxvf_suspend as their PM suspend callback and
wxvf_shutdown (which calls wxvf_suspend) as their pci_driver ->shutdown
callback:

    static DEFINE_SIMPLE_DEV_PM_OPS(ngbevf_pm_ops, wxvf_suspend, wxvf_resume);
    ...
    .shutdown = wxvf_shutdown,

wxvf_suspend() in wx_vf_common.c does:

    int wxvf_suspend(struct device *dev_d)
    {
        struct pci_dev *pdev = to_pci_dev(dev_d);
        struct wx *wx = pci_get_drvdata(pdev);

        netif_device_detach(wx->netdev);
        wx_clear_interrupt_scheme(wx);
        pci_disable_device(pdev);

        return 0;
    }

wx_clear_interrupt_scheme() ends up freeing the q_vectors (via
kfree_rcu), and the ring allocations are embedded in the q_vector, so
wx->rx_ring[i] / wx->tx_ring[i] end up pointing at memory that will be
freed after an RCU grace period.

wxvf_suspend / wxvf_shutdown do not call timer_shutdown_sync on
wx->service_timer or cancel_work_sync on wx->service_task, and they do
not set WX_STATE_DOWN.

With this hunk in place, the service_task now walks wx->rx_ring[] and
wx->tx_ring[]:

    void wx_update_stats(struct wx *wx)
    {
        ...
        for (i = 0; i < wx->num_rx_queues; i++) {
            struct wx_ring *rx_ring = wx->rx_ring[i];

            if (!rx_ring)
                continue;
            non_eop_descs += rx_ring->rx_stats.non_eop_descs;
            ...
        }
        ...
        for (i = 0; i < wx->num_tx_queues; i++) {
            struct wx_ring *tx_ring = wx->tx_ring[i];

            if (!tx_ring)
                continue;
            restart_queue += tx_ring->tx_stats.restart_queue;
            ...
        }

If the service_task latches a non-NULL rx_ring / tx_ring pointer just
before wx_clear_interrupt_scheme runs, the added NULL check does not
help, and the subsequent ->rx_stats / ->tx_stats reads touch memory
that has been kfree_rcu'd. wxvf_remove() covers this by calling
timer_shutdown_sync + cancel_work_sync before wx_clear_interrupt_scheme,
but wxvf_suspend / wxvf_shutdown do not.

Should wxvf_suspend (and hence wxvf_shutdown) quiesce the service_timer
and service_task, or set WX_STATE_DOWN, before wx_clear_interrupt_scheme
now that wx_update_stats is on the service_task's call path?
-- 
This is an AI-generated review.