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

[Pin-yen Lin]

On Tue, Oct 27, 2026 at 9:42 AM <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: 1 · Low: 1 (1)
>
> 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] The commit message describes the bug wrongly and the Fixes tag
>   points at the wrong commit.
> - [Low] Baseline decision: not valid for the final state of the series.
>
> Pre-existing issues:
> - [Low] Baseline decision: not valid for the final state of the series.
>
> --
>
> gve: Fix race between stats query and queue reconfig
>
> This commit adds a stats_lock spinlock to struct gve_priv. The lock is
> taken in the stats readers (ndo_get_stats64, the report stats worker and
> the per-queue qstats callbacks) and around the priv->tx/priv->rx updates
> in gve_queues_start() and gve_queues_mem_remove(). The qstats callbacks
> also return early when the queue arrays are NULL.
>
> > Fix a concurrency bug where network statistics queries race with queue
> > reconfiguration or interface teardown, causing a Use-After-Free (UAF) or
> > NULL pointer dereference.
>
> [ ... ]
>
> > This issue was discovered and actually triggered by the
> > drivers/net/stats.py selftest. When an interface was brought down,
> > querying per-queue statistics via Netlink caused a kernel NULL pointer
> > dereference in gve_get_rx_queue_stats():
>
> [ ... ]
>
> > Fixes: 2e5e0932dff5 ("gve: add support for basic queue stats")
>
> [Severity: Medium]
> Is the crash in this trace a race?
>
> gve sets dev->queue_mgmt_ops, so netdev_need_ops_lock() returns true.
> netdev_nl_qstats_get_dumpit() takes the instance lock through
> netdev_get_by_index_lock_ops_compat() and
> for_each_netdev_lock_ops_compat_scoped(). The struct netdev_stat_ops
> comment in include/net/netdev_queues.h says:
>
>   Ops are called under the instance lock if netdev_need_ops_lock()
>   returns true, otherwise under rtnl_lock.
>
> Every caller of gve_queues_mem_remove() also holds the instance lock:
> gve_close() via ndo_stop, gve_adjust_config() via ethtool and XDP, and
> gve_handle_reset()/suspend/shutdown via netdev_lock(). So the qstats
> callbacks can't overlap with teardown.
>
> The trace looks like a NULL dereference that happens every time instead.
> netdev_nl_stats_by_netdev() sums the queues even when the device is down:
>
>         netdev_stat_queue_sum(netdev, 0, netdev->real_num_rx_queues, &rx_sum,
>                               0, netdev->real_num_tx_queues, &tx_sum);
>
> and gve_get_rx_queue_stats() then computed &priv->rx[idx] with priv->rx
> set to NULL. The new NULL check fixes that. The spinlock in the qstats
> callbacks makes no difference on this path.
>
> The lockless readers that do race with gve_queues_mem_remove() are:
>
>   gve_get_stats(): ndo_get_stats64, reached under RCU only from
>   /proc/net/dev via dev_seq_printf_stats()->dev_get_stats()
>
>   gve_handle_report_stats(): runs from gve_wq

This patch fixes both the consistent NULL dereference (as shown in the
trace) and the race between stats query and queue reconfig. The
spinlock in per-queue stats callbacks is just to make sure everyone
who reads priv->rx / priv->tx takes stats_lock. We will update the
wording in v4.
>
> The priv->rx check-then-use in gve_get_stats() dates back to
> f5cedc84a30d ("gve: Add transmit and receive support") from 2019, which
> predates 2e5e0932dff5.
>
> Should the Fixes tag point at the older commit? Stable trees without
> qstats won't otherwise pick up the ndo_get_stats64 use-after-free fix.
> Also, could the commit message keep the qstats NULL dereference separate
> from the ndo_get_stats64 and report stats race?

