[PATCH net 0/6] gve: various XDP fixes

[PATCH net 0/6] 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

Joshua Washington (6):
  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

 drivers/net/ethernet/google/gve/gve_main.c   | 51 ++++++++++----------
 drivers/net/ethernet/google/gve/gve_rx_dqo.c | 20 ++++++--
 2 files changed, 42 insertions(+), 29 deletions(-)

-- 
2.55.0.691.gc56d675ccc-goog

[PATCH net 1/6] 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>
---
 drivers/net/ethernet/google/gve/gve_rx_dqo.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/google/gve/gve_rx_dqo.c b/drivers/net/ethernet/google/gve/gve_rx_dqo.c
index 0ece2f6fdffb..db38bc645296 100644
--- a/drivers/net/ethernet/google/gve/gve_rx_dqo.c
+++ b/drivers/net/ethernet/google/gve/gve_rx_dqo.c
@@ -1145,13 +1145,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.
-- 
2.55.0.691.gc56d675ccc-goog

[PATCH net 2/6] 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 db38bc645296..2c1ce23d1550 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.691.gc56d675ccc-goog

[PATCH net 3/6] 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 2c1ce23d1550..811db42d05c6 100644
--- a/drivers/net/ethernet/google/gve/gve_rx_dqo.c
+++ b/drivers/net/ethernet/google/gve/gve_rx_dqo.c
@@ -845,7 +845,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.691.gc56d675ccc-goog

[PATCH net 4/6] 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 e4d78ae52daf..453b304016b6 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.691.gc56d675ccc-goog

[PATCH net 5/6] 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 453b304016b6..e084b367a92d 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.691.gc56d675ccc-goog

[PATCH net 6/6] 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 | 27 ++++++++++++----------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
index e084b367a92d..ccaf5f86a1e6 100644
--- a/drivers/net/ethernet/google/gve/gve_main.c
+++ b/drivers/net/ethernet/google/gve/gve_main.c
@@ -1681,28 +1681,24 @@ 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) {
 		err = gve_configure_rings_xdp(priv, priv->rx_cfg.num_queues);
 		if (err)
-			return err;
+			goto unmap_and_return;
 	}
 
 	napi_rx = &priv->ntfy_blocks[priv->rx[qid].ntfy_id].napi;
@@ -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.691.gc56d675ccc-goog

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

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Enabling XSK pools in QPL mode registers the memory model dynamically without disabling NAPI or restarting queues, leading to data races and kernel panics.
- [Critical] Disabling XSK pools in QPL mode fails to restart queues, causing in-flight XSK buffers to be incorrectly freed as normal shared pages.
- [High] `gve_xsk_pool_disable()` incorrectly unregisters the newly created `PAGE_POOL` memory model for RDA queues, destroying the active page pool and causing memory corruption/leaks.
--

--- Patch [4]: [PATCH net 4/6] gve: don't register xsk pool on pre-existing queues in RDA mode ---
commit 9bd2a484cf9beb93c90ecf2a602f481574145671
Author: Joshua Washington <joshwash@google.com>

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

This commit addresses a race condition in RDA mode when enabling XSK pools on
pre-existing queues without re-creating them. It ensures that the XSK pool is
only registered with current queues if XSK buffers are allocated on-the-fly,
as is the case in QPL mode.

> diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> --- 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: Critical]
This isn't a bug introduced by this patch, but does enabling XSK pools in QPL
mode lead to a race condition since it registers the memory model dynamically
without disabling NAPI or restarting queues?

If the NAPI softirq processes pre-existing standard buffers that were posted
before the pool was enabled, and the XDP program returns XDP_TX or
XDP_REDIRECT, gve_xdp_tx_dqo() will see rx->xsk_pool == true:

drivers/net/ethernet/google/gve/gve_rx_dqo.c:gve_xdp_tx_dqo() {
    ...
    if (unlikely(!xdpf)) {
        if (rx->xsk_pool)
            xsk_buff_free(xdp);
        return -ENOSPC;
    }
    ...
}

Will this cause xsk_buff_free() to incorrectly assume the stack-allocated
xdp_buff is a struct xdp_buff_xsk, leading to a kernel NULL pointer
dereference?

> +	} 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: Critical]
This is a pre-existing issue, but does disabling XSK pools in QPL mode fail to
restart queues, leaving in-flight XSK UMEM buffers to be processed with an
unregistered memory model?

