1. Patchew
  2. linux
  3. View series

[PATCH net-next v2] net: stmmac: fix error path cleanup in DMA descriptor ring allocation

Ding Hui posted 1 patch 2 weeks, 6 days ago
There is a newer version of this series
.../net/ethernet/stmicro/stmmac/stmmac_main.c | 59 ++++++++++++++++---
1 file changed, 50 insertions(+), 9 deletions(-)
[PATCH net-next v2] net: stmmac: fix error path cleanup in DMA descriptor ring allocation
Posted by Ding Hui 2 weeks, 6 days ago 
From: Ding Hui <dinghui@lixiang.com>

__alloc_dma_rx_desc_resources() and __alloc_dma_tx_desc_resources()
allocate resources in multiple steps but return early on failure
without cleaning up what they have already allocated. The outer
error paths then call the free helpers on partially-initialized
queues, which dereference pointers that were never allocated:

  - dma_free_rx_skbufs() and dma_free_rx_xskbufs() dereference
    rx_q->buf_pool[i] via stmmac_free_rx_buffer(), but buf_pool
    may be NULL if its kzalloc_objs() failed.

  - dma_free_tx_skbufs() dereferences tx_q->tx_skbuff_dma[i] via
    stmmac_free_tx_buffer(), but tx_skbuff_dma may be NULL if its
    kzalloc_objs() failed.

  - stmmac_free_tx_buffer() dereferences tx_q->xdpf[i] and
    tx_q->tx_skbuff[i] (aliased through a union), but tx_skbuff
    may be NULL if its allocation failed while tx_skbuff_dma
    succeeded.

Fix this by making each allocation function responsible for undoing
its own allocations on error, following the standard kernel error
handling pattern of cleaning up in reverse order. Also add NULL
checks in the free helpers as a defensive measure, since they may
be called on partially-initialized queues.

Signed-off-by: Ding Hui <dinghui@lixiang.com>

---
Changes in v2:
  - Instead of only adding NULL checks in the free helpers, also fix
    __alloc_dma_rx_desc_resources() and __alloc_dma_tx_desc_resources()
    to clean up their own allocations on error, as suggested by Andrew.
  - Update commit message.
  - Link to v1:
    https://lore.kernel.org/netdev/20260830040610.1156008-1-dinghui1111@163.com/
---
 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 59 ++++++++++++++++---
 1 file changed, 50 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index f2fc89176654..f0e06c011b8d 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1728,7 +1728,7 @@ static void stmmac_free_tx_buffer(struct stmmac_priv *priv,
 					 DMA_TO_DEVICE);
 	}
 
