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

[Pin-yen Lin]

On Mon, Nov 2, 2026 at 1:18 PM <netdev-bot+sashiko@xxxxxxxxxx> wrote:
>
> 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?

Agreed. We will move the spin lock to this patch in v5.
>
> [ ... ]
>
> > @@ -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?

Yes, this is intended. This patch focuses on fixing the kernel panic
and the next patch addresses the stats values across ifdown.
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261029211634.2232784-1-hramamurthy%40google.com

Regards,
Pin-yen

---

pw-bot: cr