Re: [PATCH] net: stmmac: prevent kernel panic during XDP program and XSK pool transitions

[sashiko-bot@kernel.org]

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

New issues:
- [High] Unconditional `netif_tx_wake_all_queues()` on error paths forces early transmit, leading to NULL pointer dereferences.
- [High] Global TX queue operations during single-queue XSK pool reconfiguration destroy flow control state for unaffected queues.
- [Medium] The added synchronization logic in `stmmac_xdp_set_prog()` is ineffective, redundant, and based on a flawed premise.

Pre-existing issues:
- [High] Missing error handling for `stmmac_enable_tx_queue` and `stmmac_enable_rx_queue` allocations leads to crashes during XSK pool setup.
--

commit 5f08fcd5650e1c25484ffcb83c996e73d9df4bd4
Author: Carlos Fangmeier <carlos.fangmeier@gmail.com>
Subject: net: stmmac: prevent kernel panic during XDP program and XSK pool transitions

This commit attempts to prevent races with pending TX work during XDP and
XSK pool reconfigurations by wrapping the operations with netif_tx_disable()
and netif_tx_wake_all_queues().

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c
> index d7e4db7224b0c..a6611aee687fd 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c
> @@ -34,6 +34,7 @@ static int stmmac_xdp_enable_pool(struct stmmac_priv *priv,
>  	need_update = netif_running(priv->dev) && stmmac_xdp_is_enabled(priv);
>  
>  	if (need_update) {
> +		netif_tx_disable(priv->dev);
>  		napi_disable(&ch->rx_napi);
>  		napi_disable(&ch->tx_napi);
>  		stmmac_disable_rx_queue(priv, queue);
> @@ -46,6 +47,7 @@ static int stmmac_xdp_enable_pool(struct stmmac_priv *priv,
>  		stmmac_enable_rx_queue(priv, queue);
>  		stmmac_enable_tx_queue(priv, queue);
>  		napi_enable(&ch->rxtx_napi);

[Severity: High]
This isn't a bug introduced by this patch, but does the lack of error returns
from stmmac_enable_tx_queue() and stmmac_enable_rx_queue() risk kernel panics
if descriptor allocations fail?

Currently, if __alloc_dma_tx_desc_resources() fails inside
stmmac_enable_tx_queue(), it prints an error and returns early without
initializing the rings.

Since the function returns void, stmmac_xdp_enable_pool() blindly proceeds
to call napi_enable(), which could cause a crash when NAPI or the TX path
dereferences uninitialized pointers.

> +		netif_tx_wake_all_queues(priv->dev);

[Severity: High]
Does calling netif_tx_wake_all_queues() here unintentionally clear the flow
control state for other unaffected transmit queues?

Since stmmac_xdp_enable_pool() only reconfigures a single DMA queue, waking
all queues will clear the off state for every queue on the netdev.

If another queue was intentionally stopped due to actual backpressure because
its DMA ring was full during stmmac_xmit(), the network stack might
erroneously resume transmitting on that full queue, leading to dropped packets.

[ ... ]

> @@ -72,6 +74,7 @@ static int stmmac_xdp_disable_pool(struct stmmac_priv *priv, u16 queue)
>  	need_update = netif_running(priv->dev) && stmmac_xdp_is_enabled(priv);
>  
>  	if (need_update) {
> +		netif_tx_disable(priv->dev);
>  		napi_disable(&ch->rxtx_napi);
>  		stmmac_disable_rx_queue(priv, queue);
>  		stmmac_disable_tx_queue(priv, queue);
> @@ -87,6 +90,7 @@ static int stmmac_xdp_disable_pool(struct stmmac_priv *priv, u16 queue)
>  		stmmac_enable_tx_queue(priv, queue);
>  		napi_enable(&ch->rx_napi);
>  		napi_enable(&ch->tx_napi);
> +		netif_tx_wake_all_queues(priv->dev);
>  	}

[Severity: High]
Does this also suffer from the same issue as stmmac_xdp_enable_pool(), where
a global queue wake overrides the flow control state for all other unaffected
TX queues?

[ ... ]

> @@ -121,8 +125,10 @@ int stmmac_xdp_set_prog(struct stmmac_priv *priv, struct bpf_prog *prog,
>  		xdp_features_clear_redirect_target(dev);
>  
>  	need_update = !!priv->xdp_prog != !!prog;
> -	if (if_running && need_update)
> +	if (if_running && need_update) {
> +		netif_tx_disable(dev);
>  		stmmac_xdp_release(dev);
> +	}

[Severity: Medium]
Is this netif_tx_disable() redundant, and does it actually prevent the race
during transitions?

Looking at stmmac_xdp_release(), it already calls netif_tx_disable()
internally.

Furthermore, on the success path, stmmac_xdp_open() calls
netif_tx_start_all_queues() before returning, meaning TX is re-enabled early
regardless of the wrapper added here. It appears the perceived race window
might remain identical.

>  	old_prog = xchg(&priv->xdp_prog, prog);
>  	if (old_prog)
> @@ -131,8 +137,10 @@ int stmmac_xdp_set_prog(struct stmmac_priv *priv, struct bpf_prog *prog,
>  	/* Disable RX SPH for XDP operation */
>  	priv->sph_active = priv->sph_capable && !stmmac_xdp_is_enabled(priv);
>  
> -	if (if_running && need_update)
> +	if (if_running && need_update) {
>  		stmmac_xdp_open(dev);
> +		netif_tx_wake_all_queues(dev);
> +	}

[Severity: High]
Could this netif_tx_wake_all_queues() force an early transmit and lead to a
panic if stmmac_xdp_open() fails?

If stmmac_xdp_open() returns an error, for example due to DMA descriptor
allocation failure, the DMA rings are not initialized.

Because this code unconditionally wakes all TX queues regardless of success,
the network stack will immediately resume transmission, and stmmac_xmit()
may dereference freed or uninitialized rings.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260605-main-v1-1-aed15b1cf1af@gmail.com?part=1