-	if (tx_q->xdpf[i] &&
+	if (tx_q->xdpf && tx_q->xdpf[i] &&
 	    (tx_q->tx_skbuff_dma[i].buf_type == STMMAC_TXBUF_T_XDP_TX ||
 	     tx_q->tx_skbuff_dma[i].buf_type == STMMAC_TXBUF_T_XDP_NDO)) {
 		xdp_return_frame(tx_q->xdpf[i]);
@@ -1738,7 +1738,7 @@ static void stmmac_free_tx_buffer(struct stmmac_priv *priv,
 	if (tx_q->tx_skbuff_dma[i].buf_type == STMMAC_TXBUF_T_XSK_TX)
 		tx_q->xsk_frames_done++;
 
-	if (tx_q->tx_skbuff[i] &&
+	if (tx_q->tx_skbuff && tx_q->tx_skbuff[i] &&
 	    tx_q->tx_skbuff_dma[i].buf_type == STMMAC_TXBUF_T_SKB) {
 		dev_kfree_skb_any(tx_q->tx_skbuff[i]);
 		tx_q->tx_skbuff[i] = NULL;
@@ -1761,6 +1761,10 @@ static void dma_free_rx_skbufs(struct stmmac_priv *priv,
 	struct stmmac_rx_queue *rx_q = &dma_conf->rx_queue[queue];
 	int i;
 
+	/* buf_pool may not be allocated if alloc failed early */
+	if (!rx_q->buf_pool)
+		return;
+
 	for (i = 0; i < dma_conf->dma_rx_size; i++)
 		stmmac_free_rx_buffer(priv, rx_q, i);
 }
@@ -1802,6 +1806,10 @@ static void dma_free_rx_xskbufs(struct stmmac_priv *priv,
 	struct stmmac_rx_queue *rx_q = &dma_conf->rx_queue[queue];
 	int i;
 
+	/* buf_pool may not be allocated if alloc failed early */
+	if (!rx_q->buf_pool)
+		return;
+
 	for (i = 0; i < dma_conf->dma_rx_size; i++) {
 		struct stmmac_rx_buffer *buf = &rx_q->buf_pool[i];
 
@@ -2097,6 +2105,10 @@ static void dma_free_tx_skbufs(struct stmmac_priv *priv,
 	struct stmmac_tx_queue *tx_q = &dma_conf->tx_queue[queue];
 	int i;
 
+	/* tx_skbuff_dma may not be allocated if alloc failed early */
+	if (!tx_q->tx_skbuff_dma)
+		return;
+
 	tx_q->xsk_frames_done = 0;
 
 	for (i = 0; i < dma_conf->dma_tx_size; i++)
@@ -2272,15 +2284,19 @@ static int __alloc_dma_rx_desc_resources(struct stmmac_priv *priv,
 	}
 
 	rx_q->buf_pool = kzalloc_objs(*rx_q->buf_pool, dma_conf->dma_rx_size);
-	if (!rx_q->buf_pool)
-		return -ENOMEM;
+	if (!rx_q->buf_pool) {
+		ret = -ENOMEM;
+		goto err_destroy_pool;
+	}
 
 	size = stmmac_get_rx_desc_size(priv) * dma_conf->dma_rx_size;
 
 	addr = dma_alloc_coherent(priv->device, size, &rx_q->dma_rx_phy,
 				  GFP_KERNEL);
-	if (!addr)
-		return -ENOMEM;
+	if (!addr) {
+		ret = -ENOMEM;
+		goto err_free_buf_pool;
+	}
 
 	if (priv->extend_desc)
 		rx_q->dma_erx = addr;
@@ -2296,10 +2312,27 @@ 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:
+	if (priv->extend_desc)
+		dma_free_coherent(priv->device, size, rx_q->dma_erx,
+				  rx_q->dma_rx_phy);
+	else
+		dma_free_coherent(priv->device, size, rx_q->dma_rx,
+				  rx_q->dma_rx_phy);
+	rx_q->dma_erx = NULL;
+	rx_q->dma_rx = NULL;
+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;
 }
 
 static int alloc_dma_rx_desc_resources(struct stmmac_priv *priv,
@@ -2352,14 +2385,14 @@ static int __alloc_dma_tx_desc_resources(struct stmmac_priv *priv,
 
 	tx_q->tx_skbuff = kzalloc_objs(struct sk_buff *, dma_conf->dma_tx_size);
 	if (!tx_q->tx_skbuff)
-		return -ENOMEM;
+		goto err_free_skbuff_dma;
 
 	size = stmmac_get_tx_desc_size(priv, tx_q) * dma_conf->dma_tx_size;
 
 	addr = dma_alloc_coherent(priv->device, size,
 				  &tx_q->dma_tx_phy, GFP_KERNEL);
 	if (!addr)
-		return -ENOMEM;
+		goto err_free_skbuff;
 
 	if (priv->extend_desc)
 		tx_q->dma_etx = addr;
@@ -2369,6 +2402,14 @@ static int __alloc_dma_tx_desc_resources(struct stmmac_priv *priv,
 		tx_q->dma_tx = addr;
 
 	return 0;
+
+err_free_skbuff:
+	kfree(tx_q->tx_skbuff);
+	tx_q->tx_skbuff = NULL;
+err_free_skbuff_dma:
+	kfree(tx_q->tx_skbuff_dma);
+	tx_q->tx_skbuff_dma = NULL;
+	return -ENOMEM;
 }
 
 static int alloc_dma_tx_desc_resources(struct stmmac_priv *priv,
-- 
2.34.1
Re: [PATCH net-next v2] net: stmmac: fix error path cleanup in DMA descriptor ring allocation
Posted by netdev-bot+sashiko@kernel.org 2 weeks, 3 days ago 
Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

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

- [High] The newly added NULL guards encode the contract 'NULL == not
  allocated', but the free helpers never establish it:…
- [Low] Missing `Fixes:` tag (and no stable routing) on a patch that
  fixes a user-visible NULL-pointer dereference.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch
  makes it deterministic: `stmmac_enable_rx_queue()`…
- [Medium] This problem wasn't introduced by this patch, but the sibling
  error path in `alloc_dma_desc_resources()` (stmmac_main.c:2445-2457)…

--

net: stmmac: fix error path cleanup in DMA descriptor ring allocation

This makes __alloc_dma_rx_desc_resources() and
__alloc_dma_tx_desc_resources() unwind their own partial allocations on
failure, and adds NULL checks in the buffer free helpers so they can be
called on partially-initialized queues.

A few questions below about the new error paths and the surrounding
callers.

[Severity: Low]
The commit message describes three concrete NULL dereferences that are
reachable today, when a kzalloc_objs() inside
__alloc_dma_rx_desc_resources() / __alloc_dma_tx_desc_resources() fails
and the err_dma: labels in alloc_dma_rx_desc_resources() /
alloc_dma_tx_desc_resources() run the bulk free helpers over every
queue.

Should this carry a Fixes: tag so it can be routed to the stable trees?
Without one it is hard for a backporter to tell how far back the
dereference exists.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 07a6fab6460ed..8d03eb7904ef1 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c

[ ... ]

> @@ -1761,6 +1761,10 @@ static void dma_free_rx_skbufs(struct stmmac_priv *priv,
>  	struct stmmac_rx_queue *rx_q = &dma_conf->rx_queue[queue];
>  	int i;
>  
> +	/* buf_pool may not be allocated if alloc failed early */
> +	if (!rx_q->buf_pool)
> +		return;
> +
>  	for (i = 0; i < dma_conf->dma_rx_size; i++)
>  		stmmac_free_rx_buffer(priv, rx_q, i);
>  }

[Severity: High]
These new guards assume "NULL means not allocated", but do the free
helpers ever establish that? __free_dma_rx_desc_resources() releases
everything without clearing the fields:

	dma_free_coherent(priv->device, size, addr, rx_q->dma_rx_phy);

	if (xdp_rxq_info_is_reg(&rx_q->xdp_rxq))
		xdp_rxq_info_unreg(&rx_q->xdp_rxq);

	kfree(rx_q->buf_pool);
	if (rx_q->page_pool)
		page_pool_destroy(rx_q->page_pool);

__free_dma_tx_desc_resources() does the same for tx_skbuff_dma,
tx_skbuff and the coherent TX ring.

That matters because these helpers also run on the long-lived
priv->dma_conf, and it is re-used for the next allocation:

  stmmac_xdp_set_prog()
    stmmac_xdp_release()
      free_dma_desc_resources(priv, &priv->dma_conf)   /* pointers stay set */
    stmmac_xdp_open()
      alloc_dma_desc_resources(priv, &priv->dma_conf)

Unlike stmmac_open(), which works on a freshly kzalloc'ed dma_conf,
nothing zeroes priv->dma_conf here. So if any per-queue allocation fails
mid-way (for example page_pool_create() for queue 1, which is the one
early return this patch leaves as-is), alloc_dma_rx_desc_resources()
jumps to err_dma: and free_dma_rx_desc_resources() loops over all queues
0..rx_queues_to_use-1:

	for (queue = 0; queue < rx_count; queue++)
		__free_dma_rx_desc_resources(priv, dma_conf, queue);

The queues that were not touched in this pass still hold the pointers
freed by the preceding release. Since those pointers are freed but not
NULL, does the new guard fire at all for them?

Following that path through, stmmac_free_rx_buffer() would read
buf->page out of the freed buf_pool object and pass it to
page_pool_put_full_page() on an already-destroyed pool, then
dma_free_coherent() runs a second time on the released coherent memory,
kfree(rx_q->buf_pool) becomes a slab double free, and
page_pool_destroy(rx_q->page_pool) is called on a freed pool. Can that
corrupt the slab and DMA state?

The TX side looks symmetric via the new "if (!tx_q->tx_skbuff_dma)
return;" guard and the tx_skbuff/xdpf tests added in
stmmac_free_tx_buffer().

Would it make sense for __free_dma_rx_desc_resources() and
__free_dma_tx_desc_resources() to NULL the fields they free, so the new
guards actually hold?

[ ... ]

> @@ -2296,10 +2312,27 @@ 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:
> +	if (priv->extend_desc)
> +		dma_free_coherent(priv->device, size, rx_q->dma_erx,
> +				  rx_q->dma_rx_phy);
> +	else
> +		dma_free_coherent(priv->device, size, rx_q->dma_rx,
> +				  rx_q->dma_rx_phy);
> +	rx_q->dma_erx = NULL;
> +	rx_q->dma_rx = NULL;
> +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: High]
This isn't a bug introduced by this patch, but does the patch turn it
into a deterministic NULL dereference?

stmmac_enable_rx_queue() and stmmac_enable_tx_queue() return void and
simply drop the allocation error:

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

After the new labels above, the failed queue is left with
rx_q->buf_pool == NULL, rx_q->page_pool == NULL and
rx_q->dma_rx/dma_erx == NULL (tx_skbuff and tx_skbuff_dma on the TX
side).

The caller in stmmac_xdp.c continues regardless:

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

and stmmac_xsk_wakeup() reaches __napi_schedule(&ch->rxtx_napi). Its
only precondition is:

	if (!rx_q->xsk_pool && !tx_q->xsk_pool)
		return -EINVAL;

which does not help here, because dma_free_tx_skbufs() only clears
tx_q->xsk_pool when xsk_frames_done is non-zero.

Does the scheduled poll then dereference rx_q->buf_pool[entry] and
tx_q->tx_skbuff_dma[entry] in softirq context on a queue whose arrays
were just set to NULL? Should the enable helpers propagate the error and
skip the napi_enable()/wakeup instead?

[ ... ]

> @@ -2369,6 +2402,14 @@ static int __alloc_dma_tx_desc_resources(struct stmmac_priv *priv,
>  		tx_q->dma_tx = addr;
>  
>  	return 0;
> +
> +err_free_skbuff:
> +	kfree(tx_q->tx_skbuff);
> +	tx_q->tx_skbuff = NULL;
> +err_free_skbuff_dma:
> +	kfree(tx_q->tx_skbuff_dma);
> +	tx_q->tx_skbuff_dma = NULL;
> +	return -ENOMEM;
>  }
>  
>  static int alloc_dma_tx_desc_resources(struct stmmac_priv *priv,

[Severity: Medium]
This is a pre-existing issue, but the sibling function in the same
family still does not follow the rule the commit message states, namely
"making each allocation function responsible for undoing its own
allocations on error".

alloc_dma_desc_resources() allocates RX first and returns the TX error
directly:

	int ret = alloc_dma_rx_desc_resources(priv, dma_conf);

	if (ret)
		return ret;

	ret = alloc_dma_tx_desc_resources(priv, dma_conf);

	return ret;

On TX failure only free_dma_tx_desc_resources() has run (inside
alloc_dma_tx_desc_resources()), so every rx_q->page_pool,
rx_q->buf_pool and coherent RX ring is still allocated. The caller then
takes the alloc_error: path:

init_error:
	free_dma_desc_resources(priv, dma_conf);
alloc_error:
	kfree(dma_conf);
	return ERR_PTR(ret);

which only kfree()s dma_conf and drops the only pointers to those RX
resources; free_dma_desc_resources() is reached only via init_error:.

Does this leak the RX page pools, the buf_pool arrays and the
dma_alloc_coherent RX rings when a TX allocation fails during ifup or an
MTU change? stmmac_xdp_open() has the same shape, except the resources
stay reachable through priv->dma_conf, which feeds into the stale
pointer question above.

Would adding a free_dma_rx_desc_resources() call to
alloc_dma_desc_resources() on the TX failure path fit the pattern this
patch establishes?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260905154654.1725313-1-dinghui1111%40163.com
Re: [PATCH net-next v2] net: stmmac: fix error path cleanup in DMA descriptor ring allocation
Posted by Lorenzo Bianconi 2 weeks, 5 days ago 
> From: Ding Hui <dinghui@lixiang.com>

Hi Ding Hui,

just a couple of nits inline.

Regards,
Lorenzo

[...]

>  .../net/ethernet/stmicro/stmmac/stmmac_main.c | 59 ++++++++++++++++---
>  1 file changed, 50 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index f2fc89176654..f0e06c011b8d 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -1728,7 +1728,7 @@ static void stmmac_free_tx_buffer(struct stmmac_priv *priv,
>  					 DMA_TO_DEVICE);
>  	}
>  

Is it more appropriate to move the tx_q->tx_skbuff_dma check here from
dma_free_tx_skbufs()?

> -	if (tx_q->xdpf[i] &&
> +	if (tx_q->xdpf && tx_q->xdpf[i] &&
>  	    (tx_q->tx_skbuff_dma[i].buf_type == STMMAC_TXBUF_T_XDP_TX ||
>  	     tx_q->tx_skbuff_dma[i].buf_type == STMMAC_TXBUF_T_XDP_NDO)) {
>  		xdp_return_frame(tx_q->xdpf[i]);
> @@ -1738,7 +1738,7 @@ static void stmmac_free_tx_buffer(struct stmmac_priv *priv,
>  	if (tx_q->tx_skbuff_dma[i].buf_type == STMMAC_TXBUF_T_XSK_TX)
>  		tx_q->xsk_frames_done++;
>  
> -	if (tx_q->tx_skbuff[i] &&
> +	if (tx_q->tx_skbuff && tx_q->tx_skbuff[i] &&
>  	    tx_q->tx_skbuff_dma[i].buf_type == STMMAC_TXBUF_T_SKB) {
>  		dev_kfree_skb_any(tx_q->tx_skbuff[i]);
>  		tx_q->tx_skbuff[i] = NULL;
> @@ -1761,6 +1761,10 @@ static void dma_free_rx_skbufs(struct stmmac_priv *priv,
>  	struct stmmac_rx_queue *rx_q = &dma_conf->rx_queue[queue];
>  	int i;
>  
> +	/* buf_pool may not be allocated if alloc failed early */
> +	if (!rx_q->buf_pool)
> +		return;
> +
>  	for (i = 0; i < dma_conf->dma_rx_size; i++)
>  		stmmac_free_rx_buffer(priv, rx_q, i);
>  }
> @@ -1802,6 +1806,10 @@ static void dma_free_rx_xskbufs(struct stmmac_priv *priv,
>  	struct stmmac_rx_queue *rx_q = &dma_conf->rx_queue[queue];
>  	int i;
>  
> +	/* buf_pool may not be allocated if alloc failed early */
> +	if (!rx_q->buf_pool)
> +		return;
> +
>  	for (i = 0; i < dma_conf->dma_rx_size; i++) {
>  		struct stmmac_rx_buffer *buf = &rx_q->buf_pool[i];
>  
> @@ -2097,6 +2105,10 @@ static void dma_free_tx_skbufs(struct stmmac_priv *priv,
>  	struct stmmac_tx_queue *tx_q = &dma_conf->tx_queue[queue];
>  	int i;
>  
> +	/* tx_skbuff_dma may not be allocated if alloc failed early */
> +	if (!tx_q->tx_skbuff_dma)
> +		return;
> +
>  	tx_q->xsk_frames_done = 0;
>  
>  	for (i = 0; i < dma_conf->dma_tx_size; i++)
> @@ -2272,15 +2284,19 @@ static int __alloc_dma_rx_desc_resources(struct stmmac_priv *priv,
>  	}
>  
>  	rx_q->buf_pool = kzalloc_objs(*rx_q->buf_pool, dma_conf->dma_rx_size);
> -	if (!rx_q->buf_pool)
> -		return -ENOMEM;
> +	if (!rx_q->buf_pool) {
> +		ret = -ENOMEM;
> +		goto err_destroy_pool;
> +	}
>  
>  	size = stmmac_get_rx_desc_size(priv) * dma_conf->dma_rx_size;
>  
>  	addr = dma_alloc_coherent(priv->device, size, &rx_q->dma_rx_phy,
>  				  GFP_KERNEL);
> -	if (!addr)
> -		return -ENOMEM;
> +	if (!addr) {
> +		ret = -ENOMEM;
> +		goto err_free_buf_pool;
> +	}
>  
>  	if (priv->extend_desc)
>  		rx_q->dma_erx = addr;
> @@ -2296,10 +2312,27 @@ 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:
> +	if (priv->extend_desc)
> +		dma_free_coherent(priv->device, size, rx_q->dma_erx,
> +				  rx_q->dma_rx_phy);
> +	else
> +		dma_free_coherent(priv->device, size, rx_q->dma_rx,
> +				  rx_q->dma_rx_phy);

I guess you can use addr here and remove the if/else block

> +	rx_q->dma_erx = NULL;
> +	rx_q->dma_rx = NULL;
> +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;
>  }
>  
>  static int alloc_dma_rx_desc_resources(struct stmmac_priv *priv,
> @@ -2352,14 +2385,14 @@ static int __alloc_dma_tx_desc_resources(struct stmmac_priv *priv,
>  
>  	tx_q->tx_skbuff = kzalloc_objs(struct sk_buff *, dma_conf->dma_tx_size);
>  	if (!tx_q->tx_skbuff)
> -		return -ENOMEM;
> +		goto err_free_skbuff_dma;
>  
>  	size = stmmac_get_tx_desc_size(priv, tx_q) * dma_conf->dma_tx_size;
>  
>  	addr = dma_alloc_coherent(priv->device, size,
>  				  &tx_q->dma_tx_phy, GFP_KERNEL);
>  	if (!addr)
> -		return -ENOMEM;
> +		goto err_free_skbuff;
>  
>  	if (priv->extend_desc)
>  		tx_q->dma_etx = addr;
> @@ -2369,6 +2402,14 @@ static int __alloc_dma_tx_desc_resources(struct stmmac_priv *priv,
>  		tx_q->dma_tx = addr;
>  
>  	return 0;
> +
> +err_free_skbuff:
> +	kfree(tx_q->tx_skbuff);
> +	tx_q->tx_skbuff = NULL;
> +err_free_skbuff_dma:
> +	kfree(tx_q->tx_skbuff_dma);
> +	tx_q->tx_skbuff_dma = NULL;
> +	return -ENOMEM;
>  }
>  
>  static int alloc_dma_tx_desc_resources(struct stmmac_priv *priv,
> -- 
> 2.34.1
> 
>
Re:Re: [PATCH net-next v2] net: stmmac: fix error path cleanup in DMA descriptor ring allocation
Posted by Ding Hui 2 weeks, 1 day ago 
Hi Lorenzo,

Thanks for your review comments.

At 2026-09-06 18:16:39, "Lorenzo Bianconi" <lorenzo.bianconi@oss.qualcomm.com> wrote:
>> From: Ding Hui <dinghui@lixiang.com>
>
>Hi Ding Hui,
>
>just a couple of nits inline.
>
>Regards,
>Lorenzo
>
>[...]
>
>>  .../net/ethernet/stmicro/stmmac/stmmac_main.c | 59 ++++++++++++++++---
>>  1 file changed, 50 insertions(+), 9 deletions(-)
>> 
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> index f2fc89176654..f0e06c011b8d 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> @@ -1728,7 +1728,7 @@ static void stmmac_free_tx_buffer(struct stmmac_priv *priv,
>>  					 DMA_TO_DEVICE);
>>  	}
>>  
>
>Is it more appropriate to move the tx_q->tx_skbuff_dma check here from
>dma_free_tx_skbufs()?
>

tx_skbuff_dma is a per-queue resource, and stmmac_free_tx_buffer()
references it in multiple places across the function body. Moving the
NULL check into stmmac_free_tx_buffer() would require repeating it on
every call, introducing O(n) overhead proportional to dma_tx_size.
So I put it at the entry of dma_free_tx_skbufs().

>> -	if (tx_q->xdpf[i] &&
>> +	if (tx_q->xdpf && tx_q->xdpf[i] &&
>>  	    (tx_q->tx_skbuff_dma[i].buf_type == STMMAC_TXBUF_T_XDP_TX ||
>>  	     tx_q->tx_skbuff_dma[i].buf_type == STMMAC_TXBUF_T_XDP_NDO)) {
>>  		xdp_return_frame(tx_q->xdpf[i]);
>> @@ -1738,7 +1738,7 @@ static void stmmac_free_tx_buffer(struct stmmac_priv *priv,
>>  	if (tx_q->tx_skbuff_dma[i].buf_type == STMMAC_TXBUF_T_XSK_TX)
>>  		tx_q->xsk_frames_done++;
>>  
>> -	if (tx_q->tx_skbuff[i] &&
>> +	if (tx_q->tx_skbuff && tx_q->tx_skbuff[i] &&
>>  	    tx_q->tx_skbuff_dma[i].buf_type == STMMAC_TXBUF_T_SKB) {
>>  		dev_kfree_skb_any(tx_q->tx_skbuff[i]);
>>  		tx_q->tx_skbuff[i] = NULL;
>> @@ -1761,6 +1761,10 @@ static void dma_free_rx_skbufs(struct stmmac_priv *priv,
>>  	struct stmmac_rx_queue *rx_q = &dma_conf->rx_queue[queue];
>>  	int i;
>>  
>> +	/* buf_pool may not be allocated if alloc failed early */
>> +	if (!rx_q->buf_pool)
>> +		return;
>> +
>>  	for (i = 0; i < dma_conf->dma_rx_size; i++)
>>  		stmmac_free_rx_buffer(priv, rx_q, i);
>>  }

>> @@ -2272,15 +2284,19 @@ static int __alloc_dma_rx_desc_resources(struct stmmac_priv *priv,
>>  	}
>>  
>>  	rx_q->buf_pool = kzalloc_objs(*rx_q->buf_pool, dma_conf->dma_rx_size);
>> -	if (!rx_q->buf_pool)
>> -		return -ENOMEM;
>> +	if (!rx_q->buf_pool) {
>> +		ret = -ENOMEM;
>> +		goto err_destroy_pool;
>> +	}
>>  
>>  	size = stmmac_get_rx_desc_size(priv) * dma_conf->dma_rx_size;
>>  
>>  	addr = dma_alloc_coherent(priv->device, size, &rx_q->dma_rx_phy,
>>  				  GFP_KERNEL);
>> -	if (!addr)
>> -		return -ENOMEM;
>> +	if (!addr) {
>> +		ret = -ENOMEM;
>> +		goto err_free_buf_pool;
>> +	}
>>  
>>  	if (priv->extend_desc)
>>  		rx_q->dma_erx = addr;
>> @@ -2296,10 +2312,27 @@ 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:
>> +	if (priv->extend_desc)
>> +		dma_free_coherent(priv->device, size, rx_q->dma_erx,
>> +				  rx_q->dma_rx_phy);
>> +	else
>> +		dma_free_coherent(priv->device, size, rx_q->dma_rx,
>> +				  rx_q->dma_rx_phy);
>
>I guess you can use addr here and remove the if/else block
>
Excellent suggestion, will use addr directly in v3.

Thanks,
Ding Hui

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.