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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] net/gve: GQI RDA missing ring state reset causes host-device desync
- [Critical] net/gve: Use-After-Free of XSK pool in RDA mode upon disable
- [High] net/gve: QPL TX stall due to missing allocation counters reset
--

commit 94bf684b19419e50950102b96994b969bc7787bd
Author: Joshua Washington <joshwash@google.com>
Subject: gve: ensure XDP mem model is registered when disabling XSK pools

This patch modifies how the GVE driver disables XSK pools to ensure the
memory model is registered correctly. In QPL mode, it registers the memory
model live. In RDA mode, it fully restarts the rings to handle potential
memory allocation failures safely.

> 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

[ ... ]

> @@ -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_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: Critical]
In the branch for RDA mode, gve_restart_rings() is called, but it appears
to omit unregistering the XSK pool, which was previously handled by
gve_unreg_xsk_pool().

Since gve_restart_rings() stops and starts the hardware queues reusing
the existing memory arrays, the tx->xsk_pool and rx->xsk_pool pointers
are never cleared to NULL. The pool memory is unmapped and subsequently
freed by the XDP framework.

When NAPI polling resumes on the restarted rings, could fast-path
functions like gve_clean_tx_done_dqo() check and dereference the stale
tx->xsk_pool pointer, leading to a use-after-free?

[Severity: Critical]
Also regarding this RDA mode restart path: when the rings are restarted,
the start path re-registers queues with the hardware, which resets the
device's expected descriptor pointers to 0.

However, looking at gve_tx_stop_ring_gqi(), it does not seem to reset
the software ring pointers (tx->req and tx->done) unlike the DQO path.

When transmission resumes, could the host use its stale absolute pointers
and ring the doorbell with a large offset, causing the device to
interpret this as a massive burst of packets and read uninitialized memory?

> diff --git a/drivers/net/ethernet/google/gve/gve_tx_dqo.c b/drivers/net/ethernet/google/gve/gve_tx_dqo.c
> --- a/drivers/net/ethernet/google/gve/gve_tx_dqo.c
> +++ b/drivers/net/ethernet/google/gve/gve_tx_dqo.c

[ ... ]

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

[Severity: High]
This function resets the standard TX fields, but it seems to miss
resetting the QPL allocation counters (tx->dqo_tx.alloc_tx_qpl_buf_cnt
and tx->dqo_tx.free_tx_qpl_buf_cnt).

Since gve_tx_init_ring_state_dqo() later sets
tx->dqo_compl.free_tx_qpl_buf_cnt to 0, will the available buffer
calculation upon restart use a stale, large alloc count against a zeroed
free count? This might make the queue incorrectly think no QPL buffers
are available, resulting in a TX stall.

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