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

[PATCH net v3 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 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   | 179 ++++++++++++++-----
 3 files changed, 148 insertions(+), 43 deletions(-)


base-commit: 734bef548a96e8774d6eef5dfa2026c6edc4e80b
-- 
2.56.0.257.gf7497c7d27-goog

[PATCH net v3 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
   ...

Additionally, concurrent stats queries during queue reconfiguration can
race with gve_queues_mem_remove(), causing a Use-After-Free.

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.

This change was tested using the DQO RDA queue format.

Cc: stable@vger.kernel.org
Fixes: 2e5e0932dff5 ("gve: add support for basic queue stats")
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 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.257.gf7497c7d27-goog

[PATCH net v3 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().

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 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 | 83 +++++++++++++++++++---
 2 files changed, 81 insertions(+), 8 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..52c152a986eb 100644
--- a/drivers/net/ethernet/google/gve/gve_main.c
+++ b/drivers/net/ethernet/google/gve/gve_main.c
@@ -61,15 +61,25 @@ 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_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++) {
@@ -97,6 +107,38 @@ static void gve_get_stats(struct net_device *dev, struct rtnl_link_stats64 *s)
 			s->tx_bytes += 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 +688,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 +2734,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;
 }
 
@@ -2844,12 +2901,22 @@ 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;
+
+	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 = 0;
-	tx->bytes = 0;
+	tx->packets = base_stats->tx_packets;
+	tx->bytes = base_stats->tx_bytes;
+	spin_unlock_bh(&priv->stats_lock);
 }
 
 static const struct netdev_stat_ops gve_stat_ops = {
-- 
2.56.0.257.gf7497c7d27-goog

[PATCH net v3 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 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.257.gf7497c7d27-goog

[PATCH net v3 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.

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 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 | 61 +++++++++++-----------
 1 file changed, 30 insertions(+), 31 deletions(-)

diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
index 52c152a986eb..e2a016f4430a 100644
--- a/drivers/net/ethernet/google/gve/gve_main.c
+++ b/drivers/net/ethernet/google/gve/gve_main.c
@@ -1423,6 +1423,35 @@ 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;
+
+	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.
  */
@@ -1486,8 +1515,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;
 
@@ -1562,26 +1590,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)
 {
@@ -2056,15 +2064,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)
 {
-- 
2.56.0.257.gf7497c7d27-goog

Re: [PATCH net v3 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: 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

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?

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

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

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

Re: [PATCH net v3 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 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · 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] When XDP is attached, the new base snapshot counts XDP TX ring
  traffic only at teardown, so device-level netdev qstats tx-packets/tx-…

--

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

This patch keeps baseline rtnl and qstats counters in gve_priv so that
device statistics do not go backwards across queue teardown. Ring counters
are folded into the base in gve_queues_mem_remove() and in
gve_rx_queue_stop(). gve_get_base_stats() now reports the saved values
instead of zeros.

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

[ ... ]

> @@ -646,6 +688,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);

[Severity: Medium]
With an XDP program attached, will the device-level qstats tx-packets and
tx-bytes jump forward on every ifdown or reconfiguration?

The TX loop in gve_get_ring_stats() goes up to gve_num_tx_queues(), and
that count includes the XDP TX rings:

    return priv->tx_cfg.num_queues + priv->tx_cfg.num_xdp_queues;

As a result, the full pkt_done/bytes_done history of the XDP TX rings
ends up in base_net_stats.tx_*. Those counters are updated by
gve_clean_xdp_done() and by the DQO XDP path.

While the interface is up, qstats never reads those rings live:

gve_queues_start()
    netif_set_real_num_tx_queues(dev, priv->tx_cfg.num_queues);

netdev_nl_stats_by_netdev()
    netdev_stat_queue_sum(netdev, 0, netdev->real_num_rx_queues, &rx_sum,
                          0, netdev->real_num_tx_queues, &tx_sum);

So gve_get_tx_queue_stats() is only called for the non-XDP TX queues. The
new gve_get_base_stats() returns only the saved snapshot.

The qstats device total therefore leaves out live XDP TX traffic. Then,
when gve_close() or gve_adjust_config() reaches gve_queues_mem_remove(),
all of that traffic shows up at once. gve_get_stats() counts the live XDP
rings the whole time, so rtnl and qstats report different totals while
the interface is up.

The netdev_stat_ops kernel-doc says get_base_stats "can also be used to
report any miscellaneous packets transferred outside of the main set of
queues used by the networking stack".

Should gve_get_base_stats() also add the live counters of
tx[tx_cfg.num_queues .. gve_num_tx_queues())? The other option is to
limit the teardown snapshot's TX loop to tx_cfg.num_queues.

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

[ ... ]

> +	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 = 0;
> -	tx->bytes = 0;
> +	tx->packets = base_stats->tx_packets;
> +	tx->bytes = base_stats->tx_bytes;
> +	spin_unlock_bh(&priv->stats_lock);
>  }

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

Re: [PATCH net v3 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] gve_parse_device_option() (gve_adminq.c:177-197) accepts
  MODIFY_RING options with option_length 9-11, because it rejects only <…

--

gve: Block invalid ring sizes when device options are 0

When the MODIFY_RING device option reports a minimum RX or TX ring size
of 0, this patch makes gve_parse_device_option() fall back to the
driver's default minimum ring sizes. The goal is to stop ethtool -G from
accepting ring sizes small enough to crash gve_tx_alloc_rings_dqo().

> 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]
If only one of these two fields is 0, should the other minimum from the
device be kept?

Both directions share the single default_min_ring_size flag. When it is
set, gve_enable_supported_features() replaces 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 {
    ...
}

Take a device that reports min_rx_ring_size = 1024 and
min_tx_ring_size = 0. Its RX minimum would become
GVE_DEFAULT_MIN_RX_RING_SIZE (512), not 1024. Before this patch, the
non-zero value was used.

gve_set_desc_cnt() copies these values into priv->min_rx_desc_cnt and
priv->min_tx_desc_cnt. Those are the only lower bounds that
gve_validate_req_ring_size() checks:

drivers/net/ethernet/google/gve/gve_ethtool.c:gve_validate_req_ring_size() {
    ...
	if (new_tx_desc_cnt < priv->min_tx_desc_cnt ||
	    new_tx_desc_cnt > priv->max_tx_desc_cnt ||
	    new_rx_desc_cnt < priv->min_rx_desc_cnt ||
    ...
}

Would ethtool -G rx 512 then pass validation and go to the device, even
though it is below the minimum the device reported?

A per-field fallback would use the default only for the field that is 0.
Could that fix the zero-minimum crash and still keep the other limit from
the device? For older devices that report both minimums as 0, as the
commit message describes, the two approaches behave the same.

[Severity: Low]
This is a pre-existing issue, but can these new reads go past the end of
the option payload when option_length is 9, 10 or 11?

The length check earlier in this case rejects only lengths below
GVE_DEVICE_OPTION_NO_MIN_RING_SIZE (8):

drivers/net/ethernet/google/gve/gve_adminq.c:gve_parse_device_option() {
    ...
	case GVE_DEV_OPT_ID_MODIFY_RING:
		if (option_length < GVE_DEVICE_OPTION_NO_MIN_RING_SIZE ||
		    req_feat_mask != GVE_DEV_OPT_REQ_FEAT_MASK_MODIFY_RING) {
    ...
}

min_rx_ring_size and min_tx_ring_size sit at payload offsets 8 and 10,
and sizeof(struct gve_device_option_modify_ring) is 12. With a length of
9 to 11, part of each read comes from the next option header or from
unused descriptor space.

gve_get_next_option() only checks option_length against the
device-provided total_length. Also, total_length is never checked
against the GVE_ADMINQ_BUFFER_SIZE allocation.

If those out-of-option bytes are non-zero, the fallback does not fire
and the bytes are used as the device minimums.

The baseline gve_enable_supported_features() already read the same
fields in this case, because default_min_ring_size stayed false. So this
patch does not add any new reachable impact.

Would checking option_length < sizeof(**dev_op_modify_ring), instead of
comparing with == GVE_DEVICE_OPTION_NO_MIN_RING_SIZE, close this?

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

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · 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] Synchronous link-status update in gve_queues_start() races
  with gve_service_task(); the carrier can be left in the wrong state.

--

gve: Fix link flap after ethtool ops

gve_queues_start() now calls gve_turnup_and_check_status() directly
instead of queueing the service task, so the carrier state is updated
before the call returns to userspace. This avoids the transient carrier
drop after ethtool reconfiguration that was breaking the
drivers/net/ping.py selftest.

> diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> index 52c152a986eb7..e2a016f4430a8 100644
> --- a/drivers/net/ethernet/google/gve/gve_main.c
> +++ b/drivers/net/ethernet/google/gve/gve_main.c

[ ... ]

> @@ -1486,8 +1515,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);

[Severity: Medium]
Can this race with gve_service_task() and leave the carrier in the wrong
state?

Before this change, gve_queues_start() sent its link update through
service_task. A work item never runs concurrently with itself, so these
updates were serialized.

gve_queues_start() is reached from ndo_open, from gve_adjust_config(),
and from gve_reset()->gve_recover()->gve_open(). It holds rtnl and the
netdev instance lock, and it now runs the same sequence as
gve_service_task().

gve_service_task() is queued from gve_mgmnt_intr() and holds neither
lock:

gve_service_task() {
    ...
	gve_handle_reset(priv);
	ops->report_link_status(priv);
	gve_handle_link_status(priv);
}

gve_adminq_report_link_status() reads the register and writes the
cached value in two separate steps:

	status = ioread32be(&priv->reg_bar0->device_status);
	WRITE_ONCE(priv->link_up,
		   !!(GVE_DEVICE_STATUS_LINK_STATUS_MASK & status));

Here is one possible interleaving:

gve_queues_start()                  gve_service_task()
  ioread32be() -> link down
                                    (device link up, mgmt irq)
                                    ioread32be() -> link up
                                    WRITE_ONCE(link_up, true)
                                    netif_carrier_on()
  WRITE_ONCE(link_up, false)
  netif_carrier_off()

At the end the carrier is off while the device link is up. The service
task run triggered by the interrupt has already finished, so nothing
checks the link again until the device raises another status interrupt.

gve_handle_link_status() has a similar window, because its check and
its action are not done under a lock:

	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);

Suppose gve_queues_start() reads link_up == true with the carrier off,
and is then delayed in netdev_info(). In that time gve_service_task()
can write link_up = false, see that it matches netif_carrier_ok(), and
return. gve_queues_start() then calls netif_carrier_on() while the link
is down.

gve_set_xdp() and the per-queue start path already use the same
pattern without a lock. This patch also brings it to the common
gve_queues_start() path, which used to be serialized through the
service task.

Should gve_turnup_and_check_status() and gve_service_task() be
serialized against each other here?

>  	priv->interface_up_cnt++;
>  	return 0;

[ ... ]

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