In gve_xsk_pool_disable():

    napi_disable(napi_tx);
    gve_unreg_xsk_pool(priv, qid);
    smp_mb();
    napi_enable(napi_rx);

If the QPL queues are not restarted, previously allocated XSK buffers remain in
the hardware RX ring.

When these XSK buffers are processed by the resumed NAPI, will they be
initialized with the MEM_TYPE_PAGE_SHARED memory model instead of
MEM_TYPE_XSK_BUFF_POOL?

Could this lead to xdp_return_frame() calling page_frag_free() on an AF_XDP
UMEM address instead of returning it to the XSK pool, corrupting the page
allocator?

[Severity: High]
This is a pre-existing issue, but does gve_xsk_pool_disable() incorrectly
unregister the newly created PAGE_POOL memory model for RDA queues?

In gve_xsk_pool_disable():

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

For RDA mode, gve_configure_rings_xdp() properly initializes and registers the
new queue with MEM_TYPE_PAGE_POOL.

However, immediately after the restart, the driver unconditionally calls
gve_unreg_xsk_pool():

drivers/net/ethernet/google/gve/gve_main.c:gve_unreg_xsk_pool() {
    ...
    if (xdp_rxq_info_is_reg(&rx->xdp_rxq))
        xdp_rxq_info_unreg_mem_model(&rx->xdp_rxq);
    ...
}

Will this actively invoke page_pool_destroy() on the newly created page pool
and reset the memory type to MEM_TYPE_PAGE_SHARED?

If so, could packets from this queue bypass page_pool_put_full_netmem() on
free, causing memory leaks and a use-after-free since the page pool was marked
for destruction?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260814234845.773189-1-joshwash@google.com?part=4

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

[sashiko-bot]

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

Pre-existing issues:
- [High] XSK buffers are permanently leaked during runtime when the received packet descriptor indicates an rx_error.
- [High] Calling `dev_consume_skb_any()` on an XDP frame and incorrectly unmapping DMA for AF_XDP zero-copy buffers during pending packet cleanup.
- [Medium] AF_XDP (XSK) TX buffers are permanently leaked from the user-space application when the TX queue is stopped.
--

--- Patch [2]: [PATCH net 2/6] gve: fix XSK buffer leak when rings are stopped ---
commit 68d9940f45b02ca459717266e19904c11302ff2d
Author: Joshua Washington <joshwash@google.com>

gve: fix XSK buffer leak when rings are stopped
    
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.

> diff --git a/drivers/net/ethernet/google/gve/gve_rx_dqo.c b/drivers/net/ethernet/google/gve/gve_rx_dqo.c
> index db38bc6452969..2c1ce23d15508 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;
> +			}

[Severity: High]
This is a pre-existing issue, but does this cleanup loop miss leaked XSK
buffers if they were previously freed due to an rx_error?

When a packet is received with compl_desc->rx_error set (such as a CRC error),
gve_rx_dqo() calls gve_free_buffer() before checking for the XSK buffer:

drivers/net/ethernet/google/gve/gve_rx_dqo.c:gve_rx_dqo() {
    ...
    if (unlikely(compl_desc->rx_error)) {
        gve_free_buffer(rx, buf_state);
        return -EINVAL;
    }
    ...
}

gve_free_buffer() returns the buffer state to the free list without calling
xsk_buff_free(), leaving bs->xsk_buff still set. 

Since this cleanup loop checks gve_buf_state_is_allocated(), which evaluates
to false for buffers in the free list, will it completely skip these leaked
buffers?

>  		}
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but while looking at queue stop
cleanups, is there a type confusion issue when cleaning up pending TX packets?

During interface down or MTU changes, gve_tx_clean_pending_packets() and
remove_miss_completions() iterate over pending TX packets and unconditionally
treat them as SKBs:

drivers/net/ethernet/google/gve/gve_tx_dqo.c:gve_tx_clean_pending_packets() {
    ...
    gve_unmap_packet(tx->dev, cur_state);

    if (cur_state->skb) {
        dev_consume_skb_any(cur_state->skb);
        cur_state->skb = NULL;
    }
    ...
}

struct gve_tx_pending_packet_dqo uses a union for skb and xdpf. If the packet
is an XDP frame, cur_state->skb will evaluate to true, passing the XDP frame
to dev_consume_skb_any(). Can this cause a crash?

Additionally, for AF_XDP (XSK) packets, gve_xsk_tx_dqo() does not initialize
dma[0] because XSK uses pre-mapped DMA. Will the unconditional call to
gve_unmap_packet() pass uninitialized or stale dma[0] values to
dma_unmap_single() and corrupt DMA mappings?

