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.
Fixes: 30d932279dc2 ("net: stmmac: Add support for VLAN Insertion Offload")
Signed-off-by: ZhaoJinming <redacted>
---
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 29 +++++++++++++++++++----
1 file changed, 25 insertions(+), 4 deletions(-)
@@ -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:+*onfailureitisthedescriptorwhoseDMAmappingfailed,sowalk+*fromfirst_entryupto,butnotincluding,entry.Resetcur_tx+*unconditionallyasbothstmmac_vlan_insert()andstmmac_jumbo_frm()+*mayhaveadvancedit,andreleasetheVLANcontextdescriptor.+*/+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++;
From: Maxime Chevallier <maxime.chevallier@bootlin.com> Date: 2026-09-10 08:52:01
Hi,
On 9/10/26 07:19, ZhaoJinming wrote:
quoted hunk
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.
Fixes: 30d932279dc2 ("net: stmmac: Add support for VLAN Insertion Offload")
Signed-off-by: ZhaoJinming <redacted>
---
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 29 +++++++++++++++++++----
1 file changed, 25 insertions(+), 4 deletions(-)
@@ -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?
Regards,
Lorenzo
+ 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.
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.
Please follow the reverse xmas tree ordering, from longest line to shortest
Done in v2: the variable declarations in stmmac_xmit() are now ordered
from longest to shortest line.
The rest seems OK. By any chance, do you have a reproducer ?
Maxime
No runtime reproducer, sorry. This was found by code review / static
analysis. The trigger requires a DMA mapping failure (dma_map_single /
skb_frag_dma_map) combined with VLAN insertion offload or fragmented
SKBs, which needs IOMMU pressure or fault injection to reproduce
reliably.
Thanks for the review.
Regards,
ZhaoJinming