Re: [PATCH net-next v6 14/15] net: ethernet: cortina: Scope RX statistics updates

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: ethernet: cortina: Scope RX statistics updates

This patch narrows the rx_stats_syncp writer sections in gmac_napi_poll()
so they no longer span gmac_rx(). The writer sections now wrap only the
individual counter updates in gmac_skb_if_good_frame() and gmac_rx(). It
also adds u64_stats_init() calls for the tx, rx and ir syncp objects in
gemini_ethernet_port_probe().

> diff --git a/drivers/net/ethernet/cortina/gemini.c b/drivers/net/ethernet/cortina/gemini.c
> index 5e89057bb2fb6..c32e9574c8c95 100644
> --- a/drivers/net/ethernet/cortina/gemini.c
> +++ b/drivers/net/ethernet/cortina/gemini.c

[ ... ]

> @@ -2863,6 +2872,9 @@ static int gemini_ethernet_port_probe(struct platform_device *pdev)
>  	}
>  
>  	port = netdev_priv(netdev);
> +	u64_stats_init(&port->tx_stats_syncp);
> +	u64_stats_init(&port->rx_stats_syncp);
> +	u64_stats_init(&port->ir_stats_syncp);

[Severity: High]
This patch didn't introduce this, but since it touches tx_stats_syncp:
can the TX reader loop in gmac_get_ethtool_stats() write past the end of
the ethtool stats buffer?

drivers/net/ethernet/cortina/gemini.c:gmac_get_ethtool_stats() {
    ...
	/* Racing with TX start_xmit */
	do {
		p = values;
		start = u64_stats_fetch_begin(&port->tx_stats_syncp);

		for (i = 0; i < TX_MAX_FRAGS; i++) {
			*values++ = port->tx_frag_stats[i];
			port->tx_frag_stats[i] = 0;
		}
		*values++ = port->tx_frags_linearized;
		*values++ = port->tx_hw_csummed;

	} while (u64_stats_fetch_retry(&port->tx_stats_syncp, start));
}

The loop resets p at the top of each pass, but the body writes through
values++ and never uses p. After the first pass, values points at the end
of the GMAC_STATS_NUM u64 array.

ethtool_get_stats() allocates that array with
vzalloc(n_stats * sizeof(u64)). If u64_stats_fetch_retry() forces a
retry, the next pass writes another TX_MAX_FRAGS + 2 u64 values (144
bytes) past the end. Each further retry writes another 144 bytes, and
nothing bounds the number of retries.

Gemini is 32-bit, so the seqcount is real there. A transmit that runs
during the read can bump tx_stats_syncp and trigger the retry, for
example:

gmac_start_xmit()->gmac_clean_txq()
    u64_stats_update_begin(&port->tx_stats_syncp);
    port->tx_frag_stats[nfrags]++;
    u64_stats_update_end(&port->tx_stats_syncp);

ETHTOOL_GSTATS is one of the commands that does not need privileges.

This loop has two more problems. port->tx_hw_csummed is read under
tx_stats_syncp, but gmac_clean_txq() writes it under ir_stats_syncp:

	u64_stats_update_begin(&port->ir_stats_syncp);
	...
	port->tx_hw_csummed += hwchksum;
	u64_stats_update_end(&port->ir_stats_syncp);

So this reader has no protection against torn reads of that counter.

Also, the reader clears port->tx_frag_stats[i] = 0 outside any writer
section. That races with the tx_frag_stats[nfrags]++ in gmac_clean_txq()
and can lose increments. It also resets the counters on every read.

Should the loop write through *p++ like the RX and MIB loops above it,
read tx_hw_csummed under ir_stats_syncp, and stop zeroing tx_frag_stats
from the reader?

>  	SET_NETDEV_DEV(netdev, dev);
>  	port->netdev = netdev;
>  	port->id = id;