Re: [PATCH net-next 04/12] 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:
- [High] DMA mappings are permanently leaked if an error occurs mid-packet in `stmmac_xmit`, `stmmac_tso_xmit`, or `jumbo_frm`.
- [High] In `jumbo_frm()` (`chain_mode.c` and `ring_mode.c`), assigning a `dma_addr_t` to an `unsigned int` breaks `dma_mapping_error()` detection.
--

commit 152df859f4e94baeb0ca419c5b87bcb9e5f13d3d
Author: Alex Elder <elder@riscstar.com>

net: stmmac: dma: create a separate dma_device pointer

This commit adds a second dma_device pointer to the stmmac_priv structure.
The new pointer handles all DMA operations independently from the normal
device pointer, allowing better management of common internal IPs for
specialized Ethernet bridge chips.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/chain_mode.c b/drivers/net/ethernet/stmicro/stmmac/chain_mode.c
> index fc04a23342cf..331e6523ee01 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/chain_mode.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/chain_mode.c
> @@ -34,10 +34,10 @@ static int jumbo_frm(struct stmmac_tx_queue *tx_q, struct sk_buff *skb,
>  	buf_len = min_t(unsigned int, nopaged_len, bmax);
>  	len = nopaged_len - buf_len;
>  
> -	des2 = dma_map_single(priv->device, skb->data,
> +	des2 = dma_map_single(priv->dma_device, skb->data,
>  			      buf_len, DMA_TO_DEVICE);
>  	desc->des2 = cpu_to_le32(des2);
> -	if (dma_mapping_error(priv->device, des2))
> +	if (dma_mapping_error(priv->dma_device, des2))

This isn't a bug introduced by this patch, but since the patch modifies these
DMA mapping calls, it brings up a potential issue.

Does the use of an unsigned int for des2 break error detection?

On 64-bit architectures, dma_map_single() returns a 64-bit dma_addr_t.
Assigning this to the unsigned int des2 truncates the upper 32 bits.

If the mapping fails, it returns an error code that becomes 0xFFFFFFFF after
truncation. When this truncated value is passed to dma_mapping_error(), it
is cast back to a 64-bit value, which no longer matches the expected error
code representation.

Will this bypass the error check and record an invalid physical address in
tx_q->tx_skbuff_dma[entry].buf? This same pattern also appears in the
jumbo_frm() function in ring_mode.c.

[ ... ]
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index ca68248dbc78..1104cf750295 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
[ ... ]
> @@ -4876,9 +4876,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 */

Similarly, can this leak DMA mappings if an error occurs mid-packet?

If skb_frag_dma_map() fails for a multi-fragment packet, the code jumps
to the dma_map_err label. However, tx_q->cur_tx is only updated at the very
end of the function upon complete success.

Because cur_tx is not advanced on the error path, stmmac_tx_clean() won't
unmap these partially filled descriptor entries. On the next transmission,
the driver will start at the unmodified cur_tx index and blindly overwrite
the tx_skbuff_dma entries.

Could this result in permanently leaking the previously successful DMA
mappings? The same concern appears to apply to stmmac_tso_xmit() and
jumbo_frm() where multiple fragments are mapped sequentially.

[ ... ]

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