The Fixes tag will be updated to commit f5cedc84a30d in v4.
>
> > diff --git a/drivers/net/ethernet/google/gve/gve.h b/drivers/net/ethernet/google/gve/gve.h
> > index b7ce6e8632db6..104468c2c07f8 100644
> > --- a/drivers/net/ethernet/google/gve/gve.h
> > +++ b/drivers/net/ethernet/google/gve/gve.h
> > @@ -957,6 +957,8 @@ struct gve_priv {
> >       struct workqueue_struct *gve_wq;
> >       struct work_struct service_task;
> >       struct work_struct stats_report_task;
> > +     /* Synchronizes stats accesses and queue teardown */
> > +     spinlock_t stats_lock;
>
> [Severity: Low]
> Does this lock cover every place that rewrites priv->rx[idx] in place?
> gve_rx_queue_stop() still copies the ring out and clears it without
> taking stats_lock:
>
> 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;
> }
>
> gve_get_stats() (from /proc/net/dev under RCU) and
> gve_handle_report_stats() (from gve_wq) hold only stats_lock. During a
> single-queue restart they can read a partly zeroed ring, including the
> statss seqcount. The result is briefly torn or zeroed rx counters. It is
> not a use-after-free, because the priv->rx array is still allocated.
>
> The next patch in the series, "gve: Fix backward stats when interface
> goes down or configuration is adjusted", moves this copy and memset under
> spin_lock_bh(&priv->stats_lock).
>
> After that patch, two writes in gve_rx_queue_start() still happen without
> the lock: the struct copy priv->rx[idx] = *gve_per_q_mem and the
> abort-path memset. Both only replace zero counters with zero counters.
> The incoming ring is either freshly allocated (memset(rx, 0, sizeof(*rx))
> in gve_rx_alloc_ring_dqo()/gve_rx_alloc_ring_gqi()) or the old ring,
> whose counters gve_rx_queue_stop() zeroes. Readers would not see wrong
> values, but KCSAN could still report the race.

Patch 2 already moves gve_rx_queue_stop() writes under the lock. We
will add stats_lock to protect the two writes in gve_rx_queue_start(),
even though the readers would not see wrong values.
>
> >       unsigned long service_task_flags;
> >       unsigned long state_flags;
> >
> > 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
>
> [ ... ]
>
> > @@ -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;
> > +
> > +     rx = &priv->rx[idx];
> > +
>
> [ ... ]
>
> >  static void gve_get_tx_queue_stats(struct net_device *dev, int idx,
> >                                  struct netdev_queue_stats_tx *tx_stats)
> >  {
> >       struct gve_priv *priv = netdev_priv(dev);
> > -     struct gve_tx_ring *tx = &priv->tx[idx];
> > +     struct gve_tx_ring *tx;
> >       unsigned int start;
> >
> > +     spin_lock_bh(&priv->stats_lock);
> > +     if (!priv->tx)
> > +             goto unlock;
> > +
> > +     tx = &priv->tx[idx];
> > +
>
> [ ... ]
>
> > +
> > +unlock:
> > +     spin_unlock_bh(&priv->stats_lock);
> >  }
> >
> >  static void gve_get_base_stats(struct net_device *dev,
>
> [Severity: Low]
> This is a pre-existing issue and was not introduced by this patch. With
> the early returns above, device-level qstats now drop to zero after
> ifdown, because gve_get_base_stats() still hardcodes zeros:
>
> 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;
> }
>
> Once gve_queues_mem_remove() sets priv->rx and priv->tx to NULL, the
> per-queue callbacks add nothing, so the totals equal the zero base. That
> goes against the get_base_stats contract in include/net/netdev_queues.h.
> It would also fail the "went backwards" check in check_down() in
> tools/testing/selftests/drivers/net/stats.py on this commit.
>
> The next patch in the series, "gve: Fix backward stats when interface
> goes down or configuration is adjusted", fixes this. It accumulates ring
> stats into base_net_stats and base_ring_err_stats under stats_lock in
> gve_queues_mem_remove() and gve_rx_queue_stop(), and gve_get_base_stats()
> then reports those accumulators.

This is fixed by patch 2 in this series.
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261023204706.3112215-1-hramamurthy%40google.com

Regards,
Pin-yen

---

pw-bot: cr