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

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

[Pin-yen Lin]

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

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 c280ff35ee77..6f1eef11715b 100644
--- a/drivers/net/ethernet/google/gve/gve.h
+++ b/drivers/net/ethernet/google/gve/gve.h
@@ -883,6 +883,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 a6c18e2431cc..b6168bdf2ce7 100644
--- a/drivers/net/ethernet/google/gve/gve_main.c
+++ b/drivers/net/ethernet/google/gve/gve_main.c
@@ -114,6 +114,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++) {
@@ -141,6 +142,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)
@@ -1386,9 +1388,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);
 }
 
 /* The passed-in queue memory is stored into priv and the queues are made live.
@@ -1402,6 +1408,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;
 
@@ -1413,10 +1420,11 @@ static int gve_queues_start(struct gve_priv *priv,
 	priv->rx_desc_cnt = rx_alloc_cfg->ring_size;
 	priv->tx_pages_per_qpl = tx_alloc_cfg->pages_per_qpl;
 	priv->rx_pages_per_qpl = rx_alloc_cfg->pages_per_qpl;
+	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)
@@ -2296,6 +2304,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) {
@@ -2360,6 +2369,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 */
@@ -2775,9 +2785,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;
@@ -2785,20 +2801,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,
@@ -2915,6 +2943,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.385.gd3acb90ef8-goog

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

[Pin-yen Lin]

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>
Reviewed-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 6f1eef11715b..96eab15e7a74 100644
--- a/drivers/net/ethernet/google/gve/gve.h
+++ b/drivers/net/ethernet/google/gve/gve.h
@@ -797,6 +797,10 @@ struct gve_ptp {
 	struct gve_priv *priv;
 };
 
+struct gve_ring_err_stats {
+	u64 rx_alloc_fails;
+};
+
 struct gve_priv {
 	struct net_device *dev;
 	struct gve_tx_ring *tx; /* array of tx_cfg.num_queues */
@@ -888,6 +892,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 b6168bdf2ce7..5bd6549e99b2 100644
--- a/drivers/net/ethernet/google/gve/gve_main.c
+++ b/drivers/net/ethernet/google/gve/gve_main.c
@@ -106,15 +106,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++) {
@@ -142,6 +152,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);
 }
 
@@ -1390,6 +1432,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);
@@ -2678,8 +2723,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;
 }
 
@@ -2833,12 +2890,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.385.gd3acb90ef8-goog

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

[Pin-yen Lin]

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

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 f05f4895f4c7..5e2f91b63335 100644
--- a/drivers/net/ethernet/google/gve/gve_adminq.c
+++ b/drivers/net/ethernet/google/gve/gve_adminq.c
@@ -189,7 +189,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->default_min_ring_size = true;
 		break;
 	case GVE_DEV_OPT_ID_FLOW_STEERING:
-- 
2.56.0.385.gd3acb90ef8-goog

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

[Pin-yen Lin]

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() to
update the carrier status synchronously before returning to userspace,
and move the device_status register read into gve_handle_link_status()
so callers always check the current link status when NAPI is enabled.

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>
Reviewed-by: Harshitha Ramamurthy <hramamurthy@google.com>
---

Changes in v3:
- Retitled from "gve: Make ethtool config changes synchronous"
- Read device_status inside gve_handle_link_status()
- Fix the Fixes tag
- Refined the commit message

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

diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
index 5bd6549e99b2..f1eb0c6e1f9b 100644
--- a/drivers/net/ethernet/google/gve/gve_main.c
+++ b/drivers/net/ethernet/google/gve/gve_main.c
@@ -1442,6 +1442,34 @@ static void gve_queues_mem_remove(struct gve_priv *priv)
 	gve_queues_mem_free(priv, &tx_alloc_cfg, &rx_alloc_cfg);
 }
 
