Re: [PATCH net-next 04/12] net: stmmac: dma: create a separate dma_device pointer

On 5/2/26 10:56 AM, [email protected] wrote:
> 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 <[email protected]>
> 
> 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.

I think this is a legitimate report.  I will follow up with a
separate patch that will change the type of the variable holding
the value returned by dma_map_single() to be dma_addr_t so it
doesn't affect the validity of dma_mapping_error().

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

It looks to me like it could.  I don't know this code especially
well, but if the loop iterates more than once, and skb_frag_dma_map()
returns an error, it seems the mappings should be "unwound" at
the dma_map_err label.  I'll look at this as something to address
in a separate patch too, but it's a little trickier than the first
suggestion.

					-Alex
> 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.
> 
> [ ... ]
>