[PATCH net v4 0/4] gve: Fixes for issues discovered via net selftests and Sashiko

[PATCH net v4 0/4] gve: Fixes for issues discovered via net selftests and Sashiko

[Harshitha Ramamurthy]

From: Pin-yen Lin <treapking@google.com>

This patch series addresses several issues in the gve driver. All four of
these fixes were either uncovered by running the net selftests or
suggested by Sashiko in the previous version.

The series includes the following changes:

- Patch 1 introduces a spinlock (stats_lock) to robustly synchronize stats
  queries (including ndo_get_stats64 and queue stats) against queue
  teardown and restart, preventing UAF and out-of-bounds reads. This was
  discovered by the drivers/net/stats.py selftest.
- Patch 2 fixes an issue where interface stats would go backwards when the
  interface was brought down or its configuration was adjusted. This was
  also discovered by the drivers/net/stats.py selftest.
- Patch 3 ensures the driver falls back to the default minimum ring size if
  the corresponding device option values are exposed as 0. This prevents
  userspace from configuring unexpectedly small ring sizes. This was
  discovered by the drivers/net/ring_reconfig.py selftest.
- Patch 4 makes sure ethtool configuration modifications are done
  synchronously before returning to the userspace. This was discovered by
  the drivers/net/ping.py selftest.

Changes in v4:
- Link to v3: https://lore.kernel.org/netdev/20261023204706.3112215-1-hramamurthy@google.com/
- Patch 1: Clarify the commit message and change the Fixes tag (Sashiko)
- Patch 2: Report the live XDP TX ring counters in the base stats and
  take stats_lock around the priv->rx[idx] writes in
  gve_rx_queue_start() (Sashiko)
- Patch 4: Take netdev_lock() around the link status update in
  gve_service_task() (Sashiko)

Changes in v3:
- Link to v2: https://lore.kernel.org/netdev/20260425002450.163421-1-hramamurthy@google.com/
- Protect all stats queries and queue lifecycle updates with a spinlock
  (Sashiko)
- Drop u64_stats_sync for the base stats and use the spinlock instead
- Save base stats in gve_queues_mem_remove() and gve_rx_queue_stop()
- Drop unused rx_dropped and tx_dropped accumulation in
  gve_add_base_stats()
- Fix the Fixes tag of patch 4
- Refined the commit messages
- Rebased to the latest net tree

Changes in v2:
- Add a NULL pointer check in gve_get_ring_err_stats() (Sashiko)
- Use local variable to prevent inflates from u64_stats_fetch_retry()
  (Sashiko)
- Add u64_stats_fetch/begin to protect base stats (Sashiko)

Debarghya Kundu (1):
  gve: Fix backward stats when interface goes down or configuration is
    adjusted

Pin-yen Lin (3):
  gve: Fix race between stats query and queue reconfig
  gve: Block invalid ring sizes when device options are 0
  gve: Fix link flap after ethtool ops

 drivers/net/ethernet/google/gve/gve.h        |   8 +
 drivers/net/ethernet/google/gve/gve_adminq.c |   4 +-
 drivers/net/ethernet/google/gve/gve_main.c   | 236 ++++++++++++++-----
 3 files changed, 191 insertions(+), 57 deletions(-)


base-commit: 734bef548a96e8774d6eef5dfa2026c6edc4e80b
-- 
2.56.0.307.g5fe7bd680c-goog

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

[Harshitha Ramamurthy]

From: Pin-yen Lin <treapking@google.com>

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.

Currently, statistics queries access priv->tx and priv->rx without
locking. When an interface is brought down or reconfigured,
gve_queues_mem_remove() frees these arrays. A concurrent stats query
can dereference freed queue structures and crash the kernel.

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():

  BUG: kernel NULL pointer dereference, address: 00000000000000a0
  RIP: 0010:gve_get_rx_queue_stats+0x17/0x50 [gve]
  Call Trace:
   <TASK>
   netdev_stat_queue_sum+0x11c/0x250
   netdev_nl_stats_by_netdev+0x21b/0x2d0
   netdev_nl_qstats_get_dumpit+0xd2/0x220
   genl_dumpit+0x32/0x90
   netlink_dump+0x1d2/0x450
   __netlink_dump_start+0x1db/0x2a0
   genl_family_rcv_msg_dumpit+0x9b/0x100
   ...

The per-queue stats callbacks run under the netdev instance lock, so
this NULL dereference is not a race; it happens on any query while the
interface is down.

Additionally, concurrent stats queries during queue reconfiguration can
race with gve_queues_mem_remove(), causing a Use-After-Free.
This affects gve_get_stats() and gve_handle_report_stats(), which do
not hold the instance lock.

Fix this by introducing stats_lock to serialize stats queries against
queue teardown. In gve_queues_mem_remove(), queue pointers are set to
NULL under stats_lock before freeing. Stats readers acquire the lock
and verify pointers are non-NULL before reading counters.
The per-queue stats callbacks also take stats_lock so that all readers
follow the same locking rule.

This change was tested using the DQO RDA queue format.

Cc: stable@vger.kernel.org
Fixes: f5cedc84a30d ("gve: Add transmit and receive support")
Co-developed-by: Debarghya Kundu <debarghyak@google.com>
Signed-off-by: Debarghya Kundu <debarghyak@google.com>
Signed-off-by: Pin-yen Lin <treapking@google.com>
Signed-off-by: Harshitha Ramamurthy <hramamurthy@google.com>
---

Changes in v4:
- Clarify in the commit message that the qstats NULL dereference is not
  a race, and which readers race with queue teardown (Sashiko)
- Change the Fixes tag to f5cedc84a30d (Sashiko)

Changes in v3:
- Retitled from "gve: Add NULL pointer checks for per-queue statistics"
- Protect all stats queries and queue lifecycle updates with spinlock
  (sashiko)
- Refined the commit message

 drivers/net/ethernet/google/gve/gve.h      |  2 ++
 drivers/net/ethernet/google/gve/gve_main.c | 37 +++++++++++++++++++---
 2 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/google/gve/gve.h b/drivers/net/ethernet/google/gve/gve.h
index b7ce6e8632db..104468c2c07f 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;
 	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 2437cd8ba633..26fdd22644f4 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++) {
@@ -96,6 +97,7 @@ static void gve_get_stats(struct net_device *dev, struct rtnl_link_stats64 *s)
 			s->tx_bytes += bytes;
 		}
 	}
+	spin_unlock_bh(&priv->stats_lock);
 }
 
 static int gve_alloc_flow_rule_caches(struct gve_priv *priv)
@@ -642,9 +644,13 @@ static void gve_queues_mem_remove(struct gve_priv *priv)
 	struct gve_rx_alloc_rings_cfg rx_alloc_cfg = {0};
 
 	gve_get_curr_alloc_cfgs(priv, &tx_alloc_cfg, &rx_alloc_cfg);
-	gve_queues_mem_free(priv, &tx_alloc_cfg, &rx_alloc_cfg);
+
+	spin_lock_bh(&priv->stats_lock);
 	priv->tx = NULL;
 	priv->rx = NULL;
+	spin_unlock_bh(&priv->stats_lock);
+
+	gve_queues_mem_free(priv, &tx_alloc_cfg, &rx_alloc_cfg);
 }
 
 static void gve_free_control_plane_resources(struct gve_priv *priv)
