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

[sashiko-bot@kernel.org]

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