Re: [PATCH net 2/4] gve: Fix backward stats when interface goes down or configuration is adjusted

[Pin-yen Lin]

Hi Paolo,

On Thu, Apr 23, 2026 at 4:47 AM Paolo Abeni <pabeni@xxxxxxxxxx> wrote:
>
> On 4/20/26 7:18 PM, Harshitha Ramamurthy wrote:
> > From: Debarghya Kundu <debarghyak@xxxxxxxxxx>
> >
> > gve_get_base_stats() sets all the stats to 0, so the stats go backwards
> > when interface goes down or configuration is adjusted.
> >
> > Fix this by persisting baseline stats across interface down.
> >
> > This was discovered by drivers/net/stats.py selftest.
> >
> > Cc: stable@xxxxxxxxxxxxxxx
> > Fixes: 2e5e0932dff5 ("gve: add support for basic queue stats")
> > Signed-off-by: Debarghya Kundu <debarghyak@xxxxxxxxxx>
> > Signed-off-by: Pin-yen Lin <treapking@xxxxxxxxxx>
> > Signed-off-by: Harshitha Ramamurthy <hramamurthy@xxxxxxxxxx>
> > ---
> >  drivers/net/ethernet/google/gve/gve.h      |  6 ++
> >  drivers/net/ethernet/google/gve/gve_main.c | 64 +++++++++++++++++++---
> >  2 files changed, 63 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/google/gve/gve.h b/drivers/net/ethernet/google/gve/gve.h
> > index cbdf3a842cfe..ff7797043908 100644
> > --- a/drivers/net/ethernet/google/gve/gve.h
> > +++ b/drivers/net/ethernet/google/gve/gve.h
> > @@ -794,6 +794,10 @@ struct gve_ptp {
> >       struct gve_priv *priv;
> >  };
> >
> > +struct gve_ring_err_stats {
> > +     u64 rx_alloc_fails;
> > +};
> > +
> >  struct gve_priv {
> >       struct net_device *dev;
> >       struct gve_tx_ring *tx; /* array of tx_cfg.num_queues */
> > @@ -882,6 +886,8 @@ struct gve_priv {
> >       unsigned long service_task_flags;
> >       unsigned long state_flags;
> >
> > +     struct gve_ring_err_stats base_ring_err_stats;
> > +     struct rtnl_link_stats64 base_net_stats;
> >       struct gve_stats_report *stats_report;
> >       u64 stats_report_len;
> >       dma_addr_t stats_report_bus; /* dma address for the stats report */
> > diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> > index 675382e9756c..8617782791e0 100644
> > --- a/drivers/net/ethernet/google/gve/gve_main.c
> > +++ b/drivers/net/ethernet/google/gve/gve_main.c
> > @@ -105,9 +105,22 @@ static netdev_tx_t gve_start_xmit(struct sk_buff *skb, struct net_device *dev)
> >               return gve_tx_dqo(skb, dev);
> >  }
> >
> > -static void gve_get_stats(struct net_device *dev, struct rtnl_link_stats64 *s)
> > +static void gve_add_base_stats(struct gve_priv *priv,
> > +                            struct rtnl_link_stats64 *s)
> > +{
> > +     struct rtnl_link_stats64 *base_stats = &priv->base_net_stats;
> > +
> > +     s->rx_packets += base_stats->rx_packets;
> > +     s->rx_bytes += base_stats->rx_bytes;
> > +     s->rx_dropped += base_stats->rx_dropped;
> > +     s->tx_packets += base_stats->tx_packets;
> > +     s->tx_bytes += base_stats->tx_bytes;
> > +     s->tx_dropped += base_stats->tx_dropped;
> > +}

Sashiko says:

Can this result in torn reads on 32-bit architectures?
The base_net_stats struct accumulates 64-bit network statistics in gve_close()
under rtnl_lock, but these stats are read here via ndo_get_stats64 which can
execute concurrently without rtnl_lock.
On 32-bit systems, a concurrent update might result in torn reads since 64-bit
memory reads are not atomic.
Should u64_stats_sync sequence counters or atomic types be used here?

We will add u64_stats_fetch_(begin|retry) to guard this in v2.

> > +
> > +static void gve_get_ring_stats(struct gve_priv *priv,
> > +                            struct rtnl_link_stats64 *s)
> >  {
> > -     struct gve_priv *priv = netdev_priv(dev);
> >       unsigned int start;
> >       u64 packets, bytes;
> >       int num_tx_queues;
> > @@ -142,6 +155,14 @@ static void gve_get_stats(struct net_device *dev, struct rtnl_link_stats64 *s)
> >       }
> >  }
> >
> > +static void gve_get_stats(struct net_device *dev, struct rtnl_link_stats64 *s)
> > +{
> > +     struct gve_priv *priv = netdev_priv(dev);
> > +
> > +     gve_get_ring_stats(priv, s);
> > +     gve_add_base_stats(priv, s);
> > +}
> > +
> >  static int gve_alloc_flow_rule_caches(struct gve_priv *priv)
> >  {
> >       struct gve_flow_rules_cache *flow_rules_cache = &priv->flow_rules_cache;
> > @@ -1493,6 +1514,23 @@ static int gve_queues_stop(struct gve_priv *priv)
> >       return gve_reset_recovery(priv, false);
> >  }
> >
> > +static void gve_get_ring_err_stats(struct gve_priv *priv,
> > +                                struct gve_ring_err_stats *err_stats)
> > +{
> > +     int ring;
> > +
> > +     for (ring = 0; ring < priv->rx_cfg.num_queues; ring++) {
> > +             unsigned int start;
> > +             struct gve_rx_ring *rx = &priv->rx[ring];
> > +
> > +             do {
> > +                     start = u64_stats_fetch_begin(&rx->statss);
> > +                     err_stats->rx_alloc_fails +=
> > +                             rx->rx_skb_alloc_fail + rx->rx_buf_alloc_fail;
> > +             } while (u64_stats_fetch_retry(&rx->statss, start));
>
> Sashiko says:
>
> Could this loop improperly inflate the baseline metric by double counting?
> If a concurrent update causes the sequence counter to change,
> u64_stats_fetch_retry() forces the loop to restart. Because the addition
> is performed in-place on err_stats->rx_alloc_fails, the same ring's
> error values will be added again.
> Would it be safer to use local variables inside the retry loop and update
> the global accumulator only after the loop completes successfully, similar
> to the pattern established in gve_get_ring_stats()?
>
We'll fix this by using local variables in v2.
>
> > +     }
> > +}
> > +
> >  static int gve_close(struct net_device *dev)
> >  {
> >       struct gve_priv *priv = netdev_priv(dev);
> > @@ -1502,6 +1540,10 @@ static int gve_close(struct net_device *dev)
> >       if (err)
> >               return err;
> >
> > +     /* Save ring queue and err stats before closing the interface */
> > +     gve_get_ring_stats(priv, &priv->base_net_stats);
> > +     gve_get_ring_err_stats(priv, &priv->base_ring_err_stats);
>
> Sashiko says:
>
> Does this create a temporary spike in reported statistics?
> During gve_close(), the active ring stats are added to base_net_stats.
> However, priv->rx and priv->tx are not set to NULL until the memory
> teardown completes in gve_queues_mem_remove().
> If ndo_get_stats64 is called concurrently during this window, it will
> add both the active ring stats and the newly updated base stats together.
> This causes the reported statistics to temporarily double until the
> teardown finishes.

As Sashiko mentioned, this is an existing concurrency issue between
gve_close() and gve_get_stats() (i.e., .ndo_get_stats64() callback of
gve driver):

gve_queues_mem_remove(priv);
This is a pre-existing issue, but can this lead to a use-after-free
during concurrent stats retrieval?
When gve_queues_mem_remove() frees the ring memory, priv->rx and
priv->tx are not set to NULL until after the memory is freed. If
gve_get_stats() executes during this window, it may iterate over and
dereference the already freed ring memory.

We will send out a separate patch to fix this.

>
> Note that you are expected to proactively comment/reply on the ML to the
> concerns raised by sashiko reviews.

Thanks for the reminder.

>
> Thanks,
>
> Paolo
>

Regards,
Pin-yen