@@ -1383,6 +1389,7 @@ static int gve_queues_start(struct gve_priv *priv,
 	int err;
 
 	/* Record new resources into priv */
+	spin_lock_bh(&priv->stats_lock);
 	priv->tx = tx_alloc_cfg->tx;
 	priv->rx = rx_alloc_cfg->rx;
 
@@ -1396,10 +1403,11 @@ static int gve_queues_start(struct gve_priv *priv,
 	priv->rx_pages_per_qpl = rx_alloc_cfg->pages_per_qpl;
 	priv->header_split_enabled = rx_alloc_cfg->enable_header_split;
 	priv->rx_cfg.packet_buffer_size = rx_alloc_cfg->packet_buffer_size;
+	gve_init_sync_stats(priv);
+	spin_unlock_bh(&priv->stats_lock);
 
 	gve_tx_start_rings(priv, gve_num_tx_queues(priv));
 	gve_rx_start_rings(priv, rx_alloc_cfg->qcfg_rx->num_queues);
-	gve_init_sync_stats(priv);
 
 	err = netif_set_real_num_tx_queues(dev, priv->tx_cfg.num_queues);
 	if (err)
@@ -2266,6 +2274,7 @@ void gve_handle_report_stats(struct gve_priv *priv)
 	if (!gve_get_report_stats(priv))
 		return;
 
+	spin_lock_bh(&priv->stats_lock);
 	be64_add_cpu(&priv->stats_report->written_count, 1);
 	/* tx stats */
 	if (priv->tx) {
@@ -2330,6 +2339,7 @@ void gve_handle_report_stats(struct gve_priv *priv)
 			};
 		}
 	}
+	spin_unlock_bh(&priv->stats_lock);
 }
 
 /* Handle NIC status register changes, reset requests and report stats */
@@ -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];
+
 	do {
 		start = u64_stats_fetch_begin(&rx->statss);
 		rx_stats->packets = rx->rpackets;
@@ -2796,20 +2812,32 @@ static void gve_get_rx_queue_stats(struct net_device *dev, int idx,
 		rx_stats->alloc_fail = rx->rx_skb_alloc_fail +
 				       rx->rx_buf_alloc_fail;
 	} while (u64_stats_fetch_retry(&rx->statss, start));
+
+unlock:
+	spin_unlock_bh(&priv->stats_lock);
 }
 
 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];
+
 	do {
 		start = u64_stats_fetch_begin(&tx->statss);
 		tx_stats->packets = tx->pkt_done;
 		tx_stats->bytes = tx->bytes_done;
 	} while (u64_stats_fetch_retry(&tx->statss, start));
+
+unlock:
+	spin_unlock_bh(&priv->stats_lock);
 }
 
 static void gve_get_base_stats(struct net_device *dev,
@@ -2935,6 +2963,7 @@ static int gve_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	}
 	INIT_WORK(&priv->service_task, gve_service_task);
 	INIT_WORK(&priv->stats_report_task, gve_stats_report_task);
+	spin_lock_init(&priv->stats_lock);
 	priv->tx_cfg.max_queues = max_tx_queues;
 	priv->rx_cfg.max_queues = max_rx_queues;
 
-- 
2.56.0.313.g2479322e68-goog

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

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@kernel.org> 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

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

[Harshitha Ramamurthy]

From: Debarghya Kundu <debarghyak@google.com>

gve_get_base_stats() sets all the stats to 0, so the stats go backwards
when interface goes down or configuration is adjusted.

This issue was discovered and actually triggered by the
drivers/net/stats.py selftest (check_down test case). Bringing down the
interface caused statistics counters to reset to 0, triggering selftest
assertion failures because netdevice stats must not go backwards.

Fix this by persisting baseline stats across queue teardown in
gve_queues_mem_remove() and single-queue stop in gve_rx_queue_stop().

Also report the live XDP TX ring counters in gve_get_base_stats(), as
those rings are not exposed as netdev queues, and take stats_lock around
the priv->rx[idx] writes in gve_rx_queue_start().

This change was tested using the DQO RDA queue format.

Cc: stable@vger.kernel.org
Fixes: 2e5e0932dff5 ("gve: add support for basic queue stats")
Signed-off-by: Debarghya Kundu <debarghyak@google.com>
Co-developed-by: Pin-yen Lin <treapking@google.com>
Signed-off-by: Pin-yen Lin <treapking@google.com>
Signed-off-by: Harshitha Ramamurthy <hramamurthy@google.com>
---

Changes in v4:
- Report the live XDP TX ring counters in gve_get_base_stats() (Sashiko)
- Take stats_lock around the priv->rx[idx] writes in
  gve_rx_queue_start() (Sashiko)

Changes in v3:
- Drop u64_stats_sync for the base stats and use spinlock to protect
  them instead
- Save base stats in gve_queues_mem_remove() instead of gve_close()
- Save base stats in gve_rx_queue_stop() for single-queue restarts
- Drop unused rx_dropped and tx_dropped accumulation in gve_add_base_stats()
- Refined the commit message

Changes in v2:
- Add a NULL pointer check in gve_get_ring_err_stats() (Sashiko)
- Use local variable to prevent inflates from u64_stats_fetch_retry()
  (Sashiko)
- Add u64_stats_fetch/begin to protect base stats (Sashiko)

 drivers/net/ethernet/google/gve/gve.h      |   6 +
 drivers/net/ethernet/google/gve/gve_main.c | 131 +++++++++++++++++----
 2 files changed, 115 insertions(+), 22 deletions(-)

diff --git a/drivers/net/ethernet/google/gve/gve.h b/drivers/net/ethernet/google/gve/gve.h
index 104468c2c07f..37070bc14c87 100644
--- a/drivers/net/ethernet/google/gve/gve.h
+++ b/drivers/net/ethernet/google/gve/gve.h
@@ -871,6 +871,10 @@ struct gve_ctrl_ops {
 	int (*report_link_speed)(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 */
@@ -962,6 +966,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 26fdd22644f4..b5e2f39a903c 100644
--- a/drivers/net/ethernet/google/gve/gve_main.c
+++ b/drivers/net/ethernet/google/gve/gve_main.c
@@ -61,16 +61,44 @@ 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->tx_packets += base_stats->tx_packets;
+	s->tx_bytes += base_stats->tx_bytes;
+}
+
+static void gve_add_tx_ring_range_stats(struct gve_priv *priv, int start_ring,
+					int end_ring, u64 *packets, u64 *bytes)
+{
+	int ring;
+
+	for (ring = start_ring; ring < end_ring; ring++) {
+		struct gve_tx_ring *tx = &priv->tx[ring];
+		unsigned int start;
+		u64 pkts, byts;
+
+		do {
+			start = u64_stats_fetch_begin(&tx->statss);
+			pkts = tx->pkt_done;
+			byts = tx->bytes_done;
+		} while (u64_stats_fetch_retry(&tx->statss, start));
+		*packets += pkts;
+		*bytes += byts;
+	}
+}
+
+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;
 	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++) {
 			do {
@@ -84,19 +112,41 @@ static void gve_get_stats(struct net_device *dev, struct rtnl_link_stats64 *s)
 			s->rx_bytes += bytes;
 		}
 	}
-	if (priv->tx) {
-		for (ring = 0; ring < num_tx_queues; ring++) {
-			do {
-				start =
-				  u64_stats_fetch_begin(&priv->tx[ring].statss);
-				packets = priv->tx[ring].pkt_done;
-				bytes = priv->tx[ring].bytes_done;
-			} while (u64_stats_fetch_retry(&priv->tx[ring].statss,
-						       start));
-			s->tx_packets += packets;
-			s->tx_bytes += bytes;
-		}
+	if (priv->tx)
+		gve_add_tx_ring_range_stats(priv, 0, gve_num_tx_queues(priv),
+					    &s->tx_packets, &s->tx_bytes);
+}
+
+static void gve_get_ring_err_stats(struct gve_priv *priv,
+				   struct gve_ring_err_stats *err_stats)
+{
+	int ring;
+
+	if (!priv->rx)
+		return;
+
+	for (ring = 0; ring < priv->rx_cfg.num_queues; ring++) {
+		struct gve_rx_ring *rx = &priv->rx[ring];
+		unsigned int start;
+		u64 rx_alloc_fails;
+
+		do {
+			start = u64_stats_fetch_begin(&rx->statss);
+			rx_alloc_fails = rx->rx_skb_alloc_fail +
+					 rx->rx_buf_alloc_fail;
+		} while (u64_stats_fetch_retry(&rx->statss, start));
+
+		err_stats->rx_alloc_fails += rx_alloc_fails;
 	}
+}
+
+static void gve_get_stats(struct net_device *dev, struct rtnl_link_stats64 *s)
+{
+	struct gve_priv *priv = netdev_priv(dev);
+
+	spin_lock_bh(&priv->stats_lock);
+	gve_get_ring_stats(priv, s);
+	gve_add_base_stats(priv, s);
 	spin_unlock_bh(&priv->stats_lock);
 }
 
