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

[Maxime Chevallier]

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]

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.

Thanks,

Maxime

> 
> ---
> Changes in v5:
>   - Release copied descriptor resources through priv->dma_conf when
>     __stmmac_open() fails, avoiding stale pointers in the long-lived
>     configuration.
>   - Hold RTNL while descriptors_status reads descriptor rings.
>   - Correct the commit message to refer to descriptors_status as a
>     debugfs entry rather than a sysfs entry.
>   - Link to v4:
>     https://lore.kernel.org/netdev/20261001035045.3305816-1-dinghui1111@xxxxxxx/
> Changes in v4:
>   - Free the RX resources in alloc_dma_desc_resources() when the TX
>     allocation fails.
>   - Skip queues whose descriptor rings are not allocated in
>     stmmac_rings_status_show() and print a hint instead.
>   - Link to v3:
>     https://lore.kernel.org/netdev/20260919121436.1642724-1-dinghui1111@xxxxxxx/
> Changes in v3:
>   - Use addr directly in __alloc_dma_rx_desc_resources() error path
>     instead of the if/else block on rx_q->dma_erx/dma_rx.
>   - Clear the freed pointers in __free_dma_rx_desc_resources() and
>     __free_dma_tx_desc_resources().
>   - Link to v2:
>     https://lore.kernel.org/netdev/20260905154654.1725313-1-dinghui1111@xxxxxxx/
> 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@xxxxxxx/
> ---
>  .../net/ethernet/stmicro/stmmac/stmmac_main.c | 112 +++++++++++++++---
>  1 file changed, 96 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index f2fc89176654..852a8b103c45 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++)
> @@ -2154,13 +2166,20 @@ static void __free_dma_rx_desc_resources(struct stmmac_priv *priv,
>  	size = stmmac_get_rx_desc_size(priv) * dma_conf->dma_rx_size;
>  
>  	dma_free_coherent(priv->device, size, addr, rx_q->dma_rx_phy);
> +	rx_q->dma_erx = NULL;
> +	rx_q->dma_rx = NULL;
> +	rx_q->dma_rx_phy = 0;
>  
>  	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)
> +	rx_q->buf_pool = NULL;
> +
> +	if (rx_q->page_pool) {
>  		page_pool_destroy(rx_q->page_pool);
> +		rx_q->page_pool = NULL;
> +	}
>  }
>  
>  static void free_dma_rx_desc_resources(struct stmmac_priv *priv,
> @@ -2202,9 +2221,16 @@ static void __free_dma_tx_desc_resources(struct stmmac_priv *priv,
>  	size = stmmac_get_tx_desc_size(priv, tx_q) * dma_conf->dma_tx_size;
>  
>  	dma_free_coherent(priv->device, size, addr, tx_q->dma_tx_phy);
> +	tx_q->dma_etx = NULL;
> +	tx_q->dma_entx = NULL;
> +	tx_q->dma_tx = NULL;
> +	tx_q->dma_tx_phy = 0;
>  
>  	kfree(tx_q->tx_skbuff_dma);
> +	tx_q->tx_skbuff_dma = NULL;
> +
>  	kfree(tx_q->tx_skbuff);
> +	tx_q->tx_skbuff = NULL;
>  }
>  
>  static void free_dma_tx_desc_resources(struct stmmac_priv *priv,
> @@ -2272,15 +2298,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 +2326,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;
> +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 +2394,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 +2411,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,
> @@ -2411,6 +2461,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);
>  
>  	return ret;
>  }
> @@ -4182,6 +4234,7 @@ static int __stmmac_open(struct net_device *dev,
>  
>  	stmmac_release_ptp(priv);
>  init_error:
> +	free_dma_desc_resources(priv, &priv->dma_conf);
>  	return ret;
>  }
>  
> @@ -4214,8 +4267,13 @@ static int stmmac_open(struct net_device *dev)
>  	}
>  
>  	ret = __stmmac_open(dev, dma_conf);
> -	if (ret)
> -		goto err_serdes;
> +	if (ret) {
> +		stmmac_legacy_serdes_power_down(priv);
> +		phylink_disconnect_phy(priv->phylink);
> +		pm_runtime_put(priv->device);
> +		kfree(dma_conf);
> +		return ret;
> +	}
>  
>  	kfree(dma_conf);
>  
> @@ -4224,8 +4282,6 @@ static int stmmac_open(struct net_device *dev)
>  
>  	return ret;
>  
> -err_serdes:
> -	stmmac_legacy_serdes_power_down(priv);
>  err_disconnect_phy:
>  	phylink_disconnect_phy(priv->phylink);
>  err_runtime_pm:
> @@ -6130,7 +6186,6 @@ static int stmmac_change_mtu(struct net_device *dev, int new_mtu)
>  
>  		ret = __stmmac_open(dev, dma_conf);
>  		if (ret) {
> -			free_dma_desc_resources(priv, dma_conf);
>  			kfree(dma_conf);
>  			netdev_err(priv->dev, "failed reopening the interface after MTU change\n");
>  			return ret;
> @@ -6501,8 +6556,10 @@ static int stmmac_rings_status_show(struct seq_file *seq, void *v)
>  	u8 tx_count = priv->plat->tx_queues_to_use;
>  	u8 queue;
>  
> +	rtnl_lock();
> +
>  	if ((dev->flags & IFF_UP) == 0)
> -		return 0;
> +		goto out;
>  
>  	for (queue = 0; queue < rx_count; queue++) {
>  		struct stmmac_rx_queue *rx_q = &priv->dma_conf.rx_queue[queue];
> @@ -6510,10 +6567,20 @@ static int stmmac_rings_status_show(struct seq_file *seq, void *v)
>  		seq_printf(seq, "RX Queue %d:\n", queue);
>  
>  		if (priv->extend_desc) {
> +			if (!rx_q->dma_erx) {
> +				seq_puts(seq, "Extended descriptor ring not allocated\n");
> +				continue;
> +			}
> +
>  			seq_printf(seq, "Extended descriptor ring:\n");
>  			sysfs_display_ring((void *)rx_q->dma_erx,
>  					   priv->dma_conf.dma_rx_size, 1, seq, rx_q->dma_rx_phy);
>  		} else {
> +			if (!rx_q->dma_rx) {
> +				seq_puts(seq, "Descriptor ring not allocated\n");
> +				continue;
> +			}
> +
>  			seq_printf(seq, "Descriptor ring:\n");
>  			sysfs_display_ring((void *)rx_q->dma_rx,
>  					   priv->dma_conf.dma_rx_size, 0, seq, rx_q->dma_rx_phy);
> @@ -6526,16 +6593,29 @@ static int stmmac_rings_status_show(struct seq_file *seq, void *v)
>  		seq_printf(seq, "TX Queue %d:\n", queue);
>  
>  		if (priv->extend_desc) {
> +			if (!tx_q->dma_etx) {
> +				seq_puts(seq, "Extended descriptor ring not allocated\n");
> +				continue;
> +			}
> +
>  			seq_printf(seq, "Extended descriptor ring:\n");
>  			sysfs_display_ring((void *)tx_q->dma_etx,
>  					   priv->dma_conf.dma_tx_size, 1, seq, tx_q->dma_tx_phy);
>  		} else if (!(tx_q->tbs & STMMAC_TBS_AVAIL)) {
> +			if (!tx_q->dma_tx) {
> +				seq_puts(seq, "Descriptor ring not allocated\n");
> +				continue;
> +			}
> +
>  			seq_printf(seq, "Descriptor ring:\n");
>  			sysfs_display_ring((void *)tx_q->dma_tx,
>  					   priv->dma_conf.dma_tx_size, 0, seq, tx_q->dma_tx_phy);
>  		}
>  	}
>  
> +out:
> +	rtnl_unlock();
> +
>  	return 0;
>  }
>  DEFINE_SHOW_ATTRIBUTE(stmmac_rings_status);