Re: [PATCH net-next v2 06/14] net: stmmac: dma: create a separate dma_device pointer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] When DMA mapping fails during transmission, the driver either leaks mapped memory (non-TSO path) or causes a permanent hardware TX queue stall (TSO path).
- [High] Kernel panic in the DMA descriptor allocation error path due to a NULL pointer dereference when tearing down uninitialized queues.
--

commit 66acc331f7f048d75e9c4b2fb700576508be93dc
Author: Alex Elder <elder@riscstar.com>
Subject: net: stmmac: dma: create a separate dma_device pointer

The patch separates the DMA device from the MAC device in the stmmac driver.
It adds a dma_device pointer to the stmmac_priv and plat_stmmacenet_data
structures, and updates all DMA operations to use it.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 35da51c262484..09d2640a18b3c 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -1730,12 +1730,12 @@ static void stmmac_free_tx_buffer(struct stmmac_priv *priv,
>  	if (tx_q->tx_skbuff_dma[i].buf &&
>  	    tx_q->tx_skbuff_dma[i].buf_type != STMMAC_TXBUF_T_XDP_TX) {
>  		if (tx_q->tx_skbuff_dma[i].map_as_page)
> -			dma_unmap_page(priv->device,
> +			dma_unmap_page(priv->dma_device,
>  				       tx_q->tx_skbuff_dma[i].buf,
>  				       tx_q->tx_skbuff_dma[i].len,
>  				       DMA_TO_DEVICE);
>  		else
> -			dma_unmap_single(priv->device,
> +			dma_unmap_single(priv->dma_device,
>  					 tx_q->tx_skbuff_dma[i].buf,
>  					 tx_q->tx_skbuff_dma[i].len,
>  					 DMA_TO_DEVICE);

[Severity: High]
This is a pre-existing issue, but is there a risk of a NULL pointer
dereference in the DMA descriptor allocation error path?

In the error path of alloc_dma_tx_desc_resources() or
alloc_dma_rx_desc_resources(), if queue allocation fails (e.g., -ENOMEM from
kzalloc_objs), the driver unconditionally cleans up all configured queues,
including those that were never initialized.

When free_dma_rx_desc_resources() or free_dma_tx_desc_resources() processes
an uninitialized queue, it iterates over the descriptor ring and calls
stmmac_free_tx_buffer() or stmmac_free_rx_buffer(). Since tx_skbuff_dma
or buf_pool is NULL for uninitialized queues, attempting to check conditions
like the one above will dereference a pointer relative to NULL, triggering a
panic.

[ ... ]

> @@ -4571,9 +4571,9 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
>  	first = desc;
>  
>  	/* first descriptor: fill Headers on Buf1 */
> -	des = dma_map_single(priv->device, skb->data, skb_headlen(skb),
> +	des = dma_map_single(priv->dma_device, skb->data, skb_headlen(skb),
>  			     DMA_TO_DEVICE);
> -	if (dma_mapping_error(priv->device, des))
> +	if (dma_mapping_error(priv->dma_device, des))
>  		goto dma_map_err;
>  
>  	stmmac_set_desc_addr(priv, first, des);
> @@ -4599,10 +4599,10 @@ static netdev_tx_t stmmac_tso_xmit(struct sk_buff *skb, struct net_device *dev)
>  	for (i = 0; i < nfrags; i++) {
>  		const skb_frag_t *frag = &skb_shinfo(skb)->frags[i];
>  
> -		des = skb_frag_dma_map(priv->device, frag, 0,
> +		des = skb_frag_dma_map(priv->dma_device, frag, 0,
>  				       skb_frag_size(frag),
>  				       DMA_TO_DEVICE);
> -		if (dma_mapping_error(priv->device, des))
> +		if (dma_mapping_error(priv->dma_device, des))
>  			goto dma_map_err;

[Severity: High]
This is a pre-existing issue, but does a DMA mapping failure here cause a
permanent hardware TX queue stall?

In stmmac_tso_xmit(), intermediate descriptors are eagerly marked with the
OWN=1 bit by stmmac_tso_allocator() before all fragments are mapped. If a
subsequent fragment mapping fails, the code jumps to dma_map_err before
setting the OWN bit on the first descriptor.

The hardware DMA engine won't process the intermediate fragments. Later, when
stmmac_tx_clean() processes the ring, it will encounter the OWN=1 descriptor
and permanently halt, freezing the entire TX queue.

[ ... ]

> @@ -4827,9 +4827,9 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
>  	} else {
>  		bool last_segment = (nfrags == 0);
>  
> -		dma_addr = dma_map_single(priv->device, skb->data,
> +		dma_addr = dma_map_single(priv->dma_device, skb->data,
>  					  nopaged_len, DMA_TO_DEVICE);
> -		if (dma_mapping_error(priv->device, dma_addr))
> +		if (dma_mapping_error(priv->dma_device, dma_addr))
>  			goto dma_map_err;
>  
>  		stmmac_set_tx_skb_dma_entry(tx_q, first_entry, dma_addr,
> @@ -4878,9 +4878,9 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
>  
>  		desc = stmmac_get_tx_desc(priv, tx_q, entry);
>  
> -		dma_addr = skb_frag_dma_map(priv->device, frag, 0, frag_size,
> -					    DMA_TO_DEVICE);
> -		if (dma_mapping_error(priv->device, dma_addr))
> +		dma_addr = skb_frag_dma_map(priv->dma_device, frag, 0,
> +					    frag_size, DMA_TO_DEVICE);
> +		if (dma_mapping_error(priv->dma_device, dma_addr))
>  			goto dma_map_err; /* should reuse desc w/o issues */

[Severity: High]
This is a pre-existing issue, but does this error path lead to a memory leak?

In stmmac_xmit(), if dma_map_single() or skb_frag_dma_map() fails, the code
jumps to dma_map_err without advancing tx_q->cur_tx.

Subsequent transmissions will reuse the same descriptor index and overwrite
the DMA addresses in tx_q->tx_skbuff_dma. This causes a permanent memory leak
of the previously mapped fragments for this skb, as they are never unmapped.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260605010022.968612-1-elder@riscstar.com?part=6