[PATCH net v2 0/9] gve: various XDP fixes

[PATCH net v2 0/9] gve: various XDP fixes

[Joshua Washington]

This patch series includes a number of XDP-related fixes, both in the
control plane and the dataplane.

A summary of the changes:
1) fix an issue where XDP packets weren't being counted against NAPI
   budget
2) fix an issue where XSK buffers that have not been processed are
   leaked when disabling XSK pools
3) fix an XSK buffer leak when an RX error descriptor comes back from
   the hardware
4) fix a NULL dereference due to incorrect registration of XSK pools on
   queues not set up in XSK zero copy mode
5) fix a deadlock introduced by attempting to acquire the netdev lock
   after it has already been acquired
6) fix a racy NULL dereference due to DMA umapping the XSK pool before
   queues are fully stopped
7) fix potential data race on priv->xsk_pool when the field is set while
   NAPIs are enabled
8) attempt to register memory model in all cases when disabling XSK
   pools
9) clean xdp frames when tearing down DQO queues

---
v2:
- introduce 3 new patches (7, 8, 9) based on Sashiko feedback
- corrected stat counting for packets according work_done behavioral
  change (patch 1)

Eddie Phillips (1):
  gve: prevent XDP frame leak and corruption during DQO TX cleanup

Joshua Washington (8):
  gve: increment work_done for XDP and error packets
  gve: fix XSK buffer leak when rings are stopped
  gve: fix XSK buffer leak on error descriptor
  gve: don't register xsk pool on pre-existing queues in RDA mode
  gve: fix napi_disable deadlock when attempting to disable XSK pools
  gve: fix NULL dereference from premature XSK pool DMA unmap
  gve: disable NAPI when registering XSK pools in QPL mode
  gve: ensure XDP mem model is registered when disabling XSK pools

 drivers/net/ethernet/google/gve/gve_main.c   | 153 +++++++----
 drivers/net/ethernet/google/gve/gve_rx_dqo.c |  31 ++-
 drivers/net/ethernet/google/gve/gve_tx_dqo.c | 256 +++++++++++++------
 3 files changed, 314 insertions(+), 126 deletions(-)

-- 
2.55.0.1082.g2b9226bbc0-goog

[PATCH net v2 1/9] gve: increment work_done for XDP and error packets

[Joshua Washington]

The GVE RX NAPI will continue polling as long as

1) there are packets to be processed, and
2) less than NAPI budget SKBs (denoted in GVE by work_done) have been
   passed up to the kernel.

However, GVE does not account for all of the packets that don't create
SKBs, namely error packets and XDP packets.

This can result in XDP programs that scarcely return XDP_PASS failing to
exit the NAPI poll as long as the NIC is DMA'ing packets, possibly
processing the entire RX ring before returning from the NAPI.

This has 3 negative implications:

1) XDP RX path can run much longer than is desirable, hogging CPU
   resources.
2) If XDP_PASS is never returned, the work_done never increases beyond
   0, which can lead to scheduling delays due to missed chances to
   reschedule the NAPI.
3) In AF_XDP zero-copy, XSK_TX occurs after the RX poll. If the RX poll
   takes a long time, it will delay TX, leading to degraded performance.

Ensure every packet is accounted for in work_done by incrementing
work_done before checking for the existence of a SKB.

Fixes: 293b49361f91 ("gve: add XDP DROP and PASS support for DQ")
Cc: stable@vger.kernel.org
Reviewed-by: Tim Hostetler <thostet@google.com>
Reviewed-by: Jordan Rhee <jordanrhee@google.com>
Signed-off-by: Joshua Washington <joshwash@google.com>
---
v2:
  - corrected stat counting for packets relative to work_done
---
 drivers/net/ethernet/google/gve/gve_rx_dqo.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/google/gve/gve_rx_dqo.c b/drivers/net/ethernet/google/gve/gve_rx_dqo.c
index 5cf242b28557..c3f4a76b0fac 100644
--- a/drivers/net/ethernet/google/gve/gve_rx_dqo.c
+++ b/drivers/net/ethernet/google/gve/gve_rx_dqo.c
@@ -932,6 +932,10 @@ static int gve_rx_dqo(struct napi_struct *napi, struct gve_rx_ring *rx,
 		if (xdp_act != XDP_PASS) {
 			gve_xdp_done_dqo(priv, rx, &gve_xdp.xdp, xprog, xdp_act,
 					 buf_state);
+			u64_stats_update_begin(&rx->statss);
+			rx->rpackets++;
+			rx->rbytes += compl_desc->packet_len;
+			u64_stats_update_end(&rx->statss);
 			return 0;
 		}
 
@@ -1090,6 +1094,7 @@ int gve_rx_poll_dqo(struct gve_notify_block *block, int budget)
 	struct gve_rx_ring *rx;
 	struct gve_priv *priv;
 	u64 xdp_redirects;
+	u32 rx_packets = 0;
 	u32 work_done = 0;
 	u64 bytes = 0;
 	u64 xdp_txs;
@@ -1150,13 +1155,14 @@ int gve_rx_poll_dqo(struct gve_notify_block *block, int budget)
 		/* Free running counter of completed descriptors */
 		rx->cnt++;
 
-		if (!rx->ctx.skb_head)
-			continue;
-
 		if (!compl_desc->end_of_packet)
 			continue;
 
 		work_done++;
+
+		if (!rx->ctx.skb_head)
+			continue;
+
 		pkt_bytes = rx->ctx.skb_head->len;
 		/* The ethernet header (first ETH_HLEN bytes) is snipped off
 		 * by eth_type_trans.
@@ -1164,6 +1170,9 @@ int gve_rx_poll_dqo(struct gve_notify_block *block, int budget)
 		if (skb_headlen(rx->ctx.skb_head))
 			pkt_bytes += ETH_HLEN;
 
+		rx_packets++;
+		bytes += pkt_bytes;
+
 		/* gve_rx_complete_skb() will consume skb if successful */
 		if (gve_rx_complete_skb(rx, napi, compl_desc, feat) != 0) {
 			gve_rx_free_skb(napi, rx);
@@ -1173,7 +1182,6 @@ int gve_rx_poll_dqo(struct gve_notify_block *block, int budget)
 			continue;
 		}
 
-		bytes += pkt_bytes;
 		rx->ctx.skb_head = NULL;
 		rx->ctx.skb_tail = NULL;
 	}
@@ -1187,7 +1195,7 @@ int gve_rx_poll_dqo(struct gve_notify_block *block, int budget)
 	gve_rx_post_buffers_dqo(rx);
 
 	u64_stats_update_begin(&rx->statss);
-	rx->rpackets += work_done;
+	rx->rpackets += rx_packets;
 	rx->rbytes += bytes;
 	u64_stats_update_end(&rx->statss);
 
-- 
2.55.0.1082.g2b9226bbc0-goog

[PATCH net v2 2/9] gve: fix XSK buffer leak when rings are stopped

[Joshua Washington]

GVE does not free XSK buffers when resetting ring state as a part of
stopping queues. This causes all XSK buffers which are posted to the
NIC to be leaked.

Free XSK buffers attached to an allocated buf_state when stopping rings.

Fixes: c1fffc5d66a7 ("gve: implement DQO RX datapath and control path for AF_XDP zero-copy")
Cc: stable@vger.kernel.org
Reviewed-by: Tim Hostetler <thostet@google.com>
Reviewed-by: Jordan Rhee <jordanrhee@google.com>
Signed-off-by: Joshua Washington <joshwash@google.com>
---
 drivers/net/ethernet/google/gve/gve_rx_dqo.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/ethernet/google/gve/gve_rx_dqo.c b/drivers/net/ethernet/google/gve/gve_rx_dqo.c
index c3f4a76b0fac..3a88b7e98b16 100644
--- a/drivers/net/ethernet/google/gve/gve_rx_dqo.c
+++ b/drivers/net/ethernet/google/gve/gve_rx_dqo.c
@@ -113,6 +113,12 @@ static void gve_rx_reset_ring_dqo(struct gve_priv *priv, int idx)
 				gve_free_to_page_pool(rx, bs, false);
 			else
 				gve_free_qpl_page_dqo(bs);
+
+			if (gve_buf_state_is_allocated(rx, bs) &&
+			    bs->xsk_buff) {
+				xsk_buff_free(bs->xsk_buff);
+				bs->xsk_buff = NULL;
+			}
 		}
 	}
 
-- 
2.55.0.1082.g2b9226bbc0-goog

[PATCH net v2 3/9] gve: fix XSK buffer leak on error descriptor

[Joshua Washington]

When the error bit is set in the RX completion descriptor, the buf_state
and its attached buffer should be freed. In the case of AF_XDP ZC, the
XSK buffer was not freed, leading to a leak.

Fixes: c1fffc5d66a7 ("gve: implement DQO RX datapath and control path for AF_XDP zero-copy")
Cc: stable@vger.kernel.org
Reviewed-by: Jordan Rhee <jordanrhee@google.com>
Reviewed-by: Tim Hostetler <thostet@google.com>
Signed-off-by: Joshua Washington <joshwash@google.com>
---
 drivers/net/ethernet/google/gve/gve_rx_dqo.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/google/gve/gve_rx_dqo.c b/drivers/net/ethernet/google/gve/gve_rx_dqo.c
index 3a88b7e98b16..c1e97e11ff35 100644
--- a/drivers/net/ethernet/google/gve/gve_rx_dqo.c
+++ b/drivers/net/ethernet/google/gve/gve_rx_dqo.c
@@ -842,7 +842,12 @@ static int gve_rx_dqo(struct napi_struct *napi, struct gve_rx_ring *rx,
 	}
 
 	if (unlikely(compl_desc->rx_error)) {
-		gve_free_buffer(rx, buf_state);
+		if (buf_state->xsk_buff) {
+			xsk_buff_free(buf_state->xsk_buff);
+			gve_free_buf_state(rx, buf_state);
+		} else {
+			gve_free_buffer(rx, buf_state);
+		}
 		return -EINVAL;
 	}
 
-- 
2.55.0.1082.g2b9226bbc0-goog

[PATCH net v2 4/9] gve: don't register xsk pool on pre-existing queues in RDA mode

[Joshua Washington]

When XSK pools are enabled after an XDP program has already been loaded,
XSK pools are registered on pre-existing queues before queues are
re-created with the XSK pool fully registered in DQ RDA mode.

This can lead to a race condition between the RX NAPI and the control
plane thread wherein a pre-existing queue sees the live XSK pool and
attempts to use recycled buffers not backed by XSK buffs for AF_XDP ZC
traffic. This causes the following kernel panic to occur when attempting
to DMA map a NULL XSK buffer:

BUG: kernel NULL pointer dereference, address: 0000000000000050
...
RIP: 0010:gve_rx_post_buffers_dqo+0x99/0x190 [gve]
...
Call Trace:
 <TASK>
 gve_rx_poll_dqo+0x4d9/0xf10 [gve]
 gve_napi_poll_dqo+0x76/0x170 [gve]
 __napi_poll+0x28/0x160
 net_rx_action+0x2a0/0x350
 handle_softirqs+0xd4/0x280
 ? sort_range+0x20/0x20
 run_ksoftirqd+0x2d/0x40
 smpboot_thread_fn+0xd5/0x1d0
 kthread+0xd7/0x100
 ? kthread_complete_and_exit+0x20/0x20
 ret_from_fork+0x1f/0x30
 </TASK>

The XSK pool should only be registered with current queues if XSK
buffers are allocated on-the-fly, as is the case in QPL mode.

Fixes: c1fffc5d66a7 ("gve: implement DQO RX datapath and control path for AF_XDP zero-copy")
Cc: stable@vger.kernel.org
Reviewed-by: Jordan Rhee <jordanrhee@google.com>
Reviewed-by: Tim Hostetler <thostet@google.com>
Signed-off-by: Joshua Washington <joshwash@google.com>
---
 drivers/net/ethernet/google/gve/gve_main.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
