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

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

[Ding Hui]

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

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 5144 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Ding Hui]

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

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

[netdev-bot+sashiko]

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:Re: [PATCH net-next v2] net: stmmac: fix error path cleanup in DMA descriptor ring allocation

[Ding Hui]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 9740 bytes --]

At 2026-09-09 08:48:01, netdev-bot+sashiko@kernel.org wrote:
>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.
>
This patch targets net-next rather than net, as the issue only triggers
under memory allocation failure during initialization when there is memory
allocation pressure. A Fixes: tag is intentionally omitted to avoid
triggering unwanted stable backports.

>> 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?
>
It's right. Currently __free_dma_rx_desc_resources() and
__free_dma_tx_desc_resources() free the resources without NULLing the
pointers. Since priv->dma_conf is reused across xdp open/release cycles,
stale pointers could defeat the new NULL guards. I'll add NULL
assignments after each free in __free_dma_rx_desc_resources() and
__free_dma_tx_desc_resources() in v3.

>[ ... ]
>
>> @@ -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?

The above two suggestions are valid. The error handling in
stmmac_enable_rx_queue()/stmmac_enable_tx_queue() (returning void and
dropping allocation errors) and the RX resource leak in
alloc_dma_desc_resources() on TX failure are real pre-existing issues.
However, they are beyond the scope of this patch, which focuses on
making the per-queue allocation functions clean up their own partial
allocations. I plan to address them in follow-up patches.

Thanks,
Ding Hui