[Severity: Medium]
This is a pre-existing issue, but do AF_XDP TX buffers get permanently leaked
from the userspace application when the TX ring is stopped?

The gve_tx_clean_pending_packets() loop traverses and discards all pending
packets:

drivers/net/ethernet/google/gve/gve_tx_dqo.c:gve_tx_clean_pending_packets() {
    ...
    for (i = 0; i < tx->dqo.num_pending_packets; i++) {
        struct gve_tx_pending_packet_dqo *cur_state =
            &tx->dqo.pending_packets[i];
    ...
}

Unlike the normal completion path in gve_tx_process_xsk_completions(), this
cleanup path doesn't appear to call xsk_tx_completed() for XSK packets before
dropping them. 

Since the pending packets array is zeroed and freed shortly after, are these
buffers lost permanently from the perspective of the AF_XDP socket?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260814234845.773189-1-joshwash@google.com?part=2

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

[sashiko-bot]

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

Pre-existing issues:
- [High] gve_xsk_pool_enable modifies XDP memory model and xsk_pool pointers without disabling NAPI for QPL queues, leading to data races.
- [High] gve_xsk_pool_disable unconditionally unregisters the XDP RXQ memory model but fails to register a fallback, leaving the active RX queue without a valid memory model.
--

--- Patch [5]: [PATCH net 5/6] gve: fix napi_disable deadlock when attempting to disable XSK pools ---
commit 0d8b6f4e24c72240011374bd526d4eaa388afe39
Author: Joshua Washington <joshwash@google.com>

gve: fix napi_disable deadlock when attempting to disable XSK pools

This commit updates the unlocked napi_disable and napi_enable calls
in GVE's XSK pool disablement path to use the locked versions, preventing
a deadlock where the thread attempts to re-acquire the netdev lock.

> diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> index 453b304016b6f..e084b367a92dd 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);

[Severity: High]
This is a pre-existing issue, but does this unconditional call to
gve_unreg_xsk_pool() leave the active RX queue without a valid memory model?

When an AF_XDP socket is closed, gve_xsk_pool_disable() is called. For RDA
queues, it first re-creates the rings, which registers MEM_TYPE_PAGE_POOL.
Then it unconditionally calls gve_unreg_xsk_pool(), which unregisters the
memory model by setting it to 0 without registering a fallback.

If the memory model type is left as MEM_TYPE_PAGE_ORDER0 (0), when XDP
drops or redirects packets, __xdp_return() will call put_page() instead of
page_pool_put_full_page() or page_frag_free(). Could this bypass the page
pool recycling mechanism and leak pages from the pool?

>  	smp_mb(); /* Make sure it is visible to the workers on datapath */

While reviewing the XSK pool changes, I noticed another pre-existing
issue in the related enable path:

[Severity: High]
This isn't a bug introduced by this patch, but does gve_xsk_pool_enable()
modify the XDP memory model locklessly for QPL queues without disabling NAPI?

When an XSK pool is enabled on a QPL queue, gve_xsk_pool_enable() calls
gve_reg_xsk_pool() directly without disabling NAPI:

gve_xsk_pool_enable() {
        ...
        err = gve_reg_xsk_pool(priv, dev, pool, qid);
        ...
}

gve_reg_xsk_pool() {
        ...
        err = xdp_rxq_info_reg_mem_model(&rx->xdp_rxq,
                                         MEM_TYPE_XSK_BUFF_POOL, pool);
        ...
}

Could this concurrent modification of the rx->xdp_rxq.mem.type and
rx->xsk_pool pointers while NAPI is actively running lead to data races or
memory corruption in the SoftIRQ context (such as inside xdp_return_frame
or gve_xsk_tx_poll)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260814234845.773189-1-joshwash@google.com?part=5

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

[sashiko-bot]

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

Pre-existing issues:
- [High] Kernel panic via NULL pointer dereference on XSK pool disable error path.
--

--- Patch [6]: [PATCH net 6/6] gve: fix NULL dereference from premature XSK pool DMA unmap ---
commit 70486a861c2e62f343665b0b44d80a866b2cbd87
Author: Joshua Washington <joshwash@google.com>

gve: fix NULL dereference from premature XSK pool DMA unmap

This commit defers the XSK pool DMA unmapping in gve_xsk_pool_disable()
until after the NAPI rings are disabled, aiming to prevent a race
condition that causes a NULL pointer dereference.

