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

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 <[email protected]>

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/[email protected]?part=5