* [PATCH v5 net 0/4] Fix short frame transmission in enetc
@ 2026-10-21 14:53 vladimir.oltean
2026-10-21 14:53 ` [PATCH v5 net 1/4] net: enetc: reserve VLAN extension BDs for software TSO vladimir.oltean
` (3 more replies)
0 siblings, 4 replies; 15+ messages in thread
From: vladimir.oltean @ 2026-10-21 14:53 UTC (permalink / raw)
To: Andrew Lunn, David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni
Cc: Claudiu Manoil, Wei Fang, Clark Wang, Ioana Ciornei,
Zefir Kurtisi, Zefir Kurtisi, David Laight, netdev, imx,
linux-kernel
This is a belated follow-up to Zefir Kurtisi's report:
https://lore.kernel.org/netdev/20260220132930.2521155-1-zefir.kurtisi@gmail.com/
which is that the ENETC misbehaves when transmitting packets smaller
than 16 octets sans FCS: it sends them but does not update the completed
index in the transmit BD ring. I did find a sentence in the reference
manual explicitly stating these short frames are not supported.
Separately, the series stops relying on the mEMAC to pad short frames
to ETH_ZLEN, because the padding it inserts is not always zero, and we
want to avoid any risk of leaking SoC-internal memory onto the wire.
- Patch 1 makes the software TSO path reserve enough TX BDs for VLAN
extension BDs. Patch 2 adds one more BD per segment to that same
reservation, so it builds on top of this fix.
- Patch 2 pads short frames to ETH_ZLEN in software, including short
software TSO segments, using a shared read-only zero buffer.
- Patch 3 makes sure the head buffer of an skb is at least 16 octets
long, which fixes the reported stall.
- Patch 4 covers the XDP_TX and ndo_xdp_xmit() paths: it pads short
frames to ETH_ZLEN in the same way, and drops frames whose head
buffer is shorter than 16 octets.
Review of previous versions found a number of pre-existing issues in
the code surrounding these changes, and v4 grew to 8 patches to address
them. Review of v4 found even more. To be able to make progress, v5 is
reduced to the patches strictly needed for the stated purpose. The
other fixes will be submitted separately.
v1 at:
https://lore.kernel.org/netdev/20260401172246.1075883-1-vladimir.oltean@nxp.com/
v2 at:
https://lore.kernel.org/netdev/20260406204122.167237-1-vladimir.oltean@nxp.com/
v3 at:
https://lore.kernel.org/netdev/20260915222735.1016937-5-vladimir.oltean@nxp.com/
v4 at:
https://lore.kernel.org/netdev/20261015105536.1692348-7-vladimir.oltean@nxp.com/
Change log in individual patches. At a high level:
- dropped the following v4 patches, which are unrelated to short frames
and will be resubmitted separately:
- 1/8: consistently track dropped frames in enetc_xdp_xmit()
- 2/8: serialize access to the same TX ring from NAPI XDP_TX and
ndo_xdp_xmit()
- 3/8: fix bogus TX ring consumer index after reinitialization
- 7/8: fix leak of pending XDP_TX frames when going down
- 8/8: ensure enetc_xdp_xmit() calls enetc_update_tx_ring_tail()
- new patch 1/4: reserve VLAN extension BDs for software TSO
- patch 2/4: account for the pad BD in enetc_tso_count_descs() and in
the per-frame chained BD limit; explain why hardware LSO is not
affected
- patches 3/4 and 4/4 are unchanged
Vladimir Oltean (4):
net: enetc: reserve VLAN extension BDs for software TSO
net: enetc: do not rely on mEMAC hardware frame padding
net: enetc: ensure head buffer has at least 16 octets
net: enetc: pad short XDP frames in software
drivers/net/ethernet/freescale/enetc/enetc.c | 114 +++++++++++++++++--
drivers/net/ethernet/freescale/enetc/enetc.h | 8 ++
2 files changed, 113 insertions(+), 9 deletions(-)
--
2.43.0
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH v5 net 1/4] net: enetc: reserve VLAN extension BDs for software TSO 2026-10-21 14:53 [PATCH v5 net 0/4] Fix short frame transmission in enetc vladimir.oltean @ 2026-10-21 14:53 ` vladimir.oltean 2026-10-25 15:21 ` netdev-bot+sashiko 2026-10-21 14:53 ` [PATCH v5 net 2/4] net: enetc: do not rely on mEMAC hardware frame padding vladimir.oltean ` (2 subsequent siblings) 3 siblings, 1 reply; 15+ messages in thread From: vladimir.oltean @ 2026-10-21 14:53 UTC (permalink / raw) To: Andrew Lunn, David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni Cc: Claudiu Manoil, Wei Fang, Clark Wang, Ioana Ciornei, Zefir Kurtisi, Zefir Kurtisi, David Laight, netdev, imx, linux-kernel enetc_start_xmit() uses tso_count_descs() to check whether the TX ring has enough free BDs for a software TSO frame. That estimates one header BD and one data BD per segment, plus one BD per frag. However, when the skb carries a VLAN tag, enetc_map_tx_tso_hdr() also adds an extension BD to every segment, which is not accounted for. With a highly segmented VLAN TSO skb and a nearly full ring, the driver can then write past next_to_clean and overwrite BDs still owned by the hardware. Add enetc_tso_count_descs() which accounts for the extension BD of each segment when VLAN insertion is needed. Fixes: fb8629e2cbfc ("net: enetc: add support for software TSO") Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- v4->v5: patch is new --- drivers/net/ethernet/freescale/enetc/enetc.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c index c6571b5cc842..f6b352a3a49c 100644 --- a/drivers/net/ethernet/freescale/enetc/enetc.c +++ b/drivers/net/ethernet/freescale/enetc/enetc.c @@ -685,6 +685,17 @@ static int enetc_lso_count_descs(const struct sk_buff *skb) return skb_shinfo(skb)->nr_frags + 4; } +static int enetc_tso_count_descs(const struct sk_buff *skb) +{ + int count = tso_count_descs(skb); + + /* Each segment also needs an extension BD for VLAN insertion */ + if (skb_vlan_tag_present(skb)) + count += skb_shinfo(skb)->gso_segs; + + return count; +} + static int enetc_lso_get_hdr_len(const struct sk_buff *skb) { int hdr_len, tlen; @@ -1029,7 +1040,7 @@ static netdev_tx_t enetc_start_xmit(struct sk_buff *skb, count = enetc_lso_hw_offload(tx_ring, skb); } else { - if (enetc_bd_unused(tx_ring) < tso_count_descs(skb)) { + if (enetc_bd_unused(tx_ring) < enetc_tso_count_descs(skb)) { netif_stop_subqueue(ndev, tx_ring->index); return NETDEV_TX_BUSY; } -- 2.43.0 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v5 net 1/4] net: enetc: reserve VLAN extension BDs for software TSO 2026-10-21 14:53 ` [PATCH v5 net 1/4] net: enetc: reserve VLAN extension BDs for software TSO vladimir.oltean @ 2026-10-25 15:21 ` netdev-bot+sashiko 0 siblings, 0 replies; 15+ messages in thread From: netdev-bot+sashiko @ 2026-10-25 15:21 UTC (permalink / raw) To: vladimir.oltean Cc: andrew+netdev, davem, edumazet, kuba, pabeni, claudiu.manoil, wei.fang, xiaoning.wang, ioana.ciornei, zefir.kurtisi, zefir.kurtisi, david.laight.linux, netdev, imx, linux-kernel Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider. Critical: 0 · High: 1 (2) · Medium: 0 · Low: 0 If you believe none of the issues are legitimate, please reply to each issue of Medium severity or higher and explain why the AI is wrong. If any of them are legitimate and you plan to respin, please let patchwork know by including "pw-bot: cr" as a separate line at the end of your reply (one such reply per series is enough). - [High] The fix stops the ring overrun, but VLAN-tagged TSO skbs that can never fit now stall the TX queue for good. Pre-existing issues: - [High] This problem wasn't introduced by this patch, but the BUSY path the patch changes can lose a wakeup. - [High] This problem wasn't introduced by this patch, but enetc_map_tx_tso_buffs() copies the whole L2+L3+L4 header into a fixed… -- net: enetc: reserve VLAN extension BDs for software TSO This commit adds enetc_tso_count_descs(). It extends tso_count_descs() with one extension BD per segment when the skb carries a VLAN tag. enetc_start_xmit() now uses it for the free BD check on the software TSO path. > diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c > index 80f0082f6c633..ab4bfefda6c8f 100644 > --- a/drivers/net/ethernet/freescale/enetc/enetc.c > +++ b/drivers/net/ethernet/freescale/enetc/enetc.c > @@ -688,6 +688,17 @@ static int enetc_lso_count_descs(const struct sk_buff *skb) > return skb_shinfo(skb)->nr_frags + 4; > } > > +static int enetc_tso_count_descs(const struct sk_buff *skb) > +{ > + int count = tso_count_descs(skb); > + > + /* Each segment also needs an extension BD for VLAN insertion */ > + if (skb_vlan_tag_present(skb)) > + count += skb_shinfo(skb)->gso_segs; [Severity: High] Can this estimate exceed what the TX ring can ever provide? If so, the queue would stop permanently instead of overrunning. For a VLAN skb this returns 3 * gso_segs + nr_frags. The TX ring is fixed at ENETC_TX_RING_DEFAULT_SIZE (2048) and there is no set_ringparam. So enetc_bd_unused() returns at most 2047, even on an empty ring. The driver does not limit the segment count anywhere. There is no netif_set_tso_max_segs(), no gso_max_segs and no ndo_features_check. Take a 64KB TSO skb with an MSS of about 96 or less (gso_segs of roughly 683 or more). enetc_start_xmit() takes this path on every attempt: if (enetc_bd_unused(tx_ring) < enetc_tso_count_descs(skb)) { netif_stop_subqueue(ndev, tx_ring->index); return NETDEV_TX_BUSY; } The qdisc requeues the same skb. The only runtime wake is in enetc_clean_tx_ring(): if (unlikely(tx_frm_cnt && netif_carrier_ok(ndev) && __netif_subqueue_stopped(ndev, tx_ring->index) && ... netif_wake_subqueue(ndev, tx_ring->index); Once the ring drains, tx_frm_cnt stays 0. There is also no ndo_tx_timeout, so nothing restarts the queue. This can be triggered by a TCP sender on a VLAN on top of enetc using the software TSO path. A remote peer can force a small MSS through its SYN (down to tcp_min_snd_mss), and a local user can use TCP_MAXSEG. Before this patch, most of these skbs overran the ring. Now they hang the queue instead. The same stall already existed for non-VLAN skbs above about 1016 segments. This change moves VLAN skbs with about 683 to 1015 segments into it. The estimate is a worst case, so some VLAN skbs that really fit are also rejected forever. For example, gso_segs=680 with 17 segment-aligned frags needs 2040 BDs, but the estimate is 2057. The padding patch in this series ("net: enetc: do not rely on mEMAC hardware frame padding") adds another gso_segs to the estimate. That lowers the threshold to about 508 segments with VLAN and about 677 without. Would it make sense to cap the segment count to what the ring can hold? For example: netif_set_tso_max_segs(ndev, (bd_count - 1 - MAX_SKB_FRAGS) / per_seg_bds); Another option is an ndo_features_check that clears GSO when the count exceeds bd_count - 1, so the core falls back to software GSO. > + > + return count; > +} > + [ ... ] > @@ -1032,7 +1043,7 @@ static netdev_tx_t enetc_start_xmit(struct sk_buff *skb, > > count = enetc_lso_hw_offload(tx_ring, skb); > } else { > - if (enetc_bd_unused(tx_ring) < tso_count_descs(skb)) { > + if (enetc_bd_unused(tx_ring) < enetc_tso_count_descs(skb)) { > netif_stop_subqueue(ndev, tx_ring->index); > return NETDEV_TX_BUSY; > } [Severity: High] This is a pre-existing issue, not one introduced by this patch. Can this stop path lose a wakeup? enetc_bd_unused() reads tx_ring->next_to_clean without a lock. The queue is then stopped with no smp_mb() and no re-check of free space. On another CPU, enetc_clean_tx_ring() updates next_to_clean. It wakes the queue only if it already sees the queue stopped and tx_frm_cnt is non-zero: CPU0 (enetc_start_xmit) CPU1 (enetc_clean_tx_ring) enetc_bd_unused() reads stale next_to_clean, not enough room cleans the last in-flight frames tx_ring->next_to_clean = i; __netif_subqueue_stopped() is false, no wake netif_stop_subqueue() return NETDEV_TX_BUSY At that point the ring has plenty of room, but no BDs are in flight. No completion will call netif_wake_subqueue(). The only other wake is netif_tx_start_all_queues() on open, and there is no ndo_tx_timeout. The same pattern is present at the LSO, non-GSO and post-xmit stop sites. This patch raises the BD requirement for VLAN TSO skbs, so this path now runs more often, including when only a few short frames are in flight. Would netif_txq_maybe_stop() / netif_txq_completed_wake() fit here? Alternatively, an smp_mb() plus a re-check after stopping could be paired with an smp_mb() in the cleaner before it tests the stopped bit. [Severity: High] This isn't a bug introduced by this patch, but it is in the same software TSO path. Can enetc_map_tx_tso_buffs() write past its tx_ring->tso_headers slot when the headers are longer than TSO_HEADER_SIZE? hdr = tx_ring->tso_headers + i * TSO_HEADER_SIZE; tso_build_hdr(skb, hdr, &tso, data_len, total_len == 0); tso_build_hdr() copies hdr_len bytes, where hdr_len comes from tso_start() and covers the full L2+L3+L4 header. Nothing compares it against TSO_HEADER_SIZE (256). enetc_lso_map_hdr() does the same: hdr = tx_ring->tso_headers + *i * TSO_HEADER_SIZE; memcpy(hdr, skb->data, lso->hdr_len); enetc_alloc_tx_resource() allocates exactly bd_count * TSO_HEADER_SIZE bytes for this buffer. An oversized header therefore spills into the next slot. For the last ring index, it writes past the end of the dma_alloc_coherent() allocation. One possible trigger is an IPv6 TCP socket with a sticky IPV6_RTHDR type-4 SRH, which doesn't need CAP_NET_RAW. With 11 or more segments, the header is 14 + 40 + 184 + 32 = 270 bytes. The IPv6 setsockopt and net/core/tso.c side hasn't been fully traced in this tree, so the trigger may still need confirming. Should skbs with hdr_len > TSO_HEADER_SIZE fall back to software segmentation or be dropped? Clearing GSO for them in ndo_features_check would be another option. -- Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261021145314.1435049-1-vladimir.oltean%40nxp.com ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v5 net 2/4] net: enetc: do not rely on mEMAC hardware frame padding 2026-10-21 14:53 [PATCH v5 net 0/4] Fix short frame transmission in enetc vladimir.oltean 2026-10-21 14:53 ` [PATCH v5 net 1/4] net: enetc: reserve VLAN extension BDs for software TSO vladimir.oltean @ 2026-10-21 14:53 ` vladimir.oltean 2026-10-22 8:33 ` David Laight 2026-10-25 15:21 ` netdev-bot+sashiko 2026-10-21 14:53 ` [PATCH v5 net 3/4] net: enetc: ensure head buffer has at least 16 octets vladimir.oltean 2026-10-21 14:53 ` [PATCH v5 net 4/4] net: enetc: pad short XDP frames in software vladimir.oltean 3 siblings, 2 replies; 15+ messages in thread From: vladimir.oltean @ 2026-10-21 14:53 UTC (permalink / raw) To: Andrew Lunn, David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni Cc: Claudiu Manoil, Wei Fang, Clark Wang, Ioana Ciornei, Zefir Kurtisi, Zefir Kurtisi, David Laight, netdev, imx, linux-kernel The driver sets the ENETC_PM0_CMD_TXP bit to pad short buffers. This works, however sending short frames from user space and looking at the padding bytes inserted by the MAC at the link partner, we notice on LS1028A that the first octet of padding is sometimes not 0. This is fine per 802.3, which does not require that the padding is zero. However, there is an unproven suspicion that the padding bytes may come from internal memories, and unfortunately I couldn't get sufficient priority for this octet's origin to be investigated by hardware engineers. To be defensive and not leak any sensitive information from the SoC, let's pad all frames to ETH_ZLEN in software, so that the MAC doesn't have to do anything. There are 2 distinct cases to handle, for GSO and non-GSO. For non-GSO things are trivial; we need to call eth_skb_pad(). For GSO, things are not simple, because user space may set gso_size to a very small value, which could make the GSO super-skb smaller than ETH_ZLEN; padding that would produce more segments than originally intended. What needs to be done is to pad individual segments instead. When a GSO segment's hdr_len + data_len is below ETH_ZLEN, append a zero-padding data BD and advertise the padded length in the header BD's frm_len. The padding bytes are always the same zeros, so a single read-only DMA-coherent buffer (tx_pad, ETH_ZLEN bytes, zero-initialized by dma_alloc_coherent() and never written) is shared by all padding BDs of the ring. Like the header BD, a padding BD carries no per-BD DMA address, so cleanup does not unmap it. The pad BD becomes the segment's last BD, so the F flag and the skb pointer move onto it. Segment checksums are unaffected as they are computed over data_len only, before padding. The worst-case descriptor reservation grows by one BD per segment. The pad BD also counts towards the per-frame chained BD limit (15 on LS1028A, including the head and extension BD), so a padded segment may use one data BD less. Only LS1028A (NETCv1) needs this. On i.MX95 (NETC v4.1), it was observed that padding bytes are always zero, and the reference manual also documents in "106.3.5.1.2.1.3.7 FCS generation and append on transmit" that HTA Transmit pads short frames (including LSO segment frames) with zeros before appending the FCS. On LS1028A it does not say this (padding seems an exclusively mEMAC task). Nonetheless, for simplicity's sake, we keep the unconditional eth_skb_pad() call in the common transmit path, but we leave the hardware LSO transmit path unpadded in software. The latter is only present on NETC v4.1 and later (thus already contains zeroes). Fixes: fb8629e2cbfc ("net: enetc: add support for software TSO") Fixes: d4fd0404c1c9 ("enetc: Introduce basic PF and VF ENETC ethernet drivers") Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- v4->v5: - account for the pad BD in the new enetc_tso_count_descs() helper from "net: enetc: reserve VLAN extension BDs for software TSO", instead of open-coding it in enetc_start_xmit() - count the pad BD towards the per-frame chained BD limit, so a padded segment may use one data BD less - explain in the commit message why hardware LSO is not affected v3->v4: - split from "net: enetc: pad short frames in software", bring its own justification: https://lore.kernel.org/netdev/20260921112946.edo5uw6cl7jx6oez@skbuf/ --- drivers/net/ethernet/freescale/enetc/enetc.c | 61 ++++++++++++++++++-- drivers/net/ethernet/freescale/enetc/enetc.h | 6 ++ 2 files changed, 62 insertions(+), 5 deletions(-) diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c index f6b352a3a49c..a9ea124f7835 100644 --- a/drivers/net/ethernet/freescale/enetc/enetc.c +++ b/drivers/net/ethernet/freescale/enetc/enetc.c @@ -524,7 +524,7 @@ static int enetc_map_tx_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb) static int enetc_map_tx_tso_hdr(struct enetc_bdr *tx_ring, struct sk_buff *skb, struct enetc_tx_swbd *tx_swbd, union enetc_tx_bd *txbd, int *i, int hdr_len, - int data_len) + int data_len, int frm_len) { union enetc_tx_bd txbd_tmp; u8 flags = 0, e_flags = 0; @@ -541,7 +541,7 @@ static int enetc_map_tx_tso_hdr(struct enetc_bdr *tx_ring, struct sk_buff *skb, txbd_tmp.buf_len = cpu_to_le16(hdr_len); /* first BD needs frm_len and offload flags set */ - txbd_tmp.frm_len = cpu_to_le16(hdr_len + data_len); + txbd_tmp.frm_len = cpu_to_le16(frm_len); txbd_tmp.flags = flags; /* For the TSO header we do not set the dma address since we do not @@ -613,6 +613,22 @@ static int enetc_map_tx_tso_data(struct enetc_bdr *tx_ring, struct sk_buff *skb, return 0; } +static void enetc_append_tx_pad(struct enetc_bdr *tx_ring, int i, int pad_len) +{ + struct enetc_tx_swbd *tx_swbd = &tx_ring->tx_swbd[i]; + union enetc_tx_bd *txbd = ENETC_TXBD(*tx_ring, i); + + enetc_clear_tx_bd(txbd); + txbd->addr = cpu_to_le64(tx_ring->tx_pad_dma); + txbd->buf_len = cpu_to_le16(pad_len); + txbd->flags = ENETC_TXBD_FLAGS_F; + + tx_swbd->len = pad_len; + tx_swbd->is_eof = true; + tx_swbd->do_twostep_tstamp = false; + tx_swbd->check_wb = false; +} + static __wsum enetc_tso_hdr_csum(struct tso_t *tso, struct sk_buff *skb, char *hdr, int hdr_len, int *l4_hdr_len) { @@ -687,7 +703,8 @@ static int enetc_lso_count_descs(const struct sk_buff *skb) static int enetc_tso_count_descs(const struct sk_buff *skb) { - int count = tso_count_descs(skb); + /* +gso_segs to cover one pad BD per segment */ + int count = tso_count_descs(skb) + skb_shinfo(skb)->gso_segs; /* Each segment also needs an extension BD for VLAN insertion */ if (skb_vlan_tag_present(skb)) @@ -926,6 +943,9 @@ static int enetc_map_tx_tso_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb i = tx_ring->next_to_use; while (total_len > 0) { + int max_data_bds; + int frm_len; + int pad_len; char *hdr; /* Get the BD */ @@ -937,6 +957,12 @@ static int enetc_map_tx_tso_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb data_len = min_t(int, skb_shinfo(skb)->gso_size, total_len); total_len -= data_len; + /* Pad segments below ETH_ZLEN so the mEMAC does not pad them. */ + frm_len = max_t(int, hdr_len + data_len, ETH_ZLEN); + pad_len = frm_len - (hdr_len + data_len); + /* The pad BD counts towards the max chained BD limit */ + max_data_bds = priv->max_frags - !!pad_len; + /* prepare packet headers: MAC + IP + TCP */ hdr = tx_ring->tso_headers + i * TSO_HEADER_SIZE; tso_build_hdr(skb, hdr, &tso, data_len, total_len == 0); @@ -944,7 +970,7 @@ static int enetc_map_tx_tso_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb /* compute the csum over the L4 header */ csum = enetc_tso_hdr_csum(&tso, skb, hdr, hdr_len, &pos); count += enetc_map_tx_tso_hdr(tx_ring, skb, tx_swbd, txbd, - &i, hdr_len, data_len); + &i, hdr_len, data_len, frm_len); bd_data_num = 0; while (data_len > 0) { @@ -966,8 +992,10 @@ static int enetc_map_tx_tso_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb csum = csum_block_add(csum, csum2, pos); pos += size; + /* When padding, the pad BD is the last one, not this */ err = enetc_map_tx_tso_data(tx_ring, skb, tx_swbd, txbd, tso.data, size, + !pad_len && size == data_len); if (err) { if (i == 0) @@ -982,12 +1010,19 @@ static int enetc_map_tx_tso_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb bd_data_num++; tso_build_data(skb, &tso, size); - if (unlikely(bd_data_num >= priv->max_frags && data_len)) + if (unlikely(bd_data_num >= max_data_bds && data_len)) goto err_chained_bd; } enetc_tso_complete_csum(tx_ring, &tso, skb, hdr, pos, csum); + if (pad_len) { + enetc_bdr_idx_inc(tx_ring, &i); + enetc_append_tx_pad(tx_ring, i, pad_len); + tx_swbd = &tx_ring->tx_swbd[i]; + count++; + } + if (total_len == 0) tx_swbd->skb = skb; @@ -1050,6 +1085,9 @@ static netdev_tx_t enetc_start_xmit(struct sk_buff *skb, enetc_unlock_mdio(); } } else { + if (eth_skb_pad(skb)) + return NETDEV_TX_OK; + if (unlikely(skb_shinfo(skb)->nr_frags > priv->max_frags)) if (unlikely(skb_linearize(skb))) goto drop_packet_err; @@ -2285,8 +2323,18 @@ static int enetc_alloc_tx_resource(struct enetc_bdr_resource *res, goto err_alloc_tso; } + res->tx_pad = dma_alloc_coherent(dev, ETH_ZLEN, &res->tx_pad_dma, + GFP_KERNEL); + if (!res->tx_pad) { + err = -ENOMEM; + goto err_alloc_pad; + } + return 0; +err_alloc_pad: + dma_free_coherent(dev, bd_count * TSO_HEADER_SIZE, res->tso_headers, + res->tso_headers_dma); err_alloc_tso: enetc_dma_free_bdr(res); err_alloc_bdr: @@ -2298,6 +2346,7 @@ static int enetc_alloc_tx_resource(struct enetc_bdr_resource *res, static void enetc_free_tx_resource(const struct enetc_bdr_resource *res) { + dma_free_coherent(res->dev, ETH_ZLEN, res->tx_pad, res->tx_pad_dma); dma_free_coherent(res->dev, res->bd_count * TSO_HEADER_SIZE, res->tso_headers, res->tso_headers_dma); enetc_dma_free_bdr(res); @@ -2425,6 +2474,8 @@ static void enetc_assign_tx_resource(struct enetc_bdr *tx_ring, tx_ring->tx_swbd = res ? res->tx_swbd : NULL; tx_ring->tso_headers = res ? res->tso_headers : NULL; tx_ring->tso_headers_dma = res ? res->tso_headers_dma : 0; + tx_ring->tx_pad = res ? res->tx_pad : NULL; + tx_ring->tx_pad_dma = res ? res->tx_pad_dma : 0; } static void enetc_assign_rx_resource(struct enetc_bdr *rx_ring, diff --git a/drivers/net/ethernet/freescale/enetc/enetc.h b/drivers/net/ethernet/freescale/enetc/enetc.h index d9e91832a9c1..db3b3bad0f59 100644 --- a/drivers/net/ethernet/freescale/enetc/enetc.h +++ b/drivers/net/ethernet/freescale/enetc/enetc.h @@ -145,6 +145,8 @@ struct enetc_bdr_resource { }; char *tso_headers; dma_addr_t tso_headers_dma; + void *tx_pad; + dma_addr_t tx_pad_dma; }; struct enetc_bdr { @@ -182,6 +184,10 @@ struct enetc_bdr { /* DMA buffer for TSO headers */ char *tso_headers; dma_addr_t tso_headers_dma; + + /* Shared read-only zero buffer to pad short frames to ETH_ZLEN */ + void *tx_pad; + dma_addr_t tx_pad_dma; } ____cacheline_aligned_in_smp; static inline void enetc_bdr_idx_inc(struct enetc_bdr *bdr, int *i) -- 2.43.0 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v5 net 2/4] net: enetc: do not rely on mEMAC hardware frame padding 2026-10-21 14:53 ` [PATCH v5 net 2/4] net: enetc: do not rely on mEMAC hardware frame padding vladimir.oltean @ 2026-10-22 8:33 ` David Laight 2026-10-22 9:41 ` Vladimir Oltean 2026-10-25 15:21 ` netdev-bot+sashiko 1 sibling, 1 reply; 15+ messages in thread From: David Laight @ 2026-10-22 8:33 UTC (permalink / raw) To: vladimir.oltean Cc: Andrew Lunn, David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Claudiu Manoil, Wei Fang, Clark Wang, Ioana Ciornei, Zefir Kurtisi, Zefir Kurtisi, netdev, imx, linux-kernel On Wed, 21 Oct 2026 17:53:12 +0300 vladimir.oltean@nxp.com wrote: > The driver sets the ENETC_PM0_CMD_TXP bit to pad short buffers. > This works, however sending short frames from user space and looking at > the padding bytes inserted by the MAC at the link partner, we notice on > LS1028A that the first octet of padding is sometimes not 0. > > This is fine per 802.3, which does not require that the padding is zero. > > However, there is an unproven suspicion that the padding bytes may > come from internal memories, and unfortunately I couldn't get sufficient > priority for this octet's origin to be investigated by hardware > engineers. > > To be defensive and not leak any sensitive information from the SoC, > let's pad all frames to ETH_ZLEN in software, so that the MAC doesn't > have to do anything. > > There are 2 distinct cases to handle, for GSO and non-GSO. For non-GSO > things are trivial; we need to call eth_skb_pad(). For GSO, things are > not simple, because user space may set gso_size to a very small value, > which could make the GSO super-skb smaller than ETH_ZLEN; That isn't sensible. The minimum frame is 64 bytes (including FCS), subtract that, the MAC header (14 bytes), minimum IP header (20), minimum TCP (20) and that leaves 6 bytes for user data. I doubt it is sensible to do GSO for anything under the minimum mtu (128 bytes rings a bell somewhere). But does this code allow for the VLAN tag that you forgot about in the other patch? I also noticed that the code seems to dma map the fragments for each gso segment separately, just mapping each skb fragment would be better. (But that is for another patch series.) David > padding that > would produce more segments than originally intended. What needs to be > done is to pad individual segments instead. > > When a GSO segment's hdr_len + data_len is below ETH_ZLEN, append a > zero-padding data BD and advertise the padded length in the header BD's > frm_len. > > The padding bytes are always the same zeros, so a single read-only > DMA-coherent buffer (tx_pad, ETH_ZLEN bytes, zero-initialized by > dma_alloc_coherent() and never written) is shared by all padding BDs of > the ring. Like the header BD, a padding BD carries no per-BD DMA address, > so cleanup does not unmap it. The pad BD becomes the segment's last BD, > so the F flag and the skb pointer move onto it. Segment checksums are > unaffected as they are computed over data_len only, before padding. > > The worst-case descriptor reservation grows by one BD per segment. > The pad BD also counts towards the per-frame chained BD limit (15 on > LS1028A, including the head and extension BD), so a padded segment may > use one data BD less. > > Only LS1028A (NETCv1) needs this. On i.MX95 (NETC v4.1), it was observed > that padding bytes are always zero, and the reference manual also > documents in "106.3.5.1.2.1.3.7 FCS generation and append on transmit" > that HTA Transmit pads short frames (including LSO segment frames) with > zeros before appending the FCS. On LS1028A it does not say this (padding > seems an exclusively mEMAC task). > > Nonetheless, for simplicity's sake, we keep the unconditional eth_skb_pad() > call in the common transmit path, but we leave the hardware LSO transmit > path unpadded in software. The latter is only present on NETC v4.1 and > later (thus already contains zeroes). > > Fixes: fb8629e2cbfc ("net: enetc: add support for software TSO") > Fixes: d4fd0404c1c9 ("enetc: Introduce basic PF and VF ENETC ethernet drivers") > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > --- > v4->v5: > - account for the pad BD in the new enetc_tso_count_descs() helper > from "net: enetc: reserve VLAN extension BDs for software TSO", > instead of open-coding it in enetc_start_xmit() > - count the pad BD towards the per-frame chained BD limit, so a padded > segment may use one data BD less > - explain in the commit message why hardware LSO is not affected > v3->v4: > - split from "net: enetc: pad short frames in software", bring its own > justification: > https://lore.kernel.org/netdev/20260921112946.edo5uw6cl7jx6oez@skbuf/ > --- > drivers/net/ethernet/freescale/enetc/enetc.c | 61 ++++++++++++++++++-- > drivers/net/ethernet/freescale/enetc/enetc.h | 6 ++ > 2 files changed, 62 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c > index f6b352a3a49c..a9ea124f7835 100644 > --- a/drivers/net/ethernet/freescale/enetc/enetc.c > +++ b/drivers/net/ethernet/freescale/enetc/enetc.c > @@ -524,7 +524,7 @@ static int enetc_map_tx_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb) > static int enetc_map_tx_tso_hdr(struct enetc_bdr *tx_ring, struct sk_buff *skb, > struct enetc_tx_swbd *tx_swbd, > union enetc_tx_bd *txbd, int *i, int hdr_len, > - int data_len) > + int data_len, int frm_len) > { > union enetc_tx_bd txbd_tmp; > u8 flags = 0, e_flags = 0; > @@ -541,7 +541,7 @@ static int enetc_map_tx_tso_hdr(struct enetc_bdr *tx_ring, struct sk_buff *skb, > txbd_tmp.buf_len = cpu_to_le16(hdr_len); > > /* first BD needs frm_len and offload flags set */ > - txbd_tmp.frm_len = cpu_to_le16(hdr_len + data_len); > + txbd_tmp.frm_len = cpu_to_le16(frm_len); > txbd_tmp.flags = flags; > > /* For the TSO header we do not set the dma address since we do not > @@ -613,6 +613,22 @@ static int enetc_map_tx_tso_data(struct enetc_bdr *tx_ring, struct sk_buff *skb, > return 0; > } > > +static void enetc_append_tx_pad(struct enetc_bdr *tx_ring, int i, int pad_len) > +{ > + struct enetc_tx_swbd *tx_swbd = &tx_ring->tx_swbd[i]; > + union enetc_tx_bd *txbd = ENETC_TXBD(*tx_ring, i); > + > + enetc_clear_tx_bd(txbd); > + txbd->addr = cpu_to_le64(tx_ring->tx_pad_dma); > + txbd->buf_len = cpu_to_le16(pad_len); > + txbd->flags = ENETC_TXBD_FLAGS_F; > + > + tx_swbd->len = pad_len; > + tx_swbd->is_eof = true; > + tx_swbd->do_twostep_tstamp = false; > + tx_swbd->check_wb = false; > +} > + > static __wsum enetc_tso_hdr_csum(struct tso_t *tso, struct sk_buff *skb, > char *hdr, int hdr_len, int *l4_hdr_len) > { > @@ -687,7 +703,8 @@ static int enetc_lso_count_descs(const struct sk_buff *skb) > > static int enetc_tso_count_descs(const struct sk_buff *skb) > { > - int count = tso_count_descs(skb); > + /* +gso_segs to cover one pad BD per segment */ > + int count = tso_count_descs(skb) + skb_shinfo(skb)->gso_segs; > > /* Each segment also needs an extension BD for VLAN insertion */ > if (skb_vlan_tag_present(skb)) > @@ -926,6 +943,9 @@ static int enetc_map_tx_tso_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb > i = tx_ring->next_to_use; > > while (total_len > 0) { > + int max_data_bds; > + int frm_len; > + int pad_len; > char *hdr; > > /* Get the BD */ > @@ -937,6 +957,12 @@ static int enetc_map_tx_tso_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb > data_len = min_t(int, skb_shinfo(skb)->gso_size, total_len); > total_len -= data_len; > > + /* Pad segments below ETH_ZLEN so the mEMAC does not pad them. */ > + frm_len = max_t(int, hdr_len + data_len, ETH_ZLEN); > + pad_len = frm_len - (hdr_len + data_len); > + /* The pad BD counts towards the max chained BD limit */ > + max_data_bds = priv->max_frags - !!pad_len; > + > /* prepare packet headers: MAC + IP + TCP */ > hdr = tx_ring->tso_headers + i * TSO_HEADER_SIZE; > tso_build_hdr(skb, hdr, &tso, data_len, total_len == 0); > @@ -944,7 +970,7 @@ static int enetc_map_tx_tso_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb > /* compute the csum over the L4 header */ > csum = enetc_tso_hdr_csum(&tso, skb, hdr, hdr_len, &pos); > count += enetc_map_tx_tso_hdr(tx_ring, skb, tx_swbd, txbd, > - &i, hdr_len, data_len); > + &i, hdr_len, data_len, frm_len); > bd_data_num = 0; > > while (data_len > 0) { > @@ -966,8 +992,10 @@ static int enetc_map_tx_tso_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb > csum = csum_block_add(csum, csum2, pos); > pos += size; > > + /* When padding, the pad BD is the last one, not this */ > err = enetc_map_tx_tso_data(tx_ring, skb, tx_swbd, txbd, > tso.data, size, > + !pad_len && > size == data_len); > if (err) { > if (i == 0) > @@ -982,12 +1010,19 @@ static int enetc_map_tx_tso_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb > bd_data_num++; > tso_build_data(skb, &tso, size); > > - if (unlikely(bd_data_num >= priv->max_frags && data_len)) > + if (unlikely(bd_data_num >= max_data_bds && data_len)) > goto err_chained_bd; > } > > enetc_tso_complete_csum(tx_ring, &tso, skb, hdr, pos, csum); > > + if (pad_len) { > + enetc_bdr_idx_inc(tx_ring, &i); > + enetc_append_tx_pad(tx_ring, i, pad_len); > + tx_swbd = &tx_ring->tx_swbd[i]; > + count++; > + } > + > if (total_len == 0) > tx_swbd->skb = skb; > > @@ -1050,6 +1085,9 @@ static netdev_tx_t enetc_start_xmit(struct sk_buff *skb, > enetc_unlock_mdio(); > } > } else { > + if (eth_skb_pad(skb)) > + return NETDEV_TX_OK; > + > if (unlikely(skb_shinfo(skb)->nr_frags > priv->max_frags)) > if (unlikely(skb_linearize(skb))) > goto drop_packet_err; > @@ -2285,8 +2323,18 @@ static int enetc_alloc_tx_resource(struct enetc_bdr_resource *res, > goto err_alloc_tso; > } > > + res->tx_pad = dma_alloc_coherent(dev, ETH_ZLEN, &res->tx_pad_dma, > + GFP_KERNEL); > + if (!res->tx_pad) { > + err = -ENOMEM; > + goto err_alloc_pad; > + } > + > return 0; > > +err_alloc_pad: > + dma_free_coherent(dev, bd_count * TSO_HEADER_SIZE, res->tso_headers, > + res->tso_headers_dma); > err_alloc_tso: > enetc_dma_free_bdr(res); > err_alloc_bdr: > @@ -2298,6 +2346,7 @@ static int enetc_alloc_tx_resource(struct enetc_bdr_resource *res, > > static void enetc_free_tx_resource(const struct enetc_bdr_resource *res) > { > + dma_free_coherent(res->dev, ETH_ZLEN, res->tx_pad, res->tx_pad_dma); > dma_free_coherent(res->dev, res->bd_count * TSO_HEADER_SIZE, > res->tso_headers, res->tso_headers_dma); > enetc_dma_free_bdr(res); > @@ -2425,6 +2474,8 @@ static void enetc_assign_tx_resource(struct enetc_bdr *tx_ring, > tx_ring->tx_swbd = res ? res->tx_swbd : NULL; > tx_ring->tso_headers = res ? res->tso_headers : NULL; > tx_ring->tso_headers_dma = res ? res->tso_headers_dma : 0; > + tx_ring->tx_pad = res ? res->tx_pad : NULL; > + tx_ring->tx_pad_dma = res ? res->tx_pad_dma : 0; > } > > static void enetc_assign_rx_resource(struct enetc_bdr *rx_ring, > diff --git a/drivers/net/ethernet/freescale/enetc/enetc.h b/drivers/net/ethernet/freescale/enetc/enetc.h > index d9e91832a9c1..db3b3bad0f59 100644 > --- a/drivers/net/ethernet/freescale/enetc/enetc.h > +++ b/drivers/net/ethernet/freescale/enetc/enetc.h > @@ -145,6 +145,8 @@ struct enetc_bdr_resource { > }; > char *tso_headers; > dma_addr_t tso_headers_dma; > + void *tx_pad; > + dma_addr_t tx_pad_dma; > }; > > struct enetc_bdr { > @@ -182,6 +184,10 @@ struct enetc_bdr { > /* DMA buffer for TSO headers */ > char *tso_headers; > dma_addr_t tso_headers_dma; > + > + /* Shared read-only zero buffer to pad short frames to ETH_ZLEN */ > + void *tx_pad; > + dma_addr_t tx_pad_dma; > } ____cacheline_aligned_in_smp; > > static inline void enetc_bdr_idx_inc(struct enetc_bdr *bdr, int *i) ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v5 net 2/4] net: enetc: do not rely on mEMAC hardware frame padding 2026-10-22 8:33 ` David Laight @ 2026-10-22 9:41 ` Vladimir Oltean 2026-10-22 10:22 ` David Laight 0 siblings, 1 reply; 15+ messages in thread From: Vladimir Oltean @ 2026-10-22 9:41 UTC (permalink / raw) To: David Laight Cc: Andrew Lunn, David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Claudiu Manoil, Wei Fang, Clark Wang, Ioana Ciornei, Zefir Kurtisi, Zefir Kurtisi, netdev, imx, linux-kernel On Thu, Oct 22, 2026 at 09:33:59AM +0100, David Laight wrote: > On Wed, 21 Oct 2026 17:53:12 +0300 > vladimir.oltean@nxp.com wrote: > > > The driver sets the ENETC_PM0_CMD_TXP bit to pad short buffers. > > This works, however sending short frames from user space and looking at > > the padding bytes inserted by the MAC at the link partner, we notice on > > LS1028A that the first octet of padding is sometimes not 0. > > > > This is fine per 802.3, which does not require that the padding is zero. > > > > However, there is an unproven suspicion that the padding bytes may > > come from internal memories, and unfortunately I couldn't get sufficient > > priority for this octet's origin to be investigated by hardware > > engineers. > > > > To be defensive and not leak any sensitive information from the SoC, > > let's pad all frames to ETH_ZLEN in software, so that the MAC doesn't > > have to do anything. > > > > There are 2 distinct cases to handle, for GSO and non-GSO. For non-GSO > > things are trivial; we need to call eth_skb_pad(). For GSO, things are > > not simple, because user space may set gso_size to a very small value, > > which could make the GSO super-skb smaller than ETH_ZLEN; > > That isn't sensible. > The minimum frame is 64 bytes (including FCS), subtract that, the MAC > header (14 bytes), minimum IP header (20), minimum TCP (20) and that > leaves 6 bytes for user data. > I doubt it is sensible to do GSO for anything under the minimum mtu > (128 bytes rings a bell somewhere). My fault for not being clear and inviting this kind of objection. Since we do GSO in software, we advertise NETIF_F_GSO_UDP_L4 too, and there, Sashiko came on v3 with the observation that for UDP GSO, user space can set gso_size to a small value using setsockopt(UDP_SEGMENT): https://lore.kernel.org/netdev/178960175550.22033.10648880872141935672@kernel.org/ Sensible or not, it is possible, and I did generate a test program using an LLM to confirm this. Regardless of that, the last segment of a GSO super-skb would need padding anyway, because even with a 'normal' TCP MSS, the skb->len modulo gso_size could still result in a last frame < ETH_ZLEN. Padding just the GSO super-skb, as I did in v3, would also not fix this. > But does this code allow for the VLAN tag that you forgot about in the > other patch? Not sure I understand your concern here? Is it that with VLAN TX insertion offload, we pad "too much" (i.e. to ETH_ZLEN, then the NIC pushes an extra VLAN_HLEN)? I'm not sure that's a problem.. Anyway, with VLAN TX offload, the VLAN tag is not part of the frm_len. It's part of the extension BD that we forgot to account for, when determining whether the TX BD ring has enough space for the frame under transmission. > I also noticed that the code seems to dma map the fragments for each > gso segment separately, just mapping each skb fragment would be better. > (But that is for another patch series.) Noted, I can look into this separately. Thanks for the review! ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v5 net 2/4] net: enetc: do not rely on mEMAC hardware frame padding 2026-10-22 9:41 ` Vladimir Oltean @ 2026-10-22 10:22 ` David Laight 2026-10-22 12:35 ` Vladimir Oltean 0 siblings, 1 reply; 15+ messages in thread From: David Laight @ 2026-10-22 10:22 UTC (permalink / raw) To: Vladimir Oltean Cc: Andrew Lunn, David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Claudiu Manoil, Wei Fang, Clark Wang, Ioana Ciornei, Zefir Kurtisi, Zefir Kurtisi, netdev, imx, linux-kernel On Thu, 22 Oct 2026 12:41:49 +0300 Vladimir Oltean <vladimir.oltean@nxp.com> wrote: > On Thu, Oct 22, 2026 at 09:33:59AM +0100, David Laight wrote: > > On Wed, 21 Oct 2026 17:53:12 +0300 > > vladimir.oltean@nxp.com wrote: > > > > > The driver sets the ENETC_PM0_CMD_TXP bit to pad short buffers. > > > This works, however sending short frames from user space and looking at > > > the padding bytes inserted by the MAC at the link partner, we notice on > > > LS1028A that the first octet of padding is sometimes not 0. > > > > > > This is fine per 802.3, which does not require that the padding is zero. > > > > > > However, there is an unproven suspicion that the padding bytes may > > > come from internal memories, and unfortunately I couldn't get sufficient > > > priority for this octet's origin to be investigated by hardware > > > engineers. > > > > > > To be defensive and not leak any sensitive information from the SoC, > > > let's pad all frames to ETH_ZLEN in software, so that the MAC doesn't > > > have to do anything. > > > > > > There are 2 distinct cases to handle, for GSO and non-GSO. For non-GSO > > > things are trivial; we need to call eth_skb_pad(). For GSO, things are > > > not simple, because user space may set gso_size to a very small value, > > > which could make the GSO super-skb smaller than ETH_ZLEN; > > > > That isn't sensible. > > The minimum frame is 64 bytes (including FCS), subtract that, the MAC > > header (14 bytes), minimum IP header (20), minimum TCP (20) and that > > leaves 6 bytes for user data. > > I doubt it is sensible to do GSO for anything under the minimum mtu > > (128 bytes rings a bell somewhere). > > My fault for not being clear and inviting this kind of objection. > > Since we do GSO in software, we advertise NETIF_F_GSO_UDP_L4 too, and > there, Sashiko came on v3 with the observation that for UDP GSO, user > space can set gso_size to a small value using setsockopt(UDP_SEGMENT): > https://lore.kernel.org/netdev/178960175550.22033.10648880872141935672@kernel.org/ I'd fix that in the setsockopt code. But that might be a bug anyway. UDP segmentation has to use IP fragmentation and that is usually done because the interface/path mtu is too short. But there is a minimum mtu for IPv4 of (something like) 128 bytes, and it may be invalid to fragment into smaller pieces. > Sensible or not, it is possible, and I did generate a test program using > an LLM to confirm this. > > Regardless of that, the last segment of a GSO super-skb would need > padding anyway, because even with a 'normal' TCP MSS, the skb->len > modulo gso_size could still result in a last frame < ETH_ZLEN. Padding > just the GSO super-skb, as I did in v3, would also not fix this. True, but that is a different check to the one you did. > > But does this code allow for the VLAN tag that you forgot about in the > > other patch? > > Not sure I understand your concern here? Is it that with VLAN TX > insertion offload, we pad "too much" (i.e. to ETH_ZLEN, then the NIC > pushes an extra VLAN_HLEN)? I'm not sure that's a problem.. That is a problem, VLAN packets should only be padded to 64 bytes including the VLAN header and CRC. However such overlong packets are likely to be tolerated because an ethernet switch can add a VLAN header before forwarding a packet and it can't know whether a short packet is padded. > Anyway, with VLAN TX offload, the VLAN tag is not part of the frm_len. > It's part of the extension BD that we forgot to account for, when > determining whether the TX BD ring has enough space for the frame under > transmission. That is what I was thinking of. By brain thought it was a normal BD - so would be in the 15 limit. David > > I also noticed that the code seems to dma map the fragments for each > > gso segment separately, just mapping each skb fragment would be better. > > (But that is for another patch series.) > > Noted, I can look into this separately. > > Thanks for the review! ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v5 net 2/4] net: enetc: do not rely on mEMAC hardware frame padding 2026-10-22 10:22 ` David Laight @ 2026-10-22 12:35 ` Vladimir Oltean 2026-10-22 20:48 ` David Laight 0 siblings, 1 reply; 15+ messages in thread From: Vladimir Oltean @ 2026-10-22 12:35 UTC (permalink / raw) To: David Laight Cc: Andrew Lunn, David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Claudiu Manoil, Wei Fang, Clark Wang, Ioana Ciornei, Zefir Kurtisi, Zefir Kurtisi, netdev, imx, linux-kernel On Thu, Oct 22, 2026 at 11:22:18AM +0100, David Laight wrote: > On Thu, 22 Oct 2026 12:41:49 +0300 > Vladimir Oltean <vladimir.oltean@nxp.com> wrote: > > > On Thu, Oct 22, 2026 at 09:33:59AM +0100, David Laight wrote: > > > On Wed, 21 Oct 2026 17:53:12 +0300 > > > vladimir.oltean@nxp.com wrote: > > > > > > > The driver sets the ENETC_PM0_CMD_TXP bit to pad short buffers. > > > > This works, however sending short frames from user space and looking at > > > > the padding bytes inserted by the MAC at the link partner, we notice on > > > > LS1028A that the first octet of padding is sometimes not 0. > > > > > > > > This is fine per 802.3, which does not require that the padding is zero. > > > > > > > > However, there is an unproven suspicion that the padding bytes may > > > > come from internal memories, and unfortunately I couldn't get sufficient > > > > priority for this octet's origin to be investigated by hardware > > > > engineers. > > > > > > > > To be defensive and not leak any sensitive information from the SoC, > > > > let's pad all frames to ETH_ZLEN in software, so that the MAC doesn't > > > > have to do anything. > > > > > > > > There are 2 distinct cases to handle, for GSO and non-GSO. For non-GSO > > > > things are trivial; we need to call eth_skb_pad(). For GSO, things are > > > > not simple, because user space may set gso_size to a very small value, > > > > which could make the GSO super-skb smaller than ETH_ZLEN; > > > > > > That isn't sensible. > > > The minimum frame is 64 bytes (including FCS), subtract that, the MAC > > > header (14 bytes), minimum IP header (20), minimum TCP (20) and that > > > leaves 6 bytes for user data. > > > I doubt it is sensible to do GSO for anything under the minimum mtu > > > (128 bytes rings a bell somewhere). > > > > My fault for not being clear and inviting this kind of objection. > > > > Since we do GSO in software, we advertise NETIF_F_GSO_UDP_L4 too, and > > there, Sashiko came on v3 with the observation that for UDP GSO, user > > space can set gso_size to a small value using setsockopt(UDP_SEGMENT): > > https://lore.kernel.org/netdev/178960175550.22033.10648880872141935672@kernel.org/ > > I'd fix that in the setsockopt code. > But that might be a bug anyway. Why a bug? > UDP segmentation has to use IP fragmentation and that is usually done because > the interface/path mtu is too short. In my testing, UDP GSO does not generate IP fragments, but rather, each UDP segment is its own datagram. Are you thinking of UDP Fragmentation Offload instead? As per https://docs.kernel.org/networking/segmentation-offloads.html#udp-fragmentation-offload this is deprecated and AFAIU not what this is about. The docs do also explicitly say that UDP Segmentation Offload does not generate IP fragments. > But there is a minimum mtu for IPv4 of (something like) 128 bytes, and > it may be invalid to fragment into smaller pieces. I have no idea what you are referring to, here, with the minimum MTU of 128 bytes. > > Sensible or not, it is possible, and I did generate a test program using > > an LLM to confirm this. > > > > Regardless of that, the last segment of a GSO super-skb would need > > padding anyway, because even with a 'normal' TCP MSS, the skb->len > > modulo gso_size could still result in a last frame < ETH_ZLEN. Padding > > just the GSO super-skb, as I did in v3, would also not fix this. > > True, but that is a different check to the one you did. So what is your suggestion for the driver to do? > > > But does this code allow for the VLAN tag that you forgot about in the > > > other patch? > > > > Not sure I understand your concern here? Is it that with VLAN TX > > insertion offload, we pad "too much" (i.e. to ETH_ZLEN, then the NIC > > pushes an extra VLAN_HLEN)? I'm not sure that's a problem.. > > That is a problem, VLAN packets should only be padded to 64 bytes > including the VLAN header and CRC. > However such overlong packets are likely to be tolerated because > an ethernet switch can add a VLAN header before forwarding a packet > and it can't know whether a short packet is padded. Well, if it's a theoretical problem but it's not a practical problem, I'd prefer leaving it as is. Since eth_skb_pad() itself is unaware of VLAN TX offloads, I'd be opening myself up to an endless rabbit hole ("why didn't you fix this in eth_skb_pad()? It's a generic problem." followed by "changing eth_skb_pad() broke 20 drivers!")... Unless it's very clear why I'd be doing this, I'd very much prefer not to. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v5 net 2/4] net: enetc: do not rely on mEMAC hardware frame padding 2026-10-22 12:35 ` Vladimir Oltean @ 2026-10-22 20:48 ` David Laight 2026-10-23 9:35 ` Vladimir Oltean 0 siblings, 1 reply; 15+ messages in thread From: David Laight @ 2026-10-22 20:48 UTC (permalink / raw) To: Vladimir Oltean Cc: Andrew Lunn, David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Claudiu Manoil, Wei Fang, Clark Wang, Ioana Ciornei, Zefir Kurtisi, Zefir Kurtisi, netdev, imx, linux-kernel On Thu, 22 Oct 2026 15:35:05 +0300 Vladimir Oltean <vladimir.oltean@nxp.com> wrote: > On Thu, Oct 22, 2026 at 11:22:18AM +0100, David Laight wrote: > > On Thu, 22 Oct 2026 12:41:49 +0300 > > Vladimir Oltean <vladimir.oltean@nxp.com> wrote: > > > > > On Thu, Oct 22, 2026 at 09:33:59AM +0100, David Laight wrote: > > > > On Wed, 21 Oct 2026 17:53:12 +0300 > > > > vladimir.oltean@nxp.com wrote: > > > > > > > > > The driver sets the ENETC_PM0_CMD_TXP bit to pad short buffers. > > > > > This works, however sending short frames from user space and looking at > > > > > the padding bytes inserted by the MAC at the link partner, we notice on > > > > > LS1028A that the first octet of padding is sometimes not 0. > > > > > > > > > > This is fine per 802.3, which does not require that the padding is zero. > > > > > > > > > > However, there is an unproven suspicion that the padding bytes may > > > > > come from internal memories, and unfortunately I couldn't get sufficient > > > > > priority for this octet's origin to be investigated by hardware > > > > > engineers. > > > > > > > > > > To be defensive and not leak any sensitive information from the SoC, > > > > > let's pad all frames to ETH_ZLEN in software, so that the MAC doesn't > > > > > have to do anything. > > > > > > > > > > There are 2 distinct cases to handle, for GSO and non-GSO. For non-GSO > > > > > things are trivial; we need to call eth_skb_pad(). For GSO, things are > > > > > not simple, because user space may set gso_size to a very small value, > > > > > which could make the GSO super-skb smaller than ETH_ZLEN; > > > > > > > > That isn't sensible. > > > > The minimum frame is 64 bytes (including FCS), subtract that, the MAC > > > > header (14 bytes), minimum IP header (20), minimum TCP (20) and that > > > > leaves 6 bytes for user data. > > > > I doubt it is sensible to do GSO for anything under the minimum mtu > > > > (128 bytes rings a bell somewhere). > > > > > > My fault for not being clear and inviting this kind of objection. > > > > > > Since we do GSO in software, we advertise NETIF_F_GSO_UDP_L4 too, and > > > there, Sashiko came on v3 with the observation that for UDP GSO, user > > > space can set gso_size to a small value using setsockopt(UDP_SEGMENT): > > > https://lore.kernel.org/netdev/178960175550.22033.10648880872141935672@kernel.org/ > > > > I'd fix that in the setsockopt code. > > But that might be a bug anyway. > > Why a bug? > > > UDP segmentation has to use IP fragmentation and that is usually done because > > the interface/path mtu is too short. > > In my testing, UDP GSO does not generate IP fragments, but rather, each > UDP segment is its own datagram. Hmmm.... I wouldn't have thought that was generally useful. Losing data from random points in a connection is going to be hard to recover from. I guess that it why it is per-socket rather than system wide. I mean, you can run telnet over udp and just hope no packets get lost. > Are you thinking of UDP Fragmentation Offload instead? As per > https://docs.kernel.org/networking/segmentation-offloads.html#udp-fragmentation-offload > this is deprecated and AFAIU not what this is about. That is the sort of UDP segmentation I thought made sense. OTOH I think the main user of it was NFS, but even then 8k datagrams were actually so unreliable it was often much better to tell UDP to fragment the 8k blocks into 1k chunks (IIRC that was a mount option). > > The docs do also explicitly say that UDP Segmentation Offload does not > generate IP fragments. > > > But there is a minimum mtu for IPv4 of (something like) 128 bytes, and > > it may be invalid to fragment into smaller pieces. > > I have no idea what you are referring to, here, with the minimum MTU of > 128 bytes. The correct number is probably 68 (min max size of an IPv4 fragment) rather than 576 (for a reassembled message) - I think both are from the start of the IP headers. That means there is no sane reason to fragment anything so small that ethernet frames get padded. > > > > Sensible or not, it is possible, and I did generate a test program using > > > an LLM to confirm this. > > > > > > Regardless of that, the last segment of a GSO super-skb would need > > > padding anyway, because even with a 'normal' TCP MSS, the skb->len > > > modulo gso_size could still result in a last frame < ETH_ZLEN. Padding > > > just the GSO super-skb, as I did in v3, would also not fix this. > > > > True, but that is a different check to the one you did. > > So what is your suggestion for the driver to do? Clearly you need to allow a BD for padding if the final fragment is short. That is one extra BD for the entire frame, that same as a non-GSO message. You could leave the hardware padding 'stupidly short' gso fragments. I assume the driver is building the ethernet/IP/UDP/TDP header so that will be one buffer and not hit the 'fragmented after 14 bytes' issue. > > > > > But does this code allow for the VLAN tag that you forgot about in the > > > > other patch? > > > > > > Not sure I understand your concern here? Is it that with VLAN TX > > > insertion offload, we pad "too much" (i.e. to ETH_ZLEN, then the NIC > > > pushes an extra VLAN_HLEN)? I'm not sure that's a problem.. > > > > That is a problem, VLAN packets should only be padded to 64 bytes > > including the VLAN header and CRC. > > However such overlong packets are likely to be tolerated because > > an ethernet switch can add a VLAN header before forwarding a packet > > and it can't know whether a short packet is padded. > > Well, if it's a theoretical problem but it's not a practical problem, > I'd prefer leaving it as is. > > Since eth_skb_pad() itself is unaware of VLAN TX offloads, I'd be > opening myself up to an endless rabbit hole ("why didn't you fix this > in eth_skb_pad()? It's a generic problem." followed by > "changing eth_skb_pad() broke 20 drivers!")... > > Unless it's very clear why I'd be doing this, I'd very much prefer not to. VLAN headers came in after I stopped writing ethernet drivers! I think systems have to tolerate extra padding when a VLAN header is present. David ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v5 net 2/4] net: enetc: do not rely on mEMAC hardware frame padding 2026-10-22 20:48 ` David Laight @ 2026-10-23 9:35 ` Vladimir Oltean 0 siblings, 0 replies; 15+ messages in thread From: Vladimir Oltean @ 2026-10-23 9:35 UTC (permalink / raw) To: David Laight Cc: Andrew Lunn, David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Claudiu Manoil, Wei Fang, Clark Wang, Ioana Ciornei, Zefir Kurtisi, Zefir Kurtisi, netdev, imx, linux-kernel Hi David, On Thu, Oct 22, 2026 at 09:48:03PM +0100, David Laight wrote: > > > UDP segmentation has to use IP fragmentation and that is usually done because > > > the interface/path mtu is too short. > > > > In my testing, UDP GSO does not generate IP fragments, but rather, each > > UDP segment is its own datagram. > > Hmmm.... > I wouldn't have thought that was generally useful. > Losing data from random points in a connection is going to be hard > to recover from. > I guess that it why it is per-socket rather than system wide. > I mean, you can run telnet over udp and just hope no packets get lost. After a very quick search, I think the main use case for UDP GSO was QUIC: https://lpc.events/event/2/contributions/106/ In turn, QUIC has its own loss detection and congestion control, though I don't exactly know how they do it. An interesting statement I found in that paper is: "The gso size parameter communicates the length of the payload without network or transport layer headers. A gso_size all the way down to 1 is a valid choice. A value of 0 effectively disables GSO." > > Are you thinking of UDP Fragmentation Offload instead? As per > > https://docs.kernel.org/networking/segmentation-offloads.html#udp-fragmentation-offload > > this is deprecated and AFAIU not what this is about. > > That is the sort of UDP segmentation I thought made sense. > OTOH I think the main user of it was NFS, but even then 8k datagrams > were actually so unreliable it was often much better to tell UDP to fragment > the 8k blocks into 1k chunks (IIRC that was a mount option). > > > > > The docs do also explicitly say that UDP Segmentation Offload does not > > generate IP fragments. > > > > > But there is a minimum mtu for IPv4 of (something like) 128 bytes, and > > > it may be invalid to fragment into smaller pieces. > > > > I have no idea what you are referring to, here, with the minimum MTU of > > 128 bytes. > > The correct number is probably 68 (min max size of an IPv4 fragment) rather > than 576 (for a reassembled message) - I think both are from the start of > the IP headers. > That means there is no sane reason to fragment anything so small that > ethernet frames get padded. > > > > > > > Sensible or not, it is possible, and I did generate a test program using > > > > an LLM to confirm this. > > > > > > > > Regardless of that, the last segment of a GSO super-skb would need > > > > padding anyway, because even with a 'normal' TCP MSS, the skb->len > > > > modulo gso_size could still result in a last frame < ETH_ZLEN. Padding > > > > just the GSO super-skb, as I did in v3, would also not fix this. > > > > > > True, but that is a different check to the one you did. > > > > So what is your suggestion for the driver to do? > > Clearly you need to allow a BD for padding if the final fragment is short. > That is one extra BD for the entire frame, that same as a non-GSO message. > You could leave the hardware padding 'stupidly short' gso fragments. Leaving the hardware to pad 'stupidly short' GSO segments is definitely not an option. Given my context: the hardware adds some non-zero padding bytes, and customers suspect the MAC is leaking device-internal memories, this is exactly the kind of attack vector user space could use to exfiltrate data (even if not practically useful, this is a valid API). The request has been explicit to avoid this if we can't prove the origin of the non-zero padding bytes. Now, having that said, I noticed that Joe Damato had this exact problem a few weeks ago in bnxt: https://lore.kernel.org/netdev/20261009215426.2377946-1-joe@dama.to/ It looks like he did something there in ndo_features_check() to simply clear NETIF_F_GSO_MASK for the 'stupidly short' gso_size, so that they're segmented in the stack and appear as non-GSO during driver xmit. But this is just a superficial analysis. I'll have to see how much complexity it adds to do something similar in enetc, vs just adding a padding BD for each GSO segment/datagram myself. > I assume the driver is building the ethernet/IP/UDP/TDP header so that > will be one buffer and not hit the 'fragmented after 14 bytes' issue. Yes, the header buffer < ENETC_MIN_BUFF_SIZE is not the problem here, just general MAC padding contents. This is why I split the patches the way I did: first never allow the MAC to pad by itself, then address the remaining buffer geometry issues that ENETC doesn't tolerate (this causes a separate, unrelated problem, that the software padding partly fixes and partly doesn't). > > > > > But does this code allow for the VLAN tag that you forgot about in the > > > > > other patch? > > > > > > > > Not sure I understand your concern here? Is it that with VLAN TX > > > > insertion offload, we pad "too much" (i.e. to ETH_ZLEN, then the NIC > > > > pushes an extra VLAN_HLEN)? I'm not sure that's a problem.. > > > > > > That is a problem, VLAN packets should only be padded to 64 bytes > > > including the VLAN header and CRC. > > > However such overlong packets are likely to be tolerated because > > > an ethernet switch can add a VLAN header before forwarding a packet > > > and it can't know whether a short packet is padded. > > > > Well, if it's a theoretical problem but it's not a practical problem, > > I'd prefer leaving it as is. > > > > Since eth_skb_pad() itself is unaware of VLAN TX offloads, I'd be > > opening myself up to an endless rabbit hole ("why didn't you fix this > > in eth_skb_pad()? It's a generic problem." followed by > > "changing eth_skb_pad() broke 20 drivers!")... > > > > Unless it's very clear why I'd be doing this, I'd very much prefer not to. > > VLAN headers came in after I stopped writing ethernet drivers! > I think systems have to tolerate extra padding when a VLAN header is present. > > David > > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v5 net 2/4] net: enetc: do not rely on mEMAC hardware frame padding 2026-10-21 14:53 ` [PATCH v5 net 2/4] net: enetc: do not rely on mEMAC hardware frame padding vladimir.oltean 2026-10-22 8:33 ` David Laight @ 2026-10-25 15:21 ` netdev-bot+sashiko 1 sibling, 0 replies; 15+ messages in thread From: netdev-bot+sashiko @ 2026-10-25 15:21 UTC (permalink / raw) To: vladimir.oltean Cc: andrew+netdev, davem, edumazet, kuba, pabeni, claudiu.manoil, wei.fang, xiaoning.wang, ioana.ciornei, zefir.kurtisi, zefir.kurtisi, david.laight.linux, netdev, imx, linux-kernel Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider. Critical: 0 · High: 0 · Medium: 2 · Low: 1 If you believe none of the issues are legitimate, please reply to each issue of Medium severity or higher and explain why the AI is wrong. If any of them are legitimate and you plan to respin, please let patchwork know by including "pw-bot: cr" as a separate line at the end of your reply (one such reply per series is enough). - [Medium] In enetc_start_xmit(), the new early exit `if (eth_skb_pad(skb)) return NETDEV_TX_OK;` runs after the… - [Medium] enetc_tso_count_descs() now reserves tso_count_descs(skb) + gso_segs, i.e. 3*gso_segs + nr_frags, or 4*gso_segs + nr_frags with a… - [Low] This patch leaves the XDP transmit paths unpadded. -- net: enetc: do not rely on mEMAC hardware frame padding This commit pads short frames to ETH_ZLEN in software on the enetc transmit path, instead of relying on the mEMAC, because the padding the LS1028A MAC inserts is sometimes not zero. Non-GSO skbs are padded with eth_skb_pad(). Software TSO segments shorter than ETH_ZLEN get an extra BD that points to a shared, zeroed DMA-coherent buffer, and the worst-case TSO descriptor reservation grows by one BD per segment. > To be defensive and not leak any sensitive information from the SoC, > let's pad all frames to ETH_ZLEN in software, so that the MAC doesn't > have to do anything. [Severity: Low] Does this cover the XDP transmit paths? enetc_xdp_tx() is reached from XDP_TX in enetc_clean_rx_ring_xdp() and from enetc_xdp_xmit(). It still uses the raw frame length and adds no padding: while (unlikely(!tmp_tx_swbd->is_eof)) { tmp_tx_swbd++; frm_len += tmp_tx_swbd->len; } ENETC_PM0_CMD_TXP is still set in enetc_configure_port_mac(). That means the mEMAC would still pad short XDP_TX and XDP_REDIRECT frames on LS1028A after this commit. The last patch in the series, "net: enetc: pad short XDP frames in software", seems to fix this by appending a tx_pad BD in enetc_xdp_tx(). So this only affects the intermediate state of the series. > diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c > index ab4bfefda6c8f..f0f9ba94b4df6 100644 > --- a/drivers/net/ethernet/freescale/enetc/enetc.c > +++ b/drivers/net/ethernet/freescale/enetc/enetc.c [ ... ] > @@ -690,7 +706,8 @@ static int enetc_lso_count_descs(const struct sk_buff *skb) > > static int enetc_tso_count_descs(const struct sk_buff *skb) > { > - int count = tso_count_descs(skb); > + /* +gso_segs to cover one pad BD per segment */ > + int count = tso_count_descs(skb) + skb_shinfo(skb)->gso_segs; [Severity: Medium] Can reserving a pad BD for every segment push the reservation past the ring size for skbs that used to transmit fine? Every segment only needs a pad BD when hdr_len + gso_size < ETH_ZLEN. Otherwise at most the last, shorter segment needs one. With this change the reservation becomes 3 * gso_segs + nr_frags, or 4 * gso_segs + nr_frags with a VLAN tag. The TX ring is fixed at ENETC_TX_RING_DEFAULT_SIZE (2048), so enetc_bd_unused() returns at most 2047. The driver does not lower gso_max_segs or tso_max_segs. A software TSO skb with more than about 677 segments (about 507 with VLAN) can now never fit. Before this patch the limit was about 1015 segments (677 with VLAN). For example, a TCP flow with an MSS of about 88 bytes and a 64KB GSO skb has about 744 segments. enetc_start_xmit() then does this even on an empty ring: if (enetc_bd_unused(tx_ring) < enetc_tso_count_descs(skb)) { netif_stop_subqueue(ndev, tx_ring->index); return NETDEV_TX_BUSY; } enetc_clean_tx_ring() only wakes the queue after it completes a frame, and the driver has no ndo_tx_timeout. Wouldn't the requeued skb then block this TX queue until ifdown/ifup? The same stall already happened above about 1015 segments; this change lowers the threshold. Could pad BDs be reserved only when padding can happen (gso_segs pad BDs if hdr_len + gso_size < ETH_ZLEN, otherwise one)? Alternatively, netif_set_tso_max_segs() could cap the segment count so the worst case fits in the ring. > > /* Each segment also needs an extension BD for VLAN insertion */ > if (skb_vlan_tag_present(skb)) [ ... ] > @@ -1053,6 +1088,9 @@ static netdev_tx_t enetc_start_xmit(struct sk_buff *skb, > enetc_unlock_mdio(); > } > } else { > + if (eth_skb_pad(skb)) > + return NETDEV_TX_OK; > + [Severity: Medium] Can this early return leak the ENETC_TX_ONESTEP_TSTAMP_IN_PROGRESS bit? For one-step PTP Sync skbs, enetc_start_xmit() has already taken the bit by the time it gets here: if (enetc_cb->flag & ENETC_F_TX_ONESTEP_SYNC_TSTAMP) { if (test_and_set_bit_lock(ENETC_TX_ONESTEP_TSTAMP_IN_PROGRESS, &priv->flags)) { skb_queue_tail(&priv->tx_skbs, skb); return NETDEV_TX_OK; } } An L2 PTP Sync is 14 + 44 = 58 bytes, so it now always goes through eth_skb_pad(). If the skb is cloned or lacks tailroom, the call chain eth_skb_pad()->skb_put_padto()->__skb_pad() calls pskb_expand_head(GFP_ATOMIC). If that fails, the skb is freed and an error is returned. The bit is only released in enetc_tx_onestep_tstamp(): clear_bit_unlock(ENETC_TX_ONESTEP_TSTAMP_IN_PROGRESS, &priv->flags); That work is scheduled from enetc_clean_tx_ring() when the one-step skb completes on the TX ring. A dropped skb never reaches the ring, so the work never runs. After that, wouldn't every later one-step Sync fail test_and_set_bit_lock() and be added to priv->tx_skbs, with nothing left to dequeue it? One-step PTP transmit would stop for good and tx_skbs would grow without bound. The bit also survives ifdown/ifup, because enetc_tx_onestep_tstamp_init() only reinitializes the work and the queue. The worker hits the same path when it calls enetc_start_xmit() on a dequeued skb. The existing drop_packet_err and NETDEV_TX_BUSY exits have the same problem. So does the pskb_may_pull() drop added by the next patch, "net: enetc: ensure head buffer has at least 16 octets". Could the bit be released (and tx_skbs drained) on every exit that does not post the skb to the ring? Or could the padding happen before the bit-lock is taken? > if (unlikely(skb_shinfo(skb)->nr_frags > priv->max_frags)) > if (unlikely(skb_linearize(skb))) > goto drop_packet_err; [ ... ] -- Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261021145314.1435049-1-vladimir.oltean%40nxp.com ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v5 net 3/4] net: enetc: ensure head buffer has at least 16 octets 2026-10-21 14:53 [PATCH v5 net 0/4] Fix short frame transmission in enetc vladimir.oltean 2026-10-21 14:53 ` [PATCH v5 net 1/4] net: enetc: reserve VLAN extension BDs for software TSO vladimir.oltean 2026-10-21 14:53 ` [PATCH v5 net 2/4] net: enetc: do not rely on mEMAC hardware frame padding vladimir.oltean @ 2026-10-21 14:53 ` vladimir.oltean 2026-10-25 15:21 ` netdev-bot+sashiko 2026-10-21 14:53 ` [PATCH v5 net 4/4] net: enetc: pad short XDP frames in software vladimir.oltean 3 siblings, 1 reply; 15+ messages in thread From: vladimir.oltean @ 2026-10-21 14:53 UTC (permalink / raw) To: Andrew Lunn, David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni Cc: Claudiu Manoil, Wei Fang, Clark Wang, Ioana Ciornei, Zefir Kurtisi, Zefir Kurtisi, David Laight, netdev, imx, linux-kernel The ENETC does not support BUF_LEN or FRM_LEN in TX buffer descriptors less than 16. This is written in the reference manual of all SoCs supported by the driver: LS1028A, i.MX943, i.MX95 etc. Frames must not have a FRM_LEN that is less than 16 bytes. Frames of 0-15 bytes are not supported. (...) The first descriptor in a chain must not have a BUFF_LEN that is less than 16 bytes. I don't think proper attention was paid to this during development, we found the text at the end of a bug investigation. Therefore, the driver does not enforce this. But the frame length and geometry is out of the driver's control, and the network stack can actually send packets that ENETC doesn't like. Zefir Kurtisi found a case where transmitting L2 WNM keep-alive frames through ENETC would soft-lockup the host through an IRQ storm. He later distilled this into a small enetc-killer.c user space program which sends a packet with MAC DA, MAC SA and EtherType IPv4 (14 octets in length) through an AF_PACKET raw socket. Note that this particular case was already closed by the previous commit, but an identical situation still happens when the first buffer of a multi-buffer frame is < 16 bytes. The IRQ storm is actually a curious effect of a chain of events. The hardware behaviour, when an invalid BD is put in its TX ring, is that it would transmit the packet as normal, update counters, raise completion interrupt as normal, but it would just not advance the consumer index of the ring (TBaCIR) to signify that the BD has been consumed and is available for software to free. The ring will also get its TBaSR[BUSY] bit persistently set to 1 afterwards. It deserves an explanation why the behaviour above would lead to an IRQ storm, since ENETC interrupts are message-based (MSI-X), and an unhandled interrupt would typically just be lost rather than retrigger itself as a wired interrupt would. NAPI processing in ENETC has 3 steps: I. the enetc_msix() hardirq handler disables RBaIER, TBaIER and sets softirq processing to the 'pending' state. II. the enetc_poll() softirq handler for the IRQ vector walks through the TX rings affine to that vector, checks which ones have a TBCIR updated since last time - enetc_bd_ready_count() - processes those completed frames, and clears pending interrupts in these updated TX rings by writing to TBaIDR. (I've excluded RX processing due to it being irrelevant). III. After the softirq handler does its round of checking all RX and TX rings for updates, it re-enables all interrupts in RBaIER and TBaIER that were previously disabled by the hardirq handler, and exits. Because the TX ring with the short frame is skipped at step II (TBCIR wasn't updated as part of HW malfunction), its pending IRQ is not cleared in TBaIDR by enetc_clean_tx_ring(). But because enetc_msix() disables TBaIER at step I and re-enables it at step III, another MSI will be fired upon re-enabling it. This is what completes the cycle and the driver goes back to step I. So the driver misinterprets the mixed signals it's getting from the hardware, and ends up causing a software-amplified IRQ storm. Because the driver now pads linear skbs to ETH_ZLEN for other reasons too, we just need to ensure the head buffer has at least 16 bytes. This can be done using pskb_may_pull(), which moves that number of bytes from the paged area into the head. This is case 5 below. skb_headlen (H) | skb->len (L) | linear? | handled by ----------------+--------------+---------+--------------------------------------- H == L | L < 60 | yes | eth_skb_pad() (pads head to 60) H == L | L >= 60 | yes | already valid, no-op H < 16 | L < 60 | no | eth_skb_pad() (linearizes + pads to 60) H < 16 | L >= 60 | no | pskb_may_pull() (pulls frags into head) H >= 16 | L >= 60 | no | already valid, no-op Fixes: d4fd0404c1c9 ("enetc: Introduce basic PF and VF ENETC ethernet drivers") Reported-by: Zefir Kurtisi <zefir.kurtisi@westermo.com> Closes: https://lore.kernel.org/netdev/b3d9136c-2803-4203-b1ea-1f9e62de80a1@gmail.com/ Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- v4->v5: none v3->v4: - rename from "net: enetc: pad short frames in software". The padding to ETH_ZLEN is now done by "net: enetc: do not rely on mEMAC hardware frame padding", so this patch only deals with the 16 octet minimum head buffer length - move the check from enetc_xmit() to enetc_start_xmit(), after eth_skb_pad() - use pskb_may_pull() instead of skb_linearize() - drop the code comment above the head buffer length check, it is redundant with the commit message - rework the commit message, add a table of the handled cases - drop Reviewed-by from Wei Fang and Tested-by from Zefir Kurtisi due to the change of approach v1->v3: part of "pad short frames in software" --- drivers/net/ethernet/freescale/enetc/enetc.c | 6 ++++++ drivers/net/ethernet/freescale/enetc/enetc.h | 2 ++ 2 files changed, 8 insertions(+) diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c index a9ea124f7835..58a88f15fa5d 100644 --- a/drivers/net/ethernet/freescale/enetc/enetc.c +++ b/drivers/net/ethernet/freescale/enetc/enetc.c @@ -1088,6 +1088,12 @@ static netdev_tx_t enetc_start_xmit(struct sk_buff *skb, if (eth_skb_pad(skb)) return NETDEV_TX_OK; + if (unlikely(skb_headlen(skb) < ENETC_MIN_BUFF_SIZE && + !pskb_may_pull(skb, ENETC_MIN_BUFF_SIZE))) { + dev_kfree_skb_any(skb); + return NETDEV_TX_OK; + } + if (unlikely(skb_shinfo(skb)->nr_frags > priv->max_frags)) if (unlikely(skb_linearize(skb))) goto drop_packet_err; diff --git a/drivers/net/ethernet/freescale/enetc/enetc.h b/drivers/net/ethernet/freescale/enetc/enetc.h index db3b3bad0f59..6fc7001c025c 100644 --- a/drivers/net/ethernet/freescale/enetc/enetc.h +++ b/drivers/net/ethernet/freescale/enetc/enetc.h @@ -22,6 +22,8 @@ #define ENETC_MAX_MTU (ENETC_MAC_MAXFRM_SIZE - \ (ETH_FCS_LEN + ETH_HLEN + VLAN_HLEN)) +#define ENETC_MIN_BUFF_SIZE 16 + #define ENETC_CBD_DATA_MEM_ALIGN 64 #define ENETC_MADDR_HASH_TBL_SZ 64 -- 2.43.0 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v5 net 3/4] net: enetc: ensure head buffer has at least 16 octets 2026-10-21 14:53 ` [PATCH v5 net 3/4] net: enetc: ensure head buffer has at least 16 octets vladimir.oltean @ 2026-10-25 15:21 ` netdev-bot+sashiko 0 siblings, 0 replies; 15+ messages in thread From: netdev-bot+sashiko @ 2026-10-25 15:21 UTC (permalink / raw) To: vladimir.oltean Cc: andrew+netdev, davem, edumazet, kuba, pabeni, claudiu.manoil, wei.fang, xiaoning.wang, ioana.ciornei, zefir.kurtisi, zefir.kurtisi, david.laight.linux, netdev, imx, linux-kernel Thank you for your contribution! Sashiko AI review found more severe issue(s) in other patches of this series. This mail is only an FYI, in case you want to address the nit picks below when you respin. Low priority / nit picks for your consideration: - [Low] At this commit, the 16-octet minimum for the head BUFF_LEN and for FRM_LEN is enforced only in the non-GSO skb branch of… - [Low] The commit message says the pskb_may_pull() handling "is case 5 below". Pre-existing issues: - [Medium] In enetc_start_xmit(), when an skb has ENETC_F_TX_ONESTEP_SYNC_TSTAMP set, the caller takes the… -- net: enetc: ensure head buffer has at least 16 octets On the non-GSO path, enetc_start_xmit() now uses pskb_may_pull() to move bytes from the paged area into the head when the head buffer is shorter than 16 octets. The ENETC does not accept a first TX BD with BUFF_LEN below 16. Such a descriptor stalls the TX ring consumer index and leads to an IRQ storm. Closes: https://lore.kernel.org/netdev/b3d9136c-2803-4203-b1ea-1f9e62de80a1@gmail.com/ > Because the driver now pads linear skbs to ETH_ZLEN for other reasons > too, we just need to ensure the head buffer has at least 16 bytes. > This can be done using pskb_may_pull(), which moves that number of bytes > from the paged area into the head. This is case 5 below. > > skb_headlen (H) | skb->len (L) | linear? | handled by > ----------------+--------------+---------+--------------------------------------- > H == L | L < 60 | yes | eth_skb_pad() (pads head to 60) > H == L | L >= 60 | yes | already valid, no-op > H < 16 | L < 60 | no | eth_skb_pad() (linearizes + pads to 60) > H < 16 | L >= 60 | no | pskb_may_pull() (pulls frags into head) > H >= 16 | L >= 60 | no | already valid, no-op [Severity: Low] This isn't a bug, but should "case 5" be "case 4"? The pskb_may_pull() case (H < 16, L >= 60) is the fourth row. The fifth row (H >= 16, L >= 60) is the no-op case. The table also has no row for the non-linear H >= 16, L < 60 case. eth_skb_pad() handles that one too, because skb_put_padto()->__skb_pad() linearizes a non-linear skb before padding. Could that row be added so the case analysis is complete? > diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c > index f0f9ba94b4df6..f5a47d53d2bea 100644 > --- a/drivers/net/ethernet/freescale/enetc/enetc.c > +++ b/drivers/net/ethernet/freescale/enetc/enetc.c > @@ -1091,6 +1091,12 @@ static netdev_tx_t enetc_start_xmit(struct sk_buff *skb, > if (eth_skb_pad(skb)) > return NETDEV_TX_OK; > > + if (unlikely(skb_headlen(skb) < ENETC_MIN_BUFF_SIZE && > + !pskb_may_pull(skb, ENETC_MIN_BUFF_SIZE))) { [Severity: Low] This covers the non-GSO skb path. Can the XDP transmit path still build a first BD with BUFF_LEN < 16? enetc_xdp_xmit() enetc_xdp_frame_to_xdp_tx_swbd() <- head len is xdp_frame->len enetc_xdp_tx() enetc_xdp_map_tx_buff() drivers/net/ethernet/freescale/enetc/enetc.c:enetc_xdp_map_tx_buff() { ... txbd->buf_len = cpu_to_le16(tx_swbd->len); txbd->frm_len = cpu_to_le16(frm_len); ... } Neither length is checked against ENETC_MIN_BUFF_SIZE at this commit. Two kinds of frame could still produce the invalid BD: - a redirected frame that bpf_xdp_adjust_tail() shrank to ETH_HLEN - a multi-buffer xdp_frame with a 14-15 byte head Either one looks like it could cause the TBaCIR stall and IRQ storm described in the commit message. The later patch in this series, "net: enetc: pad short XDP frames in software", appears to close this. It makes enetc_xdp_tx() drop frames whose first buffer is shorter than ENETC_MIN_BUFF_SIZE, and it adds a pad BD so that FRM_LEN reaches ETH_ZLEN. enetc_xdp_xmit() and the XDP_TX path both go through enetc_xdp_tx(), so the gap only exists at this intermediate commit. > + dev_kfree_skb_any(skb); > + return NETDEV_TX_OK; > + } [Severity: Medium] This is a pre-existing issue that was not introduced by this patch, but the new drop path inherits it. If the skb has ENETC_F_TX_ONESTEP_SYNC_TSTAMP set, enetc_start_xmit() has already taken the bit at the top of the function: if (enetc_cb->flag & ENETC_F_TX_ONESTEP_SYNC_TSTAMP) { if (test_and_set_bit_lock(ENETC_TX_ONESTEP_TSTAMP_IN_PROGRESS, &priv->flags)) { skb_queue_tail(&priv->tx_skbs, skb); return NETDEV_TX_OK; } } If pskb_may_pull() fails here, the skb is freed. The bit is not cleared and priv->tx_onestep_tstamp is not scheduled. Only enetc_tx_onestep_tstamp() releases the bit. That work is scheduled only from enetc_clean_tx_ring(), when a one-step skb completes in the TX ring, and a dropped skb never gets there. After such a drop, would every later one-step Sync skb stay in priv->tx_skbs forever, with the queue growing without bound? The existing drop_packet_err label already leaks the bit the same way. It is reached on skb_linearize() failure, or when DMA mapping fails and count is zero. A common drop path that releases the bit (or schedules the work) for one-step skbs would fix both cases, separately from this patch. > > if (unlikely(skb_shinfo(skb)->nr_frags > priv->max_frags)) > if (unlikely(skb_linearize(skb))) > goto drop_packet_err; [ ... ] -- Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261021145314.1435049-1-vladimir.oltean%40nxp.com ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v5 net 4/4] net: enetc: pad short XDP frames in software 2026-10-21 14:53 [PATCH v5 net 0/4] Fix short frame transmission in enetc vladimir.oltean ` (2 preceding siblings ...) 2026-10-21 14:53 ` [PATCH v5 net 3/4] net: enetc: ensure head buffer has at least 16 octets vladimir.oltean @ 2026-10-21 14:53 ` vladimir.oltean 2026-10-25 15:21 ` netdev-bot+sashiko 3 siblings, 1 reply; 15+ messages in thread From: vladimir.oltean @ 2026-10-21 14:53 UTC (permalink / raw) To: Andrew Lunn, David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni Cc: Claudiu Manoil, Wei Fang, Clark Wang, Ioana Ciornei, Zefir Kurtisi, Zefir Kurtisi, David Laight, netdev, imx, linux-kernel Similar to the skb path, the XDP transmit path may put frames shorter than ETH_ZLEN in the TX ring, and may put frames whose first buffer is shorter than 16 bytes. The former relies on mEMAC hardware padding, which we no longer want to rely upon, and the latter is not supported by ENETC at all. There are two callers of enetc_xdp_tx(): - ndo_xdp_xmit(), which can transmit short frames for example when redirecting from a veth interface pair through a devmap. - XDP_TX from enetc_clean_rx_ring_xdp(). With RX VLAN extraction enabled (the default), a minimum-sized VLAN-tagged frame is 56 bytes long when it is sent back, and once the driver honours bpf_xdp_adjust_tail() and bpf_xdp_adjust_head() for XDP_TX (to be addressed separately), the BPF program can make the frame arbitrarily short. Handle both in enetc_xdp_tx(). When the frame is shorter than ETH_ZLEN, append a BD pointing to the shared read-only tx_pad zero buffer, the same as for short TSO segments, and advertise the padded frame length in the BDs. The pad BD becomes the frame's last BD, so the F flag, the is_eof marker and the xdp_frame pointer (for XDP_REDIRECT) move onto it. The pad BD has no DMA mapping of its own, and it is not an XDP_TX buffer, so TX completion neither unmaps nor recycles it. It is also not accounted in xdp_tx_in_flight, which counts RX buffers. The 16 byte limitation for the first buffer cannot be addressed by the pad BD, so frames with a head buffer shorter than that are dropped. Both callers already treat a false return code from enetc_xdp_tx() as a drop. Doing this in the driver rather than padding the xdp_frame in place also covers multi-buffer frames shorter than ETH_ZLEN, and avoids writing to the tailroom of buffers that, in the XDP_TX case, are recycled into the RX ring. Fixes: 7ed2bc80074e ("net: enetc: add support for XDP_TX") Fixes: 9d2b68cc108d ("net: enetc: add support for XDP_REDIRECT") Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- v4->v5: none v3->v4: - patch is new. It replaces "net: enetc: pad short XDP frames coming from devmap", which padded the xdp_frame in place using a new generic xdp_frame_pad() helper, and only covered ndo_xdp_xmit(). The XDP_TX path is now covered too, using the tx_pad buffer instead. --- drivers/net/ethernet/freescale/enetc/enetc.c | 36 +++++++++++++++++--- 1 file changed, 32 insertions(+), 4 deletions(-) diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c index 58a88f15fa5d..275441c2ffa3 100644 --- a/drivers/net/ethernet/freescale/enetc/enetc.c +++ b/drivers/net/ethernet/freescale/enetc/enetc.c @@ -1759,8 +1759,11 @@ static bool enetc_xdp_tx(struct enetc_bdr *tx_ring, { struct enetc_tx_swbd *tmp_tx_swbd = xdp_tx_arr; int i, k, frm_len = tmp_tx_swbd->len; + struct enetc_tx_swbd *last_swbd; + int pad_len; - if (unlikely(enetc_bd_unused(tx_ring) < ENETC_TXBDS_NEEDED(num_tx_swbd))) + /* Hardware does not support a first buffer size of less than 16 bytes */ + if (unlikely(xdp_tx_arr[0].len < ENETC_MIN_BUFF_SIZE)) return false; while (unlikely(!tmp_tx_swbd->is_eof)) { @@ -1768,15 +1771,24 @@ static bool enetc_xdp_tx(struct enetc_bdr *tx_ring, frm_len += tmp_tx_swbd->len; } + /* Pad frames below ETH_ZLEN so the mEMAC does not pad them. */ + pad_len = max_t(int, ETH_ZLEN - frm_len, 0); + + if (unlikely(enetc_bd_unused(tx_ring) < + ENETC_TXBDS_NEEDED(num_tx_swbd + !!pad_len))) + return false; + i = tx_ring->next_to_use; for (k = 0; k < num_tx_swbd; k++) { struct enetc_tx_swbd *xdp_tx_swbd = &xdp_tx_arr[k]; - enetc_xdp_map_tx_buff(tx_ring, i, xdp_tx_swbd, frm_len); + enetc_xdp_map_tx_buff(tx_ring, i, xdp_tx_swbd, + frm_len + pad_len); + last_swbd = &tx_ring->tx_swbd[i]; - /* last BD needs 'F' bit set */ - if (xdp_tx_swbd->is_eof) { + /* last BD needs 'F' bit set, unless a pad BD follows */ + if (xdp_tx_swbd->is_eof && !pad_len) { union enetc_tx_bd *txbd = ENETC_TXBD(*tx_ring, i); txbd->flags = ENETC_TXBD_FLAGS_F; @@ -1785,6 +1797,22 @@ static bool enetc_xdp_tx(struct enetc_bdr *tx_ring, enetc_bdr_idx_inc(tx_ring, &i); } + /* The pad BD becomes the last one, so it takes over the xdp_frame + * (if any) from the last data BD. The pad BD has no DMA mapping of + * its own, and it is not an XDP_TX buffer to be recycled. + */ + if (unlikely(pad_len)) { + struct enetc_tx_swbd *pad_swbd = &tx_ring->tx_swbd[i]; + + enetc_append_tx_pad(tx_ring, i, pad_len); + pad_swbd->is_xdp_redirect = last_swbd->is_xdp_redirect; + pad_swbd->xdp_frame = last_swbd->xdp_frame; + last_swbd->xdp_frame = NULL; + last_swbd->is_eof = false; + + enetc_bdr_idx_inc(tx_ring, &i); + } + tx_ring->next_to_use = i; return true; -- 2.43.0 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v5 net 4/4] net: enetc: pad short XDP frames in software 2026-10-21 14:53 ` [PATCH v5 net 4/4] net: enetc: pad short XDP frames in software vladimir.oltean @ 2026-10-25 15:21 ` netdev-bot+sashiko 0 siblings, 0 replies; 15+ messages in thread From: netdev-bot+sashiko @ 2026-10-25 15:21 UTC (permalink / raw) To: vladimir.oltean Cc: andrew+netdev, davem, edumazet, kuba, pabeni, claudiu.manoil, wei.fang, xiaoning.wang, ioana.ciornei, zefir.kurtisi, zefir.kurtisi, david.laight.linux, netdev, imx, linux-kernel Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider. Critical: 0 · High: 1 (1) · Medium: 0 · Low: 1 If you believe none of the issues are legitimate, please reply to each issue of Medium severity or higher and explain why the AI is wrong. If any of them are legitimate and you plan to respin, please let patchwork know by including "pw-bot: cr" as a separate line at the end of your reply (one such reply per series is enough). - [High] The new XDP pad BD in enetc_xdp_tx() is only partly initialized, so after a failed XDP reconfiguration it can inherit a stale… - [Low] The commit message understates what the new 16-byte head-buffer check in enetc_xdp_tx() fixes. Pre-existing issues: - [High] Concurrent, unsynchronized producers on the same XDP TX ring. -- net: enetc: pad short XDP frames in software This patch makes enetc_xdp_tx() drop XDP frames whose head buffer is shorter than 16 bytes. When a frame is shorter than ETH_ZLEN, it appends a BD pointing to the shared tx_pad zero buffer. The pad BD then takes over the F flag, the is_eof marker and the xdp_frame pointer from the last data BD. > Similar to the skb path, the XDP transmit path may put frames shorter > than ETH_ZLEN in the TX ring, and may put frames whose first buffer is > shorter than 16 bytes. The former relies on mEMAC hardware padding, > which we no longer want to rely upon, and the latter is not supported > by ENETC at all. [Severity: Low] Could the commit message say what happens without the 16-byte check? Before this patch, enetc_xdp_tx() only checked enetc_bd_unused(). A head BD shorter than 16 bytes went straight into the TX ring. The earlier patch in this series, "net: enetc: ensure head buffer has at least 16 octets", describes the result. TBaCIR does not advance, TBaSR[BUSY] stays set, and the NAPI/TBaIER re-enable sequence turns this into an MSI-X IRQ storm that soft-locks the host. That looks reachable today through ndo_xdp_xmit(). For example, a veth XDP program redirecting through a devmap can forward a 14 or 15 byte frame. The frame can come from bpf_xdp_adjust_tail(), or the peer can simply send one. The Fixes tags are already there. What seems to be missing is a description of the impact, a soft lockup on the XDP redirect path. > diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c > index f5a47d53d2bea..e3a08a82dc3c9 100644 > --- a/drivers/net/ethernet/freescale/enetc/enetc.c > +++ b/drivers/net/ethernet/freescale/enetc/enetc.c [ ... ] > @@ -1771,15 +1774,24 @@ static bool enetc_xdp_tx(struct enetc_bdr *tx_ring, > frm_len += tmp_tx_swbd->len; > } > > + /* Pad frames below ETH_ZLEN so the mEMAC does not pad them. */ > + pad_len = max_t(int, ETH_ZLEN - frm_len, 0); > + > + if (unlikely(enetc_bd_unused(tx_ring) < > + ENETC_TXBDS_NEEDED(num_tx_swbd + !!pad_len))) > + return false; > + > i = tx_ring->next_to_use; [Severity: High] This is a pre-existing issue and was not introduced by this patch. Can two producers write to the same XDP TX ring at the same time? enetc_xdp_xmit() picks the ring by the executing CPU: tx_ring = priv->xdp_tx_ring[smp_processor_id()]; The XDP_TX path in enetc_clean_rx_ring_xdp() picks it by RX ring index: tx_ring = priv->xdp_tx_ring[rx_ring->index]; The only thing tying a vector to a CPU is irq_set_affinity_hint() in enetc_setup_irqs(). irqbalance, /proc/irq/*/smp_affinity or threaded NAPI can run RX vector i's NAPI on CPU j. Meanwhile CPU i can call ndo_xdp_xmit() into this device, for example from a devmap redirect. enetc_lock_mdio() takes read_lock(&enetc_mdio_lock), and only when the ERR050089 workaround is active. That does not serialize the two paths. Both producers can then: - pass the enetc_bd_unused() check; - read the same next_to_use; - overwrite the same swbds and BDs, now including the pad BD and the xdp_frame handoff; - write next_to_use and the tail. Could that lose frames or leak DMA mappings and xdp_frames? It could also leave xdp_tx_in_flight undecremented for overwritten XDP_TX buffers, which throttles RX refill, or hand torn BDs to the hardware. Would per-ring producer serialization be needed here? That could be a spinlock taken by both paths, or __netif_tx_lock() as other drivers use for shared XDP queues. > > for (k = 0; k < num_tx_swbd; k++) { > struct enetc_tx_swbd *xdp_tx_swbd = &xdp_tx_arr[k]; [ ... ] > @@ -1788,6 +1800,22 @@ static bool enetc_xdp_tx(struct enetc_bdr *tx_ring, > enetc_bdr_idx_inc(tx_ring, &i); > } > > + /* The pad BD becomes the last one, so it takes over the xdp_frame > + * (if any) from the last data BD. The pad BD has no DMA mapping of > + * its own, and it is not an XDP_TX buffer to be recycled. > + */ > + if (unlikely(pad_len)) { > + struct enetc_tx_swbd *pad_swbd = &tx_ring->tx_swbd[i]; > + > + enetc_append_tx_pad(tx_ring, i, pad_len); > + pad_swbd->is_xdp_redirect = last_swbd->is_xdp_redirect; > + pad_swbd->xdp_frame = last_swbd->xdp_frame; [Severity: High] Can pad_swbd keep a stale is_xdp_tx and page from an earlier use of this slot? enetc_append_tx_pad() sets only len, is_eof, do_twostep_tstamp and check_wb. The code here adds is_xdp_redirect and xdp_frame. Nothing writes is_xdp_tx, page, page_offset, dir, dma or is_dma_page. The data BDs are fine, because enetc_xdp_map_tx_buff() memcpy()s the whole swbd. That makes the pad BD the first XDP ring slot that depends on already being zero. The slot is not always zero. On an XDP program change, enetc_reconfigure() calls enetc_stop() and then enetc_free_rxtx_rings(). For XDP_TX slots that were not cleaned, enetc_free_tx_ring()->enetc_free_tx_frame() only does this: if (tx_swbd->dma) enetc_unmap_tx_buff(tx_ring, tx_swbd); That clears dma but leaves is_xdp_tx = 1 and page set. netif_set_real_num_tx_queues() can then fail in enetc_reconfigure_xdp_cb(), for example with -ENOMEM when an XDP program is removed. In that case the out_restart path restarts the old rings with the same tx_swbd arrays: out_restart: enetc_setup_bdrs(priv, extended); enetc_start(priv->ndev); The old XDP program is restored, so XDP_TX keeps running. Every 56 byte VLAN-stripped XDP_TX frame now gets a pad BD. If a pad BD lands on a stale slot, enetc_clean_tx_ring() checks is_xdp_tx before dma: if (tx_swbd->is_xdp_tx) enetc_recycle_xdp_tx_buff(tx_ring, tx_swbd); Would that put {dma = 0, stale page, stale page_offset} into the RX ring? It would also call dma_sync_single_range_for_device() on address 0 and wrongly decrement xdp_tx_in_flight. enetc_refill_rx_ring() would then program rxbd->w.addr with 0 + page_offset, so RX DMA would go to a bus address near 0. If the RX ring is full, dma_unmap_page() would run on address 0 instead. The incomplete scrubbing on the out_restart path is older than this patch. The new pad BD is what exposes it on XDP rings. Would a memset of the swbd in enetc_append_tx_pad() be enough? A more robust option would be for enetc_free_tx_frame()/enetc_free_tx_ring() to scrub the whole swbd, so slots are zero after teardown as well. [ ... ] -- Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261021145314.1435049-1-vladimir.oltean%40nxp.com ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2026-10-25 15:21 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2026-10-21 14:53 [PATCH v5 net 0/4] Fix short frame transmission in enetc vladimir.oltean 2026-10-21 14:53 ` [PATCH v5 net 1/4] net: enetc: reserve VLAN extension BDs for software TSO vladimir.oltean 2026-10-25 15:21 ` netdev-bot+sashiko 2026-10-21 14:53 ` [PATCH v5 net 2/4] net: enetc: do not rely on mEMAC hardware frame padding vladimir.oltean 2026-10-22 8:33 ` David Laight 2026-10-22 9:41 ` Vladimir Oltean 2026-10-22 10:22 ` David Laight 2026-10-22 12:35 ` Vladimir Oltean 2026-10-22 20:48 ` David Laight 2026-10-23 9:35 ` Vladimir Oltean 2026-10-25 15:21 ` netdev-bot+sashiko 2026-10-21 14:53 ` [PATCH v5 net 3/4] net: enetc: ensure head buffer has at least 16 octets vladimir.oltean 2026-10-25 15:21 ` netdev-bot+sashiko 2026-10-21 14:53 ` [PATCH v5 net 4/4] net: enetc: pad short XDP frames in software vladimir.oltean 2026-10-25 15:21 ` netdev-bot+sashiko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®