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

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

[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 uncovered by running the net selftests.

The series includes the following changes:

- Patch 1 adds NULL pointer checks for the per-queue statistics code to
  prevent crashes when the rings are queried while the link is down. 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.

Debarghya Kundu (2):
  gve: Add NULL pointer checks for per-queue statistics
  gve: Fix backward stats when interface goes down or configuration is
    adjusted

Pin-yen Lin (2):
  gve: Use default min ring size when device option values are 0
  gve: Make ethtool config changes synchronous

 drivers/net/ethernet/google/gve/gve.h        |   6 +
 drivers/net/ethernet/google/gve/gve_adminq.c |   4 +-
 drivers/net/ethernet/google/gve/gve_main.c   | 128 +++++++++++++------
 3 files changed, 100 insertions(+), 38 deletions(-)

-- 
2.54.0.rc0.605.g598a273b03-goog

base-commit: 2dddb34dd0d07b01fa770eca89480a4da4f13153
branch: gve-misc-fixes

[PATCH net 1/4] gve: Add NULL pointer checks for per-queue statistics

[Harshitha Ramamurthy]

From: Debarghya Kundu <debarghyak@google.com>

gve_get_[tx/rx]_queue_stats references the [tx/rx] null rings when the
link is down. Add NULL pointer checks to guard this.

This was discovered by drivers/net/stats.py selftest.

Cc: stable@vger.kernel.org
Fixes: 2e5e0932dff5 ("gve: add support for basic queue stats")
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>
---
 drivers/net/ethernet/google/gve/gve_main.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
index 0ee864b0afe0..675382e9756c 100644
--- a/drivers/net/ethernet/google/gve/gve_main.c
+++ b/drivers/net/ethernet/google/gve/gve_main.c
@@ -2705,9 +2705,13 @@ 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;
 
+	if (!priv->rx)
+		return;
+	rx = &priv->rx[idx];
+
 	do {
 		start = u64_stats_fetch_begin(&rx->statss);
 		rx_stats->packets = rx->rpackets;
@@ -2721,9 +2725,13 @@ 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;
 
+	if (!priv->tx)
+		return;
+	tx = &priv->tx[idx];
+
 	do {
 		start = u64_stats_fetch_begin(&tx->statss);
 		tx_stats->packets = tx->pkt_done;
-- 
2.54.0.rc0.605.g598a273b03-goog

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

Fix this by persisting baseline stats across interface down.

This was discovered by drivers/net/stats.py selftest.

Cc: stable@vger.kernel.org
Fixes: 2e5e0932dff5 ("gve: add support for basic queue stats")
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>
---
 drivers/net/ethernet/google/gve/gve.h      |  6 ++
 drivers/net/ethernet/google/gve/gve_main.c | 64 +++++++++++++++++++---
 2 files changed, 63 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/google/gve/gve.h b/drivers/net/ethernet/google/gve/gve.h
index cbdf3a842cfe..ff7797043908 100644
--- a/drivers/net/ethernet/google/gve/gve.h
+++ b/drivers/net/ethernet/google/gve/gve.h
@@ -794,6 +794,10 @@ struct gve_ptp {
 	struct gve_priv *priv;
 };
 
+struct gve_ring_err_stats {
+	u64 rx_alloc_fails;
+};
+
 struct gve_priv {
 	struct net_device *dev;
 	struct gve_tx_ring *tx; /* array of tx_cfg.num_queues */
@@ -882,6 +886,8 @@ struct gve_priv {
 	unsigned long service_task_flags;
 	unsigned long state_flags;
 
+	struct gve_ring_err_stats base_ring_err_stats;
+	struct rtnl_link_stats64 base_net_stats;
 	struct gve_stats_report *stats_report;
 	u64 stats_report_len;
 	dma_addr_t stats_report_bus; /* dma address for the stats report */
diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
index 675382e9756c..8617782791e0 100644
--- a/drivers/net/ethernet/google/gve/gve_main.c
+++ b/drivers/net/ethernet/google/gve/gve_main.c
@@ -105,9 +105,22 @@ static netdev_tx_t gve_start_xmit(struct sk_buff *skb, struct net_device *dev)
 		return gve_tx_dqo(skb, dev);
 }
 
-static void gve_get_stats(struct net_device *dev, struct rtnl_link_stats64 *s)
+static void gve_add_base_stats(struct gve_priv *priv,
+			       struct rtnl_link_stats64 *s)
+{
+	struct rtnl_link_stats64 *base_stats = &priv->base_net_stats;
+
+	s->rx_packets += base_stats->rx_packets;
+	s->rx_bytes += base_stats->rx_bytes;
+	s->rx_dropped += base_stats->rx_dropped;
+	s->tx_packets += base_stats->tx_packets;
+	s->tx_bytes += base_stats->tx_bytes;
+	s->tx_dropped += base_stats->tx_dropped;
+}
+
+static void gve_get_ring_stats(struct gve_priv *priv,
+			       struct rtnl_link_stats64 *s)
 {
-	struct gve_priv *priv = netdev_priv(dev);
 	unsigned int start;
 	u64 packets, bytes;
 	int num_tx_queues;
@@ -142,6 +155,14 @@ static void gve_get_stats(struct net_device *dev, struct rtnl_link_stats64 *s)
 	}
 }
 
+static void gve_get_stats(struct net_device *dev, struct rtnl_link_stats64 *s)
+{
+	struct gve_priv *priv = netdev_priv(dev);
+
+	gve_get_ring_stats(priv, s);
+	gve_add_base_stats(priv, s);
+}
+
 static int gve_alloc_flow_rule_caches(struct gve_priv *priv)
 {
 	struct gve_flow_rules_cache *flow_rules_cache = &priv->flow_rules_cache;
@@ -1493,6 +1514,23 @@ static int gve_queues_stop(struct gve_priv *priv)
 	return gve_reset_recovery(priv, false);
 }
 
+static void gve_get_ring_err_stats(struct gve_priv *priv,
+				   struct gve_ring_err_stats *err_stats)
+{
+	int ring;
+
+	for (ring = 0; ring < priv->rx_cfg.num_queues; ring++) {
+		unsigned int start;
+		struct gve_rx_ring *rx = &priv->rx[ring];
+
+		do {
+			start = u64_stats_fetch_begin(&rx->statss);
+			err_stats->rx_alloc_fails +=
+				rx->rx_skb_alloc_fail + rx->rx_buf_alloc_fail;
+		} while (u64_stats_fetch_retry(&rx->statss, start));
+	}
+}
+
 static int gve_close(struct net_device *dev)
 {
 	struct gve_priv *priv = netdev_priv(dev);
@@ -1502,6 +1540,10 @@ static int gve_close(struct net_device *dev)
 	if (err)
 		return err;
 
+	/* Save ring queue and err stats before closing the interface */
+	gve_get_ring_stats(priv, &priv->base_net_stats);
+	gve_get_ring_err_stats(priv, &priv->base_ring_err_stats);
+
 	gve_queues_mem_remove(priv);
 	return 0;
 }
@@ -2743,12 +2785,20 @@ 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;
+
+	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;
 }
 
 static const struct netdev_stat_ops gve_stat_ops = {
-- 
2.54.0.rc0.605.g598a273b03-goog

[PATCH net 3/4] gve: Use default min ring size when device option values are 0

[Harshitha Ramamurthy]

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

On gvnic devices that support reporting minimum ring sizes, the device
option always includes the min_(rx|tx)_ring_size fields, and the values
will be 0 if they are not configured to be exposed. This makes the
driver allow unexpected small ring size configurations from the
userspace.

Use the default ring size in the driver if the min ring sizes from the
device option are 0.

This was discovered by drivers/net/ring_reconfig.py selftest.

Cc: stable@vger.kernel.org
Fixes: ed4fb326947d ("gve: add support to read ring size ranges from the device")
Reviewed-by: Joshua Washington <joshwash@google.com>
Reviewed-by: Jordan Rhee <jordanrhee@google.com>
Signed-off-by: Pin-yen Lin <treapking@google.com>
Signed-off-by: Harshitha Ramamurthy <hramamurthy@google.com>
---
 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 b72cc0fa2ba2..57d898f6fa82 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.54.0.rc0.605.g598a273b03-goog

[PATCH net 4/4] gve: Make ethtool config changes synchronous

[Harshitha Ramamurthy]

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

When modifying device features via ethtool, the driver queues the
carrier status update to its workqueue (gve_wq). This leads to a
short link-down state after running the ethtool command.

Use `gve_turnup_and_check_status()` instead of `gve_turnup()` in
`gve_queues_start()` to update the carrier status before returning to
the userspace.

This was discovered by drivers/net/ping.py selftest. The test calls
ping command right after an ethtool configuration, but the interface
could be down without this fix.

Cc: stable@vger.kernel.org
Fixes: 5f08cd3d6423 ("gve: Alloc before freeing when adjusting queues")
Reviewed-by: Joshua Washington <joshwash@google.com>
Signed-off-by: Pin-yen Lin <treapking@google.com>
Signed-off-by: Harshitha Ramamurthy <hramamurthy@google.com>
---
 drivers/net/ethernet/google/gve/gve_main.c | 56 +++++++++++-----------
 1 file changed, 28 insertions(+), 28 deletions(-)

diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
index 8617782791e0..d3b4bec38de5 100644
--- a/drivers/net/ethernet/google/gve/gve_main.c
+++ b/drivers/net/ethernet/google/gve/gve_main.c
@@ -1374,6 +1374,33 @@ static void gve_queues_mem_remove(struct gve_priv *priv)
 	priv->rx = NULL;
 }
 
+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 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);
+}
+
 /* 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.
  */