@@ -646,6 +696,9 @@ static void gve_queues_mem_remove(struct gve_priv *priv)
 	gve_get_curr_alloc_cfgs(priv, &tx_alloc_cfg, &rx_alloc_cfg);
 
 	spin_lock_bh(&priv->stats_lock);
+	gve_get_ring_stats(priv, &priv->base_net_stats);
+	gve_get_ring_err_stats(priv, &priv->base_ring_err_stats);
+
 	priv->tx = NULL;
 	priv->rx = NULL;
 	spin_unlock_bh(&priv->stats_lock);
@@ -2689,8 +2742,20 @@ static int gve_rx_queue_stop(struct net_device *dev, void *per_q_mem, int idx)
 	gve_turnup_and_check_status(priv);
 
 	gve_per_q_mem = (struct gve_rx_ring *)per_q_mem;
+	spin_lock_bh(&priv->stats_lock);
+	priv->base_net_stats.rx_packets += priv->rx[idx].rpackets;
+	priv->base_net_stats.rx_bytes += priv->rx[idx].rbytes;
+	priv->base_ring_err_stats.rx_alloc_fails +=
+		priv->rx[idx].rx_skb_alloc_fail +
+		priv->rx[idx].rx_buf_alloc_fail;
 	*gve_per_q_mem = priv->rx[idx];
 	memset(&priv->rx[idx], 0, sizeof(priv->rx[idx]));
+	spin_unlock_bh(&priv->stats_lock);
+
+	gve_per_q_mem->rpackets = 0;
+	gve_per_q_mem->rbytes = 0;
+	gve_per_q_mem->rx_skb_alloc_fail = 0;
+	gve_per_q_mem->rx_buf_alloc_fail = 0;
 	return 0;
 }
 
@@ -2744,7 +2809,9 @@ static int gve_rx_queue_start(struct net_device *dev,
 		return -EAGAIN;
 
 	gve_per_q_mem = (struct gve_rx_ring *)per_q_mem;
+	spin_lock_bh(&priv->stats_lock);
 	priv->rx[idx] = *gve_per_q_mem;
+	spin_unlock_bh(&priv->stats_lock);
 
 	/* Single-queue creation requires quiescence on all queues */
 	gve_turndown(priv);
@@ -2780,7 +2847,9 @@ static int gve_rx_queue_start(struct net_device *dev,
 	 * which needs the rtnl lock, will not run till this func returns and
 	 * its caller gives up the lock.
 	 */
+	spin_lock_bh(&priv->stats_lock);
 	memset(&priv->rx[idx], 0, sizeof(priv->rx[idx]));
+	spin_unlock_bh(&priv->stats_lock);
 	return err;
 }
 
@@ -2844,12 +2913,30 @@ 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;
+	const struct gve_ring_err_stats *base_err_stats;
+	const struct rtnl_link_stats64 *base_stats;
+	struct gve_priv *priv;
+
+	priv = netdev_priv(dev);
+	base_stats = &priv->base_net_stats;
+	base_err_stats = &priv->base_ring_err_stats;
 
-	tx->packets = 0;
-	tx->bytes = 0;
+	spin_lock_bh(&priv->stats_lock);
+	rx->packets = base_stats->rx_packets;
+	rx->bytes = base_stats->rx_bytes;
+	rx->alloc_fail = base_err_stats->rx_alloc_fails;
+
+	tx->packets = base_stats->tx_packets;
+	tx->bytes = base_stats->tx_bytes;
+	/* XDP TX rings are not exposed as netdev queues, so report their live
+	 * counters here to keep the device totals consistent with
+	 * ndo_get_stats64.
+	 */
+	if (priv->tx)
+		gve_add_tx_ring_range_stats(priv, priv->tx_cfg.num_queues,
+					    gve_num_tx_queues(priv),
+					    &tx->packets, &tx->bytes);
+	spin_unlock_bh(&priv->stats_lock);
 }
 
 static const struct netdev_stat_ops gve_stat_ops = {
-- 
2.56.0.313.g2479322e68-goog

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message and the Fixes tag describe only part of what
  the patch changes.

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but on 32-bit
  SMP kernels tx->statss of an XDP TX ring has writers that are not…
- [Low] This problem wasn't introduced by this patch, but check_down() in
  tools/testing/selftests/drivers/net/stats.py has the docstring "Test…
- [Low] This problem wasn't introduced by this patch, but
  pkt_byte_sum.stat_cmp() in…
- [Low] This problem wasn't introduced by this patch, but the single-
  queue RX restart path never calls u64_stats_init() on the new ring's…
- [Low] This problem wasn't introduced by this patch, but
  gve_rx_post_buffers_dqo() updates rx->statss (rx_buf_alloc_fail)…

--

gve: Fix backward stats when interface goes down or configuration is adjusted

This patch keeps baseline rx/tx packet, byte and alloc-fail counters in
gve_priv. They are saved during full queue teardown in
gve_queues_mem_remove() and during a single RX queue stop in
gve_rx_queue_stop(). Both ndo_get_stats64 and the netdev_stat_ops base
stats report these baselines, and gve_get_base_stats() also reports the
live XDP TX ring counters.

> gve_get_base_stats() sets all the stats to 0, so the stats go backwards
> when interface goes down or configuration is adjusted.

[ ... ]

> Fixes: 2e5e0932dff5 ("gve: add support for basic queue stats")

[Severity: Low]

Does this description cover everything the patch changes? The message
blames gve_get_base_stats(). The patch also changes ndo_get_stats64:
gve_get_stats() now calls gve_add_base_stats().

That part fixes an older bug. In the tree before 2e5e0932dff5,
gve_close() already freed the rings on every ifdown:

gve_close()
    gve_queues_stop(priv);
    gve_queues_mem_remove(priv);

gve_queues_mem_remove() set priv->tx and priv->rx to NULL, and
gve_get_stats() only summed the live rings. So the ip -s link and
/proc/net/dev counters already went back to 0 on ifdown.
Documentation/networking/statistics.rst says:

  Statistics must persist across routine operations like bringing the
  interface down and up.

Could the changelog mention the rtnl counter change, and could the
Fixes: tag be revisited? With the current tag, stable backporters may
treat trees without netdev_stat_ops (older than 6.13) as unaffected,
even though they have the rtnl reset on ifdown.

> diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> index 26fdd22644f4d..b5e2f39a903c5 100644
> --- a/drivers/net/ethernet/google/gve/gve_main.c
> +++ b/drivers/net/ethernet/google/gve/gve_main.c

[ ... ]

> @@ -84,19 +112,41 @@ 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);
> +
> +	spin_lock_bh(&priv->stats_lock);
> +	gve_get_ring_stats(priv, s);
> +	gve_add_base_stats(priv, s);
>  	spin_unlock_bh(&priv->stats_lock);
>  }

[Severity: Low]

This is a pre-existing issue and was not introduced by this patch. The
commit message cites the check_down test case in
tools/testing/selftests/drivers/net/stats.py. Its docstring says:

    """ Test statistics (interface and qstat) are not impacted by ifdown """

However, check_down() only compares qstats from netfam.qstats_get()
before and after ip link set down. It never reads the rtnl stats64.

If the new gve_add_base_stats() call in gve_get_stats() regressed,
wouldn't check_down() still pass?

[ ... ]

