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

quoted

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

[...]

quoted

? 	if (unlikely(is_jumbo)) {

quoted

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

quoted

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

quoted

+		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.

quoted

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

quoted

? 			goto dma_map_err;

quoted

+		entry = ret;

quoted

? 	} else {

quoted

? 		bool last_segment = (nfrags == 0);

quoted

? 

quoted

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

quoted

? 

quoted

? dma_map_err:

quoted

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

quoted

+

quoted

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

quoted

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

quoted

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

quoted

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

quoted

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

quoted

+	 */

quoted

+	while (first_entry != entry) {

quoted

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

quoted

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

quoted

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

quoted

+		first_entry = STMMAC_NEXT_ENTRY(first_entry,

quoted

+						priv->dma_conf.dma_tx_size);

quoted

+	}

quoted

+

quoted

+	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

quoted

+	if (has_vlan) {

quoted

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

quoted

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

quoted

+	}

quoted

? max_sdu_err:

quoted

? 	dev_kfree_skb(skb);

quoted

? 	priv->xstats.tx_dropped++;

quoted

quoted

---

quoted

base-commit: 893e11787f78e43b534e252249ac3fff4d1333f8

quoted

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

quoted

quoted

Best regards,

quoted

-- 

quoted

ZhaoJinming [off-list ref]

quoted

quoted