@@ -1434,8 +1461,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;
 
@@ -1548,23 +1574,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)
 {
@@ -2039,15 +2048,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)
 {
-- 
2.54.0.rc0.605.g598a273b03-goog

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

[Paolo Abeni]

On 4/20/26 7:18 PM, Harshitha Ramamurthy wrote:
> 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.
> 
> Fix this by persisting baseline stats across interface down.
> 
> This was discovered by drivers/net/stats.py selftest.
> 
> Cc: stable@vger.kernel.org
> Fixes: 2e5e0932dff5 ("gve: add support for basic queue stats")
> 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>
> ---
>  drivers/net/ethernet/google/gve/gve.h      |  6 ++
>  drivers/net/ethernet/google/gve/gve_main.c | 64 +++++++++++++++++++---
>  2 files changed, 63 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/ethernet/google/gve/gve.h b/drivers/net/ethernet/google/gve/gve.h
> index cbdf3a842cfe..ff7797043908 100644
> --- a/drivers/net/ethernet/google/gve/gve.h
> +++ b/drivers/net/ethernet/google/gve/gve.h
> @@ -794,6 +794,10 @@ struct gve_ptp {
>  	struct gve_priv *priv;
>  };
>  
> +struct gve_ring_err_stats {
> +	u64 rx_alloc_fails;
> +};
> +
>  struct gve_priv {
>  	struct net_device *dev;
>  	struct gve_tx_ring *tx; /* array of tx_cfg.num_queues */
> @@ -882,6 +886,8 @@ struct gve_priv {
>  	unsigned long service_task_flags;
>  	unsigned long state_flags;
>  
> +	struct gve_ring_err_stats base_ring_err_stats;
> +	struct rtnl_link_stats64 base_net_stats;
>  	struct gve_stats_report *stats_report;
>  	u64 stats_report_len;
>  	dma_addr_t stats_report_bus; /* dma address for the stats report */
> diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> index 675382e9756c..8617782791e0 100644
> --- a/drivers/net/ethernet/google/gve/gve_main.c
> +++ b/drivers/net/ethernet/google/gve/gve_main.c
> @@ -105,9 +105,22 @@ static netdev_tx_t gve_start_xmit(struct sk_buff *skb, struct net_device *dev)
>  		return gve_tx_dqo(skb, dev);
>  }
>  
> -static void gve_get_stats(struct net_device *dev, struct rtnl_link_stats64 *s)
> +static void gve_add_base_stats(struct gve_priv *priv,
> +			       struct rtnl_link_stats64 *s)
> +{
> +	struct rtnl_link_stats64 *base_stats = &priv->base_net_stats;
> +
> +	s->rx_packets += base_stats->rx_packets;
> +	s->rx_bytes += base_stats->rx_bytes;
> +	s->rx_dropped += base_stats->rx_dropped;
> +	s->tx_packets += base_stats->tx_packets;
> +	s->tx_bytes += base_stats->tx_bytes;
> +	s->tx_dropped += base_stats->tx_dropped;
> +}
> +
> +static void gve_get_ring_stats(struct gve_priv *priv,
> +			       struct rtnl_link_stats64 *s)
>  {
> -	struct gve_priv *priv = netdev_priv(dev);
>  	unsigned int start;
>  	u64 packets, bytes;
>  	int num_tx_queues;
> @@ -142,6 +155,14 @@ static void gve_get_stats(struct net_device *dev, struct rtnl_link_stats64 *s)
>  	}
>  }
>  
> +static void gve_get_stats(struct net_device *dev, struct rtnl_link_stats64 *s)
> +{
> +	struct gve_priv *priv = netdev_priv(dev);
> +
> +	gve_get_ring_stats(priv, s);
> +	gve_add_base_stats(priv, s);
> +}
> +
>  static int gve_alloc_flow_rule_caches(struct gve_priv *priv)
>  {
>  	struct gve_flow_rules_cache *flow_rules_cache = &priv->flow_rules_cache;
> @@ -1493,6 +1514,23 @@ static int gve_queues_stop(struct gve_priv *priv)
>  	return gve_reset_recovery(priv, false);
>  }
>  
> +static void gve_get_ring_err_stats(struct gve_priv *priv,
> +				   struct gve_ring_err_stats *err_stats)
> +{
> +	int ring;
> +
> +	for (ring = 0; ring < priv->rx_cfg.num_queues; ring++) {
> +		unsigned int start;
> +		struct gve_rx_ring *rx = &priv->rx[ring];
> +
> +		do {
> +			start = u64_stats_fetch_begin(&rx->statss);
> +			err_stats->rx_alloc_fails +=
> +				rx->rx_skb_alloc_fail + rx->rx_buf_alloc_fail;
> +		} while (u64_stats_fetch_retry(&rx->statss, start));

Sashiko says:

Could this loop improperly inflate the baseline metric by double counting?
If a concurrent update causes the sequence counter to change,
u64_stats_fetch_retry() forces the loop to restart. Because the addition
is performed in-place on err_stats->rx_alloc_fails, the same ring's
error values will be added again.
Would it be safer to use local variables inside the retry loop and update
the global accumulator only after the loop completes successfully, similar
to the pattern established in gve_get_ring_stats()?


> +	}
> +}
> +
>  static int gve_close(struct net_device *dev)
>  {
>  	struct gve_priv *priv = netdev_priv(dev);
> @@ -1502,6 +1540,10 @@ static int gve_close(struct net_device *dev)
>  	if (err)
>  		return err;
>  
> +	/* Save ring queue and err stats before closing the interface */
> +	gve_get_ring_stats(priv, &priv->base_net_stats);
> +	gve_get_ring_err_stats(priv, &priv->base_ring_err_stats);

Sashiko says:

Does this create a temporary spike in reported statistics?
During gve_close(), the active ring stats are added to base_net_stats.
However, priv->rx and priv->tx are not set to NULL until the memory
teardown completes in gve_queues_mem_remove().
If ndo_get_stats64 is called concurrently during this window, it will
add both the active ring stats and the newly updated base stats together.
This causes the reported statistics to temporarily double until the
teardown finishes.

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

Thanks,

Paolo

Re: [PATCH net 4/4] gve: Make ethtool config changes synchronous

[Paolo Abeni]

On 4/20/26 7:18 PM, Harshitha Ramamurthy wrote:
> From: Pin-yen Lin <treapking@google.com>
> 
> When modifying device features via ethtool, the driver queues the
> carrier status update to its workqueue (gve_wq). This leads to a
> short link-down state after running the ethtool command.
> 
> Use `gve_turnup_and_check_status()` instead of `gve_turnup()` in
> `gve_queues_start()` to update the carrier status before returning to
> the userspace.
> 
> This was discovered by drivers/net/ping.py selftest. The test calls
> ping command right after an ethtool configuration, but the interface
> could be down without this fix.
> 
> Cc: stable@vger.kernel.org
> Fixes: 5f08cd3d6423 ("gve: Alloc before freeing when adjusting queues")
> Reviewed-by: Joshua Washington <joshwash@google.com>
> Signed-off-by: Pin-yen Lin <treapking@google.com>
> Signed-off-by: Harshitha Ramamurthy <hramamurthy@google.com>
> ---
>  drivers/net/ethernet/google/gve/gve_main.c | 56 +++++++++++-----------
>  1 file changed, 28 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> index 8617782791e0..d3b4bec38de5 100644
> --- a/drivers/net/ethernet/google/gve/gve_main.c
> +++ b/drivers/net/ethernet/google/gve/gve_main.c
> @@ -1374,6 +1374,33 @@ static void gve_queues_mem_remove(struct gve_priv *priv)
>  	priv->rx = NULL;
>  }
>  
> +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 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);
> +}
> +
>  /* 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.
>   */
> @@ -1434,8 +1461,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);