> @@ -2744,7 +2809,9 @@ static int gve_rx_queue_start(struct net_device *dev,
>  		return -EAGAIN;
>  
>  	gve_per_q_mem = (struct gve_rx_ring *)per_q_mem;
> +	spin_lock_bh(&priv->stats_lock);
>  	priv->rx[idx] = *gve_per_q_mem;
> +	spin_unlock_bh(&priv->stats_lock);

[Severity: Low]

This isn't a bug introduced by this patch, but is rx->statss ever
initialized on the single-queue restart path?

gve_rx_queue_mem_alloc() calls gve_rx_alloc_ring_gqi() or
gve_rx_alloc_ring_dqo(), which zero the ring. The only u64_stats_init()
calls for RX rings are in gve_init_sync_stats():

	for (i = 0; i < priv->rx_cfg.num_queues; i++)
		u64_stats_init(&priv->rx[i].statss);

That function only runs on a full queue start. gve_rx_queue_start() then
copies the uninitialized statss into priv->rx[idx].

On 32-bit kernels with CONFIG_DEBUG_LOCK_ALLOC, the seqcount's lockdep
map is never initialized. Can later stats updates or reads then trigger
lockdep warnings? The stats_lock added around this copy doesn't change
that.

[Severity: Low]

This is also a pre-existing issue and not introduced by this patch. Later
in gve_rx_queue_start(), the DQO path calls gve_rx_post_buffers_dqo()
from preemptible process context:

	else
		gve_rx_post_buffers_dqo(&priv->rx[idx]);

When a buffer allocation fails, that function writes rx->statss:

		if (unlikely(gve_alloc_buffer(rx, desc))) {
			u64_stats_update_begin(&rx->statss);
			rx->rx_buf_alloc_fail++;
			u64_stats_update_end(&rx->statss);

On 32-bit non-RT kernels, __u64_stats_update_begin() uses
preempt_disable_nested(). That only asserts that preemption is already
disabled; it does not disable it.

If the writer is preempted while the sequence is odd, can a reader on
the same CPU spin until the writer runs again? gve_get_stats() under
spin_lock_bh(&priv->stats_lock) is one such reader. Lockdep builds would
also warn here. gve_create_rings() reaches the same code regardless of
this patch.

[ ... ]

> @@ -2844,12 +2913,30 @@ static void gve_get_base_stats(struct net_device *dev,
>  			       struct netdev_queue_stats_rx *rx,
>  			       struct netdev_queue_stats_tx *tx)
>  {

[ ... ]

> +	tx->packets = base_stats->tx_packets;
> +	tx->bytes = base_stats->tx_bytes;
> +	/* XDP TX rings are not exposed as netdev queues, so report their live
> +	 * counters here to keep the device totals consistent with
> +	 * ndo_get_stats64.
> +	 */
> +	if (priv->tx)
> +		gve_add_tx_ring_range_stats(priv, priv->tx_cfg.num_queues,
> +					    gve_num_tx_queues(priv),
> +					    &tx->packets, &tx->bytes);
> +	spin_unlock_bh(&priv->stats_lock);
>  }

[Severity: Low]

This is a pre-existing issue in the selftest and was not introduced by
this patch. Does pkt_byte_sum() in stats.py really check that these
totals match ndo_get_stats64? stat_cmp() returns at the first key that
differs:

    def stat_cmp(rstat, qstat):
        for key in ['tx-packets', 'tx-bytes', 'rx-packets', 'rx-bytes']:
            if rstat[key] != qstat[key]:
                return rstat[key] - qstat[key]
        return 0

With live TX traffic, a correctly increasing tx-packets decides the
result before tx-bytes or the RX counters are compared. The test can
therefore pass while rtnl and qstats disagree in the later fields. That
includes the XDP TX contribution added here.

[Severity: Medium]

This isn't a bug introduced by this patch, but are the writers of
tx->statss on the XDP TX rings serialized on 32-bit SMP kernels?

gve_xdp_xmit_dqo() picks the ring as smp_processor_id() %
num_xdp_queues, so several CPUs can share one ring. It updates statss
after releasing the lock that serializes those CPUs:

	spin_unlock(&tx->dqo_tx.xdp_lock);

	u64_stats_update_begin(&tx->statss);
	tx->xdp_xmit += n;
	tx->xdp_xmit_errors += n - i;
	u64_stats_update_end(&tx->statss);

gve_xdp_xmit() in gve_tx.c does the same after
spin_unlock(&tx->xdp_lock). gve_clean_tx_done_dqo() also updates the
same statss from NAPI:

	u64_stats_update_begin(&tx->statss);
	tx->bytes_done += pkt_compl_bytes + reinject_compl_bytes;
	tx->pkt_done += pkt_compl_pkts + reinject_compl_pkts;

Could overlapping write_seqcount_begin()/end() calls tear the 64-bit
reads? Could they lose a sequence increment and leave the count odd? If
so, the retry loop in gve_add_tx_ring_range_stats() would spin forever
with stats_lock held and BH disabled.

gve_get_stats() and gve_get_ethtool_stats() already read these counters
before this patch, so the new reader in gve_get_base_stats() neither
creates nor widens the exposure. Hitting it needs a 32-bit kernel and
XDP_REDIRECT into gve from several CPUs while completions are running.

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

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

[Pin-yen Lin]

On Mon, Nov 2, 2026 at 1:18 PM <netdev-bot+sashiko@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found more severe
> issue(s) in other patches of this series. This mail is only an FYI, in case
> you want to address the nit picks below when you respin.
>
> Low priority / nit picks for your consideration:
>
> - [Low] The commit message and the Fixes tag describe only part of what
>   the patch changes.
>
> Pre-existing issues:
> - [Medium] This problem wasn't introduced by this patch, but on 32-bit
>   SMP kernels tx->statss of an XDP TX ring has writers that are not…
> - [Low] This problem wasn't introduced by this patch, but check_down() in
>   tools/testing/selftests/drivers/net/stats.py has the docstring "Test…
> - [Low] This problem wasn't introduced by this patch, but
>   pkt_byte_sum.stat_cmp() in…
> - [Low] This problem wasn't introduced by this patch, but the single-
>   queue RX restart path never calls u64_stats_init() on the new ring's…
> - [Low] This problem wasn't introduced by this patch, but
>   gve_rx_post_buffers_dqo() updates rx->statss (rx_buf_alloc_fail)…
>
> --
>
> gve: Fix backward stats when interface goes down or configuration is adjusted
>
> This patch keeps baseline rx/tx packet, byte and alloc-fail counters in
> gve_priv. They are saved during full queue teardown in
> gve_queues_mem_remove() and during a single RX queue stop in
> gve_rx_queue_stop(). Both ndo_get_stats64 and the netdev_stat_ops base
> stats report these baselines, and gve_get_base_stats() also reports the
> live XDP TX ring counters.
>
> > gve_get_base_stats() sets all the stats to 0, so the stats go backwards
> > when interface goes down or configuration is adjusted.
>
> [ ... ]
>
> > Fixes: 2e5e0932dff5 ("gve: add support for basic queue stats")
>
> [Severity: Low]
>
> Does this description cover everything the patch changes? The message
> blames gve_get_base_stats(). The patch also changes ndo_get_stats64:
> gve_get_stats() now calls gve_add_base_stats().
>
> That part fixes an older bug. In the tree before 2e5e0932dff5,
> gve_close() already freed the rings on every ifdown:
>
> gve_close()
>     gve_queues_stop(priv);
>     gve_queues_mem_remove(priv);
>
> gve_queues_mem_remove() set priv->tx and priv->rx to NULL, and
> gve_get_stats() only summed the live rings. So the ip -s link and
> /proc/net/dev counters already went back to 0 on ifdown.
> Documentation/networking/statistics.rst says:
>
>   Statistics must persist across routine operations like bringing the
>   interface down and up.
>
> Could the changelog mention the rtnl counter change, and could the
> Fixes: tag be revisited? With the current tag, stable backporters may
> treat trees without netdev_stat_ops (older than 6.13) as unaffected,
> even though they have the rtnl reset on ifdown.

Yes, ndo_get_stats64 has dropped to 0 on ifdown since commit
f5cedc84a30d ("gve: Add transmit and receive support"). We will
mention the ndo_get_stats64 counters in the commit message and add a
Fixes tag for it in v5.
>
> > diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> > index 26fdd22644f4d..b5e2f39a903c5 100644
> > --- a/drivers/net/ethernet/google/gve/gve_main.c
> > +++ b/drivers/net/ethernet/google/gve/gve_main.c
>
> [ ... ]
>
> > @@ -84,19 +112,41 @@ 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);
> > +
> > +     spin_lock_bh(&priv->stats_lock);
> > +     gve_get_ring_stats(priv, s);
> > +     gve_add_base_stats(priv, s);
> >       spin_unlock_bh(&priv->stats_lock);
> >  }
>
> [Severity: Low]
>
> This is a pre-existing issue and was not introduced by this patch. The
> commit message cites the check_down test case in
> tools/testing/selftests/drivers/net/stats.py. Its docstring says:
>
>     """ Test statistics (interface and qstat) are not impacted by ifdown """
>
> However, check_down() only compares qstats from netfam.qstats_get()
> before and after ip link set down. It never reads the rtnl stats64.
>
> If the new gve_add_base_stats() call in gve_get_stats() regressed,
> wouldn't check_down() still pass?
>
> [ ... ]
>
> > @@ -2744,7 +2809,9 @@ static int gve_rx_queue_start(struct net_device *dev,
> >               return -EAGAIN;
> >
> >       gve_per_q_mem = (struct gve_rx_ring *)per_q_mem;
> > +     spin_lock_bh(&priv->stats_lock);
> >       priv->rx[idx] = *gve_per_q_mem;
> > +     spin_unlock_bh(&priv->stats_lock);
>
> [Severity: Low]
>
> This isn't a bug introduced by this patch, but is rx->statss ever
> initialized on the single-queue restart path?
>
> gve_rx_queue_mem_alloc() calls gve_rx_alloc_ring_gqi() or
> gve_rx_alloc_ring_dqo(), which zero the ring. The only u64_stats_init()
> calls for RX rings are in gve_init_sync_stats():
>
>         for (i = 0; i < priv->rx_cfg.num_queues; i++)
>                 u64_stats_init(&priv->rx[i].statss);
>
> That function only runs on a full queue start. gve_rx_queue_start() then
> copies the uninitialized statss into priv->rx[idx].
>
> On 32-bit kernels with CONFIG_DEBUG_LOCK_ALLOC, the seqcount's lockdep
> map is never initialized. Can later stats updates or reads then trigger
> lockdep warnings? The stats_lock added around this copy doesn't change
> that.
>
> [Severity: Low]
>
> This is also a pre-existing issue and not introduced by this patch. Later
> in gve_rx_queue_start(), the DQO path calls gve_rx_post_buffers_dqo()
> from preemptible process context:
>
>         else
>                 gve_rx_post_buffers_dqo(&priv->rx[idx]);
>
> When a buffer allocation fails, that function writes rx->statss:
>
>                 if (unlikely(gve_alloc_buffer(rx, desc))) {
>                         u64_stats_update_begin(&rx->statss);
>                         rx->rx_buf_alloc_fail++;
>                         u64_stats_update_end(&rx->statss);
>
> On 32-bit non-RT kernels, __u64_stats_update_begin() uses
> preempt_disable_nested(). That only asserts that preemption is already
> disabled; it does not disable it.
>
> If the writer is preempted while the sequence is odd, can a reader on
> the same CPU spin until the writer runs again? gve_get_stats() under
> spin_lock_bh(&priv->stats_lock) is one such reader. Lockdep builds would
> also warn here. gve_create_rings() reaches the same code regardless of
> this patch.
>
> [ ... ]
>
> > @@ -2844,12 +2913,30 @@ static void gve_get_base_stats(struct net_device *dev,
> >                              struct netdev_queue_stats_rx *rx,
> >                              struct netdev_queue_stats_tx *tx)
> >  {
>
> [ ... ]
>
> > +     tx->packets = base_stats->tx_packets;
> > +     tx->bytes = base_stats->tx_bytes;
> > +     /* XDP TX rings are not exposed as netdev queues, so report their live
> > +      * counters here to keep the device totals consistent with
> > +      * ndo_get_stats64.
> > +      */
> > +     if (priv->tx)
> > +             gve_add_tx_ring_range_stats(priv, priv->tx_cfg.num_queues,
> > +                                         gve_num_tx_queues(priv),
> > +                                         &tx->packets, &tx->bytes);
> > +     spin_unlock_bh(&priv->stats_lock);
> >  }
>
> [Severity: Low]
>
> This is a pre-existing issue in the selftest and was not introduced by
> this patch. Does pkt_byte_sum() in stats.py really check that these
> totals match ndo_get_stats64? stat_cmp() returns at the first key that
> differs:
>
>     def stat_cmp(rstat, qstat):
>         for key in ['tx-packets', 'tx-bytes', 'rx-packets', 'rx-bytes']:
>             if rstat[key] != qstat[key]:
>                 return rstat[key] - qstat[key]
>         return 0
>
> With live TX traffic, a correctly increasing tx-packets decides the
> result before tx-bytes or the RX counters are compared. The test can
> therefore pass while rtnl and qstats disagree in the later fields. That
> includes the XDP TX contribution added here.
>
> [Severity: Medium]
>
> This isn't a bug introduced by this patch, but are the writers of
> tx->statss on the XDP TX rings serialized on 32-bit SMP kernels?
>
> gve_xdp_xmit_dqo() picks the ring as smp_processor_id() %
> num_xdp_queues, so several CPUs can share one ring. It updates statss
> after releasing the lock that serializes those CPUs:
>
>         spin_unlock(&tx->dqo_tx.xdp_lock);
>
>         u64_stats_update_begin(&tx->statss);
>         tx->xdp_xmit += n;
>         tx->xdp_xmit_errors += n - i;
>         u64_stats_update_end(&tx->statss);
>
> gve_xdp_xmit() in gve_tx.c does the same after
> spin_unlock(&tx->xdp_lock). gve_clean_tx_done_dqo() also updates the
> same statss from NAPI:
>
>         u64_stats_update_begin(&tx->statss);
>         tx->bytes_done += pkt_compl_bytes + reinject_compl_bytes;
>         tx->pkt_done += pkt_compl_pkts + reinject_compl_pkts;
>
> Could overlapping write_seqcount_begin()/end() calls tear the 64-bit
> reads? Could they lose a sequence increment and leave the count odd? If
> so, the retry loop in gve_add_tx_ring_range_stats() would spin forever
> with stats_lock held and BH disabled.
>
> gve_get_stats() and gve_get_ethtool_stats() already read these counters
> before this patch, so the new reader in gve_get_base_stats() neither
> creates nor widens the exposure. Hitting it needs a 32-bit kernel and
> XDP_REDIRECT into gve from several CPUs while completions are running.

All the other comments are pre-existing issues and out of scope for this series.
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261029211634.2232784-1-hramamurthy%40google.com

Regards,
Pin-yen

[PATCH net v4 3/4] gve: Block invalid ring sizes when device options are 0

[Harshitha Ramamurthy]

From: Pin-yen Lin <treapking@google.com>

On older device versions, minimum ring sizes in the device option are
reported as 0. The driver imports these 0 values, allowing userspace
(`ethtool -G`) to request invalidly small ring sizes.

This issue was discovered and actually triggered by the
drivers/net/ring_reconfig.py selftest (ringparam test case). Halving the
ring size down to 1 triggered a kernel NULL pointer dereference in
gve_tx_alloc_rings_dqo():

  BUG: kernel NULL pointer dereference, address: 0000000000000002
  RIP: 0010:gve_tx_alloc_rings_dqo+0x1c4/0x510 [gve]
  Call Trace:
   <TASK>
   gve_queues_mem_alloc+0x29/0xa0 [gve]
   gve_adjust_config+0x1b/0x140 [gve]
   gve_set_ringparam+0x1f4/0x250 [gve]
   ethnl_set_rings+0x260/0x440
   ...

Fix this by falling back to the driver's default minimum ring sizes
(GVE_DEFAULT_MIN_RX_RING_SIZE and GVE_DEFAULT_MIN_TX_RING_SIZE) whenever
the device option reports minimum ring sizes as 0.

This change was tested using the DQO RDA queue format.

Cc: stable@vger.kernel.org
Fixes: ed4fb326947d ("gve: add support to read ring size ranges from the device")
Signed-off-by: Pin-yen Lin <treapking@google.com>
Reviewed-by: Joshua Washington <joshwash@google.com>
Reviewed-by: Jordan Rhee <jordanrhee@google.com>
Signed-off-by: Harshitha Ramamurthy <hramamurthy@google.com>
---

Changes in v4:
- No changes

Changes in v3:
- Retitled and refined the commit message

 drivers/net/ethernet/google/gve/gve_adminq.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/google/gve/gve_adminq.c b/drivers/net/ethernet/google/gve/gve_adminq.c
index 8b215fca9c1b..2c848f0c23dc 100644
--- a/drivers/net/ethernet/google/gve/gve_adminq.c
+++ b/drivers/net/ethernet/google/gve/gve_adminq.c
@@ -191,7 +191,9 @@ void gve_parse_device_option(struct gve_priv *priv,
 		*dev_op_modify_ring = (void *)(option + 1);
 
 		/* device has not provided min ring size */
-		if (option_length == GVE_DEVICE_OPTION_NO_MIN_RING_SIZE)
+		if (option_length == GVE_DEVICE_OPTION_NO_MIN_RING_SIZE ||
+		    be16_to_cpu((*dev_op_modify_ring)->min_rx_ring_size) == 0 ||
+		    be16_to_cpu((*dev_op_modify_ring)->min_tx_ring_size) == 0)
 			priv->device_info.default_min_ring_size = true;
 		break;
 	case GVE_DEV_OPT_ID_FLOW_STEERING:
-- 
2.56.0.313.g2479322e68-goog

Re: [PATCH net v4 3/4] gve: Block invalid ring sizes when device options are 0

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] In gve_parse_device_option()
  (drivers/net/ethernet/google/gve/gve_adminq.c:194-197), the new…

Pre-existing issues:
- [Low] The MODIFY_RING option is accepted whenever option_length >= 8.

--

gve: Block invalid ring sizes when device options are 0

Older devices report the minimum ring sizes in the MODIFY_RING device
option as 0. This let userspace shrink the rings far enough to hit a NULL
pointer dereference in gve_tx_alloc_rings_dqo(). With this patch,
gve_parse_device_option() sets default_min_ring_size when either reported
minimum is 0, so the driver's default minimums are used instead.

> diff --git a/drivers/net/ethernet/google/gve/gve_adminq.c b/drivers/net/ethernet/google/gve/gve_adminq.c
> index 8b215fca9c1b6..2c848f0c23dc2 100644
> --- a/drivers/net/ethernet/google/gve/gve_adminq.c
> +++ b/drivers/net/ethernet/google/gve/gve_adminq.c
> @@ -191,7 +191,9 @@ void gve_parse_device_option(struct gve_priv *priv,
>  		*dev_op_modify_ring = (void *)(option + 1);
>  
>  		/* device has not provided min ring size */
> -		if (option_length == GVE_DEVICE_OPTION_NO_MIN_RING_SIZE)
> +		if (option_length == GVE_DEVICE_OPTION_NO_MIN_RING_SIZE ||
> +		    be16_to_cpu((*dev_op_modify_ring)->min_rx_ring_size) == 0 ||
> +		    be16_to_cpu((*dev_op_modify_ring)->min_tx_ring_size) == 0)
>  			priv->device_info.default_min_ring_size = true;

[Severity: Low]
Should the fallback be done per direction here? One shared flag is set
when either min_rx_ring_size or min_tx_ring_size is 0. After that,
gve_enable_supported_features() overwrites both minimums:

drivers/net/ethernet/google/gve/gve_adminq.c:gve_enable_supported_features() {
    ...
		if (info->default_min_ring_size) {
			/* If device hasn't provided minimums, use default minimums */
			info->min_tx_ring_size = GVE_DEFAULT_MIN_TX_RING_SIZE;
			info->min_rx_ring_size = GVE_DEFAULT_MIN_RX_RING_SIZE;
		} else {
    ...
}

Say a device reports min_tx=1024 and min_rx=0. gve_set_desc_cnt() then
sets the TX minimum to 256, and gve_validate_req_ring_size() would accept
a TX ring size below the device's advertised minimum.

Now say the non-zero minimum is below the default, for example min_tx=64.
In that case, sizes the device supports are now rejected. Before this
patch, the else branch kept the non-zero value.

struct gve_device_option_modify_ring has a separate field for each
minimum. Would a per-field fallback be closer to the intent? For example:

	info->min_tx_ring_size =
		be16_to_cpu(dev_op_modify_ring->min_tx_ring_size) ?:
		GVE_DEFAULT_MIN_TX_RING_SIZE;

Both values would then only be overridden for the legacy 8-byte option,
which has neither field.

It's not clear that any firmware reports only one minimum as 0. The
commit message says older devices report both minimums as 0.

[Severity: Low]
This is a pre-existing issue, but can these reads go past the end of the
option payload? The length check at the top of this case only requires
the legacy 8-byte size:

	case GVE_DEV_OPT_ID_MODIFY_RING:
		if (option_length < GVE_DEVICE_OPTION_NO_MIN_RING_SIZE ||

min_rx_ring_size sits at payload offset 8 and min_tx_ring_size at offset
10. An option_length of 9, 10 or 11 passes this check. The new condition
then reads one or both fields partly or wholly beyond the declared option
length. That is either the next option's header, or memory past the
descriptor's total_length if this is the last option.
gve_get_next_option() only checks the option's declared length.

The else branch of gve_enable_supported_features() already over-reads in
the same way. The new check adds another read at parse time, and that
read runs even when GVE_SUP_MODIFY_RING_MASK isn't set.

Would it help to require option_length >= sizeof(**dev_op_modify_ring)
before reading either minimum? Triggering this needs a malformed device
response.

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

Re: [PATCH net v4 3/4] gve: Block invalid ring sizes when device options are 0

[Pin-yen Lin]

On Mon, Nov 2, 2026 at 1:18 PM <netdev-bot+sashiko@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found more severe
> issue(s) in other patches of this series. This mail is only an FYI, in case
> you want to address the nit picks below when you respin.
>
> Low priority / nit picks for your consideration:
>
> - [Low] In gve_parse_device_option()
>   (drivers/net/ethernet/google/gve/gve_adminq.c:194-197), the new…
>
> Pre-existing issues:
> - [Low] The MODIFY_RING option is accepted whenever option_length >= 8.
>
> --
>
> gve: Block invalid ring sizes when device options are 0
>
> Older devices report the minimum ring sizes in the MODIFY_RING device
> option as 0. This let userspace shrink the rings far enough to hit a NULL
> pointer dereference in gve_tx_alloc_rings_dqo(). With this patch,
> gve_parse_device_option() sets default_min_ring_size when either reported
> minimum is 0, so the driver's default minimums are used instead.
>
> > diff --git a/drivers/net/ethernet/google/gve/gve_adminq.c b/drivers/net/ethernet/google/gve/gve_adminq.c
> > index 8b215fca9c1b6..2c848f0c23dc2 100644
> > --- a/drivers/net/ethernet/google/gve/gve_adminq.c
> > +++ b/drivers/net/ethernet/google/gve/gve_adminq.c
> > @@ -191,7 +191,9 @@ void gve_parse_device_option(struct gve_priv *priv,
> >               *dev_op_modify_ring = (void *)(option + 1);
> >
> >               /* device has not provided min ring size */
> > -             if (option_length == GVE_DEVICE_OPTION_NO_MIN_RING_SIZE)
> > +             if (option_length == GVE_DEVICE_OPTION_NO_MIN_RING_SIZE ||
> > +                 be16_to_cpu((*dev_op_modify_ring)->min_rx_ring_size) == 0 ||
> > +                 be16_to_cpu((*dev_op_modify_ring)->min_tx_ring_size) == 0)
> >                       priv->device_info.default_min_ring_size = true;
>
> [Severity: Low]
> Should the fallback be done per direction here? One shared flag is set
> when either min_rx_ring_size or min_tx_ring_size is 0. After that,
> gve_enable_supported_features() overwrites both minimums:
>
> drivers/net/ethernet/google/gve/gve_adminq.c:gve_enable_supported_features() {
>     ...
>                 if (info->default_min_ring_size) {
>                         /* If device hasn't provided minimums, use default minimums */
>                         info->min_tx_ring_size = GVE_DEFAULT_MIN_TX_RING_SIZE;
>                         info->min_rx_ring_size = GVE_DEFAULT_MIN_RX_RING_SIZE;
>                 } else {
>     ...
> }
>
> Say a device reports min_tx=1024 and min_rx=0. gve_set_desc_cnt() then
> sets the TX minimum to 256, and gve_validate_req_ring_size() would accept
> a TX ring size below the device's advertised minimum.
>
> Now say the non-zero minimum is below the default, for example min_tx=64.
> In that case, sizes the device supports are now rejected. Before this
> patch, the else branch kept the non-zero value.
>
> struct gve_device_option_modify_ring has a separate field for each
> minimum. Would a per-field fallback be closer to the intent? For example:
>
>         info->min_tx_ring_size =
>                 be16_to_cpu(dev_op_modify_ring->min_tx_ring_size) ?:
>                 GVE_DEFAULT_MIN_TX_RING_SIZE;
>
> Both values would then only be overridden for the legacy 8-byte option,
> which has neither field.
>
> It's not clear that any firmware reports only one minimum as 0. The
> commit message says older devices report both minimums as 0.

As my reply in v3, the gvnic device either reports both minimums or
both as zero.
>
> [Severity: Low]
> This is a pre-existing issue, but can these reads go past the end of the
> option payload? The length check at the top of this case only requires
> the legacy 8-byte size:
>
>         case GVE_DEV_OPT_ID_MODIFY_RING:
>                 if (option_length < GVE_DEVICE_OPTION_NO_MIN_RING_SIZE ||
>
> min_rx_ring_size sits at payload offset 8 and min_tx_ring_size at offset
> 10. An option_length of 9, 10 or 11 passes this check. The new condition
> then reads one or both fields partly or wholly beyond the declared option
> length. That is either the next option's header, or memory past the
> descriptor's total_length if this is the last option.
> gve_get_next_option() only checks the option's declared length.
>
> The else branch of gve_enable_supported_features() already over-reads in
> the same way. The new check adds another read at parse time, and that
> read runs even when GVE_SUP_MODIFY_RING_MASK isn't set.
>
> Would it help to require option_length >= sizeof(**dev_op_modify_ring)
> before reading either minimum? Triggering this needs a malformed device
> response.

As my reply in v3, the gvnic device will always return either 8 or 12
for the option_length.
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261029211634.2232784-1-hramamurthy%40google.com

Regards,
Pin-yen

[PATCH net v4 4/4] gve: Fix link flap after ethtool ops

[Harshitha Ramamurthy]

From: Pin-yen Lin <treapking@google.com>

When modifying device features via ethtool, GVE causes a short link-down
after the ethtool command completes, which can break scripts that use
the network interface right after the ethtool command.

This issue was discovered and actually triggered by the
drivers/net/ping.py selftest. Modifying checksum offload features via
ethtool caused a transient carrier drop ("Lost carrier", DHCP lease lost),
causing the immediate ping invocation to fail.

Fix it by calling gve_turnup_and_check_status() in gve_queues_start()
instead of deferring the link status update to the service task, so the
carrier status is updated synchronously before returning to userspace.

Take netdev_lock() around the link status update in gve_service_task(),
so that it is serialized with the synchronous update. All other callers
of gve_handle_link_status() already hold the instance lock.

This change was tested using the DQO RDA queue format.

Cc: stable@vger.kernel.org
Fixes: 3b7cc73628ff ("gve: Use link status register to report link status")
Signed-off-by: Pin-yen Lin <treapking@google.com>
Reviewed-by: Joshua Washington <joshwash@google.com>
Signed-off-by: Harshitha Ramamurthy <hramamurthy@google.com>
---

Changes in v4:
- Take netdev_lock() around the link status update in
  gve_service_task() to serialize it with gve_queues_start() (Sashiko)

Changes in v3:
- Retitled from "gve: Make ethtool config changes synchronous"
- Fix the Fixes tag
- Refined the commit message

 drivers/net/ethernet/google/gve/gve_main.c | 70 ++++++++++++----------
 1 file changed, 39 insertions(+), 31 deletions(-)

diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
index b5e2f39a903c..bc7d7c41948c 100644
--- a/drivers/net/ethernet/google/gve/gve_main.c
+++ b/drivers/net/ethernet/google/gve/gve_main.c
@@ -17,6 +17,7 @@
 #include <linux/sched.h>
 #include <linux/timer.h>
 #include <linux/workqueue.h>
+#include <net/netdev_lock.h>
 #include <net/netdev_queues.h>
 #include <net/sch_generic.h>
 #include <net/xdp_sock_drv.h>
@@ -1431,6 +1432,37 @@ static void gve_queues_stop(struct gve_priv *priv)
 	gve_rx_stop_rings(priv, priv->rx_cfg.num_queues);
 }
 
+static void gve_handle_link_status(struct gve_priv *priv)
+{
+	bool link_up;
+
+	netdev_assert_locked(priv->dev);
+
+	if (!gve_get_napi_enabled(priv))
+		return;
+
+	link_up = READ_ONCE(priv->link_up);
+	if (link_up == netif_carrier_ok(priv->dev))
+		return;
+
+	if (link_up) {
+		netdev_info(priv->dev, "Device link is up.\n");
+		netif_carrier_on(priv->dev);
+	} else {
+		netdev_info(priv->dev, "Device link is down.\n");
+		netif_carrier_off(priv->dev);
+	}
+}
+
+static void gve_turnup_and_check_status(struct gve_priv *priv)
+{
+	const struct gve_ctrl_ops *ops = priv->ctrl_ops;
+
+	gve_turnup(priv);
+	ops->report_link_status(priv);
+	gve_handle_link_status(priv);
+}
+
 /* The passed-in queue memory is stored into priv and the queues are made live.
  * No memory is allocated. Passed-in memory is freed on errors.
  */
@@ -1494,8 +1526,7 @@ static int gve_queues_start(struct gve_priv *priv,
 			  round_jiffies(jiffies +
 				msecs_to_jiffies(priv->stats_report_timer_period)));
 
-	gve_turnup(priv);
-	queue_work(priv->gve_wq, &priv->service_task);
+	gve_turnup_and_check_status(priv);
 	priv->interface_up_cnt++;
 	return 0;
 
@@ -1570,26 +1601,6 @@ static int gve_close(struct net_device *dev)
 	return err;
 }
 
-static void gve_handle_link_status(struct gve_priv *priv)
-{
-	bool link_up;
-
-	if (!gve_get_napi_enabled(priv))
-		return;
-
-	link_up = READ_ONCE(priv->link_up);
-	if (link_up == netif_carrier_ok(priv->dev))
-		return;
-
-	if (link_up) {
-		netdev_info(priv->dev, "Device link is up.\n");
-		netif_carrier_on(priv->dev);
-	} else {
-		netdev_info(priv->dev, "Device link is down.\n");
-		netif_carrier_off(priv->dev);
-	}
-}
-
 static int gve_configure_rings_xdp(struct gve_priv *priv,
 				   u16 num_xdp_rings)
 {
@@ -2064,15 +2075,6 @@ static void gve_turnup(struct gve_priv *priv)
 	gve_set_napi_enabled(priv);
 }
 
-static void gve_turnup_and_check_status(struct gve_priv *priv)
-{
-	const struct gve_ctrl_ops *ops = priv->ctrl_ops;
-
-	gve_turnup(priv);
-	ops->report_link_status(priv);
-	gve_handle_link_status(priv);
-}
-
 static struct gve_notify_block *gve_get_tx_notify_block(struct gve_priv *priv,
 							unsigned int txqueue)
 {
@@ -2406,8 +2408,14 @@ static void gve_service_task(struct work_struct *work)
 	gve_handle_status(priv, status);
 
 	gve_handle_reset(priv);
+
+	/* Serialize with the synchronous link status updates, which all run
+	 * under the netdev instance lock.
+	 */
+	netdev_lock(priv->dev);
 	ops->report_link_status(priv);
 	gve_handle_link_status(priv);
+	netdev_unlock(priv->dev);
 }
 
 static void gve_set_netdev_xdp_features(struct gve_priv *priv)
-- 
2.56.0.313.g2479322e68-goog

Re: [PATCH net v4 4/4] gve: Fix link flap after ethtool ops

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The patch removes `queue_work(priv->gve_wq, &priv->service_task)`
  from the end of gve_queues_start()…

--

gve: Fix link flap after ethtool ops

This patch changes gve_queues_start() to update the carrier state
synchronously through gve_turnup_and_check_status(), instead of queueing
the service task to do it. It also takes netdev_lock() around the link
status update in gve_service_task(), so that update is serialized with
the synchronous path.

> diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> index b5e2f39a903c5..bc7d7c41948c3 100644
> --- a/drivers/net/ethernet/google/gve/gve_main.c
> +++ b/drivers/net/ethernet/google/gve/gve_main.c

[ ... ]

> @@ -1494,8 +1526,7 @@ static int gve_queues_start(struct gve_priv *priv,
>  			  round_jiffies(jiffies +
>  				msecs_to_jiffies(priv->stats_report_timer_period)));
>  
> -	gve_turnup(priv);
> -	queue_work(priv->gve_wq, &priv->service_task);
> +	gve_turnup_and_check_status(priv);
>  	priv->interface_up_cnt++;
>  	return 0;

[Severity: Low]
Commit 3b7cc73628ff added this queue_work() for the link status update,
and the new synchronous check handles that. Does removing it also drop
the service task run that used to follow every reset recovery?

gve_reset() clears the reset flag first and then disables the service
task:

gve_reset() {
    ...
	gve_clear_do_reset(priv);
	gve_set_reset_in_progress(priv);

	if (was_up) {
		gve_turndown(priv);
		...
	}

	disable_work(&priv->service_task);
	...
	enable_work(&priv->service_task);
	err = gve_recover(priv, was_up && !skip_queue_setup);
    ...
}

gve_schedule_reset() could run after gve_clear_do_reset() but before
disable_work(). One way is gve_tx_timeout() from dev_watchdog racing
with gve_turndown(). Another is an RX NAPI error path just before
napi_disable:

gve_schedule_reset() {
	gve_set_do_reset(priv);
	queue_work(priv->gve_wq, &priv->service_task);
}

In that case, disable_work() cancels the queued work, or queue_work()
is rejected while the work is disabled. enable_work() does not queue it
again.

Before this patch, gve_recover()->gve_open()->gve_queues_start() ended
with queue_work(), so a leftover GVE_PRIV_FLAGS_DO_RESET was handled
right after recovery.

After this patch, the service task is not queued again until some
unrelated event, such as a later gve_mgmnt_intr(). When it does run,
gve_service_task()->gve_handle_reset() would perform a full gve_reset()
because of the stale flag. That reset could happen long after, and
unrelated to, the event that set the flag.

gve_suspend() and gve_resume() seem to have the same gap. They call
disable_work_sync() in gve_suspend() and enable_work() before
gve_recover() in gve_resume().

The other parts of this look fine. A missed device REPORT_STATS request
only skips a stats_report_trigger_cnt increment, because
gve_stats_report_schedule() sets do_report_stats on every timer period.
A device RESET request raised inside the disable window is covered by
the reset that is already running.

Could gve_reset() and gve_resume() queue the service task again after
recovery, or re-check gve_get_do_reset() once the work is re-enabled?

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

Re: [PATCH net v4 4/4] gve: Fix link flap after ethtool ops

[Pin-yen Lin]

On Mon, Nov 2, 2026 at 1:18 PM <netdev-bot+sashiko@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found more severe
> issue(s) in other patches of this series. This mail is only an FYI, in case
> you want to address the nit picks below when you respin.
>
> Low priority / nit picks for your consideration:
>
> - [Low] The patch removes `queue_work(priv->gve_wq, &priv->service_task)`
>   from the end of gve_queues_start()…
>
> --
>
> gve: Fix link flap after ethtool ops
>
> This patch changes gve_queues_start() to update the carrier state
> synchronously through gve_turnup_and_check_status(), instead of queueing
> the service task to do it. It also takes netdev_lock() around the link
> status update in gve_service_task(), so that update is serialized with
> the synchronous path.
>
> > diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> > index b5e2f39a903c5..bc7d7c41948c3 100644
> > --- a/drivers/net/ethernet/google/gve/gve_main.c
> > +++ b/drivers/net/ethernet/google/gve/gve_main.c
>
> [ ... ]
>
> > @@ -1494,8 +1526,7 @@ static int gve_queues_start(struct gve_priv *priv,
> >                         round_jiffies(jiffies +
> >                               msecs_to_jiffies(priv->stats_report_timer_period)));
> >
> > -     gve_turnup(priv);
> > -     queue_work(priv->gve_wq, &priv->service_task);
> > +     gve_turnup_and_check_status(priv);
> >       priv->interface_up_cnt++;
> >       return 0;
>
> [Severity: Low]
> Commit 3b7cc73628ff added this queue_work() for the link status update,
> and the new synchronous check handles that. Does removing it also drop
> the service task run that used to follow every reset recovery?
>
> gve_reset() clears the reset flag first and then disables the service
> task:
>
> gve_reset() {
>     ...
>         gve_clear_do_reset(priv);
>         gve_set_reset_in_progress(priv);
>
>         if (was_up) {
>                 gve_turndown(priv);
>                 ...
>         }
>
>         disable_work(&priv->service_task);
>         ...
>         enable_work(&priv->service_task);
>         err = gve_recover(priv, was_up && !skip_queue_setup);
>     ...
> }
>
> gve_schedule_reset() could run after gve_clear_do_reset() but before
> disable_work(). One way is gve_tx_timeout() from dev_watchdog racing
> with gve_turndown(). Another is an RX NAPI error path just before
> napi_disable:
>
> gve_schedule_reset() {
>         gve_set_do_reset(priv);
>         queue_work(priv->gve_wq, &priv->service_task);
> }
>
> In that case, disable_work() cancels the queued work, or queue_work()
> is rejected while the work is disabled. enable_work() does not queue it
> again.
>
> Before this patch, gve_recover()->gve_open()->gve_queues_start() ended
> with queue_work(), so a leftover GVE_PRIV_FLAGS_DO_RESET was handled
> right after recovery.
>
> After this patch, the service task is not queued again until some
> unrelated event, such as a later gve_mgmnt_intr(). When it does run,
> gve_service_task()->gve_handle_reset() would perform a full gve_reset()
> because of the stale flag. That reset could happen long after, and
> unrelated to, the event that set the flag.
>
> gve_suspend() and gve_resume() seem to have the same gap. They call
> disable_work_sync() in gve_suspend() and enable_work() before
> gve_recover() in gve_resume().
>
> The other parts of this look fine. A missed device REPORT_STATS request
> only skips a stats_report_trigger_cnt increment, because
> gve_stats_report_schedule() sets do_report_stats on every timer period.
> A device RESET request raised inside the disable window is covered by
> the reset that is already running.
>
> Could gve_reset() and gve_resume() queue the service task again after
> recovery, or re-check gve_get_do_reset() once the work is re-enabled?

Yes, this is indeed a regression introduced by this patch. When the
reset/suspend races with the Rx NAPI polling error path, the reset
will be delayed until the next service task is queued.

We will add an additional queue_work() after the
gve_turnup_and_check_status() in gve_queues_start().
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261029211634.2232784-1-hramamurthy%40google.com

Regards,
Pin-yen