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

[Pin-yen Lin]

On Thu, Apr 23, 2026 at 6:17 PM Pin-yen Lin <treapking@xxxxxxxxxx> wrote:
>
> 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;

Another Sashiko comment:

If the interface is brought down when ring memory allocation previously
failed, could priv->rx be NULL here?
Unlike gve_get_ring_stats(), there is no check for whether priv->rx is
allocated before dereferencing it.

This will also be fixed in v2.

> > > +
> > > +     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

Regards,
Pin-yen