index 9cc343a16271..b9bcdc7619b2 100644
--- a/drivers/net/ethernet/google/gve/gve_main.c
+++ b/drivers/net/ethernet/google/gve/gve_main.c
@@ -1654,20 +1654,18 @@ static int gve_xsk_pool_enable(struct net_device *dev,
 	if (!priv->xdp_prog || !netif_running(dev))
 		return 0;
 
-	err = gve_reg_xsk_pool(priv, dev, pool, qid);
-	if (err)
-		goto err_xsk_pool_dma_mapped;
-
-	/* Stop and start RDA queues to repost buffers. */
-	if (!gve_is_qpl(priv)) {
+	if (gve_is_qpl(priv)) {
+		err = gve_reg_xsk_pool(priv, dev, pool, qid);
+		if (err)
+			goto err_xsk_pool_dma_mapped;
+	} else {
+		/* Stop and start RDA queues to repost buffers. */
 		err = gve_configure_rings_xdp(priv, priv->rx_cfg.num_queues);
 		if (err)
-			goto err_xsk_pool_registered;
+			goto err_xsk_pool_dma_mapped;
 	}
 	return 0;
 
-err_xsk_pool_registered:
-	gve_unreg_xsk_pool(priv, qid);
 err_xsk_pool_dma_mapped:
 	clear_bit(qid, priv->xsk_pools);
 	xsk_pool_dma_unmap(pool,
-- 
2.55.0.1082.g2b9226bbc0-goog

[PATCH net v2 5/9] gve: fix napi_disable deadlock when attempting to disable XSK pools

[Joshua Washington]

When disabling XSK pools, GVE calls the unlocked versions of
napi_disable and napi_enable. However, the netdev lock has already been
acquired before ndo_bpf is called because GVE supports queue management
ops. Calling the unlocked versions of napi_disable/enable results in a
deadlock when attempting to disable XSK pools, as the thread attempts to
re-acquire a lock it already holds.

Update the NAPI calls to use the locked versions.

Fixes: 606048cbd834 ("net: designate XSK pool pointers in queues as "ops protected"")
Cc: stable@vger.kernel.org
Reviewed-by: Harshitha Ramamurthy <hramamurthy@google.com>
Reviewed-by: Jordan Rhee <jordanrhee@google.com>
Signed-off-by: Joshua Washington <joshwash@google.com>
---
 drivers/net/ethernet/google/gve/gve_main.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
index b9bcdc7619b2..3712ff364cbd 100644
--- a/drivers/net/ethernet/google/gve/gve_main.c
+++ b/drivers/net/ethernet/google/gve/gve_main.c
@@ -1706,17 +1706,17 @@ static int gve_xsk_pool_disable(struct net_device *dev,
 	}
 
 	napi_rx = &priv->ntfy_blocks[priv->rx[qid].ntfy_id].napi;
-	napi_disable(napi_rx); /* make sure current rx poll is done */
+	napi_disable_locked(napi_rx); /* make sure current rx poll is done */
 
 	tx_qid = gve_xdp_tx_queue_id(priv, qid);
 	napi_tx = &priv->ntfy_blocks[priv->tx[tx_qid].ntfy_id].napi;
-	napi_disable(napi_tx); /* make sure current tx poll is done */
+	napi_disable_locked(napi_tx); /* make sure current tx poll is done */
 
 	gve_unreg_xsk_pool(priv, qid);
 	smp_mb(); /* Make sure it is visible to the workers on datapath */
 
-	napi_enable(napi_rx);
-	napi_enable(napi_tx);
+	napi_enable_locked(napi_rx);
+	napi_enable_locked(napi_tx);
 	if (gve_is_gqi(priv)) {
 		if (gve_rx_work_pending(&priv->rx[qid]))
 			napi_schedule(napi_rx);
-- 
2.55.0.1082.g2b9226bbc0-goog

[PATCH net v2 6/9] gve: fix NULL dereference from premature XSK pool DMA unmap

[Joshua Washington]

To ensure that XSK pools are DMA unmapped in all scenarios, GVE performs
the unmapping before validating if the interface is up and early
returning.

However, if rings are up, this introduces a race between the RX NAPI and
the control plane. As part of DMA unmapping the XSK pool, the kernel
sets pool->dev to NULL. Because xsk_buff_dma_sync_for_cpu() relies on
pool->dev, this results in a kernel panic:

BUG: kernel NULL pointer dereference, address: 000000000000030c
...
RIP: 0010:gve_rx_poll_dqo+0x2e2/0x13b0 [gve]
...
Call Trace:
 <IRQ>
 gve_napi_poll_dqo+0x88/0x170 [gve]
 __napi_poll+0x30/0x210
 net_rx_action+0x210/0x410
 ? dst_destroy_rcu+0x12/0x20
 handle_softirqs+0xe4/0x310
 __irq_exit_rcu+0x10e/0x130
 irq_exit_rcu+0xe/0x20
 common_interrupt+0xb6/0xe0
 </IRQ>

Leave the XSK pool DMA mapped until after rings are guaranteed to no
longer rely on the pool.

Fixes: d57ae093c887 ("gve: deduplicate xdp info and xsk pool registration logic")
Cc: stable@vger.kernel.org
Reviewed-by: Jordan Rhee <jordanrhee@google.com>
Signed-off-by: Joshua Washington <joshwash@google.com>
---
 drivers/net/ethernet/google/gve/gve_main.c | 25 ++++++++++++----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
index 3712ff364cbd..49ae2b8c6a27 100644
--- a/drivers/net/ethernet/google/gve/gve_main.c
+++ b/drivers/net/ethernet/google/gve/gve_main.c
@@ -1681,22 +1681,18 @@ static int gve_xsk_pool_disable(struct net_device *dev,
 	struct napi_struct *napi_rx;
 	struct napi_struct *napi_tx;
 	struct xsk_buff_pool *pool;
+	int err = 0;
 	int tx_qid;
-	int err;
 
-	if (qid >= priv->rx_cfg.num_queues)
-		return -EINVAL;
+	if (qid >= priv->rx_cfg.num_queues) {
+		err = -EINVAL;
+		goto unmap_and_return;
+	}
 
 	clear_bit(qid, priv->xsk_pools);
 
-	pool = xsk_get_pool_from_qid(dev, qid);
-	if (pool)
-		xsk_pool_dma_unmap(pool,
-				   DMA_ATTR_SKIP_CPU_SYNC |
-				   DMA_ATTR_WEAK_ORDERING);
-
 	if (!netif_running(dev) || !priv->tx_cfg.num_xdp_queues)
-		return 0;
+		goto unmap_and_return;
 
 	/* Stop and start RDA queues to repost buffers. */
 	if (!gve_is_qpl(priv) && priv->xdp_prog) {
@@ -1725,7 +1721,14 @@ static int gve_xsk_pool_disable(struct net_device *dev,
 			napi_schedule(napi_tx);
 	}
 
-	return 0;
+unmap_and_return:
+	pool = xsk_get_pool_from_qid(dev, qid);
+	if (pool)
+		xsk_pool_dma_unmap(pool,
+				   DMA_ATTR_SKIP_CPU_SYNC |
+				   DMA_ATTR_WEAK_ORDERING);
+
+	return err;
 }
 
 static int gve_xsk_wakeup(struct net_device *dev, u32 queue_id, u32 flags)
-- 
2.55.0.1082.g2b9226bbc0-goog

[PATCH net v2 7/9] gve: disable NAPI when registering XSK pools in QPL mode

[Joshua Washington]

As a result of QPL modes not posting XSK buffers directly to the NIC,
they do not need to recreate queues or re-post DMA buffers.

However, traffic should still be quiesced because both the driver and
the XDP redirect stack must have the same knowledge about whether a
given packet is being processed with AF_XDP zero-copy enabled or not.
GVE in QPL mode does not current respect this, which could lead to a
race condition between packet processing and the XSK_BUFF_POOL memory
model registration.

Quiesce traffic by disabling the NAPI while the XSK_BUFF_POOL memory
model is being registered with the kernel.

Fixes: fd8e40321a12 ("gve: Add AF_XDP zero-copy support for GQI-QPL format")
Cc: stable@vger.kernel.org
Reviewed-by: Harshitha Ramamurthy <hramamurthy@google.com>
Signed-off-by: Joshua Washington <joshwash@google.com>
---
v2:
- newly introduced
---
 drivers/net/ethernet/google/gve/gve_main.c | 41 ++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
index 49ae2b8c6a27..f2bd4011de23 100644
--- a/drivers/net/ethernet/google/gve/gve_main.c
+++ b/drivers/net/ethernet/google/gve/gve_main.c
@@ -1626,6 +1626,40 @@ static int gve_xdp_xmit(struct net_device *dev, int n,
 	return -EOPNOTSUPP;
 }
 
+static void gve_disable_xsk_napis(struct gve_priv *priv, u16 qid)
+{
+	struct napi_struct *napi_rx, *napi_tx;
+	u16 tx_qid;
+
+	napi_rx = &priv->ntfy_blocks[priv->rx[qid].ntfy_id].napi;
+	napi_disable_locked(napi_rx);
+
+	tx_qid = gve_xdp_tx_queue_id(priv, qid);
+	napi_tx = &priv->ntfy_blocks[priv->tx[tx_qid].ntfy_id].napi;
+	napi_disable_locked(napi_tx);
+}
+
+static void gve_enable_xsk_napis(struct gve_priv *priv, u16 qid)
+{
+	struct napi_struct *napi_rx, *napi_tx;
+	u16 tx_qid;
+
+	napi_rx = &priv->ntfy_blocks[priv->rx[qid].ntfy_id].napi;
+	napi_enable_locked(napi_rx);
+
+	tx_qid = gve_xdp_tx_queue_id(priv, qid);
+	napi_tx = &priv->ntfy_blocks[priv->tx[tx_qid].ntfy_id].napi;
+	napi_enable_locked(napi_tx);
+
+	if (gve_is_gqi(priv)) {
+		if (gve_rx_work_pending(&priv->rx[qid]))
+			napi_schedule(napi_rx);
+
+		if (gve_tx_clean_pending(priv, &priv->tx[tx_qid]))
+			napi_schedule(napi_tx);
+	}
+}
+
 static int gve_xsk_pool_enable(struct net_device *dev,
 			       struct xsk_buff_pool *pool,
 			       u16 qid)
@@ -1655,7 +1689,14 @@ static int gve_xsk_pool_enable(struct net_device *dev,
 		return 0;
 
 	if (gve_is_qpl(priv)) {
+		gve_disable_xsk_napis(priv, qid);
+
 		err = gve_reg_xsk_pool(priv, dev, pool, qid);
+		/* Make sure it is visible to the workers on datapath */
+		smp_mb();
+
+		gve_enable_xsk_napis(priv, qid);
+
 		if (err)
 			goto err_xsk_pool_dma_mapped;
 	} else {
-- 
2.55.0.1082.g2b9226bbc0-goog

[PATCH net v2 8/9] gve: ensure XDP mem model is registered when disabling XSK pools

[Joshua Washington]

When disabling XSK pools, QPL and RDA modes have different behaviors,
but they are both incorrect in that the function returns just after
unregistering the memory model.

RDA mode performs an internally consistent re-configuration of rings,
making the extra logic, including the XSK pool unregistration,
unnecessary. However, it is possible for the reconfiguration to fail due
to memory allocation. Instead of freeing and re-allocating ring memory,
stop the rings and reinitialize the ring state. That way, failure to
stop the rings would result in a safer device reset, making it safe to
unregister the pool regardless of the error condition when stopping
queues. This change involves a bit of refactoring in the TX
initialization path, introducing new methods to reset ring state when
stopping the rings.

QPL mode, which does not need to reconfigure rings due to not posting
XSK umem to the hardware ring, simply misses registering the RXQ XDP
info with the MEM_TYPE_PAGE_SHARED memory model.

Make a best-effort attempt to register memory model in both cases.
Because memory model registration can fail and xp_release_deferred
cannot, the XSK pool must be unregistered and DMA-unmapped regardless of
whether memory model registration succeeds. In both cases, there should
be a guarantee against the device DMA'ing into freed memory, however.

Fixes: 077f7153fd25 ("gve: merge xdp and xsk registration")
Cc: stable@vger.kernel.org
Reviewed-by: Harshitha Ramamurthy <hramamurthy@google.com>
Signed-off-by: Joshua Washington <joshwash@google.com>
---
v2:
- Newly introduced
---
 drivers/net/ethernet/google/gve/gve_main.c   |  77 +++++++++-----
 drivers/net/ethernet/google/gve/gve_tx_dqo.c | 101 ++++++++++++++-----
 2 files changed, 126 insertions(+), 52 deletions(-)

diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
index f2bd4011de23..787d311ff99b 100644
--- a/drivers/net/ethernet/google/gve/gve_main.c
+++ b/drivers/net/ethernet/google/gve/gve_main.c
@@ -1715,15 +1715,56 @@ static int gve_xsk_pool_enable(struct net_device *dev,
 	return err;
 }
 
+static int gve_unreg_xsk_pool_live(struct gve_priv *priv,
+				   struct net_device *dev, u16 qid)
+{
+	struct gve_rx_ring *rx;
+	int err;
+
+	rx = &priv->rx[qid];
+	gve_disable_xsk_napis(priv, qid);
+
+	gve_unreg_xsk_pool(priv, qid);
+	if (gve_is_qpl(priv))
+		err = xdp_rxq_info_reg_mem_model(&rx->xdp_rxq,
+						 MEM_TYPE_PAGE_SHARED,
+						 NULL);
+	else
+		err = xdp_rxq_info_reg_mem_model(&rx->xdp_rxq,
+						 MEM_TYPE_PAGE_POOL,
+						 rx->dqo.page_pool);
+	if (err)
+		netdev_warn(dev,
+			    "Failed to register memory model after unregistering XSK pool");
+
+	smp_mb(); /* Make sure it is visible to the workers on datapath */
+
+	gve_enable_xsk_napis(priv, qid);
+
+	return err;
+}
+
+static int gve_restart_rings(struct gve_priv *priv)
+{
+	struct gve_tx_alloc_rings_cfg tx_alloc_cfg = {0};
+	struct gve_rx_alloc_rings_cfg rx_alloc_cfg = {0};
+	int err;
+
+	gve_get_curr_alloc_cfgs(priv, &tx_alloc_cfg, &rx_alloc_cfg);
+	err = gve_queues_stop(priv);
+	if (err)
+		return err;
+
+	err = gve_queues_start(priv, &tx_alloc_cfg, &rx_alloc_cfg);
+	return err;
+}
+
 static int gve_xsk_pool_disable(struct net_device *dev,
 				u16 qid)
 {
 	struct gve_priv *priv = netdev_priv(dev);
-	struct napi_struct *napi_rx;
-	struct napi_struct *napi_tx;
 	struct xsk_buff_pool *pool;
 	int err = 0;
-	int tx_qid;
 
 	if (qid >= priv->rx_cfg.num_queues) {
 		err = -EINVAL;
@@ -1735,31 +1776,11 @@ static int gve_xsk_pool_disable(struct net_device *dev,
 	if (!netif_running(dev) || !priv->tx_cfg.num_xdp_queues)
 		goto unmap_and_return;
 
-	/* Stop and start RDA queues to repost buffers. */
-	if (!gve_is_qpl(priv) && priv->xdp_prog) {
-		err = gve_configure_rings_xdp(priv, priv->rx_cfg.num_queues);
-		if (err)
-			return err;
-	}
-
-	napi_rx = &priv->ntfy_blocks[priv->rx[qid].ntfy_id].napi;
-	napi_disable_locked(napi_rx); /* make sure current rx poll is done */
-
-	tx_qid = gve_xdp_tx_queue_id(priv, qid);
-	napi_tx = &priv->ntfy_blocks[priv->tx[tx_qid].ntfy_id].napi;
-	napi_disable_locked(napi_tx); /* make sure current tx poll is done */
-
-	gve_unreg_xsk_pool(priv, qid);
-	smp_mb(); /* Make sure it is visible to the workers on datapath */
-
-	napi_enable_locked(napi_rx);
-	napi_enable_locked(napi_tx);
-	if (gve_is_gqi(priv)) {
-		if (gve_rx_work_pending(&priv->rx[qid]))
-			napi_schedule(napi_rx);
-
-		if (gve_tx_clean_pending(priv, &priv->tx[tx_qid]))
-			napi_schedule(napi_tx);
+	if (gve_is_qpl(priv)) {
+		err = gve_unreg_xsk_pool_live(priv, dev, qid);
+	} else {
+		/* Stop and start RDA queues to repost buffers. */
+		err = gve_restart_rings(priv);
 	}
 
 unmap_and_return:
diff --git a/drivers/net/ethernet/google/gve/gve_tx_dqo.c b/drivers/net/ethernet/google/gve/gve_tx_dqo.c
index 80ab0a449ff5..78f946ae7264 100644
--- a/drivers/net/ethernet/google/gve/gve_tx_dqo.c
+++ b/drivers/net/ethernet/google/gve/gve_tx_dqo.c
@@ -208,6 +208,78 @@ static void gve_tx_clean_pending_packets(struct gve_tx_ring *tx)
 	}
 }
 
+static void gve_tx_init_ring_state_dqo(struct gve_tx_ring *tx)
+{
+	int i;
+
+	atomic_set_release(&tx->dqo_compl.hw_tx_head, 0);
+
+	/* Set up linked list of pending packets */
+	for (i = 0; i < tx->dqo.num_pending_packets - 1; i++)
+		tx->dqo.pending_packets[i].next = i + 1;
+
+	tx->dqo.pending_packets[tx->dqo.num_pending_packets - 1].next = -1;
+	atomic_set_release(&tx->dqo_compl.free_pending_packets, -1);
+
+	tx->dqo_compl.miss_completions.head = -1;
+	tx->dqo_compl.miss_completions.tail = -1;
+	tx->dqo_compl.timed_out_completions.head = -1;
+	tx->dqo_compl.timed_out_completions.tail = -1;
+
+	/* Generate free TX buf list */
+	if (tx->dqo.tx_qpl_buf_next) {
+		for (i = 0; i < tx->dqo.num_tx_qpl_bufs - 1; i++)
+			tx->dqo.tx_qpl_buf_next[i] = i + 1;
+		tx->dqo.tx_qpl_buf_next[tx->dqo.num_tx_qpl_bufs - 1] = -1;
+
+		atomic_set_release(&tx->dqo_compl.free_tx_qpl_buf_head, -1);
+		atomic_set_release(&tx->dqo_compl.free_tx_qpl_buf_cnt, 0);
+	}
+}
+
+static void gve_tx_reset_ring_dqo(struct gve_tx_ring *tx)
+{
+	size_t size;
+
+	/* Reset dqo_tx fields. */
+	tx->dqo_tx.head = 0;
+	tx->dqo_tx.tail = 0;
+	tx->dqo_tx.last_re_idx = 0;
+	tx->dqo_tx.posted_packet_desc_cnt = 0;
+	tx->dqo_tx.completed_packet_desc_cnt = 0;
+	tx->dqo_tx.free_pending_packets = 0;
+
+	/* Reset dqo_compl fields. */
+	tx->dqo_compl.head = 0;
+	tx->dqo_compl.cur_gen_bit = 0;
+	tx->dqo_compl.xsk_reorder_queue_head = 0;
+	tx->dqo_compl.xsk_reorder_queue_tail = 0;
+
+	if (tx->dqo.xsk_reorder_queue) {
+		size = (tx->dqo.complq_mask + 1) *
+			sizeof(*tx->dqo.xsk_reorder_queue);
+		memset(tx->dqo.xsk_reorder_queue, 0, size);
+		atomic_set(&tx->dqo_tx.xsk_reorder_queue_tail, 0);
+	}
+
+	size = sizeof(tx->dqo.tx_ring[0]) * (tx->mask + 1);
+	memset(tx->dqo.tx_ring, 0, size);
+
+	size = sizeof(tx->dqo.compl_ring[0]) * (tx->dqo.complq_mask + 1);
+	memset(tx->dqo.compl_ring, 0, size);
+
+	memset(tx->q_resources, 0, sizeof(*tx->q_resources));
+
+	if (tx->dqo.tx_qpl_buf_next) {
+		tx->dqo_tx.free_tx_qpl_buf_head = 0;
+		size = sizeof(tx->dqo.tx_qpl_buf_next[0]) *
+			tx->dqo.num_tx_qpl_bufs;
+		memset(tx->dqo.tx_qpl_buf_next, 0, size);
+	}
+
+	gve_tx_init_ring_state_dqo(tx);
+}
+
 void gve_tx_stop_ring_dqo(struct gve_priv *priv, int idx)
 {
 	int ntfy_idx = gve_tx_idx_to_ntfy(priv, idx);
@@ -222,6 +294,7 @@ void gve_tx_stop_ring_dqo(struct gve_priv *priv, int idx)
 		netdev_tx_reset_queue(tx->netdev_txq);
 	gve_tx_clean_pending_packets(tx);
 	gve_tx_remove_from_block(priv, idx);
+	gve_tx_reset_ring_dqo(tx);
 }
 
 static void gve_tx_free_ring_dqo(struct gve_priv *priv, struct gve_tx_ring *tx,
@@ -270,11 +343,10 @@ static void gve_tx_free_ring_dqo(struct gve_priv *priv, struct gve_tx_ring *tx,
 	netif_dbg(priv, drv, priv->dev, "freed tx queue %d\n", idx);
 }
 
-static int gve_tx_qpl_buf_init(struct gve_tx_ring *tx)
+static int gve_tx_qpl_buf_list_alloc(struct gve_tx_ring *tx)
 {
 	int num_tx_qpl_bufs = GVE_TX_BUFS_PER_PAGE_DQO *
 		tx->dqo.qpl->num_entries;
-	int i;
 
 	tx->dqo.tx_qpl_buf_next = kvzalloc_objs(tx->dqo.tx_qpl_buf_next[0],
 						num_tx_qpl_bufs);
@@ -282,13 +354,6 @@ static int gve_tx_qpl_buf_init(struct gve_tx_ring *tx)
 		return -ENOMEM;
 
 	tx->dqo.num_tx_qpl_bufs = num_tx_qpl_bufs;
-
-	/* Generate free TX buf list */
-	for (i = 0; i < num_tx_qpl_bufs - 1; i++)
-		tx->dqo.tx_qpl_buf_next[i] = i + 1;
-	tx->dqo.tx_qpl_buf_next[num_tx_qpl_bufs - 1] = -1;
-
-	atomic_set_release(&tx->dqo_compl.free_tx_qpl_buf_head, -1);
 	return 0;
 }
 
@@ -304,6 +369,7 @@ void gve_tx_start_ring_dqo(struct gve_priv *priv, int idx)
 	gve_add_napi(priv, ntfy_idx, gve_napi_poll_dqo);
 }
 
+
 static int gve_tx_alloc_ring_dqo(struct gve_priv *priv,
 				 struct gve_tx_alloc_rings_cfg *cfg,
 				 struct gve_tx_ring *tx,
@@ -313,13 +379,11 @@ static int gve_tx_alloc_ring_dqo(struct gve_priv *priv,
 	int num_pending_packets;
 	size_t bytes;
 	u32 qpl_id;
-	int i;
 
 	memset(tx, 0, sizeof(*tx));
 	tx->q_num = idx;
 	tx->dev = hdev;
 	spin_lock_init(&tx->dqo_tx.xdp_lock);
-	atomic_set_release(&tx->dqo_compl.hw_tx_head, 0);
 
 	/* Queue sizes must be a power of 2 */
 	tx->mask = cfg->ring_size - 1;
@@ -350,13 +414,6 @@ static int gve_tx_alloc_ring_dqo(struct gve_priv *priv,
 	if (!tx->dqo.pending_packets)
 		goto err;
 
-	/* Set up linked list of pending packets */
-	for (i = 0; i < tx->dqo.num_pending_packets - 1; i++)
-		tx->dqo.pending_packets[i].next = i + 1;
-
-	tx->dqo.pending_packets[tx->dqo.num_pending_packets - 1].next = -1;
-	atomic_set_release(&tx->dqo_compl.free_pending_packets, -1);
-
 	/* Only alloc xsk pool for XDP queues */
 	if (idx >= cfg->qcfg->num_queues && cfg->num_xdp_rings) {
 		tx->dqo.xsk_reorder_queue =
@@ -367,11 +424,6 @@ static int gve_tx_alloc_ring_dqo(struct gve_priv *priv,
 			goto err;
 	}
 
-	tx->dqo_compl.miss_completions.head = -1;
-	tx->dqo_compl.miss_completions.tail = -1;
-	tx->dqo_compl.timed_out_completions.head = -1;
-	tx->dqo_compl.timed_out_completions.tail = -1;
-
 	bytes = sizeof(tx->dqo.tx_ring[0]) * (tx->mask + 1);
 	tx->dqo.tx_ring = dma_alloc_coherent(hdev, bytes, &tx->bus, GFP_KERNEL);
 	if (!tx->dqo.tx_ring)
@@ -397,10 +449,11 @@ static int gve_tx_alloc_ring_dqo(struct gve_priv *priv,
 		if (!tx->dqo.qpl)
 			goto err;
 
-		if (gve_tx_qpl_buf_init(tx))
+		if (gve_tx_qpl_buf_list_alloc(tx))
 			goto err;
 	}
 
+	gve_tx_init_ring_state_dqo(tx);
 	return 0;
 
 err:
-- 
2.55.0.1082.g2b9226bbc0-goog

[PATCH net v2 9/9] gve: prevent XDP frame leak and corruption during DQO TX cleanup

[Joshua Washington]

From: Eddie Phillips <eddiephillips@google.com>

When tearing down a DQO TX ring or processing miss completions,
pending packets were assumed to be SKBs. If an XDP frame was pending
during cleanup or timeout, cur_state->skb was accessed on an xdpf
union pointer or the XDP frame was leaked without calling
xdp_return_frame().

Refactor gve_tx_clean_pending_packets(), gve_handle_miss_completion(),
and remove_miss_completions() to switch on pending_packet->type.

Fixes: d8a8ca14c937 ("gve: add XDP_TX and XDP_REDIRECT support for DQ RDA")
Cc: stable@vger.kernel.org
Signed-off-by: Eddie Phillips <eddiephillips@google.com>
Signed-off-by: Joshua Washington <joshwash@google.com>
---
v2:
- Newly introduced.
---
 drivers/net/ethernet/google/gve/gve_tx_dqo.c | 155 +++++++++++++------
 1 file changed, 104 insertions(+), 51 deletions(-)

diff --git a/drivers/net/ethernet/google/gve/gve_tx_dqo.c b/drivers/net/ethernet/google/gve/gve_tx_dqo.c
index 78f946ae7264..e8481b993beb 100644
--- a/drivers/net/ethernet/google/gve/gve_tx_dqo.c
+++ b/drivers/net/ethernet/google/gve/gve_tx_dqo.c
@@ -186,6 +186,49 @@ static void gve_unmap_packet(struct device *dev,
 	pkt->num_bufs = 0;
 }
 
+static struct gve_tx_pending_packet_dqo *
+gve_xsk_reorder_queue_head(struct gve_tx_ring *tx)
+{
+	u32 head = tx->dqo_compl.xsk_reorder_queue_head;
+
+	if (head == tx->dqo_compl.xsk_reorder_queue_tail) {
+		tx->dqo_compl.xsk_reorder_queue_tail =
+			atomic_read_acquire(&tx->dqo_tx.xsk_reorder_queue_tail);
+
+		if (head == tx->dqo_compl.xsk_reorder_queue_tail)
+			return NULL;
+	}
+
+	return &tx->dqo.pending_packets[tx->dqo.xsk_reorder_queue[head]];
+}
+
+static void gve_xsk_reorder_queue_pop_dqo(struct gve_tx_ring *tx)
+{
+	tx->dqo_compl.xsk_reorder_queue_head++;
+	tx->dqo_compl.xsk_reorder_queue_head &= tx->dqo.complq_mask;
+}
+
+static void gve_tx_process_xsk_completions(struct gve_tx_ring *tx)
+{
+	u32 num_xsks = 0;
+
+	while (true) {
+		struct gve_tx_pending_packet_dqo *pending_packet =
+			gve_xsk_reorder_queue_head(tx);
+
+		if (!pending_packet ||
+		    pending_packet->state != GVE_PACKET_STATE_XSK_COMPLETE)
+			break;
+
+		num_xsks++;
+		gve_xsk_reorder_queue_pop_dqo(tx);
+		gve_free_pending_packet(tx, pending_packet);
+	}
+
+	if (num_xsks)
+		xsk_tx_completed(tx->xsk_pool, num_xsks);
+}
+
 /* gve_tx_free_desc - Cleans up all pending tx requests and buffers.
  */
 static void gve_tx_clean_pending_packets(struct gve_tx_ring *tx)
@@ -201,11 +244,30 @@ static void gve_tx_clean_pending_packets(struct gve_tx_ring *tx)
 		else
 			gve_unmap_packet(tx->dev, cur_state);
 
-		if (cur_state->skb) {
-			dev_consume_skb_any(cur_state->skb);
-			cur_state->skb = NULL;
+		switch (cur_state->type) {
+		case GVE_TX_PENDING_PACKET_DQO_SKB:
+			if (cur_state->skb) {
+				dev_consume_skb_any(cur_state->skb);
+				cur_state->skb = NULL;
+			}
+			break;
+		case GVE_TX_PENDING_PACKET_DQO_XDP_FRAME:
+			if (cur_state->xdpf) {
+				xdp_return_frame(cur_state->xdpf);
+				cur_state->xdpf = NULL;
+			}
+			break;
+		case GVE_TX_PENDING_PACKET_DQO_XSK:
+			cur_state->state = GVE_PACKET_STATE_XSK_COMPLETE;
+			break;
+		default:
+			WARN_ON_ONCE(1);
+			break;
 		}
 	}
+
+	if (tx->xsk_pool)
+		gve_tx_process_xsk_completions(tx);
 }
 
 static void gve_tx_init_ring_state_dqo(struct gve_tx_ring *tx)
@@ -1089,28 +1151,6 @@ static void gve_xsk_reorder_queue_push_dqo(struct gve_tx_ring *tx,
 	atomic_set_release(&tx->dqo_tx.xsk_reorder_queue_tail, tail);
 }
 
-static struct gve_tx_pending_packet_dqo *
-gve_xsk_reorder_queue_head(struct gve_tx_ring *tx)
-{
-	u32 head = tx->dqo_compl.xsk_reorder_queue_head;
-
-	if (head == tx->dqo_compl.xsk_reorder_queue_tail) {
-		tx->dqo_compl.xsk_reorder_queue_tail =
-			atomic_read_acquire(&tx->dqo_tx.xsk_reorder_queue_tail);
-
-		if (head == tx->dqo_compl.xsk_reorder_queue_tail)
-			return NULL;
-	}
-
-	return &tx->dqo.pending_packets[tx->dqo.xsk_reorder_queue[head]];
-}
-
-static void gve_xsk_reorder_queue_pop_dqo(struct gve_tx_ring *tx)
-{
-	tx->dqo_compl.xsk_reorder_queue_head++;
-	tx->dqo_compl.xsk_reorder_queue_head &= tx->dqo.complq_mask;
-}
-
 /* Transmit a given skb and ring the doorbell. */
 netdev_tx_t gve_tx_dqo(struct sk_buff *skb, struct net_device *dev)
 {
@@ -1347,8 +1387,25 @@ static void gve_handle_miss_completion(struct gve_priv *priv,
 			secs_to_jiffies(GVE_REINJECT_COMPL_TIMEOUT);
 	add_to_list(tx, &tx->dqo_compl.miss_completions, pending_packet);
 
-	*bytes += pending_packet->skb->len;
-	(*pkts)++;
+	switch (pending_packet->type) {
+	case GVE_TX_PENDING_PACKET_DQO_SKB:
+		if (pending_packet->skb) {
+			*bytes += pending_packet->skb->len;
+			(*pkts)++;
+		}
+		break;
+	case GVE_TX_PENDING_PACKET_DQO_XDP_FRAME:
+		if (pending_packet->xdpf) {
+			*bytes += pending_packet->xdpf->len;
+			(*pkts)++;
+		}
+		break;
+	case GVE_TX_PENDING_PACKET_DQO_XSK:
+		break;
+	default:
+		WARN_ON_ONCE(1);
+		break;
+	}
 }
 
 static void remove_miss_completions(struct gve_priv *priv,
@@ -1377,9 +1434,26 @@ static void remove_miss_completions(struct gve_priv *priv,
 		else
 			gve_unmap_packet(tx->dev, pending_packet);
 
-		/* This indicates the packet was dropped. */
-		dev_kfree_skb_any(pending_packet->skb);
-		pending_packet->skb = NULL;
+		switch (pending_packet->type) {
+		case GVE_TX_PENDING_PACKET_DQO_SKB:
+			if (pending_packet->skb) {
+				/* This indicates the packet was dropped. */
+				dev_kfree_skb_any(pending_packet->skb);
+				pending_packet->skb = NULL;
+			}
+			break;
+		case GVE_TX_PENDING_PACKET_DQO_XDP_FRAME:
+			if (pending_packet->xdpf) {
+				xdp_return_frame(pending_packet->xdpf);
+				pending_packet->xdpf = NULL;
+			}
+			break;
+		case GVE_TX_PENDING_PACKET_DQO_XSK:
+			break;
+		default:
+			WARN_ON_ONCE(1);
+			break;
+		}
 
 		u64_stats_update_begin(&tx->statss);
 		tx->dropped_pkt++;
@@ -1426,27 +1500,6 @@ static void remove_timed_out_completions(struct gve_priv *priv,
 	}
 }
 
-static void gve_tx_process_xsk_completions(struct gve_tx_ring *tx)
-{
-	u32 num_xsks = 0;
-
-	while (true) {
-		struct gve_tx_pending_packet_dqo *pending_packet =
-			gve_xsk_reorder_queue_head(tx);
-
-		if (!pending_packet ||
-		    pending_packet->state != GVE_PACKET_STATE_XSK_COMPLETE)
-			break;
-
-		num_xsks++;
-		gve_xsk_reorder_queue_pop_dqo(tx);
-		gve_free_pending_packet(tx, pending_packet);
-	}
-
-	if (num_xsks)
-		xsk_tx_completed(tx->xsk_pool, num_xsks);
-}
-
 int gve_clean_tx_done_dqo(struct gve_priv *priv, struct gve_tx_ring *tx,
 			  struct napi_struct *napi)
 {
-- 
2.55.0.1082.g2b9226bbc0-goog

Re: [PATCH net v2 1/9] gve: increment work_done for XDP and error packets

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 2 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message describes only a NAPI `work_done`
  accounting fix, but the patch also changes the semantics of the…
- [Medium] The newly added receive packet/byte accounting for XDP-
  consumed packets is unreachable for AF_XDP zero-copy buffers, so the…

--

gve: increment work_done for XDP and error packets

The DQO RX poll loop now increments work_done for every end-of-packet
completion instead of only for completions that produced an SKB, so XDP
and error packets are charged against the NAPI budget. The packet and
byte counters were moved into a separate rx_packets accumulator plus a
new increment on the XDP non-PASS path.

> diff --git a/drivers/net/ethernet/google/gve/gve_rx_dqo.c b/drivers/net/ethernet/google/gve/gve_rx_dqo.c
> index 5cf242b285577..c3f4a76b0fac5 100644
> --- a/drivers/net/ethernet/google/gve/gve_rx_dqo.c
> +++ b/drivers/net/ethernet/google/gve/gve_rx_dqo.c
> @@ -932,6 +932,10 @@ static int gve_rx_dqo(struct napi_struct *napi, struct gve_rx_ring *rx,
>  		if (xdp_act != XDP_PASS) {
>  			gve_xdp_done_dqo(priv, rx, &gve_xdp.xdp, xprog, xdp_act,
>  					 buf_state);
> +			u64_stats_update_begin(&rx->statss);
> +			rx->rpackets++;
> +			rx->rbytes += compl_desc->packet_len;
> +			u64_stats_update_end(&rx->statss);
>  			return 0;
>  		}

[Severity: Medium]
Is this new accounting reachable for AF_XDP zero-copy buffers?

gve_rx_dqo() dispatches those buffers before the xprog block above:

	xprog = READ_ONCE(priv->xdp_prog);
	if (buf_state->xsk_buff)
		return gve_rx_xsk_dqo(napi, rx, compl_desc, buf_state, xprog);

and gve_rx_xsk_dqo() takes its own non-PASS exit, which updates only
xdp_actions[] via gve_xsk_done_dqo() and leaves rx->ctx.skb_head NULL:

	if (xprog) {
		xdp_act = bpf_prog_run_xdp(xprog, xdp);
		buf_len = xdp->data_end - xdp->data;
		if (xdp_act != XDP_PASS) {
			gve_xsk_done_dqo(priv, rx, xdp, xprog, xdp_act);
			gve_free_buf_state(rx, buf_state);
			return 0;
		}
	}

With the reworked loop below, such a completion bumps work_done and then
hits "if (!rx->ctx.skb_head) continue;", so it is never added to
rx_packets/bytes either. Should a DQO queue running zero-copy with a
program that mostly returns XDP_DROP/XDP_TX/XDP_REDIRECT keep reporting
0 rx_packets/rx_bytes while the same program on the copy path now
reports them?

The GQI path counts XDP-consumed packets in rpackets/rbytes
(gve_rx_no_hsplit() does "ctx->total_size += frag_size; goto
finish_ok_pkt;"), so the DQO zero-copy path looks like the only
remaining outlier.

[Severity: Medium]
The commit message only describes the work_done change:

    Ensure every packet is accounted for in work_done by incrementing
    work_done before checking for the existence of a SKB.

Could it also mention that rx->rpackets/rx->rbytes semantics change?
XDP_DROP/XDP_ABORTED/XDP_TX/XDP_REDIRECT packets on the non-zero-copy
DQO path did not previously contribute to these counters, and
gve_get_stats() folds them into rtnl_link_stats64 rx_packets/rx_bytes:

	packets = priv->rx[ring].rpackets;
	bytes = priv->rx[ring].rbytes;
	...
	s->rx_packets += packets;
	s->rx_bytes += bytes;

The note "corrected stat counting for packets relative to work_done"
sits below the --- separator, so it will not be part of the git history,
and the patch carries Fixes: 293b49361f91 plus Cc: stable@vger.kernel.org.

> @@ -1090,6 +1094,7 @@ int gve_rx_poll_dqo(struct gve_notify_block *block, int budget)
>  	struct gve_rx_ring *rx;
>  	struct gve_priv *priv;
>  	u64 xdp_redirects;
> +	u32 rx_packets = 0;
>  	u32 work_done = 0;
>  	u64 bytes = 0;
>  	u64 xdp_txs;
> @@ -1150,13 +1155,14 @@ int gve_rx_poll_dqo(struct gve_notify_block *block, int budget)
>  		/* Free running counter of completed descriptors */
>  		rx->cnt++;
>  
> -		if (!rx->ctx.skb_head)
> -			continue;
> -
>  		if (!compl_desc->end_of_packet)
>  			continue;
>  
>  		work_done++;
> +
> +		if (!rx->ctx.skb_head)
> +			continue;
> +
>  		pkt_bytes = rx->ctx.skb_head->len;
>  		/* The ethernet header (first ETH_HLEN bytes) is snipped off
>  		 * by eth_type_trans.
> @@ -1164,6 +1170,9 @@ int gve_rx_poll_dqo(struct gve_notify_block *block, int budget)
>  		if (skb_headlen(rx->ctx.skb_head))
>  			pkt_bytes += ETH_HLEN;
>  
> +		rx_packets++;
> +		bytes += pkt_bytes;
> +
>  		/* gve_rx_complete_skb() will consume skb if successful */
>  		if (gve_rx_complete_skb(rx, napi, compl_desc, feat) != 0) {
>  			gve_rx_free_skb(napi, rx);

[Severity: Medium]
Related to the commit message question above: moving "bytes += pkt_bytes"
ahead of the gve_rx_complete_skb() failure branch means packets the
driver drops here (and also counted in rx_desc_err_dropped_pkt) now add
their bytes to rx_bytes, where previously they did not. Is that change
intended, and should it be described in the changelog?

> @@ -1173,7 +1182,6 @@ int gve_rx_poll_dqo(struct gve_notify_block *block, int budget)
>  			continue;
>  		}
>  
> -		bytes += pkt_bytes;
>  		rx->ctx.skb_head = NULL;
>  		rx->ctx.skb_tail = NULL;
>  	}
> @@ -1187,7 +1195,7 @@ int gve_rx_poll_dqo(struct gve_notify_block *block, int budget)
>  	gve_rx_post_buffers_dqo(rx);
>  
>  	u64_stats_update_begin(&rx->statss);
> -	rx->rpackets += work_done;
> +	rx->rpackets += rx_packets;
>  	rx->rbytes += bytes;
>  	u64_stats_update_end(&rx->statss);
>

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

Re: [PATCH net v2 4/9] gve: don't register xsk pool on pre-existing queues in RDA mode

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 (2) · 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).

- [High] Peer-call-site asymmetry: this patch makes in-place XSK
  registration conditional on gve_is_qpl() in gve_xsk_pool_enable(), but…
- [Medium] In the QPL branch kept by this patch (gve_main.c:1656-1659),
  gve_reg_xsk_pool() publishes the XSK pool into a LIVE queue…

Pre-existing issues:
- [High] gve_xsk_pool_disable()
  (drivers/net/ethernet/google/gve/gve_main.c, ~line 1690-1696 at this…
- [High] NULL-pointer dereference on the error path of
  gve_xsk_pool_enable(): the RDA branch routes a…

--

gve: don't register xsk pool on pre-existing queues in RDA mode

This makes gve_xsk_pool_enable() register the XSK pool in place only for
QPL devices, and rely on the ring reconfiguration in
gve_configure_rings_xdp() for DQO RDA, dropping the now unused
err_xsk_pool_registered unwind label.

> diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> index 9cc343a162712..b9bcdc7619b23 100644
> --- a/drivers/net/ethernet/google/gve/gve_main.c
> +++ b/drivers/net/ethernet/google/gve/gve_main.c
> @@ -1654,20 +1654,18 @@ static int gve_xsk_pool_enable(struct net_device *dev,
>  	if (!priv->xdp_prog || !netif_running(dev))
>  		return 0;
>  
> -	err = gve_reg_xsk_pool(priv, dev, pool, qid);
> -	if (err)
> -		goto err_xsk_pool_dma_mapped;
> -
> -	/* Stop and start RDA queues to repost buffers. */
> -	if (!gve_is_qpl(priv)) {
> +	if (gve_is_qpl(priv)) {
> +		err = gve_reg_xsk_pool(priv, dev, pool, qid);
> +		if (err)
> +			goto err_xsk_pool_dma_mapped;

[Severity: Medium]
Is the QPL branch still exposed to the same class of race the commit
message describes?  gve_reg_xsk_pool() publishes the pool into a queue
that is still being polled, with no NAPI quiescing and no barrier:

gve_main.c:gve_reg_xsk_pool() {
	err = xdp_rxq_info_reg_mem_model(&rx->xdp_rxq,
					 MEM_TYPE_XSK_BUFF_POOL, pool);
	...
	rx->xsk_pool = pool;

	tx_qid = gve_xdp_tx_queue_id(priv, qid);
	priv->tx[tx_qid].xsk_pool = pool;
}

The teardown side treats those same stores as datapath visible state and
brackets them:

gve_main.c:gve_xsk_pool_disable() {
	napi_disable(napi_rx); /* make sure current rx poll is done */
	...
	napi_disable(napi_tx); /* make sure current tx poll is done */

	gve_unreg_xsk_pool(priv, qid);
	smp_mb(); /* Make sure it is visible to the workers on datapath */
	...
}

so the smp_mb() on the disable side has no counterpart on the enable
side.  A concurrent poll in gve_xdp_redirect() reads rx->xsk_pool and
rx->xdp_rxq:

gve_rx.c:gve_xdp_redirect() {
	if (rx->xsk_pool)
		return gve_xsk_pool_redirect(dev, rx, orig->data,
					     len, xdp_prog);
	...
	xdp_init_buff(&new, total_len, &rx->xdp_rxq);
}

Can it observe rx->xsk_pool set before the mem model switch is visible,
or the mem model already switched to MEM_TYPE_XSK_BUFF_POOL while
rx->xsk_pool still reads NULL, so a buffer is freed through the wrong
allocator?

A later patch in this series, "gve: disable NAPI when registering XSK
pools in QPL mode", adds a gve_disable_xsk_napis()/smp_mb()/
gve_enable_xsk_napis() bracket around this call, which would close the
window.  Would it make sense to order that change before this one?

> +	} else {
> +		/* Stop and start RDA queues to repost buffers. */
>  		err = gve_configure_rings_xdp(priv, priv->rx_cfg.num_queues);
>  		if (err)
> -			goto err_xsk_pool_registered;
> +			goto err_xsk_pool_dma_mapped;
>  	}
>  	return 0;
>  
> -err_xsk_pool_registered:
> -	gve_unreg_xsk_pool(priv, qid);
>  err_xsk_pool_dma_mapped:
>  	clear_bit(qid, priv->xsk_pools);

[Severity: High]
This isn't a bug introduced by this patch, since the same clear_bit() was
already reachable through the removed err_xsk_pool_registered label, but
can priv->xsk_pools be NULL here?

The error returned by gve_configure_rings_xdp() can come from a path that
already tore down and reallocated the bitmap:

gve_main.c:gve_queues_stop() {
err:
	if (gve_get_reset_in_progress(priv))
		return err;
	/* Otherwise reset before returning */
	gve_reset_and_teardown(priv, true);
	return gve_reset_recovery(priv, false);
}

gve_main.c:gve_teardown_priv_resources() {
	...
	bitmap_free(priv->xsk_pools);
	priv->xsk_pools = NULL;
}

If the subsequent gve_init_priv() fails (adminq allocation, the
bitmap_zalloc() returning -ENOMEM, or gve_setup_device_resources()
failing and taking the err_free_xsk_bitmap path which re-NULLs the
bitmap), the non-zero error propagates back here with priv->xsk_pools ==
NULL and clear_bit(qid, NULL) faults.

Even when the recovery succeeds, the bitmap is a freshly zeroed
allocation, so bits for other queues whose pools are still DMA mapped are
silently dropped.  Should this path check priv->xsk_pools before clearing
the bit?

> 	xsk_pool_dma_unmap(pool,

[Severity: High]
This is a pre-existing issue rather than something this patch introduces,
but the mirror of the ordering problem fixed here still exists on the
disable side.  gve_xsk_pool_disable() unmaps the pool while the RX
datapath for that queue is still live:

gve_main.c:gve_xsk_pool_disable() {
	clear_bit(qid, priv->xsk_pools);

	pool = xsk_get_pool_from_qid(dev, qid);
	if (pool)
		xsk_pool_dma_unmap(pool,
				   DMA_ATTR_SKIP_CPU_SYNC |
				   DMA_ATTR_WEAK_ORDERING);

	if (!netif_running(dev) || !priv->tx_cfg.num_xdp_queues)
		return 0;
	...
}

rx->xsk_pool is only cleared much later, by the ring restart in
gve_configure_rings_xdp() or by gve_unreg_xsk_pool() after
napi_disable().  In that window the consumer still gates on rx->xsk_pool
alone, not on the priv->xsk_pools bit:

gve_buffer_mgmt_dqo.c:gve_alloc_buffer() {
	if (rx->xsk_pool) {
		buf_state = gve_alloc_buf_state(rx);
		...
		buf_state->xsk_buff = xsk_buff_alloc(rx->xsk_pool);
		...
		desc->buf_addr =
			cpu_to_le64(xsk_buff_xdp_get_dma(buf_state->xsk_buff));
	}
}

Can a concurrent gve_rx_post_buffers_dqo() therefore post descriptors
built from a pool whose dma_pages have already been kvfree()d by
xp_dma_unmap(), i.e. the same failure signature quoted in the commit
message but on the teardown side?  The DQO RDA branch does not stop the
queue until gve_configure_rings_xdp(), which first does a full
gve_queues_mem_alloc(), so the window looks wide.

The later patch "gve: fix NULL dereference from premature XSK pool DMA
unmap" moves this unmap to an unmap_and_return: label that runs only
after the queue has been quiesced.

[Severity: High]
Now that in-place registration is conditional on gve_is_qpl(), should the
unregister in gve_xsk_pool_disable() become conditional too?  It is still
unconditional, and in DQO RDA mode it runs after the rings were already
re-created:

gve_main.c:gve_xsk_pool_disable() {
	clear_bit(qid, priv->xsk_pools);
	...
	if (!gve_is_qpl(priv) && priv->xdp_prog) {
		err = gve_configure_rings_xdp(priv, priv->rx_cfg.num_queues);
		if (err)
			return err;
	}
	...
	gve_unreg_xsk_pool(priv, qid);
	smp_mb(); /* Make sure it is visible to the workers on datapath */
	...
}

Since the qid bit was cleared first, the ring restart registers the page
pool mem model for that queue:

gve_main.c:gve_reg_xdp_info() {
	...
	else
		err = xdp_rxq_info_reg_mem_model(&rx->xdp_rxq,
						 MEM_TYPE_PAGE_POOL,
						 rx->dqo.page_pool);
}

and gve_unreg_xsk_pool() is mode agnostic:

gve_main.c:gve_unreg_xsk_pool() {
	rx->xsk_pool = NULL;
	if (xdp_rxq_info_is_reg(&rx->xdp_rxq))
		xdp_rxq_info_unreg_mem_model(&rx->xdp_rxq);
	...
}

Does this end up calling page_pool_destroy() on the page pool that the
running queue is still allocating from, with the ring teardown later
destroying the same pool again?

The final state of this series drops the unconditional call, using
gve_unreg_xsk_pool_live() only for gve_is_qpl(priv) and
gve_restart_rings() for RDA, in "gve: ensure XDP mem model is registered
when disabling XSK pools".

>  	xsk_pool_dma_unmap(pool,

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

Re: [PATCH net v2 6/9] gve: fix NULL dereference from premature XSK pool DMA unmap

[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] In gve_xsk_pool_disable()
  (drivers/net/ethernet/google/gve/gve_main.c), this patch moves the…

--

gve: fix NULL dereference from premature XSK pool DMA unmap

This patch moves the XSK pool DMA unmap in gve_xsk_pool_disable() to a new
unmap_and_return label at the end of the function. The goal is to keep
the pool mapped until the rings no longer use it. This fixes a race where
RX NAPI calls xsk_buff_dma_sync_for_cpu() after pool->dev has been set
to NULL.

> diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> index 3712ff364cbd5..49ae2b8c6a270 100644
> --- a/drivers/net/ethernet/google/gve/gve_main.c
> +++ b/drivers/net/ethernet/google/gve/gve_main.c
> @@ -1681,22 +1681,18 @@ static int gve_xsk_pool_disable(struct net_device *dev,

[ ... ]

>  	clear_bit(qid, priv->xsk_pools);
>  
> -	pool = xsk_get_pool_from_qid(dev, qid);
> -	if (pool)
> -		xsk_pool_dma_unmap(pool,
> -				   DMA_ATTR_SKIP_CPU_SYNC |
> -				   DMA_ATTR_WEAK_ORDERING);
> -
>  	if (!netif_running(dev) || !priv->tx_cfg.num_xdp_queues)
> -		return 0;
> +		goto unmap_and_return;
>  
>  	/* Stop and start RDA queues to repost buffers. */
>  	if (!gve_is_qpl(priv) && priv->xdp_prog) {

[Severity: Medium]
Does the RDA branch right after this still skip the new unmap? The
unchanged code that follows is:

	if (!gve_is_qpl(priv) && priv->xdp_prog) {
		err = gve_configure_rings_xdp(priv, priv->rx_cfg.num_queues);
		if (err)
			return err;
	}

Before this patch the unmap ran before this branch, so every exit
unmapped the pool. Now xsk_pool_dma_unmap() is only called at
unmap_and_return. If gve_configure_rings_xdp()->gve_adjust_config()
fails, the bare return skips it.

That failure can come from gve_queues_mem_alloc() returning -ENOMEM
before gve_close(), from gve_close(), or from gve_queues_start().

The XSK core does not recover from this. xp_disable_drv_zc() in
net/xdp/xsk_buff_pool.c only warns and continues the teardown:

	err = pool->netdev->netdev_ops->ndo_bpf(pool->netdev, &bpf);

	if (err)
		WARN(1, "Failed to disable zero-copy!\n");

Would that leak pool->dma_pages, the xsk_dma_map reference and the IOMMU
mappings for the UMEM pages?

The error path in gve_xsk_pool_enable() does unmap the pool when
gve_configure_rings_xdp() fails:

err_xsk_pool_dma_mapped:
	clear_bit(qid, priv->xsk_pools);
	xsk_pool_dma_unmap(pool,
			   DMA_ATTR_SKIP_CPU_SYNC |
			   DMA_ATTR_WEAK_ORDERING);
	return err;

Could the disable path do the same, for example with
"goto unmap_and_return"?

This also seems to go against the commit message, which says pools are
DMA unmapped "in all scenarios".

This is a pre-existing issue, but the same early return also skips
napi_disable_locked(), gve_unreg_xsk_pool() and napi_enable_locked(). On
the gve_queues_mem_alloc() failure path the old rings are still live.
Their rx->xsk_pool and tx->xsk_pool would then keep pointing at a pool
that the core goes on to free.

A later commit in this series, "gve: ensure XDP mem model is registered
when disabling XSK pools", appears to fix this. It replaces the
gve_configure_rings_xdp() block with gve_restart_rings(), and every path
falls through to unmap_and_return. So the problem looks limited to this
intermediate commit. It would still matter for bisection, or for a stable
backport that takes this patch without the later one, since this patch
is tagged Cc: stable.

> @@ -1725,7 +1721,14 @@ static int gve_xsk_pool_disable(struct net_device *dev,
>  			napi_schedule(napi_tx);
>  	}
>  
> -	return 0;
> +unmap_and_return:
> +	pool = xsk_get_pool_from_qid(dev, qid);
> +	if (pool)
> +		xsk_pool_dma_unmap(pool,
> +				   DMA_ATTR_SKIP_CPU_SYNC |
> +				   DMA_ATTR_WEAK_ORDERING);
> +
> +	return err;
>  }

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

Re: [PATCH net v2 7/9] gve: disable NAPI when registering XSK pools in QPL mode

[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

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] On GQI-QPL, the new
  gve_disable_xsk_napis()/gve_enable_xsk_napis() window can leave a…
- [Low] The Fixes: tag and root-cause description in commit "gve: disable
  NAPI when registering XSK pools in QPL mode" appear to point at the…

--

gve: disable NAPI when registering XSK pools in QPL mode

This patch disables the RX NAPI and the XDP TX NAPI of the target queue
while gve_xsk_pool_enable() registers the MEM_TYPE_XSK_BUFF_POOL memory
model in QPL mode. The NAPIs are enabled again afterwards through a new
gve_enable_xsk_napis() helper. The goal is to close a race between packet
processing and the memory model registration.

> GVE in QPL mode does not current respect this, which could lead to a
> race condition between packet processing and the XSK_BUFF_POOL memory
> model registration.

[ ... ]

> Fixes: fd8e40321a12 ("gve: Add AF_XDP zero-copy support for GQI-QPL format")
> Cc: stable@vger.kernel.org

[Severity: Low]
Is fd8e40321a12 the right Fixes: target? The race described above needs
the memory model of the live rx->xdp_rxq to change while NAPI is running.

In fd8e40321a12, gve_xsk_pool_enable() registered MEM_TYPE_XSK_BUFF_POOL
on a separate rx->xsk_rxq:

    xdp_rxq_info_reg(&rx->xsk_rxq, ...);
    xdp_rxq_info_reg_mem_model(&rx->xsk_rxq, MEM_TYPE_XSK_BUFF_POOL, NULL);
    xsk_pool_set_rxq_info(pool, &rx->xsk_rxq);

The page-backed buffers that NAPI built kept using rx->xdp_rxq, and that
rxq stayed MEM_TYPE_PAGE_SHARED.

The switch on the live rxq seems to come from 077f7153fd25 ("gve: merge
xdp and xsk registration"). That commit changed gve_reg_xsk_pool() to do:

    err = xdp_rxq_info_reg_mem_model(&rx->xdp_rxq,
                                     MEM_TYPE_XSK_BUFF_POOL, pool);

The next patch in this series, "gve: ensure XDP mem model is registered
when disabling XSK pools", fixes the same problem on the disable path, and
it tags 077f7153fd25.

With the current tag, stable would try to backport this to 6.8 through
6.16, including 6.12 LTS. Those kernels don't have the mem-model switch
described here. The patch also won't apply there: gve_reg_xsk_pool() came
from d57ae093c887, and napi_disable_locked() isn't in the older trees.

fd8e40321a12 may have had a separate ordering race, because rx->xsk_pool
was published without an smp_wmb() after xsk_pool_set_rxq_info(). This
NAPI quiesce would close that race too, but the commit message doesn't
describe it.

Should the tag be 077f7153fd25 instead?

Also, the message only says the race "could lead to" a problem and doesn't
name a symptom. Could it mention that the mismatch can end with
container_of(xdp, struct xdp_buff_xsk, xdp) being applied to a page-backed
xdp_buff in the XSK receive path?

> diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> index 49ae2b8c6a270..f2bd4011de23f 100644
> --- a/drivers/net/ethernet/google/gve/gve_main.c
> +++ b/drivers/net/ethernet/google/gve/gve_main.c
> @@ -1626,6 +1626,40 @@ static int gve_xdp_xmit(struct net_device *dev, int n,

[ ... ]

> +static void gve_enable_xsk_napis(struct gve_priv *priv, u16 qid)
> +{
> +	struct napi_struct *napi_rx, *napi_tx;
> +	u16 tx_qid;
> +
> +	napi_rx = &priv->ntfy_blocks[priv->rx[qid].ntfy_id].napi;
> +	napi_enable_locked(napi_rx);
> +
> +	tx_qid = gve_xdp_tx_queue_id(priv, qid);
> +	napi_tx = &priv->ntfy_blocks[priv->tx[tx_qid].ntfy_id].napi;
> +	napi_enable_locked(napi_tx);
> +
> +	if (gve_is_gqi(priv)) {
> +		if (gve_rx_work_pending(&priv->rx[qid]))
> +			napi_schedule(napi_rx);
> +
> +		if (gve_tx_clean_pending(priv, &priv->tx[tx_qid]))
> +			napi_schedule(napi_tx);
> +	}
> +}

[Severity: Medium]
On GQI-QPL, can this leave the notify block's IRQ doorbell masked while
NAPI is idle?

In GQI mode, gve_intr() masks the doorbell before it schedules NAPI:

gve_intr() {
    ...
    iowrite32be(GVE_IRQ_MASK, gve_irq_doorbell(priv, block));
    napi_schedule_irqoff(&block->napi);
    ...
}

Only gve_napi_poll() re-arms the doorbell, by writing
GVE_IRQ_ACK | GVE_IRQ_EVENT after napi_complete_done(). A poll that uses
its full budget returns before that write:

gve_napi_poll() {
    ...
    if (reschedule)
        return budget;
    ...
}

Suppose napi_disable_locked() in gve_disable_xsk_napis() overlaps an RX
poll that uses exactly its budget and drains the ring. __napi_poll() then
completes the NAPI without calling the driver again:

__napi_poll() {
    ...
    if (unlikely(napi_disable_pending(n))) {
        napi_complete(n);
        return work;
    }
    ...
}

An interrupt that arrives while the NAPI is disabled has the same effect.
gve_intr() masks the doorbell, but napi_schedule_prep() fails, so no poll
is queued.

gve_enable_xsk_napis() never unmasks the doorbell. It only reschedules if
gve_rx_work_pending() or gve_tx_clean_pending() sees work. If no new
descriptor arrived during the window, would the RX queue and its XDP TX
queue stop getting interrupts?

gve_turnup() unmasks the doorbell and schedules unconditionally:

gve_turnup() {
    ...
    if (gve_is_gqi(priv)) {
        iowrite32be(0, gve_irq_doorbell(priv, block));
    } ...
    mb();
    napi_schedule(&block->napi);
    ...
}

GQI-QPL never sets the XSK RX need_wakeup flag. xsk_set_rx_need_wakeup()
is only used in gve_buffer_mgmt_dqo.c, so an RX-only AF_XDP application
won't kick the NAPI through gve_xsk_wakeup(). The netdev TX watchdog
doesn't cover the XDP TX queue. Regular XDP_PASS traffic on that queue
would stall as well.

This pattern is copied from the existing gve_xsk_pool_disable() code. The
later patch in this series, "gve: ensure XDP mem model is registered when
disabling XSK pools", reuses this helper on the disable path, so the end
of the series still reschedules only conditionally.

Should gve_enable_xsk_napis() unmask the doorbell and schedule NAPI
unconditionally, as gve_turnup() does?

[ ... ]

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

Re: [PATCH net v2 8/9] gve: ensure XDP mem model is registered when disabling XSK pools

[netdev-bot+sashiko]

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

Critical: 1 · High: 1 · Medium: 2 · Low: 3

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

- [Critical] Use-after-free in gve_restart_rings()
  (drivers/net/ethernet/google/gve/gve_main.c) when gve_queues_stop()…
- [High] gve_xsk_pool_disable()
  (drivers/net/ethernet/google/gve/gve_main.c) only checks…
- [Medium] After an XSK pool is disabled, DQO-RDA RX page pools
  permanently lose NAPI direct recycling.
- [Medium] Stale pending_packets[].state after the in-place TX ring reset
  weakens completion validation.
- [Low] Incomplete QPL counter reset in gve_tx_reset_ring_dqo() /
  gve_tx_init_ring_state_dqo()…
- [Low] Stray blank line in drivers/net/ethernet/google/gve/gve_tx_dqo.c.
- [Low] The commit message describes the QPL half of the change as a
  functional bug fix, but it has no functional effect.

--

gve: ensure XDP mem model is registered when disabling XSK pools

In QPL mode, this patch changes gve_xsk_pool_disable() to re-register the
XDP memory model in place through a new helper, gve_unreg_xsk_pool_live().
In RDA mode, a new helper, gve_restart_rings(), stops and restarts the
existing rings instead of re-allocating them through
gve_configure_rings_xdp(). To support the in-place restart, the DQO TX ring
state setup moves into gve_tx_init_ring_state_dqo(), and
gve_tx_stop_ring_dqo() now calls a new gve_tx_reset_ring_dqo().

> QPL mode, which does not need to reconfigure rings due to not posting
> XSK umem to the hardware ring, simply misses registering the RXQ XDP
> info with the MEM_TYPE_PAGE_SHARED memory model.

[Severity: Low]
Is this description accurate for QPL mode?

gve_unreg_xsk_pool() calls xdp_rxq_info_unreg_mem_model(), and
xdp_unreg_mem_model() does:

net/core/xdp.c:xdp_unreg_mem_model() {
    ...
	mem->id = 0;
	mem->type = 0;
    ...
}

MEM_TYPE_PAGE_SHARED is 0. When the allocator is NULL,
__xdp_reg_mem_model() only sets mem->type = type and returns. So after the
unregister the QPL rxq is already MEM_TYPE_PAGE_SHARED. The new
registration in gve_unreg_xsk_pool_live() leaves it in the same state.

Should the commit message say that only RDA mode had a functional problem?

> diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> index f2bd4011de23f..787d311ff99bd 100644
> --- a/drivers/net/ethernet/google/gve/gve_main.c
> +++ b/drivers/net/ethernet/google/gve/gve_main.c

[ ... ]

> +static int gve_restart_rings(struct gve_priv *priv)
> +{
> +	struct gve_tx_alloc_rings_cfg tx_alloc_cfg = {0};
> +	struct gve_rx_alloc_rings_cfg rx_alloc_cfg = {0};
> +	int err;
> +
> +	gve_get_curr_alloc_cfgs(priv, &tx_alloc_cfg, &rx_alloc_cfg);
> +	err = gve_queues_stop(priv);
> +	if (err)
> +		return err;
> +
> +	err = gve_queues_start(priv, &tx_alloc_cfg, &rx_alloc_cfg);

[Severity: Critical]
Can this reuse freed ring memory if gve_queues_stop() takes its error path?

tx_alloc_cfg.tx and rx_alloc_cfg.rx are copies of priv->tx and priv->rx,
taken before the stop. Suppose gve_destroy_rings() or gve_unregister_qpls()
fails while no reset is in progress. Then gve_queues_stop() does:

gve_queues_stop()
  gve_reset_and_teardown(priv, true)
    gve_close()
      gve_queues_mem_remove()   <- frees priv->tx / priv->rx and the rings
  return gve_reset_recovery(priv, false)

With was_up == false, gve_reset_recovery() only calls gve_init_priv(). It
returns 0 without allocating new rings.

gve_restart_rings() then sees err == 0 and passes the stale pointers to
gve_queues_start():

gve_queues_start()
    priv->tx = tx_alloc_cfg->tx;
    priv->rx = rx_alloc_cfg->rx;
    ...
    gve_tx_start_rings()
      gve_tx_start_ring_dqo()
        gve_tx_add_to_block()
          tx->ntfy_id = ntfy_idx;   <- write into freed memory

After that, gve_reg_xdp_info() and gve_create_rings() would also give the
device the DMA addresses (q_resources_bus, bus, complq_bus_dqo) of freed
coherent buffers.

The commit message says "failure to stop the rings would result in a safer
device reset". Isn't this the path where it goes wrong?

The old gve_configure_rings_xdp() -> gve_adjust_config() path allocated
new rings before calling gve_close(), so it didn't have this problem.

[Severity: Medium]
Does this restart leave the RX page pools without direct recycling?

gve_rx_create_page_pool() sets .napi = &priv->ntfy_blocks[ntfy_id].napi,
but only when it creates the pool. gve_queues_stop() -> gve_rx_stop_rings()
-> gve_rx_stop_ring_dqo() calls:

	if (rx->dqo.page_pool)
		page_pool_disable_direct_recycling(rx->dqo.page_pool);

That clears pool->p.napi.

gve_queues_start() -> gve_rx_start_ring_dqo() keeps the same
rx->dqo.page_pool and only calls gve_rx_add_to_block(), timer_setup() and
gve_add_napi(). Nothing calls page_pool_enable_direct_recycling().

So once an XSK pool is disabled on a DQO-RDA interface, no RX queue's pool
has a NAPI owner until the next full reconfiguration. The old
gve_adjust_config() path created new pools, so this didn't happen before.

> +	return err;
> +}
> +

[ ... ]

> @@ -1735,31 +1776,11 @@ static int gve_xsk_pool_disable(struct net_device *dev,
>  	if (!netif_running(dev) || !priv->tx_cfg.num_xdp_queues)
>  		goto unmap_and_return;
>  

[ ... ]

> +	if (gve_is_qpl(priv)) {
> +		err = gve_unreg_xsk_pool_live(priv, dev, qid);
> +	} else {
> +		/* Stop and start RDA queues to repost buffers. */
> +		err = gve_restart_rings(priv);

[Severity: High]
Can this dereference a NULL priv->tx when the netdev is running but has no
rings?

The code above only checks netif_running() and num_xdp_queues. priv->tx
and priv->rx can be NULL while the device is still running, for example:

- gve_reset() -> gve_reset_recovery(was_up=true) -> gve_open() fails.
- An earlier gve_restart_rings() failed in gve_reg_xdp_info() and
  gve_queues_start() took its stop_and_free_rings path.

In that state gve_restart_rings() copies tx = NULL and rx = NULL.
gve_queues_stop() does nothing and returns 0: device_rings_ok is clear,
and gve_unreg_xdp_info(), gve_tx_stop_rings() and gve_rx_stop_rings() all
return early on NULL rings. Then:

gve_queues_start()
    priv->tx = tx_alloc_cfg->tx;   <- NULL
    gve_tx_start_rings()           <- no !priv->tx check
      gve_tx_start_ring_dqo()
        gve_tx_add_to_block()
          struct gve_tx_ring *tx = &priv->tx[queue_idx];
          tx->ntfy_id = ntfy_idx;

The old RDA path allocated rings in gve_adjust_config() before using them,
so it didn't crash here.

On the QPL side, gve_unreg_xsk_pool_live() also reads
priv->rx[qid].ntfy_id without a check, but the old code already did that.

Should both live paths first check that the rings exist?

>  	}
>  
>  unmap_and_return:
> diff --git a/drivers/net/ethernet/google/gve/gve_tx_dqo.c b/drivers/net/ethernet/google/gve/gve_tx_dqo.c
> index 80ab0a449ff54..78f946ae72649 100644
> --- a/drivers/net/ethernet/google/gve/gve_tx_dqo.c
> +++ b/drivers/net/ethernet/google/gve/gve_tx_dqo.c
> @@ -208,6 +208,78 @@ static void gve_tx_clean_pending_packets(struct gve_tx_ring *tx)
>  	}
>  }
>  
> +static void gve_tx_init_ring_state_dqo(struct gve_tx_ring *tx)
> +{
> +	int i;
> +
> +	atomic_set_release(&tx->dqo_compl.hw_tx_head, 0);
> +
> +	/* Set up linked list of pending packets */
> +	for (i = 0; i < tx->dqo.num_pending_packets - 1; i++)
> +		tx->dqo.pending_packets[i].next = i + 1;

[Severity: Medium]
When this runs from gve_tx_reset_ring_dqo(), should it also reset
pending_packets[i].state and type?

Before this patch, pending_packets was always freshly kvzalloc'd, and
gve.h relies on that:

	/* Packet is in free list, available to be allocated.
	 * This should always be zero since state is not explicitly initialized.
	 */
	GVE_PACKET_STATE_UNALLOCATED,

With the in-place restart, gve_tx_clean_pending_packets() frees skb/xdpf
and sets the pointer to NULL. In-flight slots still keep their old state
(PENDING_DATA_COMPL, PENDING_REINJECT_COMPL or TIMED_OUT_COMPL). This loop
then puts every slot back on the free list without touching state, type
or prev.

gve_handle_packet_completion() relies on this check to reject completions
for unallocated slots:

	if (unlikely(pending_packet->state !=
		     GVE_PACKET_STATE_PENDING_DATA_COMPL)) {

After the restart, a stale completion from the device for one of these
slots would pass the check. For an SKB slot, the code then reads
pending_packet->skb->len while skb is NULL. It also calls
gve_free_pending_packet() on a slot that is already on the free list.

Could that corrupt the free list and let the same slot be handed out
twice?

> +
> +	tx->dqo.pending_packets[tx->dqo.num_pending_packets - 1].next = -1;
> +	atomic_set_release(&tx->dqo_compl.free_pending_packets, -1);

[ ... ]

> +	/* Generate free TX buf list */
> +	if (tx->dqo.tx_qpl_buf_next) {
> +		for (i = 0; i < tx->dqo.num_tx_qpl_bufs - 1; i++)
> +			tx->dqo.tx_qpl_buf_next[i] = i + 1;
> +		tx->dqo.tx_qpl_buf_next[tx->dqo.num_tx_qpl_bufs - 1] = -1;
> +
> +		atomic_set_release(&tx->dqo_compl.free_tx_qpl_buf_head, -1);
> +		atomic_set_release(&tx->dqo_compl.free_tx_qpl_buf_cnt, 0);

[Severity: Low]
Should dqo_tx.alloc_tx_qpl_buf_cnt and dqo_tx.free_tx_qpl_buf_cnt also be
reset here?

gve_has_free_tx_qpl_bufs() uses them as a pair:

	num_avail = tx->dqo.num_tx_qpl_bufs -
		(tx->dqo_tx.alloc_tx_qpl_buf_cnt -
		 tx->dqo_tx.free_tx_qpl_buf_cnt);

It also refreshes the cached dqo_tx.free_tx_qpl_buf_cnt from
dqo_compl.free_tx_qpl_buf_cnt. gve_tx_reset_ring_dqo() leaves
alloc_tx_qpl_buf_cnt at its running value.

If a QPL ring were restarted in place, the first refresh would compare 0
against a large alloc count. num_avail would go negative and the queue
would stall.

This can't happen today: gve_restart_rings() only runs in RDA mode, and
the other callers of gve_tx_stop_ring_dqo() free the ring right after.

> +	}
> +}

[ ... ]

> @@ -304,6 +369,7 @@ void gve_tx_start_ring_dqo(struct gve_priv *priv, int idx)
>  	gve_add_napi(priv, ntfy_idx, gve_napi_poll_dqo);
>  }
>  
> +

[Severity: Low]
This isn't a bug, but it adds a second blank line before
gve_tx_alloc_ring_dqo().

>  static int gve_tx_alloc_ring_dqo(struct gve_priv *priv,
>  				 struct gve_tx_alloc_rings_cfg *cfg,
>  				 struct gve_tx_ring *tx,

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

Re: [PATCH net v2 9/9] gve: prevent XDP frame leak and corruption during DQO TX cleanup

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 2 · Low: 2

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

- [High] The patch still unmaps XSK pending packets as if the driver
  owned their DMA mapping, so it never closes the bug class its message…
- [Medium] Now that gve_handle_miss_completion() no longer crashes on XSK
  packets, those packets can reach the late-reinjection branch of…
- [Medium] The new XSK teardown handling in
  gve_tx_clean_pending_packets() never runs when XSK packets can…
- [Low] The commit message describes only XDP frames being mistaken for
  SKBs, but the patch also changes AF_XDP zero-copy (XSK) behaviour…
- [Low] The comments were not updated to match the rewritten cleanup
  code.

--

gve: prevent XDP frame leak and corruption during DQO TX cleanup

This commit changes gve_tx_clean_pending_packets(),
gve_handle_miss_completion() and remove_miss_completions() to switch on
pending_packet->type, so XDP frames are returned with xdp_return_frame()
instead of being handled as SKBs. It also marks XSK pending packets
complete during teardown. It moves the XSK reorder queue helpers earlier
in the file so the cleanup path can call gve_tx_process_xsk_completions().

> When tearing down a DQO TX ring or processing miss completions,
> pending packets were assumed to be SKBs. If an XDP frame was pending
> during cleanup or timeout, cur_state->skb was accessed on an xdpf
> union pointer or the XDP frame was leaked without calling
> xdp_return_frame().
>
> Refactor gve_tx_clean_pending_packets(), gve_handle_miss_completion(),
> and remove_miss_completions() to switch on pending_packet->type.
>
> Fixes: d8a8ca14c937 ("gve: add XDP_TX and XDP_REDIRECT support for DQ RDA")

[Severity: Low]
The commit message only talks about XDP frames, but the patch also
changes AF_XDP zero-copy (XSK) behaviour. Could the message describe that
as well?

Before this patch, gve_handle_miss_completion() did:

	*bytes += pending_packet->skb->len;

For XSK pending packets the skb/xdpf union is always NULL, because
gve_xsk_tx_dqo() never sets it. So a miss completion on an XSK packet
would be a NULL pointer dereference in NAPI context. The new XSK case
fixes that crash.

That code came from 2236836eab26 ("gve: implement DQO TX datapath for
AF_XDP zero-copy"), which added GVE_TX_PENDING_PACKET_DQO_XSK. Should
that commit get a Fixes: tag too?

The message also doesn't mention two other changes. At teardown,
gve_tx_clean_pending_packets() now marks XSK slots XSK_COMPLETE and tries
to report them through xsk_tx_completed(). And gve_xsk_reorder_queue_head(),
gve_xsk_reorder_queue_pop_dqo() and gve_tx_process_xsk_completions() have
been moved.

Both commits landed in the same development cycle, so this is mostly
about traceability.

> diff --git a/drivers/net/ethernet/google/gve/gve_tx_dqo.c b/drivers/net/ethernet/google/gve/gve_tx_dqo.c
> index 78f946ae72649..e8481b993beb0 100644
> --- a/drivers/net/ethernet/google/gve/gve_tx_dqo.c
> +++ b/drivers/net/ethernet/google/gve/gve_tx_dqo.c

[ ... ]

> @@ -201,11 +244,30 @@ static void gve_tx_clean_pending_packets(struct gve_tx_ring *tx)
>  		else
>  			gve_unmap_packet(tx->dev, cur_state);
>  
> -		if (cur_state->skb) {
> -			dev_consume_skb_any(cur_state->skb);
> -			cur_state->skb = NULL;
> +		switch (cur_state->type) {
> +		case GVE_TX_PENDING_PACKET_DQO_SKB:

[ ... ]

> +		case GVE_TX_PENDING_PACKET_DQO_XSK:
> +			cur_state->state = GVE_PACKET_STATE_XSK_COMPLETE;
> +			break;

[Severity: High]
The gve_unmap_packet() call above the switch still runs for every slot,
XSK slots included. Can this unmap a DMA address the driver doesn't own?

gve_unmap_packet() assumes the driver mapped the slot:

	/* SKB linear portion is guaranteed to be mapped */
	dma_unmap_single(dev, dma_unmap_addr(pkt, dma[0]),
			 dma_unmap_len(pkt, len[0]), DMA_TO_DEVICE);

For XSK, gve_xsk_tx_dqo() posts a pool-owned address and increments
num_bufs, but it never records dma[0]/len[0]:

	pkt->type = GVE_TX_PENDING_PACKET_DQO_XSK;
	pkt->num_bufs = 0;
	...
	addr = xsk_buff_raw_get_dma(pool, desc.addr);
	...
	++pkt->num_bufs;

The XSK case in gve_handle_packet_completion() only sets
GVE_PACKET_STATE_XSK_COMPLETE, and gve_free_pending_packet() doesn't
reset num_bufs. A completed XSK slot therefore keeps num_bufs == 1, along
with whatever dma[0] an earlier user of the slot left behind.

On the XDP TX queue, the same slot can first carry an XDP frame from
gve_xdp_xmit_one_dqo(). That fills dma[0] with a real mapping, which is
unmapped at completion. If the slot is then reused for XSK, a later
ifdown, reset, or XSK pool enable/disable (via gve_restart_rings())
reaches:

gve_queues_stop()
  gve_tx_stop_rings()
    gve_tx_stop_ring_dqo()
      gve_tx_clean_pending_packets()
        gve_unmap_packet()
          dma_unmap_single(dma[0])   <- already unmapped

Slots that were never used for anything else would instead unmap address
0 with length 0.

Under swiotlb or an IOMMU, could this release a bounce slot or IOVA that
now belongs to another mapping?

remove_miss_completions() has the same type-blind unmap right above its
new XSK case. Before this patch, an XSK miss completion crashed in
gve_handle_miss_completion(). Now the packet reaches
remove_miss_completions() after GVE_REINJECT_COMPL_TIMEOUT and hits the
stale unmap at runtime.

Would it be better to move the unmap into the SKB and XDP_FRAME cases,
the way gve_handle_packet_completion() already does?

> +		default:
> +			WARN_ON_ONCE(1);
> +			break;
>  		}
>  	}
> +
> +	if (tx->xsk_pool)
> +		gve_tx_process_xsk_completions(tx);
>  }

[Severity: Medium]
Can this drain ever run while XSK packets are pending?

gve_queues_stop() clears the XSK pool pointers before it stops the TX
rings:

gve_main.c:gve_queues_stop() {
	...
	gve_unreg_xdp_info(priv);

	gve_tx_stop_rings(priv, gve_num_tx_queues(priv));
	...
}

gve_unreg_xdp_info() calls gve_unreg_xsk_pool(), which does:

	priv->tx[gve_xdp_tx_queue_id(priv, qid)].xsk_pool = NULL;

So when gve_tx_stop_ring_dqo() calls gve_clean_tx_done_dqo() with a NULL
napi and then gve_tx_clean_pending_packets(), tx->xsk_pool is already
NULL and both drains are skipped. gve_tx_reset_ring_dqo() then zeroes
xsk_reorder_queue_head/tail and the reorder queue.

As a result, outstanding XSK descriptors are never passed to
xsk_tx_completed(). That includes descriptors the device completed during
the final gve_clean_tx_done_dqo() pass.

gve_queues_stop() is used by gve_close(), gve_restart_rings() and the RDA
XSK pool enable/disable paths. In those paths, AF_XDP sockets on other
queues stay bound. Won't their completion rings lose entries on every
stop/start that had TX in flight?

The remaining caller is the gve_queues_start() error path, which runs
before any XSK packets could be pending. So the new drain looks like it
can never run in practice.

[ ... ]

> @@ -1347,8 +1387,25 @@ static void gve_handle_miss_completion(struct gve_priv *priv,
>  			secs_to_jiffies(GVE_REINJECT_COMPL_TIMEOUT);
>  	add_to_list(tx, &tx->dqo_compl.miss_completions, pending_packet);
>  
> -	*bytes += pending_packet->skb->len;
> -	(*pkts)++;
> +	switch (pending_packet->type) {

[ ... ]

> +	case GVE_TX_PENDING_PACKET_DQO_XSK:
> +		break;

[Severity: Medium]
XSK packets now get past this point instead of crashing. Can they reach
the late reinjection branch of gve_handle_packet_completion(), which frees
the slot without checking its type?

After GVE_REINJECT_COMPL_TIMEOUT, remove_miss_completions() moves the XSK
packet to GVE_PACKET_STATE_TIMED_OUT_COMPL. Its completion tag stays in
tx->dqo.xsk_reorder_queue. If the reinjection completion then arrives
before GVE_DEALLOCATE_COMPL_TIMEOUT:

gve_handle_packet_completion() {
	...
		if (unlikely(pending_packet->state ==
			     GVE_PACKET_STATE_TIMED_OUT_COMPL)) {
			...
			remove_from_list(tx,
					 &tx->dqo_compl.timed_out_completions,
					 pending_packet);
			gve_free_pending_packet(tx, pending_packet);
			return;
		}
	...
}

The slot becomes GVE_PACKET_STATE_UNALLOCATED while the reorder queue
still points at it. gve_tx_process_xsk_completions() stops at the first
entry that isn't XSK_COMPLETE. Wouldn't every later XSK completion on this
queue then be held back?

This doesn't look like a double free, since gve_free_pending_packet() sets
the slot to UNALLOCATED. The queue only unblocks if that exact slot is
later reused for XSK and completes. Even then, a second reorder entry
points at the same slot, so one entry stays behind for good.

If the AF_XDP completion ring fills while the queue is blocked,
xsk_tx_peek_desc() stops handing out descriptors. TX on that queue could
then stall until the rings are restarted.

remove_timed_out_completions() already handles this case:

		if (pending_packet->type == GVE_TX_PENDING_PACKET_DQO_XSK)
			pending_packet->state = GVE_PACKET_STATE_XSK_COMPLETE;
		else
			gve_free_pending_packet(tx, pending_packet);

Should the late reinjection branch do the same?

> @@ -1377,9 +1434,26 @@ static void remove_miss_completions(struct gve_priv *priv,
>  		else
>  			gve_unmap_packet(tx->dev, pending_packet);
>  
> -		/* This indicates the packet was dropped. */
> -		dev_kfree_skb_any(pending_packet->skb);
> -		pending_packet->skb = NULL;
> +		switch (pending_packet->type) {
> +		case GVE_TX_PENDING_PACKET_DQO_SKB:
> +			if (pending_packet->skb) {
> +				/* This indicates the packet was dropped. */
> +				dev_kfree_skb_any(pending_packet->skb);
> +				pending_packet->skb = NULL;
> +			}
> +			break;
> +		case GVE_TX_PENDING_PACKET_DQO_XDP_FRAME:

[Severity: Low]
This isn't a bug, but the comments here no longer match the code. The
comment above the unmap in remove_miss_completions() still says:

		/* Unmap/free TX buffers and free skb but do not unallocate packet i.e.

The code now also returns XDP frames and skips XSK packets.

The "This indicates the packet was dropped" comment has also moved into
the SKB case only. The XDP_FRAME case is the same drop and also
increments tx->dropped_pkt.

Separately, the header comment above gve_tx_clean_pending_packets()
still names a function that doesn't exist:

/* gve_tx_free_desc - Cleans up all pending tx requests and buffers.
 */

That name predates this patch. Since the function is being rewritten
here anyway, could it be fixed as well?

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