Re: [PATCH net v4 1/4] gve: Fix race between stats query and queue reconfig

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 2 · 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] At this commit, the new priv->stats_lock is not taken by the
  single-queue (queue-mgmt API) writers of priv->rx[idx].
- [Medium] At this commit, the device-level qstats totals drop to 0 once
  the interface is down.

--

gve: Fix race between stats query and queue reconfig

This commit adds priv->stats_lock to serialize the stats readers
(gve_get_stats(), gve_handle_report_stats() and the per-queue qstats
callbacks) against queue teardown in gve_queues_mem_remove(). Queue
pointers are now cleared under the lock before the ring memory is freed.
The per-queue callbacks also return early when priv->rx or priv->tx is
NULL.

> diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> index 2437cd8ba6331..26fdd22644f4d 100644
> --- a/drivers/net/ethernet/google/gve/gve_main.c
> +++ b/drivers/net/ethernet/google/gve/gve_main.c
> @@ -69,6 +69,7 @@ static void gve_get_stats(struct net_device *dev, struct rtnl_link_stats64 *s)
>  	int num_tx_queues;
>  	int ring;
>  
> +	spin_lock_bh(&priv->stats_lock);
>  	num_tx_queues = gve_num_tx_queues(priv);
>  	if (priv->rx) {
>  		for (ring = 0; ring < priv->rx_cfg.num_queues; ring++) {

[Severity: Medium]
Does stats_lock cover every writer of priv->rx[idx]? The single-queue
restart path still rewrites the ring contents without taking the lock,
and priv->rx stays non-NULL the whole time:

drivers/net/ethernet/google/gve/gve_main.c:gve_rx_queue_stop() {
    ...
	gve_per_q_mem = (struct gve_rx_ring *)per_q_mem;
	*gve_per_q_mem = priv->rx[idx];
	memset(&priv->rx[idx], 0, sizeof(priv->rx[idx]));
	return 0;
}

drivers/net/ethernet/google/gve/gve_main.c:gve_rx_queue_start() {
    ...
	gve_per_q_mem = (struct gve_rx_ring *)per_q_mem;
	priv->rx[idx] = *gve_per_q_mem;
    ...
abort:
    ...
	memset(&priv->rx[idx], 0, sizeof(priv->rx[idx]));
	return err;
}

These run under rtnl and the instance lock. gve_get_stats() (via
ndo_get_stats64) and gve_handle_report_stats() (from the stats report
worker) hold neither of those. They only take stats_lock.

Suppose a devmem, XSK or page-pool queue restart races with a read of
/proc/net/dev or with the stats report worker. Could the reader then see
a partly copied or zeroed ring, including its statss? It would then
report torn rpackets, rbytes, desc.seqno or fill_cnt values.

The readers only copy scalar counters, so this looks like wrong
statistics, not memory corruption.

The later patch in this series, "gve: Fix backward stats when interface
goes down or configuration is adjusted", wraps these writes in
spin_lock_bh(&priv->stats_lock). This patch is tagged for stable. Would
it make sense to move that part into this patch?

[ ... ]

> @@ -2786,9 +2796,15 @@ static void gve_get_rx_queue_stats(struct net_device *dev, int idx,
>  				   struct netdev_queue_stats_rx *rx_stats)
>  {
>  	struct gve_priv *priv = netdev_priv(dev);
> -	struct gve_rx_ring *rx = &priv->rx[idx];
> +	struct gve_rx_ring *rx;
>  	unsigned int start;
>  
> +	spin_lock_bh(&priv->stats_lock);
> +	if (!priv->rx)
> +		goto unlock;
> +

[Severity: Medium]
When the interface is down, this early return (and the matching one in
gve_get_tx_queue_stats()) leaves rx_stats and tx_stats untouched.
netdev_nl_stats_add() then skips them as NETDEV_STAT_NOT_SET. At this
commit gve_get_base_stats() still reports zero:

static void gve_get_base_stats(struct net_device *dev,
			       struct netdev_queue_stats_rx *rx,
			       struct netdev_queue_stats_tx *tx)
{
	rx->packets = 0;
	rx->bytes = 0;
	rx->alloc_fail = 0;

	tx->packets = 0;
	tx->bytes = 0;
}

After some traffic and then ip link set dev down, does the device-level
qstats total drop from the live counters to 0? That looks like it would
still fail check_down() in tools/testing/selftests/drivers/net/stats.py.
That check requires every qstats key to be non-decreasing across ifdown.

The NULL dereference is fixed, but the totals now go backwards. The next
patch in the series, "gve: Fix backward stats when interface goes down
or configuration is adjusted", seems to fix this. It folds the ring
counters into priv->base_net_stats and priv->base_ring_err_stats in
gve_queues_mem_remove() and reports them from gve_get_base_stats().

Given the stable tag, is it intended that this patch alone leaves the
qstats totals non-monotonic across ifdown?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261029211634.2232784-1-hramamurthy%40google.com