Sashiko says:

Since gve_handle_link_status() can now be called from process context
via gve_turnup_and_check_status(), while also being concurrently
executed by gve_service_task() on the workqueue, could this create a
time-of-check to time-of-use race?
If the physical link toggles rapidly, could the workqueue thread sample
the later hardware state (e.g. OFF) but see the software state is
already OFF and return early, while the process context thread sampled
the earlier state (e.g. ON), evaluated netif_carrier_ok() as OFF, and
proceeded to call netif_carrier_on()?
This might leave the software carrier state stuck ON when the most
recent hardware state is OFF, because the condition check and update are
no longer serialized by the workqueue.

Notes that there more comments:

https://sashiko.dev/#/patchset/20260420171837.455487-1-hramamurthy%40google.com

but I'm not sure if they are actual regressions introduced by this series.

/P

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

[Pin-yen Lin]

Hi Paolo,

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

Sashiko says:

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

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

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

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

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

We will send out a separate patch to fix this.

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

Thanks for the reminder.

>
> Thanks,
>
> Paolo
>

Regards,
Pin-yen

Re: [PATCH net 4/4] gve: Make ethtool config changes synchronous

[Pin-yen Lin]

Hi Paolo,

On Thu, Apr 23, 2026 at 6:25 AM Paolo Abeni <pabeni@redhat.com> wrote:
>
> On 4/20/26 7:18 PM, Harshitha Ramamurthy wrote:
> > From: Pin-yen Lin <treapking@google.com>
> >
> > When modifying device features via ethtool, the driver queues the
> > carrier status update to its workqueue (gve_wq). This leads to a
> > short link-down state after running the ethtool command.
> >
> > Use `gve_turnup_and_check_status()` instead of `gve_turnup()` in
> > `gve_queues_start()` to update the carrier status before returning to
> > the userspace.
> >
> > This was discovered by drivers/net/ping.py selftest. The test calls
> > ping command right after an ethtool configuration, but the interface
> > could be down without this fix.
> >
> > Cc: stable@vger.kernel.org
> > Fixes: 5f08cd3d6423 ("gve: Alloc before freeing when adjusting queues")
> > Reviewed-by: Joshua Washington <joshwash@google.com>
> > Signed-off-by: Pin-yen Lin <treapking@google.com>
> > Signed-off-by: Harshitha Ramamurthy <hramamurthy@google.com>
> > ---
> >  drivers/net/ethernet/google/gve/gve_main.c | 56 +++++++++++-----------
> >  1 file changed, 28 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> > index 8617782791e0..d3b4bec38de5 100644
> > --- a/drivers/net/ethernet/google/gve/gve_main.c
> > +++ b/drivers/net/ethernet/google/gve/gve_main.c
> > @@ -1374,6 +1374,33 @@ static void gve_queues_mem_remove(struct gve_priv *priv)
> >       priv->rx = NULL;
> >  }
> >
> > +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 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);
> > +}
> > +
> >  /* 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.
> >   */
> > @@ -1434,8 +1461,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);
>
> Sashiko says:
>
> Since gve_handle_link_status() can now be called from process context
> via gve_turnup_and_check_status(), while also being concurrently
> executed by gve_service_task() on the workqueue, could this create a
> time-of-check to time-of-use race?
> If the physical link toggles rapidly, could the workqueue thread sample
> the later hardware state (e.g. OFF) but see the software state is
> already OFF and return early, while the process context thread sampled
> the earlier state (e.g. ON), evaluated netif_carrier_ok() as OFF, and
> proceeded to call netif_carrier_on()?
> This might leave the software carrier state stuck ON when the most
> recent hardware state is OFF, because the condition check and update are
> no longer serialized by the workqueue.
>

