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

[Ding Hui <dinghui1111@163.com>]

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