+static void gve_handle_link_status(struct gve_priv *priv)
+{
+	bool link_status;
+	u32 status;
+
+	if (!gve_get_napi_enabled(priv))
+		return;
+
+	status = ioread32be(&priv->reg_bar0->device_status);
+	link_status = GVE_DEVICE_STATUS_LINK_STATUS_MASK & status;
+	if (link_status == netif_carrier_ok(priv->dev))
+		return;
+
+	if (link_status) {
+		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)
+{
+	gve_turnup(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.
  */
@@ -1506,8 +1534,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;
 
@@ -1599,23 +1626,6 @@ static int gve_close(struct net_device *dev)
 	return 0;
 }
 
-static void gve_handle_link_status(struct gve_priv *priv, bool link_status)
-{
-	if (!gve_get_napi_enabled(priv))
-		return;
-
-	if (link_status == netif_carrier_ok(priv->dev))
-		return;
-
-	if (link_status) {
-		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)
 {
@@ -1634,7 +1644,6 @@ static int gve_set_xdp(struct gve_priv *priv, struct bpf_prog *prog,
 {
 	struct bpf_prog *old_prog;
 	int err = 0;
-	u32 status;
 
 	old_prog = READ_ONCE(priv->xdp_prog);
 	if (!netif_running(priv->dev)) {
@@ -1661,8 +1670,7 @@ static int gve_set_xdp(struct gve_priv *priv, struct bpf_prog *prog,
 		bpf_prog_put(old_prog);
 
 out:
-	status = ioread32be(&priv->reg_bar0->device_status);
-	gve_handle_link_status(priv, GVE_DEVICE_STATUS_LINK_STATUS_MASK & status);
+	gve_handle_link_status(priv);
 	return err;
 }
 
@@ -2090,15 +2098,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)
-{
-	u32 status;
-
-	gve_turnup(priv);
-	status = ioread32be(&priv->reg_bar0->device_status);
-	gve_handle_link_status(priv, GVE_DEVICE_STATUS_LINK_STATUS_MASK & status);
-}
-
 static struct gve_notify_block *gve_get_tx_notify_block(struct gve_priv *priv,
 							unsigned int txqueue)
 {
@@ -2427,7 +2426,7 @@ static void gve_service_task(struct work_struct *work)
 	gve_handle_status(priv, status);
 
 	gve_handle_reset(priv);
-	gve_handle_link_status(priv, GVE_DEVICE_STATUS_LINK_STATUS_MASK & status);
+	gve_handle_link_status(priv);
 }
 
 static void gve_set_netdev_xdp_features(struct gve_priv *priv)
-- 
2.56.0.385.gd3acb90ef8-goog

[PATCH net v3 5/5] gve: Fix UAF due to stats report race with teardown

[Pin-yen Lin]

While stats_report_timer is deleted during queue stop and teardown, the
corresponding stats_report_task work item is not explicitly cancelled.

This issue was identified by static analysis and code review via Sashiko
during review of earlier revisions. The race condition is purely
theoretical and was not observed triggering a crash in practice.

This leads to the following issues:
1. During queue stop: stats_report_task can still be pending after
   stats_report_timer is deleted and run after queues are stopped.
2. During teardown/reset/suspend: if stats_report_task was queued by
   the timer, freeing priv->stats_report in
   gve_free_stats_report() without cancelling the work item can cause a
   NULL pointer dereference or UAF on priv->stats_report when
   gve_stats_report_task() runs.

Fix this by calling cancel_work_sync(&priv->stats_report_task) after
timer_delete_sync() in both gve_queues_stop() and gve_free_stats_report().

The change is theoretical and was not tested on real hardware.

Link: https://sashiko.dev/#/patchset/20260425002450.163421-1-hramamurthy%40google.com?part=1
Cc: stable@vger.kernel.org
Fixes: 24aeb56f2d38 ("gve: Add Gvnic stats AQ command and ethtool show/set-priv-flags.")
Signed-off-by: Pin-yen Lin <treapking@google.com>
---

Changes in v3:
- New in v3

 drivers/net/ethernet/google/gve/gve_main.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
index f1eb0c6e1f9b..28624ded5f3b 100644
--- a/drivers/net/ethernet/google/gve/gve_main.c
+++ b/drivers/net/ethernet/google/gve/gve_main.c
@@ -350,6 +350,7 @@ static void gve_free_stats_report(struct gve_priv *priv)
 		return;
 
 	timer_delete_sync(&priv->stats_report_timer);
+	cancel_work_sync(&priv->stats_report_task);
 	dma_free_coherent(&priv->pdev->dev, priv->stats_report_len,
 			  priv->stats_report, priv->stats_report_bus);
 	priv->stats_report = NULL;
@@ -1593,6 +1594,7 @@ static int gve_queues_stop(struct gve_priv *priv)
 		gve_clear_device_rings_ok(priv);
 	}
 	timer_delete_sync(&priv->stats_report_timer);
+	cancel_work_sync(&priv->stats_report_task);
 
 	gve_unreg_xdp_info(priv);
 
-- 
2.56.0.385.gd3acb90ef8-goog

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

[Pin-yen Lin]

Hi everyone,

Please disregard this patch series. It was sent to the mailing list by
mistake. I intended to send it to myself for verification and
accidentally included the stable list in the CC.

My apologies for the noise and any confusion caused.

Best regards,
Pin-yen


On Tue, Oct 13, 2026 at 5:38 PM Pin-yen Lin <treapking@google.com> wrote:
>
> 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>
> ---
>
> 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 c280ff35ee77..6f1eef11715b 100644
> --- a/drivers/net/ethernet/google/gve/gve.h
> +++ b/drivers/net/ethernet/google/gve/gve.h
> @@ -883,6 +883,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 a6c18e2431cc..b6168bdf2ce7 100644
> --- a/drivers/net/ethernet/google/gve/gve_main.c
> +++ b/drivers/net/ethernet/google/gve/gve_main.c
> @@ -114,6 +114,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++) {
> @@ -141,6 +142,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)
> @@ -1386,9 +1388,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);
>  }
>
>  /* The passed-in queue memory is stored into priv and the queues are made live.
> @@ -1402,6 +1408,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;
>
> @@ -1413,10 +1420,11 @@ static int gve_queues_start(struct gve_priv *priv,
>         priv->rx_desc_cnt = rx_alloc_cfg->ring_size;
>         priv->tx_pages_per_qpl = tx_alloc_cfg->pages_per_qpl;
>         priv->rx_pages_per_qpl = rx_alloc_cfg->pages_per_qpl;
> +       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)
> @@ -2296,6 +2304,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) {
> @@ -2360,6 +2369,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 */
> @@ -2775,9 +2785,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;
> @@ -2785,20 +2801,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,
> @@ -2915,6 +2943,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.385.gd3acb90ef8-goog
>