This is a legitimate concern, but it is not introduced by this series.

gve_turnup_and_check_status() is an existing function. And, given that
there is no locking around gve_service_task(), there could be
concurrency issues on gve_handle_link_status().

We will upload a separate patch to fix this.

> Notes that there more comments:
>
> https://sashiko.dev/#/patchset/20260420171837.455487-1-hramamurthy%40google.com
>
> but I'm not sure if they are actual regressions introduced by this series.

Thanks for the pointers. I'll reply to all the Sashiko comments on the
mailing list.
>
> /P
>

Regards,
Pin-yen

Re: [PATCH net 3/4] gve: Use default min ring size when device option values are 0

[Pin-yen Lin]

On Mon, Apr 20, 2026 at 10:18 AM Harshitha Ramamurthy
<hramamurthy@google.com> wrote:
>
> From: Pin-yen Lin <treapking@google.com>
>
> On gvnic devices that support reporting minimum ring sizes, the device
> option always includes the min_(rx|tx)_ring_size fields, and the values
> will be 0 if they are not configured to be exposed. This makes the
> driver allow unexpected small ring size configurations from the
> userspace.
>
> Use the default ring size in the driver if the min ring sizes from the
> device option are 0.
>
> This was discovered by drivers/net/ring_reconfig.py selftest.
>
> Cc: stable@vger.kernel.org
> Fixes: ed4fb326947d ("gve: add support to read ring size ranges from the device")
> Reviewed-by: Joshua Washington <joshwash@google.com>
> Reviewed-by: Jordan Rhee <jordanrhee@google.com>
> Signed-off-by: Pin-yen Lin <treapking@google.com>
> Signed-off-by: Harshitha Ramamurthy <hramamurthy@google.com>
> ---
>  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 b72cc0fa2ba2..57d898f6fa82 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)

