Re: [PATCH net v3 05/10] net: stmmac: unwind partially allocated DMA configurations

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 1 · Low: 0 (2)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] Possible double free of a saved RX skb.
- [Medium] Baseline decision: discard, because it is resolved later in
  the series.

Pre-existing issues:
- [Low] Baseline decision: discard, because it is resolved later in the
  series.
- [Low] Baseline decision: discard, because it predates this patch and is
  resolved later in the series.

--

net: stmmac: unwind partially allocated DMA configurations

This commit lets the RX and TX DMA free paths handle partially allocated
queues, and releases RX resources when TX allocation fails. It propagates
xdp_rxq_info_reg_mem_model() errors and clears XSK RXQ bindings. It also
frees any saved partial RX skb and NULLs the released resource pointers.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index f7e91ae214483..3b918cf898067 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c

[ ... ]

> @@ -1884,17 +1887,19 @@ static int __init_dma_rx_desc_rings(struct stmmac_priv *priv,
>  	rx_q->xsk_pool = stmmac_get_xsk_pool(priv, queue);
>  
>  	if (rx_q->xsk_pool) {
> -		WARN_ON(xdp_rxq_info_reg_mem_model(&rx_q->xdp_rxq,
> -						   MEM_TYPE_XSK_BUFF_POOL,
> -						   NULL));
> +		ret = xdp_rxq_info_reg_mem_model(&rx_q->xdp_rxq,
> +						 MEM_TYPE_XSK_BUFF_POOL, NULL);
> +		if (ret)
> +			return ret;
>  		netdev_info(priv->dev,
>  			    "Register MEM_TYPE_XSK_BUFF_POOL RxQ-%d\n",
>  			    queue);
>  		xsk_pool_set_rxq_info(rx_q->xsk_pool, &rx_q->xdp_rxq);

[Severity: Low]
This problem already existed before this patch, but is the XSK pool bound
here to an xdp_rxq that is about to be freed?

On the stmmac_open() path, __init_dma_rx_desc_rings() works on the
temporary dma_conf from stmmac_setup_dma_desc(). __stmmac_open() then
copies that conf:

	memcpy(&priv->dma_conf, dma_conf, sizeof(*dma_conf));

After that, stmmac_open() calls kfree(dma_conf). Nothing rebinds the pool
to &priv->dma_conf.rx_queue[queue].xdp_rxq, so the pool's rxq pointers
seem to point into freed memory.

The next patch in the series, "net: stmmac: keep DMA configurations at
stable addresses", appears to fix this. It keeps the allocated dma_conf
and removes the memcpy.

>  	} else {
> -		WARN_ON(xdp_rxq_info_reg_mem_model(&rx_q->xdp_rxq,
> -						   MEM_TYPE_PAGE_POOL,
> -						   rx_q->page_pool));
> +		ret = xdp_rxq_info_reg_mem_model(&rx_q->xdp_rxq,
> +						 MEM_TYPE_PAGE_POOL, rx_q->page_pool);
> +		if (ret)
> +			return ret;

[Severity: Medium]
Now that this error is returned, how do the per-queue callers handle it?

stmmac_enable_rx_queue() returns void. When this call fails, it releases
the queue and returns:

	ret = __init_dma_rx_desc_rings(priv, &priv->dma_conf, queue, GFP_KERNEL);
	if (ret) {
		__free_dma_rx_desc_resources(priv, &priv->dma_conf, queue);
		netdev_err(priv->dev, "Failed to init RX desc.\n");
		return;
	}

With this patch, __free_dma_rx_desc_resources() also sets buf_pool,
dma_rx, dma_erx and page_pool to NULL. After that,
stmmac_xdp_enable_pool() and stmmac_xdp_disable_pool() in stmmac_xdp.c
carry on as usual:

		stmmac_enable_rx_queue(priv, queue);
		stmmac_enable_tx_queue(priv, queue);
		napi_enable(&ch->rx_napi);
		napi_enable(&ch->tx_napi);

Both return 0, so NAPI can run on a released RX queue whose ring pointers
are NULL. stmmac_xdp_set_prog() also ignores the return value of
stmmac_xdp_open().

These callers already behaved this way for the -ENOMEM return from
stmmac_alloc_rx_buffers(). This patch adds a second way to reach it.

Two later patches in the series appear to fix this: "net: stmmac: account
for failed live XDP reopening" and "net: stmmac: use the tracked datapath
restart for XSK pool changes". They check the stmmac_xdp_open() result
and remove the per-queue enable helpers.

[ ... ]

> @@ -2137,10 +2147,16 @@ static void __free_dma_rx_desc_resources(struct stmmac_priv *priv,
>  	void *addr;
>  
>  	/* Release the DMA RX socket buffers */
> -	if (rx_q->xsk_pool)
> +	if (rx_q->xsk_pool) {
>  		dma_free_rx_xskbufs(priv, dma_conf, queue);
> -	else
> +		xsk_pool_set_rxq_info(rx_q->xsk_pool, NULL);
> +	} else {
>  		dma_free_rx_skbufs(priv, dma_conf, queue);
> +	}
> +	if (rx_q->state_saved)
> +		dev_kfree_skb_any(rx_q->state.skb);
> +	rx_q->state.skb = NULL;
> +	rx_q->state_saved = 0;

[Severity: High]
Can this free an skb that the driver no longer owns?

This free assumes that state_saved means the driver still owns
rx_q->state.skb. However, stmmac_rx() does not clear the saved state when
it takes it:

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_rx() {
    ...
		if (!count && rx_q->state_saved) {
			skb = rx_q->state.skb;
			error = rx_q->state.error;
			len = rx_q->state.len;
		} else {
			rx_q->state_saved = false;
    ...
}

state_saved is only cleared in the else branch, which runs on a later
iteration with count > 0. With limit == 1, the first iteration can finish
the restored skb. It can pass it to napi_gro_receive(), or free it on the
error path:

		if (unlikely(error)) {
			dev_kfree_skb(skb);
			skb = NULL;
			count++;
			continue;
		}

count is now 1, so the while (count < limit) loop exits. The save block
after the loop is skipped, because skb is NULL and the last descriptor
did not have rx_not_ls set:

	if (status & rx_not_ls || skb) {
		rx_q->state_saved = true;
		rx_q->state.skb = skb;
		...
	}

state_saved stays set, and state.skb points to an skb that was freed or
handed to the stack. A later poll that hits a DMA-owned descriptor right
away breaks before using the skb and saves the same stale pointer again,
so the stale pointer can survive across polls.

No privileges are needed to get a budget of 1. The EPIOCSPARAMS ioctl in
fs/eventpoll.c only requires CAP_NET_ADMIN when busy_poll_budget >
NAPI_POLL_WEIGHT, and stmmac_napi_poll_rx() passes the budget straight to
stmmac_rx(). A frame that spans several descriptors needs a jumbo MTU.

Suppose the interface is then closed (ndo_stop -> free_dma_desc_resources)
or the XSK pool or XDP program is changed. __free_dma_rx_desc_resources()
then calls dev_kfree_skb_any() on the stale pointer. Before this patch,
teardown dropped the stale pointer without freeing it.

The stale state in stmmac_rx() is older than this patch, and so is its
reuse on the next poll. This patch adds a new path that frees it a second
time. A later patch in the series copies the same pattern into
stmmac_reinit_dma_desc(), and stmmac_rx() is still unchanged at the end
of the series.

Could stmmac_rx() clear rx_q->state_saved and rx_q->state.skb as soon as
it takes the saved state?

[ ... ]

> @@ -2411,6 +2438,8 @@ static int alloc_dma_desc_resources(struct stmmac_priv *priv,
>  		return ret;
>  
>  	ret = alloc_dma_tx_desc_resources(priv, dma_conf);
> +	if (ret)
> +		free_dma_rx_desc_resources(priv, dma_conf);
>  
>  	return ret;
>  }

[Severity: Low]
This one also predates the patch: stmmac_enable_rx_queue() and
stmmac_enable_tx_queue() do not unwind in the same way. When
__alloc_dma_rx_desc_resources() fails:

	ret = __alloc_dma_rx_desc_resources(priv, &priv->dma_conf, queue);
	if (ret) {
		netdev_err(priv->dev, "Failed to alloc RX desc.\n");
		return;
	}

Does this leak the page_pool, buf_pool and descriptor memory that were
already allocated? stmmac_enable_tx_queue() has the same early return,
which would leak tx_skbuff_dma, tx_skbuff and the TX descriptor memory.

"net: stmmac: use the tracked datapath restart for XSK pool changes",
later in the series, removes both helpers. After that, pool changes go
through stmmac_xdp_open() and alloc_dma_desc_resources().

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260924-submit-stmmac-reset-fixes-v1-v3-0-c031e3f3a282%40gmail.com