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

[Ding Hui]

Hi,
Thanks for the review.

At 2026-10-16 22:44:35, "Maxime Chevallier" <maxime.chevallier@xxxxxxxxxxx> wrote:
>Hi,
>
>On 10/12/26 16:24, Ding Hui wrote:
>> From: Ding Hui <dinghui@xxxxxxxxxxx>
>> 
>> __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.
>> Additionally, make __free_dma_rx_desc_resources() and
>> __free_dma_tx_desc_resources() clear the pointers they free, so that
>> the NULL guards in the free helpers hold reliably when the long-lived
>> priv->dma_conf is reused across XDP open/release cycles.
>> Free the RX resources in alloc_dma_desc_resources() when the TX
>> allocation fails, as the callers only free dma_conf itself on
>> that path.
>> After __stmmac_open() copies a temporary dma_conf into priv->dma_conf,
>> release the descriptor resources through priv->dma_conf on an open
>> failure. This keeps the long-lived configuration consistent with the
>> resource cleanup while callers only free the temporary container.
>> Finally, hold RTNL while descriptors_status inspects the descriptor
>> rings. This serializes the debugfs reader with control-plane resource
>> teardown paths and allows it to safely report queues whose descriptor
>> rings are not allocated. Skip queues whose descriptor rings are not
>> allocated in stmmac_rings_status_show() to avoid dereferencing a NULL
>> pointer.
>> 
>> Signed-off-by: Ding Hui <dinghui@xxxxxxxxxxx>
>
>In V1 Andrew requested that this targets net-next yet this series kept
>targetting the "net" tree since then.
>
>I think this is an omission on your side as you replied to the bots that
>you removed the Fixes: tag on purpose :) You need to have "net-next" in
>the patch topic : [PATCH net-next v5]
>
The "net" prefix is indeed an omission on my side. Andrew asked for
net-next targeting back in v1, and I kept the wrong subject prefix
when respinning. The next revision will be sent as [PATCH net-next v6].

>This is starting to look good, but as you need to respin, is it possible
>that you split that patch down into simpler patches ? There are multiple
>classes of problems being addressed here :
> - Error paths being reworked
> - Null checks being added
> - Some pointers being Nulled
>
>It would make it easier to digest during review.
>
Agreed. Here is the plan — 4 patches, ordered as follows:

  1/4: add NULL guards in the free helpers and clear the pointers
       they free, so that a NULL pointer reliably means "not
       allocated";

  2/4: rework the allocation error paths, making
       __alloc_dma_rx_desc_resources() and
       __alloc_dma_tx_desc_resources() undo their own allocations on
       error, and free the RX resources when the TX allocation
       fails;

  3/4: release the descriptor resources through priv->dma_conf when
       __stmmac_open() fails after copying the temporary dma_conf,
       adjusting the callers accordingly;

  4/4: hold RTNL while the descriptors_status debugfs entry inspects
       the rings, and skip queues whose rings are not allocated.

Does this split and ordering look reasonable? If so, I'll
send v6 shortly.

Best regards,
Ding Hui