Sashiko says:

Does this introduce an out-of-bounds read regression if the option length
is between 9 and 11?
The length check earlier in the switch case only ensures option_length is at
least GVE_DEVICE_OPTION_NO_MIN_RING_SIZE (which is 8). The new condition
reads min_tx_ring_size, which sits at bytes 10-11 of the structure.
If an option length of 10 is provided, the condition would read past the
provided data length. Could this trigger a page fault if the option payload
happens to be placed at the very end of an unmapped DMA page boundary?
Could this introduce a time-of-check to time-of-use regression? Can these
values be modified by the device between this check and when they are used
later?
Since dev_op_modify_ring points into shared DMA memory, the values are
fetched here for validation. If the device alters the DMA memory to 0 after
this check has completed, would the driver bypass this validation when
applying the values later, leading to a minimum ring size of 0?
Does this introduce a logic regression by discarding a valid configuration
for one ring if the other ring's size is 0?
For example, if min_rx_ring_size is 0 but min_tx_ring_size is explicitly
configured to a valid size like 1024, setting a single default_min_ring_size
flag seems to force both rings to use their default fallback values instead
of evaluating their validity independently.

This should not be an issue because the GVNIC never returns an option
length between 9 and 11. Depending on the GVNIC version, it either
omits the ring sizes or includes both RX and TX ring sizes (even if
the values are 0).