> diff --git a/drivers/net/ethernet/google/gve/gve_main.c b/drivers/net/ethernet/google/gve/gve_main.c
> --- a/drivers/net/ethernet/google/gve/gve_main.c
> +++ b/drivers/net/ethernet/google/gve/gve_main.c
[ ... ]
> @@ -1681,28 +1681,24 @@ static int gve_xsk_pool_disable(struct net_device *dev,
>  	/* 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;
> +			goto unmap_and_return;

[Severity: High]
This is a pre-existing issue, but if gve_configure_rings_xdp() fails
(for example, due to a memory allocation failure), doesn't this goto bypass
the calls to napi_disable_locked() and gve_unreg_xsk_pool()?

[ ... ]
> -	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;
>  }

[Severity: High]
If we reached this label via the gve_configure_rings_xdp() error path,
is it safe to call xsk_pool_dma_unmap() here?

Since the NAPI polling wasn't disabled and the active hardware ring's
xsk_pool pointer wasn't cleared, couldn't the still-active NAPI thread
dereference the unmapped pool->dev (which xsk_pool_dma_unmap() sets to
NULL) on the next packet, resulting in the exact same kernel panic this
patch is trying to prevent?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260814234845.773189-1-joshwash@google.com?part=6

Re: [PATCH net 3/6] gve: fix XSK buffer leak on error descriptor

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Fragment desynchronization resulting in truncated or malformed packets being incorrectly pushed to the network stack upon intermediate descriptor errors.
- [High] Massive unmitigated leak of AF_XDP zero-copy buffers (`xsk_buff`) and XDP frames (`xdpf`) on XDP_TX and XDP_REDIRECT paths.
- [High] `gve_rx_xsk_dqo` unconditionally overwrites `rx->ctx.skb_head` on multi-fragment XDP sequences, causing leaks.
- [High] The driver inherently trusts the `packet_len` provided by the device DMA completion descriptor without verifying it against the allocated buffer size.
- [High] Missing bounds check on `compl_desc->header_len` allows out-of-bounds reads from the header buffer array.
--

--- Patch [3]: [PATCH net 3/6] gve: fix XSK buffer leak on error descriptor ---
commit 92aa7b2aab7b9d9aa0aed8717f991d9e6e5c13b2
Author: Joshua Washington <joshwash@google.com>

gve: fix XSK buffer leak on error descriptor

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

> diff --git a/drivers/net/ethernet/google/gve/gve_rx_dqo.c b/drivers/net/ethernet/google/gve/gve_rx_dqo.c
> index 2c1ce23d15508..811db42d05c67 100644
> --- a/drivers/net/ethernet/google/gve/gve_rx_dqo.c
> +++ b/drivers/net/ethernet/google/gve/gve_rx_dqo.c
> @@ -845,7 +845,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;
>  	}

[Severity: High]
This is a pre-existing issue, but does the driver verify the length fields
provided by the device DMA completion descriptor?

In gve_rx_dqo(), the driver reads packet_len and header_len directly from
the untrusted descriptor:

        buf_len = compl_desc->packet_len;
        hdr_len = compl_desc->header_len;

It then uses hdr_len to copy data:

        rx->ctx.skb_head =
                gve_rx_copy_data(priv->dev, napi,
                                 rx->dqo.hdr_bufs.data + offset,
                                 hdr_len);

Could a buggy or malicious device supply a large header length (up to 1023
bytes), causing out-of-bounds memory reads from the header buffer array?
Similarly, could an unchecked packet_len result in out-of-bounds memory
reads being copied into SKBs and pushed to the network stack?

[Severity: High]
This isn't a bug introduced by this patch, but are there other paths where
AF_XDP zero-copy buffers or XDP frames can leak?

In drivers/net/ethernet/google/gve/gve_rx_dqo.c:gve_xsk_done_dqo(), the
XDP_REDIRECT failure path appears to fall through without freeing the
original xsk_buff:

        case XDP_REDIRECT:
                if (unlikely(xdp_do_redirect(priv->dev, xdp, xprog)))
                        goto err;

Also, in drivers/net/ethernet/google/gve/gve_tx_dqo.c:gve_xdp_xmit_one_dqo(),
the error path nullifies the packet frame without calling xdp_return_frame():

err:
        pkt->xdpf = NULL;
        pkt->num_bufs = 0;
        gve_free_pending_packet(tx, pkt);
        return err;

Does this lead to resource exhaustion if XDP_TX or XDP_REDIRECT actions fail?

[Severity: Critical]
This is a pre-existing issue, but does the RX poll loop correctly track
dropped packets across multiple descriptors?

In gve_rx_poll_dqo(), an error causes the skb to be freed:

        if (err < 0) {
                gve_rx_free_skb(napi, rx);

However, if this was the first or intermediate descriptor of a multi-fragment
packet, there doesn't seem to be a flag (like rx->ctx.drop_pkt) to ignore
the remaining descriptors. As a result, when the next fragment is processed in
gve_rx_dqo(), a new skb is allocated:

        rx->ctx.skb_head = napi_get_frags(napi);

Could this fragment desynchronization allow subsequent fragments of the same
packet to be parsed as the start of an entirely new packet?

[Severity: High]
This isn't a bug introduced by this patch, but does gve_rx_xsk_dqo() leak
skbs on multi-fragment XDP sequences?

If a multi-fragment packet arrives while AF_XDP zero-copy is active,
gve_rx_xsk_dqo() unconditionally overwrites rx->ctx.skb_head:

        /* Copy the data to skb */
        rx->ctx.skb_head = gve_rx_copy_data(priv->dev, napi,
                                            xdp->data, buf_len);

Could this unconditionally overwrite the globally stored skb_head and
skb_tail without appending to or freeing the existing partial SKB,
causing a permanent leak of SKBs allocated for prior fragments?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260814234845.773189-1-joshwash@google.com?part=3

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

[sashiko-bot]

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

New issues:
- [Medium] The patch inadvertently corrupts `rx->rpackets` statistics by counting descriptor errors, allocation failures, and XDP drops as successfully received packets, and creates a mismatch with `rx->rbytes`.

Pre-existing issues:
- [Critical] Missing state tracking for dropped multi-descriptor packets in the DQO RX path causes subsequent fragments to be treated as new packets, leading to payload corruption, bypass of XDP drop rules, and unsafe XDP execution on partial data.
--

--- Patch [1]: [PATCH net 1/6] gve: increment work_done for XDP and error packets ---
commit 705a50c36a672a5d7810174d670e9330d7aada5f
Author: Joshua Washington <joshwash@google.com>

gve: increment work_done for XDP and error packets

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

Cc: stable@vger.kernel.org

> diff --git a/drivers/net/ethernet/google/gve/gve_rx_dqo.c b/drivers/net/ethernet/google/gve/gve_rx_dqo.c
> index 0ece2f6fdffb4..db38bc6452969 100644
> --- a/drivers/net/ethernet/google/gve/gve_rx_dqo.c
> +++ b/drivers/net/ethernet/google/gve/gve_rx_dqo.c
> @@ -1145,13 +1145,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;

[Severity: Critical]
This is a pre-existing issue, but does the polling loop lack state tracking
for dropped multi-descriptor packets?

If a multi-fragment packet is dropped (for example, by an XDP rule
in gve_rx_dqo()), rx->ctx.skb_head remains NULL. When the loop handles the
next fragment of that same packet, it skips to it because of the
end_of_packet check.

Since skb_head is NULL, gve_rx_dqo() will treat the middle fragment as a
new packet:

drivers/net/ethernet/google/gve/gve_rx_dqo.c:gve_rx_dqo() {
        ...
        /* Append to current skb if one exists. */
        if (rx->ctx.skb_head) {
        ...
}

Can this lead to XDP programs running on partial data and malicious inner
payloads bypassing XDP drop rules?

>  		work_done++;
> +
> +		if (!rx->ctx.skb_head)
> +			continue;
> +

[Severity: Medium]
Does this change inadvertently increment the interface packet statistics
for dropped packets?

By moving the work_done increment before the skb_head check, work_done
now correctly accounts for dropped packets, XDP drops, and allocation
failures in the NAPI budget.

However, work_done is later used to update rx->rpackets at the end of
gve_rx_poll_dqo():

        u64_stats_update_begin(&rx->statss);
        rx->rpackets += work_done;
        rx->rbytes += bytes;
        u64_stats_update_end(&rx->statss);

Will this cause dropped packets to be incorrectly counted as successfully
received packets, causing the rpackets stat to diverge from rbytes?

>  		pkt_bytes = rx->ctx.skb_head->len;
>  		/* The ethernet header (first ETH_HLEN bytes) is snipped off

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260814234845.773189-1-joshwash@google.com?part=1