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

[Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>]

[-- Attachment #1: Type: text/plain, Size: 8538 bytes --]

> In stmmac_xmit(), when the DMA mapping of the linear part or of a
> fragment fails, the error path only frees the skb.  This leaves behind
> the DMA mappings already created for the linear part and for the
> fragments mapped before the failure, which are never unmapped.
> 
> The VLAN context descriptor programmed by stmmac_vlan_insert() is also
> left behind with its OWN bit set while tx_q->cur_tx has been advanced
> past it, so the DMA engine later consumes the orphaned descriptor and
> applies its stale VLAN tag to an unrelated frame.
> 
> Release the descriptors and their DMA mappings in the dma_map_err path
> with stmmac_release_tx_desc() and stmmac_free_tx_buffer(), walking from
> first_entry to entry, then roll back tx_q->cur_tx and release the VLAN
> context descriptor.
> 
> jumbo_frm() has the same issue: when the DMA mapping of a subsequent
> jumbo buffer fails, the buffers already mapped are never unmapped.
> Unmap them before returning an error in both ring and chain modes.
> 
> Fixes: 30d932279dc2 ("net: stmmac: Add support for VLAN Insertion Offload")
> Signed-off-by: ZhaoJinming <zhaojinming@uniontech.com>

Hi ZhaoJinming,

some comments inline.

Regards,
Lorenzo

> ---
> Changes in v2:
> - Reorder stmmac_xmit() variable declarations in reverse xmas tree
>   order.
> - Unmap partially-mapped buffers in jumbo_frm() when a subsequent
>   buffer mapping fails (ring and chain modes).
> - Link to v1: https://lore.kernel.org/r/20260910-stmmac-fix-vlan-desc-leak-v1-1-6b07dca5e5e8@uniontech.com
> ---
>  drivers/net/ethernet/stmicro/stmmac/chain_mode.c  | 19 +++++++++++++--
>  drivers/net/ethernet/stmicro/stmmac/ring_mode.c   | 10 +++++++-
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 29 +++++++++++++++++++----
>  3 files changed, 51 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/chain_mode.c b/drivers/net/ethernet/stmicro/stmmac/chain_mode.c
> index 66025e2509e91f863571041f5b148d04e3708832..8415c740ee9eee32cd8b4c3e9b61517690b58f9d 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/chain_mode.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/chain_mode.c
> @@ -20,6 +20,7 @@ static int jumbo_frm(struct stmmac_tx_queue *tx_q, struct sk_buff *skb,
>  	unsigned int nopaged_len = skb_headlen(skb);
>  	struct stmmac_priv *priv = tx_q->priv_data;
>  	unsigned int entry = tx_q->cur_tx;
> +	unsigned int first_entry = entry;
>  	unsigned int bmax, buf_len;
>  	unsigned int i = 1, len;
>  	struct dma_desc *desc;
> @@ -57,7 +58,7 @@ static int jumbo_frm(struct stmmac_tx_queue *tx_q, struct sk_buff *skb,
>  					      bmax, DMA_TO_DEVICE);
>  			desc->des2 = cpu_to_le32(lower_32_bits(des2));
>  			if (dma_mapping_error(priv->device, des2))
> -				return -1;
> +				goto err_unmap;
>  			tx_q->tx_skbuff_dma[entry].buf = des2;
>  			tx_q->tx_skbuff_dma[entry].len = bmax;
>  			stmmac_prepare_tx_desc(priv, desc, 0, bmax, csum,
> @@ -70,7 +71,7 @@ static int jumbo_frm(struct stmmac_tx_queue *tx_q, struct sk_buff *skb,
>  					      DMA_TO_DEVICE);
>  			desc->des2 = cpu_to_le32(lower_32_bits(des2));
>  			if (dma_mapping_error(priv->device, des2))
> -				return -1;
> +				goto err_unmap;
>  			tx_q->tx_skbuff_dma[entry].buf = des2;
>  			tx_q->tx_skbuff_dma[entry].len = len;
>  			/* last descriptor can be set now */
> @@ -83,6 +84,20 @@ static int jumbo_frm(struct stmmac_tx_queue *tx_q, struct sk_buff *skb,
>  	tx_q->cur_tx = entry;
>  
>  	return entry;
> +
> +err_unmap:
> +	while (first_entry != entry) {
> +		dma_unmap_single(priv->device,
> +				 tx_q->tx_skbuff_dma[first_entry].buf,
> +				 tx_q->tx_skbuff_dma[first_entry].len,
> +				 DMA_TO_DEVICE);
> +		tx_q->tx_skbuff_dma[first_entry].buf = 0;
> +		tx_q->tx_skbuff_dma[first_entry].len = 0;

are you missing stmmac_release_tx_desc() here?

> +		first_entry = STMMAC_NEXT_ENTRY(first_entry,
> +						priv->dma_conf.dma_tx_size);
> +	}
> +
> +	return -1;
>  }
>  
>  static bool is_jumbo_frm(unsigned int len, bool enh_desc)
> diff --git a/drivers/net/ethernet/stmicro/stmmac/ring_mode.c b/drivers/net/ethernet/stmicro/stmmac/ring_mode.c
> index f7949419eb9fdca891a50ad0956f40e17f9b0967..4170c897bd1a8b5b757b576250e82e5407cfa325 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/ring_mode.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/ring_mode.c
> @@ -37,6 +37,7 @@ static int jumbo_frm(struct stmmac_tx_queue *tx_q, struct sk_buff *skb,
>  	len = nopaged_len - bmax;
>  
>  	if (nopaged_len > BUF_SIZE_8KiB) {
> +		unsigned int first_entry = entry;
>  
>  		des2 = dma_map_single(priv->device, skb->data, bmax,
>  				      DMA_TO_DEVICE);
> @@ -62,8 +63,15 @@ static int jumbo_frm(struct stmmac_tx_queue *tx_q, struct sk_buff *skb,
>  		des2 = dma_map_single(priv->device, skb->data + bmax, len,
>  				      DMA_TO_DEVICE);
>  		desc->des2 = cpu_to_le32(lower_32_bits(des2));
> -		if (dma_mapping_error(priv->device, des2))
> +		if (dma_mapping_error(priv->device, des2)) {
> +			dma_unmap_single(priv->device,
> +					 tx_q->tx_skbuff_dma[first_entry].buf,
> +					 bmax, DMA_TO_DEVICE);

are you missing stmmac_release_tx_desc() here?

> +			tx_q->tx_skbuff_dma[first_entry].buf = 0;
> +			tx_q->tx_skbuff_dma[first_entry].len = 0;
> +			tx_q->tx_skbuff_dma[first_entry].is_jumbo = false;
>  			return -1;
> +		}
>  		tx_q->tx_skbuff_dma[entry].buf = des2;
>  		tx_q->tx_skbuff_dma[entry].len = len;
>  		tx_q->tx_skbuff_dma[entry].is_jumbo = true;
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 24656b35350b14454fb10deced6516eb89e2c0c9..9050611862108065dfa19c1a80961bd78073df9a 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -4767,15 +4767,15 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
>  	bool enh_desc, has_vlan, set_ic, is_jumbo = false;
>  	struct stmmac_priv *priv = netdev_priv(dev);
>  	unsigned int nopaged_len = skb_headlen(skb);
> +	unsigned int first_entry, entry, tx_packets;
>  	u32 queue = skb_get_queue_mapping(skb);
>  	int nfrags = skb_shinfo(skb)->nr_frags;
> -	unsigned int first_entry, tx_packets;
>  	struct stmmac_txq_stats *txq_stats;
>  	struct dma_desc *desc, *first_desc;
>  	struct stmmac_tx_queue *tx_q;
>  	int i, csum_insertion = 0;
> -	int entry, first_tx;
>  	dma_addr_t dma_addr;
> +	int first_tx, ret;
>  	u32 sdu_len;
>  
>  	if (priv->tx_path_in_lpi_mode && priv->eee_sw_timer_en)
> @@ -4832,9 +4832,10 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
>  	csum_insertion = skb->ip_summed == CHECKSUM_PARTIAL;
>  
>  	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 (unlikely(ret < 0) && (ret != -EINVAL))

can ret be really -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;
> +	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@uniontech.com>
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]