Re: [PATCH net] net: stmmac: fix stale descriptors and DMA mapping leak on Tx map failure

[赵金明]

>> In stmmac_xmit(), when the DMA mapping of the linear part or of a



>



>[...]



>



>>? 	if (unlikely(is_jumbo)) {



>> -		entry = stmmac_jumbo_frm(priv, tx_q, skb, csum_insertion);



>> -		if (unlikely(entry < 0) && (entry != -EINVAL))



>> +		ret = stmmac_jumbo_frm(priv, tx_q, skb, csum_insertion);



>



>if jumbo_frm() returns an error on the subsequent frames, entry is not updated



>here, so we will end up with a DMA leak. Am I missing something?



>
You're right, that was a real leak in v1. If jumbo_frm() failed while mapping a subsequent jumbo buffer, the buffers mapped before the failure were never unmapped, because jumbo_frm() returns -1 without reporting how many buffers it had already mapped, so the error path in stmmac_xmit() could not release them.

v2 fixes this inside jumbo_frm() itself: it now saves the starting entry and, when a subsequent dma_map_single() fails, unmaps the buffers it has already mapped before returning an error, in both ring and chain modes. This keeps the stmmac_xmit() error path unchanged and avoids changing jumbo_frm()'s return semantics.



>



>> +		if (unlikely(ret < 0) && (ret != -EINVAL))



>>? 			goto dma_map_err;



>> +		entry = ret;



>>? 	} else {



>>? 		bool last_segment = (nfrags == 0);



>>? 



>> @@ -4984,6 +4985,26 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)



>>? 



>>? dma_map_err:



>>? 	netdev_err(priv->dev, "Tx DMA map failed\n");



>> +



>> +	/* entry points one past the last descriptor written for this frame:



>> +	 * on failure it is the descriptor whose DMA mapping failed, so walk



>> +	 * from first_entry up to, but not including, entry.? Reset cur_tx



>> +	 * unconditionally as both stmmac_vlan_insert() and stmmac_jumbo_frm()



>> +	 * may have advanced it, and release the VLAN context descriptor.



>> +	 */



>> +	while (first_entry != entry) {



>> +		desc = stmmac_get_tx_desc(priv, tx_q, first_entry);



>> +		stmmac_release_tx_desc(priv, desc, priv->descriptor_mode);



>> +		stmmac_free_tx_buffer(priv, &priv->dma_conf, queue, first_entry);



>> +		first_entry = STMMAC_NEXT_ENTRY(first_entry,



>> +						priv->dma_conf.dma_tx_size);



>> +	}



>> +



>> +	tx_q->cur_tx = first_tx;



>

>do we really need to update tx_q->cur_tx here?

Yes, it is needed. Two helpers advance tx_q->cur_tx before we reach the error path: stmmac_vlan_insert() (when a VLAN tag is present) moves it past the context descriptor, and stmmac_jumbo_frm() moves it past the jumbo head descriptors on success.

If we do not roll it back to first_tx, the released VLAN context descriptor (and any jumbo head descriptors) would be left inside the [dirty_tx, cur_tx) in-flight window, so stmmac_tx_clean() would treat them as completed frames, and the next xmit would skip those slots.

Unlike the TSO path, where the allocator uses a local entry and tx_q->cur_tx is only assigned on success, these are shared helpers that advance cur_tx as a side effect, so rolling it back here is the minimal correct fix.



>



>Regards,



>Lorenzo



>



>> +	if (has_vlan) {



>> +		desc = stmmac_get_tx_desc(priv, tx_q, first_tx);



>> +		stmmac_release_tx_desc(priv, desc, priv->descriptor_mode);



>> +	}



>>? max_sdu_err:



>>? 	dev_kfree_skb(skb);



>>? 	priv->xstats.tx_dropped++;



>> 



>> ---



>> base-commit: 893e11787f78e43b534e252249ac3fff4d1333f8



>> change-id: 20260909-stmmac-fix-vlan-desc-leak-f057bb061daa



>> 



>> Best regards,



>> -- 



>> ZhaoJinming <zhaojinming@xxxxxxxxxxxxx>



>> 



>>