>                         priv->default_min_ring_size = true;
>                 break;
>         case GVE_DEV_OPT_ID_FLOW_STEERING:
> --
> 2.54.0.rc0.605.g598a273b03-goog
>

Regards,
Pin-yen

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

[Pin-yen Lin]

On Thu, Apr 23, 2026 at 6:17 PM Pin-yen Lin <treapking@google.com> wrote:
>
> Hi Paolo,
>
> On Thu, Apr 23, 2026 at 4:47 AM Paolo Abeni <pabeni@redhat.com> wrote:
> >
> > On 4/20/26 7:18 PM, Harshitha Ramamurthy wrote:
> > > 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.
> > >
> > > Fix this by persisting baseline stats across interface down.
> > >
> > > This was discovered by drivers/net/stats.py selftest.
> > >
> > > Cc: stable@vger.kernel.org
> > > Fixes: 2e5e0932dff5 ("gve: add support for basic queue stats")
> > > 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>
> > > ---
> > >  drivers/net/ethernet/google/gve/gve.h      |  6 ++
> > >  drivers/net/ethernet/google/gve/gve_main.c | 64 +++++++++++++++++++---
> > >  2 files changed, 63 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/google/gve/gve.h b/drivers/net/ethernet/google/gve/gve.h
> > > index cbdf3a842cfe..ff7797043908 100644
> > > --- a/drivers/net/ethernet/google/gve/gve.h
> > > +++ b/drivers/net/ethernet/google/gve/gve.h
> > > @@ -794,6 +794,10 @@ struct gve_ptp {
> > >       struct gve_priv *priv;
> > >  };
> > >
> > > +struct gve_ring_err_stats {
> > > +     u64 rx_alloc_fails;
> > > +};
> > > +
> > >  struct gve_priv {
> > >       struct net_device *dev;
> > >       struct gve_tx_ring *tx; /* array of tx_cfg.num_queues */
> > > @@ -882,6 +886,8 @@ struct gve_priv {
> > >       unsigned long service_task_flags;
> > >       unsigned long state_flags;
> > >
> > > +     struct gve_ring_err_stats base_ring_err_stats;
> > > +     struct rtnl_link_stats64 base_net_stats;
> > >       struct gve_stats_report *stats_report;
> > >       u64 stats_report_len;
> > >       dma_addr_t stats_report_bus; /* dma address for the stats report */
> > > diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> > > index 675382e9756c..8617782791e0 100644
> > > --- a/drivers/net/ethernet/google/gve/gve_main.c
> > > +++ b/drivers/net/ethernet/google/gve/gve_main.c
> > > @@ -105,9 +105,22 @@ static netdev_tx_t gve_start_xmit(struct sk_buff *skb, struct net_device *dev)
> > >               return gve_tx_dqo(skb, dev);
> > >  }
> > >
> > > -static void gve_get_stats(struct net_device *dev, struct rtnl_link_stats64 *s)
> > > +static void gve_add_base_stats(struct gve_priv *priv,
> > > +                            struct rtnl_link_stats64 *s)
> > > +{
> > > +     struct rtnl_link_stats64 *base_stats = &priv->base_net_stats;
> > > +
> > > +     s->rx_packets += base_stats->rx_packets;
> > > +     s->rx_bytes += base_stats->rx_bytes;
> > > +     s->rx_dropped += base_stats->rx_dropped;
> > > +     s->tx_packets += base_stats->tx_packets;
> > > +     s->tx_bytes += base_stats->tx_bytes;
> > > +     s->tx_dropped += base_stats->tx_dropped;
> > > +}
>
> Sashiko says:
>
> Can this result in torn reads on 32-bit architectures?
> The base_net_stats struct accumulates 64-bit network statistics in gve_close()
> under rtnl_lock, but these stats are read here via ndo_get_stats64 which can
> execute concurrently without rtnl_lock.
> On 32-bit systems, a concurrent update might result in torn reads since 64-bit
> memory reads are not atomic.
> Should u64_stats_sync sequence counters or atomic types be used here?
>
> We will add u64_stats_fetch_(begin|retry) to guard this in v2.
>
> > > +
> > > +static void gve_get_ring_stats(struct gve_priv *priv,
> > > +                            struct rtnl_link_stats64 *s)
> > >  {
> > > -     struct gve_priv *priv = netdev_priv(dev);
> > >       unsigned int start;
> > >       u64 packets, bytes;
> > >       int num_tx_queues;
> > > @@ -142,6 +155,14 @@ static void gve_get_stats(struct net_device *dev, struct rtnl_link_stats64 *s)
> > >       }
> > >  }
> > >
> > > +static void gve_get_stats(struct net_device *dev, struct rtnl_link_stats64 *s)
> > > +{
> > > +     struct gve_priv *priv = netdev_priv(dev);
> > > +
> > > +     gve_get_ring_stats(priv, s);
> > > +     gve_add_base_stats(priv, s);
> > > +}
> > > +
> > >  static int gve_alloc_flow_rule_caches(struct gve_priv *priv)
> > >  {
> > >       struct gve_flow_rules_cache *flow_rules_cache = &priv->flow_rules_cache;
> > > @@ -1493,6 +1514,23 @@ static int gve_queues_stop(struct gve_priv *priv)
> > >       return gve_reset_recovery(priv, false);
> > >  }
> > >
> > > +static void gve_get_ring_err_stats(struct gve_priv *priv,
> > > +                                struct gve_ring_err_stats *err_stats)
> > > +{
> > > +     int ring;

Another Sashiko comment:

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

This will also be fixed in v2.

> > > +
> > > +     for (ring = 0; ring < priv->rx_cfg.num_queues; ring++) {
> > > +             unsigned int start;
> > > +             struct gve_rx_ring *rx = &priv->rx[ring];
> > > +
> > > +             do {
> > > +                     start = u64_stats_fetch_begin(&rx->statss);
> > > +                     err_stats->rx_alloc_fails +=
> > > +                             rx->rx_skb_alloc_fail + rx->rx_buf_alloc_fail;
> > > +             } while (u64_stats_fetch_retry(&rx->statss, start));
> >
> > Sashiko says:
> >
> > Could this loop improperly inflate the baseline metric by double counting?
> > If a concurrent update causes the sequence counter to change,
> > u64_stats_fetch_retry() forces the loop to restart. Because the addition
> > is performed in-place on err_stats->rx_alloc_fails, the same ring's
> > error values will be added again.
> > Would it be safer to use local variables inside the retry loop and update
> > the global accumulator only after the loop completes successfully, similar
> > to the pattern established in gve_get_ring_stats()?
> >
> We'll fix this by using local variables in v2.
> >
> > > +     }
> > > +}
> > > +
> > >  static int gve_close(struct net_device *dev)
> > >  {
> > >       struct gve_priv *priv = netdev_priv(dev);
> > > @@ -1502,6 +1540,10 @@ static int gve_close(struct net_device *dev)
> > >       if (err)
> > >               return err;
> > >
> > > +     /* Save ring queue and err stats before closing the interface */
> > > +     gve_get_ring_stats(priv, &priv->base_net_stats);
> > > +     gve_get_ring_err_stats(priv, &priv->base_ring_err_stats);
> >
> > Sashiko says:
> >
> > Does this create a temporary spike in reported statistics?
> > During gve_close(), the active ring stats are added to base_net_stats.
> > However, priv->rx and priv->tx are not set to NULL until the memory
> > teardown completes in gve_queues_mem_remove().
> > If ndo_get_stats64 is called concurrently during this window, it will
> > add both the active ring stats and the newly updated base stats together.
> > This causes the reported statistics to temporarily double until the
> > teardown finishes.
>
> As Sashiko mentioned, this is an existing concurrency issue between
> gve_close() and gve_get_stats() (i.e., .ndo_get_stats64() callback of
> gve driver):
>
> gve_queues_mem_remove(priv);
> This is a pre-existing issue, but can this lead to a use-after-free
> during concurrent stats retrieval?
> When gve_queues_mem_remove() frees the ring memory, priv->rx and
> priv->tx are not set to NULL until after the memory is freed. If
> gve_get_stats() executes during this window, it may iterate over and
> dereference the already freed ring memory.
>
> We will send out a separate patch to fix this.
>
> >
> > Note that you are expected to proactively comment/reply on the ML to the
> > concerns raised by sashiko reviews.
>
> Thanks for the reminder.
>
> >
> > Thanks,
> >
> > Paolo
> >
>
> Regards,
> Pin-yen

Regards,
Pin-yen