[PATCH v5 net 0/4] Fix short frame transmission in enetc

[PATCH v5 net 0/4] Fix short frame transmission in enetc

[vladimir.oltean]

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

[PATCH v5 net 1/4] net: enetc: reserve VLAN extension BDs for software TSO

[vladimir.oltean]

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

[PATCH v5 net 2/4] net: enetc: do not rely on mEMAC hardware frame padding

[vladimir.oltean]

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

Re: [PATCH v5 net 2/4] net: enetc: do not rely on mEMAC hardware frame padding

[David Laight]

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)

Re: [PATCH v5 net 2/4] net: enetc: do not rely on mEMAC hardware frame padding

[Vladimir Oltean]

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!

Re: [PATCH v5 net 2/4] net: enetc: do not rely on mEMAC hardware frame padding

[David Laight]

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!

Re: [PATCH v5 net 2/4] net: enetc: do not rely on mEMAC hardware frame padding

[Vladimir Oltean]

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.

Re: [PATCH v5 net 2/4] net: enetc: do not rely on mEMAC hardware frame padding

[David Laight]

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

[PATCH v5 net 3/4] net: enetc: ensure head buffer has at least 16 octets

[vladimir.oltean]

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

[PATCH v5 net 4/4] net: enetc: pad short XDP frames in software

[vladimir.oltean]

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