Re: [PATCH net-next v6 RESEND 2/4] net: stmmac: clean up partially allocated DMA rings on error

[netdev-bot+sashiko@kernel.org]

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

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

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).

- [Medium] The commit message misdescribes the patch, and a real leak fix
  ships without a Fixes: tag.

Pre-existing issues:
- [High] stmmac_enable_rx_queue() and stmmac_enable_tx_queue() return
  void.
- [Low] The kernel-doc for __alloc_dma_rx_desc_resources() says it
  'allocates the resources for TX and RX paths' and 'pre-allocated the…

--

net: stmmac: clean up partially allocated DMA rings on error

This commit adds per-queue error unwinding to
__alloc_dma_rx_desc_resources() and __alloc_dma_tx_desc_resources(), so
each function frees what it allocated before it returns an error. It also
makes alloc_dma_desc_resources() free the RX resources when the TX
allocation fails.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index fcc80ece37679..b02371ed2d062 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c

[ ... ]

> @@ -2361,10 +2365,22 @@ static int __alloc_dma_rx_desc_resources(struct stmmac_priv *priv,
>  	ret = xdp_rxq_info_reg(&rx_q->xdp_rxq, priv->dev, queue, napi_id);
>  	if (ret) {
>  		netdev_err(priv->dev, "Failed to register xdp rxq info\n");
> -		return -EINVAL;
> +		goto err_free_dma;
>  	}
>  
>  	return 0;
> +
> +err_free_dma:
> +	dma_free_coherent(priv->device, size, addr, rx_q->dma_rx_phy);
> +	rx_q->dma_erx = NULL;
> +	rx_q->dma_rx = NULL;

[Severity: High]

This is a pre-existing issue, but this path now leaves rx_q->dma_rx and
rx_q->dma_erx NULL on failure. Can stmmac_xdp_enable_pool() then run the
zero-copy RX path on a queue that has no descriptor ring?

stmmac_enable_rx_queue() returns void and only logs the failure:

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

stmmac_xdp_enable_pool() has just torn the queue down with
stmmac_disable_rx_queue(). It then continues without checking anything:

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

		err = stmmac_xsk_wakeup(priv->dev, queue, XDP_WAKEUP_RX);

Suppose the RX re-allocation fails under memory pressure (in
page_pool_create(), kzalloc_objs() or dma_alloc_coherent()) but the TX side
succeeds. Then tx_q->xsk_pool is set, so stmmac_xsk_wakeup() schedules
rxtx_napi, and we get:

stmmac_napi_poll_rxtx()
  stmmac_rx_zc()
    p = stmmac_get_rx_desc(priv, rx_q, entry);   /* &NULL[entry] */
    status = stmmac_rx_status(priv, &priv->xstats, p);

Would that be a NULL pointer dereference in softirq context?

stmmac_xdp_disable_pool() has the same pattern: it calls the enable helpers
without checking the result, then calls napi_enable() on rx_napi and
tx_napi.

The allocation failures, which are the realistic triggers, already left
NULL rings before this patch. The only change here is the
xdp_rxq_info_reg() failure case, which used to leave a zeroed but allocated
ring and now leaves NULL pointers.

> +err_free_buf_pool:
> +	kfree(rx_q->buf_pool);
> +	rx_q->buf_pool = NULL;
> +err_destroy_pool:
> +	page_pool_destroy(rx_q->page_pool);
> +	rx_q->page_pool = NULL;
> +	return ret;
>  }

[Severity: Low]

This isn't a bug introduced by this patch, but the kernel-doc above
__alloc_dma_rx_desc_resources() still says:

 * this function allocates the resources for TX and RX paths. In case of
 * reception, for example, it pre-allocated the RX socket buffer in order to
 * allow zero-copy mechanism.

This per-queue helper only handles RX. It also does not allocate RX
buffers; that happens later, in __init_dma_rx_desc_rings().
alloc_dma_desc_resources() has the same comment.

This patch reworks the error handling in both functions. Could these
comments be updated at the same time?

[ ... ]

> @@ -2476,6 +2500,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);

[Severity: Medium]

This hunk looks like the only part of the patch that fixes a real leak.
Before this patch, if alloc_dma_tx_desc_resources() failed here, every RX
queue kept its page_pool, buf_pool, coherent descriptor ring and xdp_rxq
registration. stmmac_setup_dma_desc() then only did:

alloc_error:
	kfree(dma_conf);
	return ERR_PTR(ret);

This structure seems to date back to commit 71fedb0198cbbd ("stmmac: break
some functions into RX and TX scopes"). Should this hunk have a Fixes: tag?
It could also go in its own patch so stable can pick it up.

The commit message says the outer error paths are "relying on the NULL
guards added previously". That reads as if the per-queue unwinding removes
the dependency. Does it?

alloc_dma_rx_desc_resources() still does:

err_dma:
	free_dma_rx_desc_resources(priv, dma_conf);

That loop walks all rx_queues_to_use queues. It includes the failed queue,
which is now already cleaned up, and any later queues that were never
allocated. alloc_dma_tx_desc_resources() works the same way. So the free
helpers still depend on checks such as !rx_q->buf_pool in
dma_free_rx_skbufs().

Also, before this patch those err_dma labels already freed whatever a
failed queue had partly allocated. The per-queue unwinding therefore
doesn't appear to fix a leak on the open, MTU change or ringparam paths.

The only callers whose behaviour changes are stmmac_enable_rx_queue() and
stmmac_enable_tx_queue(). On those paths the leftovers stayed in
priv->dma_conf until the next disable or release, so nothing leaked.

The commit message also says "the callers only free dma_conf itself on
that path". stmmac_xdp_open() frees nothing on that path:

dma_desc_error:
	return ret;

Could the commit message be reworded to describe what the patch actually
fixes?

>  
>  	return ret;
>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261106090507.1020751-1-dinghui1111%40163.com