Re: [PATCH net] net/mlx5e: Fix queue stats access to queues without stats

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] When HTB leaves are configured and the netdev is brought down,
  the device-wide qstats lose all HTB TX counters.

--

net/mlx5e: Fix queue stats access to queues without stats

This commit fixes NULL pointer dereferences in the mlx5e per-queue
netdev-genl stats callbacks. The core calls them for every queue below
real_num_{rx,tx}_queues, and some of those queues have no stats behind
them. The RX callback now returns early for queues at or above stats_nch,
and the TX callback returns early when txq2sq_stats[i] is NULL.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> index c3c2c5720c6f3..72bca10c579df 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c

[ ... ]

> @@ -5521,6 +5521,9 @@ static void mlx5e_get_queue_stats_tx(struct net_device *dev, int i,
>  	 * inactive sqs.
>  	 */
>  	sq_stats = priv->txq2sq_stats[i];
> +	if (!sq_stats)
> +		return;
> +

[Severity: Medium]
With HTB offload leaves configured, do the HTB TX counters drop out of
the device-wide qstats totals while the netdev is down?

The early return depends on the comment above it, which says
get_base_stats "takes care of inactive sqs". For HTB SQs that does not
seem to hold.

On ifdown, mlx5e_deactivate_channel()->mlx5e_qos_deactivate_queues()
clears the entry for every HTB txq:

drivers/net/ethernet/mellanox/mlx5/core/en/qos.c:mlx5e_qos_deactivate_queues() {
    ...
		c->priv->txq2sq[txq_ix] = NULL;
		c->priv->txq2sq_stats[txq_ix] = NULL;
    ...
}

real_num_tx_queues still includes those txqs, because
mlx5e_update_tx_netdev_queues() sets it to nch * ntc +
mlx5e_htb_cur_leaf_nodes(priv->htb) whether or not the channels are
open. So netdev_nl_stats_by_netdev() still calls
mlx5e_get_queue_stats_tx() for those indices, and with this change it
now returns without reporting anything.

mlx5e_get_base_stats() only walks channel_stats[]->sq[tc] and
ptp_stats.sq[tc]:

	for (i = 0; i < nch; i++) {
		struct mlx5e_channel_stats *channel_stats = priv->channel_stats[i];
		...

It never reads priv->htb_qos_sq_stats[], which is where the persistent
HTB counters live. So tx packets, bytes, hw-gso-packets, hw-gso-bytes,
csum-none, stop and wake for HTB queues disappear from the total on
ifdown and come back on ifup.

That looks like it would make device qstats go backwards on ifdown,
which tools/testing/selftests/drivers/net/stats.py check_down() tests
for:

    ksft_ge(qstat2[k], qstat[k], comment=f"{k} went backwards on device down")

The qstats totals would also stop matching the SW/rtnl totals, since
mlx5e_stats_grp_sw_update_stats_qos() still adds every
htb_qos_sq_stats[i].

Should mlx5e_get_base_stats() add priv->htb_qos_sq_stats[] for HTB
queues whose txq2sq_stats entry is NULL? If not, could the comment
above be corrected to document this gap?

>  	stats->packets = sq_stats->packets;
>  	stats->bytes = sq_stats->bytes;
>

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261104-mlx5e-qstats-null-v1-1-a6a003245cec%40debian.org