[PATCH v4 net 0/8] Fix short frame transmission in enetc

[PATCH v4 net 0/8] 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.

The original series was focused on handling the invalid frame
geometries (now patches 4, 5 and 6). However, review pointed out further
issues in annex code. The other patches handle those.

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/

Change log in individual patches. At a high level:
- applied feedback from Wei Fang, David Laight and Sashiko
- dropped "[PATCH v3 net 7/7] net: enetc: drain and cancel one-step TX
  tstamp queue when going down" because addressing Sashiko's feedback
  would grow this series to a point where it would become impossible to
  make any progress; plus it becomes more and more unrelated to the main
  objective. Will address that part separately.
- dropped "[PATCH v3 net 6/7] net: enetc: linearize PTP event packets
  with one-step TX timestamping"; also unrelated to short frames, will
  resubmit separately.
- split v3's "pad short frames in software" into patch 4 (do not rely on
  mEMAC hardware frame padding) and patch 5 (ensure head buffer has at
  least 16 octets).
- reworked v3's "pad short XDP frames coming from devmap": instead of
  padding the xdp_frame in place through a new generic xdp_frame_pad()
  helper (which only covered ndo_xdp_xmit()), patch 6 now pads in the
  driver using the shared tx_pad buffer, covering the XDP_TX path too.
  Consequently, there is no longer a change to include/net/xdp.h, so the
  BPF/XDP folks are no longer CCed.
- new patches:
  - 2/8: serialize access to the same TX ring from NAPI XDP_TX and
    ndo_xdp_xmit()
  - 7/8: fix leak of pending XDP_TX frames when going down

Vladimir Oltean (8):
  net: enetc: consistently track dropped frames in enetc_xdp_xmit()
  net: enetc: serialize access to the same TX ring from NAPI XDP_TX and
    ndo_xdp_xmit()
  net: enetc: fix bogus TX ring consumer index after reinitialization
  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
  net: enetc: fix leak of pending XDP_TX frames when going down
  net: enetc: ensure enetc_xdp_xmit() calls enetc_update_tx_ring_tail()

 drivers/net/ethernet/freescale/enetc/enetc.c | 160 ++++++++++++++++---
 drivers/net/ethernet/freescale/enetc/enetc.h |  12 ++
 2 files changed, 148 insertions(+), 24 deletions(-)

-- 
2.43.0

[PATCH v4 net 1/8] net: enetc: consistently track dropped frames in enetc_xdp_xmit()

[vladimir.oltean]

2 distinct classes of problems:
(a) the following events from enetc_xdp_xmit() will result in the frames
    being silently dropped without any counter tracking that action:
    - failures in enetc_xdp_frame_to_xdp_tx_swbd(),
    - frames split across too many buffers
    - frames enqueued while the interface is down
    Similar to how enetc_xdp_tx() increments xdp_tx_drops, let's reuse
    this ethtool counter for the above cases too.
(b) enetc_xdp_tx() failures increment xdp_tx_drops by 1, but that is not
    necessarily correct. Wei Fang points out that on .ndo_xdp_xmit()
    failure, bq_xmit_all() drops all remaining frames from the batch,
    not just the current one. So we should in any case increment the
    xdp_tx_drops counter by the remainder, not just by 1.

A limitation that remains is that the xdp_tx_drops ethtool statistics
counter (already) tracks XDP_TX and XDP_REDIRECT drops in the same
value. We should distinguish between these at some point, but that is an
ABI change and is out of scope for a bug fix.

Fixes: 9d2b68cc108d ("net: enetc: add support for XDP_REDIRECT")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v3->v4:
- make sure the ENETC_TX_DOWN case also increments the xdp_tx_drops
  counter
- reword the commit message to enumerate the cases of silently dropped
  frames
v2->v3:
- patch is new; replaces:
https://lore.kernel.org/netdev/20260406204122.167237-2-vladimir.oltean@nxp.com/
---
 drivers/net/ethernet/freescale/enetc/enetc.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
index e91ba77a1532..103335f3d418 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc.c
@@ -1811,15 +1811,18 @@ int enetc_xdp_xmit(struct net_device *ndev, int num_frames,
 	struct enetc_bdr *tx_ring;
 	int xdp_tx_bd_cnt, i, k;
 	int xdp_tx_frm_cnt = 0;
+	int err = 0;
+
+	tx_ring = priv->xdp_tx_ring[smp_processor_id()];
 
 	if (unlikely(test_bit(ENETC_TX_DOWN, &priv->flags) ||
-		     !netif_carrier_ok(ndev)))
-		return -ENETDOWN;
+		     !netif_carrier_ok(ndev))) {
+		err = -ENETDOWN;
+		goto out;
+	}
 
 	enetc_lock_mdio();
 
-	tx_ring = priv->xdp_tx_ring[smp_processor_id()];
-
 	prefetchw(ENETC_TXBD(*tx_ring, tx_ring->next_to_use));
 
 	for (k = 0; k < num_frames; k++) {
@@ -1840,7 +1843,6 @@ int enetc_xdp_xmit(struct net_device *ndev, int num_frames,
 			for (i = 0; i < xdp_tx_bd_cnt; i++)
 				enetc_unmap_tx_buff(tx_ring,
 						    &xdp_redirect_arr[i]);
-			tx_ring->stats.xdp_tx_drops++;
 			break;
 		}
 
@@ -1850,11 +1852,13 @@ int enetc_xdp_xmit(struct net_device *ndev, int num_frames,
 	if (unlikely((flags & XDP_XMIT_FLUSH) || k != xdp_tx_frm_cnt))
 		enetc_update_tx_ring_tail(tx_ring);
 
-	tx_ring->stats.xdp_tx += xdp_tx_frm_cnt;
-
 	enetc_unlock_mdio();
 
-	return xdp_tx_frm_cnt;
+out:
+	tx_ring->stats.xdp_tx += xdp_tx_frm_cnt;
+	tx_ring->stats.xdp_tx_drops += num_frames - xdp_tx_frm_cnt;
+
+	return err ?: xdp_tx_frm_cnt;
 }
 EXPORT_SYMBOL_GPL(enetc_xdp_xmit);
 
-- 
2.43.0

[PATCH v4 net 2/8] net: enetc: serialize access to the same TX ring from NAPI XDP_TX and ndo_xdp_xmit()

[vladimir.oltean]

The enetc driver uses a struct enetc_int_vector which holds a fixed
mapping between the interrupt vector and the RX ring it services.
While this mapping is fixed, it has been pointed out that the IRQ
affinity to a CPU can be changed by user space, so there is no fixed
mapping between that RX ring and a CPU.

That means that on LS1028A, NAPI on CPU 0 can service RX ring 1, and if
a BPF program requests the XDP_TX verdict, received traffic will be sent
to the TX ring reserved for XDP_TX associated with that RX ring.

At the same time, XDP_REDIRECT on CPU 1 can also call ndo_xdp_xmit(),
and this selects for transmission the XDP TX ring of the current CPU,
aka the same as of RX ring 1.

This is bad, because the transmission code is not written with
concurrency in mind, and access to the TX BD ring pointers as well as
the statistics counters is unsynchronized.

Create a spin lock in struct enetc_bdr (common between RX and TX rings),
initialize the lock for TX rings and use it to serialize these two
problematic code paths. Since the calling contexts are always softirq,
we can use the plain spin_lock() over spin_lock_bh() which is also more
expensive.

Specifically, the spin lock only synchronizes the stats counters
relevant to XDP (xdp_tx/xdp_tx_drops) and the TX BDR producer state
(next_to_use, tx_swbd[] and hardware counterparts). The rest should be
not necessary.

The lock placement is inside a 52 byte hole in cacheline 1 of struct
enetc_bdr. The hole is now 48 bytes in size.

Testing shows for a single-CPU XDP_TX flow on 1 interface, 64B packets:

 Unmitigated MPPS    Naive mitigation MPPS   Batched mitigation MPPS
 2.32                1.9                     2.2

where 'naive' means locking per packet in enetc_clean_rx_ring_xdp(), and
'batched' means what this patch is doing, i.e. acquire the TX ring lock
at the beginning of the poll cycle, irrespective of XDP program verdict
(necessary only for XDP_TX), and release it around xdp_do_redirect()
and napi_gro_receive(). We then reacquire it around
enetc_update_tx_ring_tail(), so that no matter which CPU rings the
doorbell, the correct producer index (tx_ring->next_to_use) is committed
to hardware.

Note that in principle we could drop the rx_ring from struct
enetc_int_vector, and as in the ndo_xdp_xmit() case, dynamically pick up
in the NET_RX softirq that RX ring which corresponds to the current CPU.
Because of the softirq context, ndo_xdp_xmit() would be excluded from
running in a lockless way.

However, doing this seems counterproductive to the user space intention
of changing the IRQ affinity. It opens the door for user space to move
all interrupt vectors to the same CPU, which will leave the other RX
ring unprocessed. This seems like a bad idea considering RSS/RFS.

Fixes: 9d2b68cc108d ("net: enetc: add support for XDP_REDIRECT")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v3->v4: patch is new
---
 drivers/net/ethernet/freescale/enetc/enetc.c | 21 ++++++++++++++++++--
 drivers/net/ethernet/freescale/enetc/enetc.h |  4 ++++
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
index 103335f3d418..bd824b55a916 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc.c
@@ -1814,6 +1814,7 @@ int enetc_xdp_xmit(struct net_device *ndev, int num_frames,
 	int err = 0;
 
 	tx_ring = priv->xdp_tx_ring[smp_processor_id()];
+	spin_lock(&tx_ring->xdp_lock);
 
 	if (unlikely(test_bit(ENETC_TX_DOWN, &priv->flags) ||
 		     !netif_carrier_ok(ndev))) {
@@ -1858,6 +1859,8 @@ int enetc_xdp_xmit(struct net_device *ndev, int num_frames,
 	tx_ring->stats.xdp_tx += xdp_tx_frm_cnt;
 	tx_ring->stats.xdp_tx_drops += num_frames - xdp_tx_frm_cnt;
 
+	spin_unlock(&tx_ring->xdp_lock);
+
 	return err ?: xdp_tx_frm_cnt;
 }
 EXPORT_SYMBOL_GPL(enetc_xdp_xmit);
@@ -1998,6 +2001,9 @@ static int enetc_clean_rx_ring_xdp(struct enetc_bdr *rx_ring,
 	/* next descriptor to process */
 	i = rx_ring->next_to_clean;
 
+	tx_ring = priv->xdp_tx_ring[rx_ring->index];
+	spin_lock(&tx_ring->xdp_lock);
+
 	enetc_lock_mdio();
 
 	while (likely(rx_frm_cnt < work_limit)) {
@@ -2064,11 +2070,12 @@ static int enetc_clean_rx_ring_xdp(struct enetc_bdr *rx_ring,
 			enetc_bulk_flip_buff(rx_ring, orig_i, i);
 
 			enetc_unlock_mdio();
+			spin_unlock(&tx_ring->xdp_lock);
 			napi_gro_receive(napi, skb);
+			spin_lock(&tx_ring->xdp_lock);
 			enetc_lock_mdio();
 			break;
 		case XDP_TX:
-			tx_ring = priv->xdp_tx_ring[rx_ring->index];
 			if (unlikely(test_bit(ENETC_TX_DOWN, &priv->flags))) {
 				enetc_xdp_drop(rx_ring, orig_i, i);
 				tx_ring->stats.xdp_tx_drops++;
@@ -2101,7 +2108,11 @@ static int enetc_clean_rx_ring_xdp(struct enetc_bdr *rx_ring,
 			break;
 		case XDP_REDIRECT:
 			enetc_unlock_mdio();
+			spin_unlock(&tx_ring->xdp_lock);
+
 			err = xdp_do_redirect(rx_ring->ndev, &xdp_buff, prog);
+
+			spin_lock(&tx_ring->xdp_lock);
 			enetc_lock_mdio();
 			if (unlikely(err)) {
 				enetc_xdp_drop(rx_ring, orig_i, i);
@@ -2117,6 +2128,8 @@ static int enetc_clean_rx_ring_xdp(struct enetc_bdr *rx_ring,
 	}
 
 out:
+	spin_unlock(&tx_ring->xdp_lock);
+
 	rx_ring->next_to_clean = i;
 
 	rx_ring->stats.packets += rx_frm_cnt;
@@ -2128,8 +2141,11 @@ static int enetc_clean_rx_ring_xdp(struct enetc_bdr *rx_ring,
 		enetc_lock_mdio();
 	}
 
-	if (xdp_tx_frm_cnt)
+	if (xdp_tx_frm_cnt) {
+		spin_lock(&tx_ring->xdp_lock);
 		enetc_update_tx_ring_tail(tx_ring);
+		spin_unlock(&tx_ring->xdp_lock);
+	}
 
 	if (cleaned_cnt > rx_ring->xdp.xdp_tx_in_flight)
 		enetc_refill_rx_ring(rx_ring, enetc_bd_unused(rx_ring) -
@@ -3591,6 +3607,7 @@ static int enetc_int_vector_init(struct enetc_ndev_priv *priv, int i,
 		idx = priv->bdr_int_num * j + i;
 		__set_bit(idx, &v->tx_rings_map);
 		bdr = &v->tx_ring[j];
+		spin_lock_init(&bdr->xdp_lock);
 		bdr->index = idx;
 		bdr->ndev = priv->ndev;
 		bdr->dev = priv->dev;
diff --git a/drivers/net/ethernet/freescale/enetc/enetc.h b/drivers/net/ethernet/freescale/enetc/enetc.h
index d9e91832a9c1..bc7c0740498d 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.h
+++ b/drivers/net/ethernet/freescale/enetc/enetc.h
@@ -171,6 +171,10 @@ struct enetc_bdr {
 	void __iomem *idr; /* Interrupt Detect Register pointer */
 
 	int buffer_offset;
+
+	/* Serialize NAPI and ndo_xdp_xmit() targeting the same TX ring */
+	spinlock_t xdp_lock;
+
 	struct enetc_xdp_data xdp;
 
 	struct enetc_ring_stats stats;
-- 
2.43.0

[PATCH v4 net 3/8] net: enetc: fix bogus TX ring consumer index after reinitialization

[vladimir.oltean]

The TBCIR (Transmit Buffer Descriptor Ring Consumer Index) register has
the BD index as the lower 16 bits, but the upper 16 bits contain this
field:

  STAT_ID: Status identifier. Incremented each time the BDR_INDEX is
  updated and an error status bit was set for one of the processed BDs.
  Clears on read.

If there was any transmit error prior to the ring reinitialization and
this is the first time we re-read the TBCIR register, reading it will
give us a value with non-zero upper bits, which is saved in
bdr->next_to_clean.

If subsequently NAPI gets invoked and enetc_clean_tx_ring() runs, this
will dereference the &tx_ring->tx_swbd[] for the bogus (and huge)
next_to_clean index, and will result in an out-of-bounds memory access.

Other places like enetc_bd_ready_count() do mask out the upper bits, so
let's do that here as well.

Fixes: d4fd0404c1c9 ("enetc: Introduce basic PF and VF ENETC ethernet drivers")
Reviewed-by: Wei Fang <wei.fang@nxp.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v4: none
---
 drivers/net/ethernet/freescale/enetc/enetc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
index bd824b55a916..a2692761a6e2 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc.c
@@ -2647,7 +2647,8 @@ static void enetc_setup_txbdr(struct enetc_hw *hw, struct enetc_bdr *tx_ring)
 	 * adjust sw indexes
 	 */
 	tx_ring->next_to_use = enetc_txbdr_rd(hw, idx, ENETC_TBPIR);
-	tx_ring->next_to_clean = enetc_txbdr_rd(hw, idx, ENETC_TBCIR);
+	tx_ring->next_to_clean = enetc_txbdr_rd(hw, idx, ENETC_TBCIR) &
+				 ENETC_TBCIR_IDX_MASK;
 
 	if (tx_ring->next_to_use != tx_ring->next_to_clean &&
 	    !is_enetc_rev1(si)) {
-- 
2.43.0

[PATCH v4 net 4/8] 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 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.

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>
---
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 | 58 ++++++++++++++++++--
 drivers/net/ethernet/freescale/enetc/enetc.h |  6 ++
 2 files changed, 60 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
index a2692761a6e2..9ab90145dcfa 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,23 @@ static int enetc_map_tx_tso_data(struct enetc_bdr *tx_ring, struct sk_buff *skb,
 	return 0;
 }
 
+/* Append a zero-padding BD to a frame shorter than ETH_ZLEN */
+static void enetc_map_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 = 1;
+	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)
 {
@@ -915,6 +932,8 @@ 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 frm_len;
+		int pad_len;
 		char *hdr;
 
 		/* Get the BD */
@@ -926,6 +945,10 @@ 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);
+
 		/* 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);
@@ -933,7 +956,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) {
@@ -955,8 +978,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)
@@ -977,6 +1002,13 @@ static int enetc_map_tx_tso_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb
 
 		enetc_tso_complete_csum(tx_ring, &tso, skb, hdr, pos, csum);
 
+		if (pad_len) {
+			enetc_bdr_idx_inc(tx_ring, &i);
+			enetc_map_tx_pad(tx_ring, i, pad_len);
+			tx_swbd = &tx_ring->tx_swbd[i];
+			count++;
+		}
+
 		if (total_len == 0)
 			tx_swbd->skb = skb;
 
@@ -1029,7 +1061,9 @@ 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)) {
+			/* +gso_segs to cover one pad BD per segment */
+			if (enetc_bd_unused(tx_ring) < tso_count_descs(skb) +
+			    skb_shinfo(skb)->gso_segs) {
 				netif_stop_subqueue(ndev, tx_ring->index);
 				return NETDEV_TX_BUSY;
 			}
@@ -1039,6 +1073,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;
@@ -2294,8 +2331,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:
@@ -2307,6 +2354,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);
@@ -2434,6 +2482,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 bc7c0740498d..08613e6a1d28 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 {
@@ -186,6 +188,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

[PATCH v4 net 5/8] 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>
---
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 9ab90145dcfa..08870df613b8 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc.c
@@ -1076,6 +1076,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 08613e6a1d28..b5f38ad17280 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 v4 net 6/8] 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. Furthermore, once the driver honours
  bpf_xdp_adjust_tail() and bpf_xdp_adjust_head() for XDP_TX, 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>
---
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 08870df613b8..15c48c938a4e 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc.c
@@ -1747,8 +1747,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)) {
@@ -1756,15 +1759,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;
@@ -1773,6 +1785,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_map_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

[PATCH v4 net 7/8] net: enetc: fix leak of pending XDP_TX frames when going down

[vladimir.oltean]

enetc_stop() calls enetc_disable_tx_bdrs() and this abruptly breaks the
natural flow where enetc_recycle_xdp_tx_buff() returns XDP_TX buffers to
the RX ring.

There are two problems. One is that frames remain stuck in the TX ring,
the other is that enetc_free_tx_ring() removes their DMA mapping but
otherwise loses track of them, i.e. does not do what
enetc_recycle_xdp_tx_buff() does, and leaves the RX ring waiting for
buffers that will never come.

We concentrate only on the latter here. The RX ring refuses to refill
itself because xdp_tx_in_flight is high, but after we bring up the
interface again, in reality we have no buffers. So all ingress traffic
will be dropped.

Fixes: 7ed2bc80074e ("net: enetc: add support for XDP_TX")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v3->v4: patch is new
---
 drivers/net/ethernet/freescale/enetc/enetc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
index 15c48c938a4e..fe5db840d2cc 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc.c
@@ -165,6 +165,14 @@ static void enetc_free_tx_frame(struct enetc_bdr *tx_ring,
 
 	if (tx_swbd->dma)
 		enetc_unmap_tx_buff(tx_ring, tx_swbd);
+	if (tx_swbd->is_xdp_tx) {
+		struct enetc_ndev_priv *priv = netdev_priv(tx_ring->ndev);
+		struct enetc_bdr *rx_ring;
+
+		rx_ring = enetc_rx_ring_from_xdp_tx_ring(priv, tx_ring);
+		rx_ring->xdp.xdp_tx_in_flight--;
+		__free_page(tx_swbd->page);
+	}
 
 	if (xdp_frame) {
 		xdp_return_frame(tx_swbd->xdp_frame);
-- 
2.43.0

[PATCH v4 net 8/8] net: enetc: ensure enetc_xdp_xmit() calls enetc_update_tx_ring_tail()

[vladimir.oltean]

enetc_xdp_xmit() keeps track of 3 numbers:
- num_frames: total length of passed struct xdp_frame **frames array
- xdp_tx_frm_cnt: number of frames successfully sent
- k: index of currently sent frame from array

With "k != xdp_tx_frm_cnt", the intention was to detect an early break
due to an inability to send a frame, and to trigger a TX doorbell
anyway.

However, that doesn't work because every time when the loop breaks,
k and xdp_tx_frm_cnt are mathematically equal.

The correct condition on which we should ring the doorbell is when at
least one frame was sent, and either the caller required us to flush, or
we couldn't enqueue the entire passed array.

After updating the enetc_update_tx_ring_tail() calling condition, we can
delete the 'xdp_tx_frm_cnt' variable, since it is equal to 'k' after the
loop exits.

Fixes: 9d2b68cc108d ("net: enetc: add support for XDP_REDIRECT")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Reviewed-by: Wei Fang <wei.fang@nxp.com>
---
v3->v4:
- context change after introduction of &bdr->xdp_lock
- make sure k is initialized with 0 for the ENETC_TX_DOWN ||
  !netif_carrier_ok() early exit, like xdp_tx_frm_cnt was
v2->v3: delete redundant xdp_tx_frm_cnt variable
v1->v2: patch is new
---
 drivers/net/ethernet/freescale/enetc/enetc.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
index fe5db840d2cc..9ae7fef377d1 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc.c
@@ -1888,8 +1888,8 @@ int enetc_xdp_xmit(struct net_device *ndev, int num_frames,
 	struct enetc_ndev_priv *priv = netdev_priv(ndev);
 	struct skb_shared_info *shinfo;
 	struct enetc_bdr *tx_ring;
-	int xdp_tx_bd_cnt, i, k;
-	int xdp_tx_frm_cnt = 0;
+	int xdp_tx_bd_cnt;
+	int i, k = 0;
 	int err = 0;
 
 	tx_ring = priv->xdp_tx_ring[smp_processor_id()];
@@ -1925,22 +1925,20 @@ int enetc_xdp_xmit(struct net_device *ndev, int num_frames,
 						    &xdp_redirect_arr[i]);
 			break;
 		}
-
-		xdp_tx_frm_cnt++;
 	}
 
-	if (unlikely((flags & XDP_XMIT_FLUSH) || k != xdp_tx_frm_cnt))
+	if (unlikely(k && ((flags & XDP_XMIT_FLUSH) || k < num_frames)))
 		enetc_update_tx_ring_tail(tx_ring);
 
 	enetc_unlock_mdio();
 
 out:
-	tx_ring->stats.xdp_tx += xdp_tx_frm_cnt;
-	tx_ring->stats.xdp_tx_drops += num_frames - xdp_tx_frm_cnt;
+	tx_ring->stats.xdp_tx += k;
+	tx_ring->stats.xdp_tx_drops += num_frames - k;
 
 	spin_unlock(&tx_ring->xdp_lock);
 
-	return err ?: xdp_tx_frm_cnt;
+	return err ?: k;
 }
 EXPORT_SYMBOL_GPL(enetc_xdp_xmit);
 
-- 
2.43.0

Re: [PATCH v4 net 2/8] net: enetc: serialize access to the same TX ring from NAPI XDP_TX and ndo_xdp_xmit()

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 3 (1) · 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 new xdp_lock does not serialize enetc_xdp_xmit()
  (ndo_xdp_xmit) against the regular stack transmit path,…
- [High] Lock-order inversion between tx_ring->xdp_lock and the ERR050089
  MDIO rwlock (enetc_mdio_lock), introduced in enetc_clean_rx_ring_xdp().
- [High] net/enetc: concurrent TX ring corruption from stack and
  XDP_REDIRECT

Pre-existing issues:
- [High] enetc_stop() does not synchronize with ndo_xdp_xmit() calls
  already in progress.

--

net: enetc: serialize access to the same TX ring from NAPI XDP_TX and ndo_xdp_xmit()

This patch adds a spinlock (xdp_lock) to struct enetc_bdr. The lock
serializes the NAPI XDP_TX path and ndo_xdp_xmit() when both target the
same XDP TX ring. NAPI takes the lock for the whole poll cycle, drops it
around napi_gro_receive() and xdp_do_redirect(), and takes it again
around enetc_update_tx_ring_tail().

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
> index 26abc5fafd188..418be387af18b 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.c
> @@ -1817,6 +1817,7 @@ int enetc_xdp_xmit(struct net_device *ndev, int num_frames,
>  	int err = 0;
>  
>  	tx_ring = priv->xdp_tx_ring[smp_processor_id()];
> +	spin_lock(&tx_ring->xdp_lock);

[Severity: High]
When no XDP program is attached to this interface, is the ring picked
here also used by the regular stack transmit path?

With priv->xdp_prog NULL, enetc_num_stack_tx_queues() exposes every TX
ring to the stack:

	if (priv->xdp_prog)
		return num_tx_rings - num_possible_cpus();

	return num_tx_rings;

This makes the rings that priv->xdp_tx_ring[] points to ordinary netdev
queues as well. enetc_start_xmit() selects them through
priv->tx_ring[skb->queue_mapping]. It then updates next_to_use,
tx_swbd[] and the TBPIR doorbell while holding only the netdev_queue
xmit lock.

A redirect from another interface does not need a program on this
port. Devmap only checks the advertised NETDEV_XDP_ACT_NDO_XMIT bit
before calling ndo_xdp_xmit(). enetc_xdp_xmit() does not look at
priv->xdp_prog before using priv->xdp_tx_ring[smp_processor_id()].
That path now takes xdp_lock, but enetc_start_xmit() never does, so
the two producers are still not serialized against each other.

Can they then claim the same next_to_use slots and overwrite each
other's tx_swbd[] entries? tx_swbd[] holds a union of skb and
xdp_frame, and the is_xdp_redirect bit selects between them. Completion
could therefore treat an skb as an xdp_frame or the reverse. It could
also free a buffer twice or leak it, and the hardware could be handed a
BD that points at memory that is already unmapped. Any traffic that
another interface redirects into this port is enough to trigger it.

This race predates the patch, since ndo_xdp_xmit() had no locking at
all before. However, the commit message and the new comment on
struct enetc_bdr::xdp_lock present the lock as covering the TX BDR
producer state. It does not do that while the stack shares the ring.

Would it work to take the netdev_queue xmit lock in both XDP
producers instead? That would be __netif_tx_lock() on
netdev_get_tx_queue(ndev, tx_ring->index), as fec, stmmac and igb do.
That would serialize all three writers and make xdp_lock unnecessary.

Alternatively, should enetc_xdp_xmit() refuse frames, or NDO_XMIT be
cleared with xdp_features_clear_redirect_target(), while no program
has reserved the XDP rings?

[Severity: High]
If no XDP program is attached to this port, can enetc_start_xmit() be
writing to this same ring at the same time?

With priv->xdp_prog NULL, enetc_num_stack_tx_queues() gives every ring to
the stack:

	if (priv->xdp_prog)
		return num_tx_rings - num_possible_cpus();

	return num_tx_rings;

So the rings that priv->xdp_tx_ring[] points to are also regular stack
queues. There is no ndo_select_queue, and enetc_start_xmit() uses them
while holding only the netdev_queue xmit lock:

	tx_ring = priv->tx_ring[skb->queue_mapping];

enetc_pf_netdev_setup() always advertises NETDEV_XDP_ACT_NDO_XMIT, and
devmap only checks that feature bit. A redirect from another interface
into this port therefore reaches enetc_xdp_xmit() even when no program
is loaded here. enetc_xdp_xmit() does not check priv->xdp_prog. It takes
only xdp_lock, and the stack path never takes that lock.

Could both producers then claim the same next_to_use slots and
tx_swbd[] entries? tx_swbd[] holds a union of skb and xdp_frame,
selected by is_xdp_redirect and the other bitfields. Completion could
then free an skb as an xdp_frame or the reverse, free a buffer twice,
leak one, or give the hardware a DMA address that is already unmapped.

This window also existed before the patch, because ndo_xdp_xmit() had no
locking at all. However, the commit message says:

  Specifically, the spin lock only synchronizes the stats counters
  relevant to XDP (xdp_tx/xdp_tx_drops) and the TX BDR producer state
  (next_to_use, tx_swbd[] and hardware counterparts). The rest should be
  not necessary.

The new comment on struct enetc_bdr::xdp_lock also names only NAPI and
ndo_xdp_xmit() as writers.

Would it work to take the netdev_queue xmit lock in both XDP producers
instead? That would be __netif_tx_lock() on
netdev_get_tx_queue(ndev, tx_ring->index), as fec, stmmac and igb do. It
would cover all three writers and replace xdp_lock.

Alternatively, could ndo_xdp_xmit be refused, or NDO_XMIT cleared with
xdp_features_clear_redirect_target(), while no program reserves the XDP
rings?

>  
>  	if (unlikely(test_bit(ENETC_TX_DOWN, &priv->flags) ||
>  		     !netif_carrier_ok(ndev))) {

[Severity: High]
This is a pre-existing issue, not one introduced by this patch. Does
enetc_stop() synchronize with an enetc_xdp_xmit() call that has already
passed this check?

enetc_stop() does the following without taking xdp_lock or waiting for a
grace period:

	set_bit(ENETC_TX_DOWN, &priv->flags);

	netif_tx_stop_all_queues(ndev);

	enetc_disable_rx_bdrs(priv);

	enetc_wait_bdrs(priv);

	enetc_disable_tx_bdrs(priv);

The napi_disable() calls that follow cover only the enetc vectors. They
do not cover the redirecting device's NAPI, which runs enetc_xdp_xmit()
from the devmap flush.

Suppose the ENETC_TX_DOWN test runs just before set_bit(). Then
enetc_xdp_xmit() keeps writing TX BDs, tx_swbd[] and next_to_use, and
rings TBPIR, while enetc_stop() drains and disables the ring.

After that, enetc_close() calls enetc_free_rxtx_rings() and
enetc_assign_tx_resources(priv, NULL). enetc_reconfigure() frees the
resources in a similar way.

Can the redirecting CPU then write into the freed tx_swbd array and BD
ring?

The core does not seem to close this window either. __dev_close_many()
calls synchronize_net() before ndo_stop, not after, and
enetc_reconfigure() keeps the device up the whole time. The race is
easier to hit on PREEMPT_RT, where softirq processing can be preempted.

The NAPI XDP_TX path has a smaller window of the same kind. It tests
ENETC_TX_DOWN per frame but rings the doorbell at the end of the poll.
napi_synchronize() runs only after enetc_wait_bdrs() and
enetc_disable_tx_bdrs().

On ENETC v1, enetc_setup_txbdr() copies next_to_use and next_to_clean
back from TBPIR/TBCIR. A late doorbell could therefore carry a stale
producer index into the newly allocated ring. Whether the hardware
latches a TBPIR write to a disabled ring has not been verified.

Now that xdp_lock exists, would taking it around set_bit(ENETC_TX_DOWN)
in enetc_stop(), or calling synchronize_net() after the set_bit(), close
this window?

[ ... ]

> @@ -2120,6 +2131,8 @@ static int enetc_clean_rx_ring_xdp(struct enetc_bdr *rx_ring,
>  	}
>  
>  out:
> +	spin_unlock(&tx_ring->xdp_lock);
> +
>  	rx_ring->next_to_clean = i;
>  
>  	rx_ring->stats.packets += rx_frm_cnt;

[ ... ]

> @@ -2131,8 +2144,11 @@ static int enetc_clean_rx_ring_xdp(struct enetc_bdr *rx_ring,
>  		enetc_lock_mdio();
>  	}
>  
> -	if (xdp_tx_frm_cnt)
> +	if (xdp_tx_frm_cnt) {
> +		spin_lock(&tx_ring->xdp_lock);
>  		enetc_update_tx_ring_tail(tx_ring);
> +		spin_unlock(&tx_ring->xdp_lock);
> +	}

[Severity: High]
Does this reverse the lock order between xdp_lock and enetc_mdio_lock?

In the poll loop and in enetc_xdp_xmit(), xdp_lock is taken first and
enetc_lock_mdio() second:

	spin_lock(&tx_ring->xdp_lock);
	...
	enetc_lock_mdio();

At out:, xdp_lock is dropped while the MDIO read lock is still held, and
the xdp_do_flush() block takes the MDIO read lock again. So this
spin_lock() runs with the MDIO read lock held, which is the opposite
order.

On PREEMPT_RT both are sleeping locks, and this cycle seems possible:

CPU A (enetc_clean_rx_ring_xdp)
  holds enetc_mdio_lock (read)
  spin_lock(&tx_ring->xdp_lock)          <- waits for CPU B

CPU B (enetc_xdp_xmit, same XDP TX ring)
  holds tx_ring->xdp_lock
  enetc_lock_mdio() -> read_lock()       <- waits for CPU C

CPU C (_enetc_rd_mdio_reg_wa)
  write_lock_irqsave(&enetc_mdio_lock)   <- waits for CPU A's reader

rwbase_write_lock() takes the rtmutex and removes READER_BIAS. After
that, __rwbase_read_lock() always goes to
rwbase_rtmutex_slowlock_locked(), so CPU B blocks on the rtmutex owned
by CPU C while CPU C waits for CPU A to drop its read lock.

Can this deadlock on LS1028A with PREEMPT_RT? There, enetc_has_err050089
is enabled and periodic PHY polling is enough to provide the MDIO
writer. On non-RT kernels, qrwlock readers in softirq context are not
blocked by a writer that is only waiting, so this looks RT-specific.

Would ringing the doorbell before dropping xdp_lock at out: keep the
order consistent? Another option would be to call enetc_unlock_mdio()
before taking xdp_lock here and take the MDIO lock again afterwards.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261015105536.1692348-1-vladimir.oltean%40nxp.com

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 6 potential
issue(s) to consider.

Critical: 0 · High: 2 (2) · Medium: 3 · 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] net: enetc: under-reserved TX ring capacity for VLAN TSO packets
- [High] net/enetc: TX BD hardware limit violation from pad BD
- [Medium] The hardware LSO path, enetc_lso_hw_offload() ->
  enetc_lso_start()/enetc_lso_map_hdr(), gets no minimum-frame-length…
- [Medium] In enetc_start_xmit(), the new software-TSO admission check is
  enetc_bd_unused(tx_ring) < tso_count_descs(skb) + gso_segs, i.e.…
- [Medium] The new early return in enetc_start_xmit(), 'if
  (eth_skb_pad(skb)) return NETDEV_TX_OK;', leaks the…
- [Low] XDP transmit paths are not padded in this patch.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the software-
  TSO ring reservation in enetc_start_xmit() still does not count the…
- [High] This problem wasn't introduced by this patch, but the
  priv->tx_onestep_tstamp work item is never cancelled or flushed.

--

net: enetc: do not rely on mEMAC hardware frame padding

This patch stops relying on the mEMAC TXP hardware padding. Non-GSO skbs
are now padded with eth_skb_pad(). Software TSO segments shorter than
ETH_ZLEN get an extra BD pointing to a shared zeroed coherent buffer
(tx_pad), and the TSO ring reservation in enetc_start_xmit() grows by
gso_segs.

> 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]
Is "all frames" accurate for this patch?

The XDP transmit paths are XDP_TX from enetc_clean_rx_ring_xdp() and
ndo_xdp_xmit via enetc_xdp_xmit(). Both end up in enetc_xdp_tx(), which
still queues frames at their raw length:

    int i, k, frm_len = tmp_tx_swbd->len;

ENETC_PM0_CMD_TXP is also still set in enetc_configure_port_mac(), so
short XDP frames still depend on the MAC padding.

A later patch in this series, "net: enetc: pad short XDP frames in
software", appears to fix this by appending a tx_pad BD in enetc_xdp_tx().
So for this patch it may only be a matter of the commit message wording.

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
> index e464eca482f78..707c17fe4a4c5 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.c

[ ... ]

> @@ -980,6 +1005,13 @@ static int enetc_map_tx_tso_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb
>  
>  		enetc_tso_complete_csum(tx_ring, &tso, skb, hdr, pos, csum);
>  
> +		if (pad_len) {
> +			enetc_bdr_idx_inc(tx_ring, &i);
> +			enetc_map_tx_pad(tx_ring, i, pad_len);
> +			tx_swbd = &tx_ring->tx_swbd[i];
> +			count++;
> +		}

[Severity: High]
Can a padded TSO segment exceed the hardware limit on chained BDs?

Per the comment in enetc.h, a frame can use at most 15 chained TX BDs,
including the head and extension BD. That is why ENETC_MAX_SKB_FRAGS
is 13. The data loop in enetc_map_tx_tso_buffs() enforces this with:

    if (unlikely(bd_data_num >= priv->max_frags && data_len))
        goto err_chained_bd;

So a segment whose data_len is used up by exactly max_frags data BDs
leaves the loop normally. If that segment is also shorter than
ETH_ZLEN, the pad BD is appended unconditionally.

With a VLAN tag, that gives one frame:

  - 1 header BD
  - 1 extension BD
  - 13 data BDs
  - 1 pad BD

That is 16 BDs, one over the limit.

Such a segment only needs a few payload bytes spread over many tiny
frags, plus a very small gso_size. A vnet_hdr packet socket or a VM
guest can construct that.

What does the LS1028A MAC do with a 16 BD chain? If it stalls or
raises a DMA error, should the bd_data_num limit leave room for the pad
BD when pad_len is non-zero? Segments that do not fit could then take
the err_chained_bd path.

[Severity: High]
This is a pre-existing issue, but can the ring overrun here for
VLAN-tagged TSO skbs?

When skb_vlan_tag_present() is true, enetc_map_tx_tso_hdr() emits an
extension BD for every segment:

    if (skb_vlan_tag_present(skb))
        flags |= ENETC_TXBD_FLAGS_EX;

Each segment can then use a header BD, an extension BD, its data BDs, and
now a pad BD. The worst case is 3 * gso_segs + nr_frags + P, where P is
the number of padded segments.

The reservation in enetc_start_xmit() is tso_count_descs(skb) + gso_segs,
which is 3 * gso_segs + nr_frags. Before this patch the extension BDs were
not counted at all. Now the extra gso_segs absorbs them, and the pad BDs
are left uncovered. So the "+gso_segs to cover one pad BD per segment"
comment does not hold for VLAN-tagged skbs.

For local TCP over an 8021q device, P is at most 1. That uses up the
one-slot gap and makes next_to_use equal to next_to_clean, so the ring
looks empty to software.

For DODGY TSO skbs with a tiny gso_size, P can approach gso_segs. These
can come from a bridged VM guest or a vnet_hdr packet socket, with the tag
kept as hwaccel.

Could enetc_map_tx_tso_buffs() then write past next_to_clean? That would
overwrite tx_swbd entries and BDs that are still in flight, and lose
their skb and DMA references.

> +
>  		if (total_len == 0)
>  			tx_swbd->skb = skb;
>  
> @@ -1032,7 +1064,9 @@ static netdev_tx_t enetc_start_xmit(struct sk_buff *skb,
>  
>  			count = enetc_lso_hw_offload(tx_ring, skb);

[Severity: Medium]
Does the hardware LSO path need the same minimum frame length handling?

The commit message says "user space may set gso_size to a very small
value", but only enetc_map_tx_tso_buffs() is changed. When ENETC_F_LSO is
active (ENETC v4), GSO skbs go to enetc_lso_hw_offload(), and
enetc_lso_start() takes the segment size straight from gso_size:

    lso->lso_seg_size = skb_shinfo(skb)->gso_size;

Two cases can produce short segments:

  - an unprivileged UDP_SEGMENT socket with a small gso_size
  - the last segment of a TCP TSO skb without timestamps (54 bytes of
    headers plus a few bytes of payload)

These segments would still reach the MAC below 60 bytes. On the same v4
device, non-GSO skbs are now padded with eth_skb_pad().

The code does not show whether the v4 MAC or LSO engine pads with the
same suspect bytes. enetc4_pf.c does not appear to set TXP explicitly.
If it does pad that way, is this fix incomplete for LSO?

If hardware-padded LSO segments are fine, could the commit message say
why?

>  		} else {
> -			if (enetc_bd_unused(tx_ring) < tso_count_descs(skb)) {
> +			/* +gso_segs to cover one pad BD per segment */
> +			if (enetc_bd_unused(tx_ring) < tso_count_descs(skb) +
> +			    skb_shinfo(skb)->gso_segs) {
>  				netif_stop_subqueue(ndev, tx_ring->index);
>  				return NETDEV_TX_BUSY;
>  			}

[Severity: High]
Does the new reservation still fall short for VLAN-tagged software TSO
skbs?

tso_count_descs() returns 2 * gso_segs + nr_frags. That covers one
header BD and one data BD per segment, plus one extra data BD per frag
boundary.

When skb_vlan_tag_present() is true, enetc_map_tx_tso_hdr() also emits
an extension BD for every segment:

    if (skb_vlan_tag_present(skb))
        flags |= ENETC_TXBD_FLAGS_EX;

enetc_map_tx_tso_buffs() can now add a pad BD per segment on top of
that. The worst case is then 4 * gso_segs + nr_frags (header,
extension, data and pad). enetc_start_xmit() only checks for
3 * gso_segs + nr_frags.

The extension BDs were already unaccounted for before this patch.
Still, the "+gso_segs to cover one pad BD per segment" comment only
holds when there is no VLAN tag.

With a tiny gso_size, nearly every segment needs both an extension BD
and a pad BD. Examples are a DODGY skb from a VM guest, or one from a
vnet_hdr packet socket, with the tag carried as hwaccel.

Suppose the ring is close to full when such a skb is admitted. Can
enetc_map_tx_tso_buffs() then advance past next_to_clean and overwrite
BDs and tx_swbd entries that hardware still owns? That would lose their
skb and DMA references, and could corrupt or hang TX.

Should the check add another gso_segs when skb_vlan_tag_present(skb)?
Or would it be better to count only the segments that actually need
padding?

[Severity: Medium]
Can this check still succeed for software TSO skbs with a large
gso_segs?

The reservation is now 3 * gso_segs + nr_frags. But
enetc_map_tx_tso_buffs() only emits a pad BD when hdr_len + data_len <
ETH_ZLEN, and with a normal TCP MSS that is at most the last segment.

ENETC_TX_RING_DEFAULT_SIZE is 2048, so enetc_bd_unused() is at most 2047.
The driver does not call netif_set_tso_max_segs() and has no
ndo_features_check. So a skb with gso_segs between roughly 677 and 1015
used to be admitted and now never is.

The queue is stopped and NETDEV_TX_BUSY is returned. enetc_clean_tx_ring()
only wakes the queue after frames complete:

    if (unlikely(tx_frm_cnt && netif_carrier_ok(ndev) &&
                 __netif_subqueue_stopped(ndev, tx_ring->index) &&

Once the ring drains, the requeued skb fails the check again and nothing
restarts the queue. Would that TX queue stay stalled until the interface
is restarted?

Possible triggers:

  - a local socket with a small TCP_MAXSEG (e.g. 88)
  - a peer advertising a small MSS
  - DODGY TSO skbs from a VM guest with gso_size around 64-96

The same stall already existed for gso_segs above about 1015; this patch
lowers the threshold. Could the pad BDs be reserved only for segments
that need them, or could gso_max_segs be capped?

> @@ -1042,6 +1076,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]
Does this early return leak the ENETC_TX_ONESTEP_TSTAMP_IN_PROGRESS bit?

For skbs flagged ENETC_F_TX_ONESTEP_SYNC_TSTAMP, the bit is taken at the
top of enetc_start_xmit():

    if (test_and_set_bit_lock(ENETC_TX_ONESTEP_TSTAMP_IN_PROGRESS,
                              &priv->flags)) {
        skb_queue_tail(&priv->tx_skbs, skb);

The only release is in enetc_tx_onestep_tstamp(). That work is scheduled
from enetc_clean_tx_ring() only when a one-step skb completes TX.

An L2 one-step PTP Sync frame is 58 bytes, so it now always goes through
eth_skb_pad() -> skb_put_padto() -> skb_pad(). If the skb is cloned or
has no tailroom, pskb_expand_head(GFP_ATOMIC) can fail. The skb is then
freed and NETDEV_TX_OK is returned with no BD queued.

No completion will ever schedule the work. Wouldn't the bit stay set for
good, leaving every later one-step Sync skb on priv->tx_skbs?

The existing drop_packet_err and NETDEV_TX_BUSY exits seem to have the
same problem. The later pskb_may_pull() drop in this series adds another
instance.

> +
>  		if (unlikely(skb_shinfo(skb)->nr_frags > priv->max_frags))
>  			if (unlikely(skb_linearize(skb)))
>  				goto drop_packet_err;

[ ... ]

> @@ -2437,6 +2485,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;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but is the tx_onestep_tstamp
work cancelled before these resources go away?

enetc_clean_tx_ring() calls schedule_work(&priv->tx_onestep_tstamp) when
a one-step Sync skb completes. The driver has no cancel_work_sync() or
flush_work() for that work, and never purges priv->tx_skbs.

enetc_close() runs enetc_stop(), enetc_free_rxtx_rings() and
enetc_assign_tx_resources(priv, NULL). That NULLs tx_ring->bd_base and
tx_ring->tx_swbd and frees the resources. The work may run after that
with a skb queued on priv->tx_skbs:

enetc_tx_onestep_tstamp()
    skb = skb_dequeue(&priv->tx_skbs);
    if (skb)
        enetc_start_xmit(skb, priv->ndev);

enetc_start_xmit() has no ENETC_TX_DOWN check. Can enetc_map_tx_buffs()
then dereference a NULL or freed bd_base/tx_swbd?

On device removal, a work item that is still pending would also use priv
after free_netdev(), in container_of() and netif_tx_lock_bh(priv->ndev).
That happens even when tx_skbs is empty.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261015105536.1692348-1-vladimir.oltean%40nxp.com

Re: [PATCH v4 net 6/8] net: enetc: pad short XDP frames in software

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 1 (1) · 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 new XDP pad BD in enetc_xdp_tx() is only partly initialised.
- [Medium] In enetc_xdp_tx()
  (drivers/net/ethernet/freescale/enetc/enetc.c), the new early `return…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but the XDP_TX
  path in enetc_clean_rx_ring_xdp() ignores changes the BPF program…

--

net: enetc: pad short XDP frames in software

When an XDP frame is shorter than ETH_ZLEN, enetc_xdp_tx() now appends a
BD that points to the shared tx_pad zero buffer. The F flag, is_eof and
xdp_frame move onto that pad BD. Frames whose head buffer is shorter than
ENETC_MIN_BUFF_SIZE are dropped, because both callers treat a false return
as a drop.

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
> index 89b47accbac84..d456b9b488acf 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.c
> @@ -1750,8 +1750,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;

[Severity: Medium]
Before this patch, enetc_xdp_tx() returned false only when the TX ring
was full. With this check it can also return false because of what is in
the frame.

enetc_xdp_xmit() still treats every false return as a full ring and stops
processing the bulk:

drivers/net/ethernet/freescale/enetc/enetc.c:enetc_xdp_xmit() {
    ...
		if (unlikely(!enetc_xdp_tx(tx_ring, xdp_redirect_arr,
					   xdp_tx_bd_cnt))) {
			for (i = 0; i < xdp_tx_bd_cnt; i++)
				enetc_unmap_tx_buff(tx_ring,
						    &xdp_redirect_arr[i]);
			break;
		}
    ...
}

Then bq_xmit_all() in kernel/bpf/devmap.c frees every frame from the
returned count onward:

	for (i = sent; unlikely(i < to_send); i++)
		xdp_return_frame_rx_napi(bq->q[i]);

Take a redirected frame with a 14 or 15 byte head. One example is a
14-byte AF_PACKET frame sent on a veth peer and redirected through a
devmap. Another is a frame trimmed to ETH_HLEN with bpf_xdp_adjust_tail().

In that case, are the valid frames after it in the same ndo_xdp_xmit()
bulk dropped and counted in xdp_tx_drops too, even though the ring has
room?

The commit message says:

    Both callers already treat a false return code from enetc_xdp_tx() as
    a drop.

It doesn't mention that the rest of the bulk is dropped as well, and the
later patches in the series don't change this.

Could enetc_xdp_xmit() check the head length before the DMA mapping, and
drop only that frame instead of breaking out of the loop?

>  	while (unlikely(!tmp_tx_swbd->is_eof)) {
> @@ -1759,15 +1762,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);

[Severity: Medium]
This is a pre-existing issue, not one introduced by this patch. On the
XDP_TX path, frm_len is built from lengths that leave out any changes the
BPF program made. enetc_clean_rx_ring_xdp() fills xdp_tx_arr through
enetc_rx_swbd_to_xdp_tx_swbd():

	tx_swbd->dma = rx_swbd->dma;
	...
	tx_swbd->page_offset = rx_swbd->page_offset;
	tx_swbd->len = rx_swbd->len;

rx_swbd->len is set in enetc_map_rx_buff_to_xdp() and
enetc_add_rx_buff_to_xdp() before bpf_prog_run_xdp() runs. The driver
never reads back xdp_buff.data/data_end or any frag sizes the program
changed.

If a program calls bpf_xdp_adjust_head() or bpf_xdp_adjust_tail() and
returns XDP_TX, does it still send the original byte range and length?

The commit message mentions this ("once the driver honours
bpf_xdp_adjust_tail() and bpf_xdp_adjust_head() for XDP_TX"), and no patch
in the series fixes it. The padding added here uses the same stale
lengths, which do match what is actually sent.

> +
> +	if (unlikely(enetc_bd_unused(tx_ring) <
> +		     ENETC_TXBDS_NEEDED(num_tx_swbd + !!pad_len)))
> +		return false;
> +
>  	i = tx_ring->next_to_use;

[ ... ]

> @@ -1776,6 +1788,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_map_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]
Is pad_swbd fully initialized here? enetc_map_tx_pad() sets only these
swbd fields:

	tx_swbd->len = pad_len;
	tx_swbd->is_eof = 1;
	tx_swbd->do_twostep_tstamp = false;
	tx_swbd->check_wb = false;

This block then adds is_xdp_redirect and xdp_frame. The other fields
(is_xdp_tx, page, page_offset, dma, is_dma_page, dir and qbv_en) keep
whatever the slot held before. The data BDs are fine, because
enetc_xdp_map_tx_buff() memcpy()s the whole swbd.

enetc_clean_tx_ring() normally memsets each slot it cleans, but slots can
be left stale. enetc_stop() doesn't clean BDs that have already completed.
After that, enetc_free_tx_frame() only does:

	if (tx_swbd->dma)
		enetc_unmap_tx_buff(tx_ring, tx_swbd);

That clears dma but leaves is_xdp_tx and page set.

Now suppose enetc_reconfigure_xdp_cb() fails, for example when
netif_set_real_num_tx_queues() returns -ENOMEM on XDP detach.
enetc_reconfigure() then restarts the same tx_swbd arrays without
scrubbing them:

out_restart:
	enetc_setup_bdrs(priv, extended);
	enetc_start(priv->ndev);

enetc_setup_txbdr() reloads next_to_clean and next_to_use from the
hardware, so these uncleaned slots end up in the free part of the ring.

Later, a short XDP frame may put its pad BD in one of those slots. At
completion, enetc_clean_tx_ring() would then run:

		if (tx_swbd->is_xdp_tx)
			enetc_recycle_xdp_tx_buff(tx_ring, tx_swbd);

on the pad BD. Couldn't this put the stale page back into the RX ring
with dma == 0, or unmap and free a page that was already freed? Either
way, xdp_tx_in_flight would be decremented for a BD that never
incremented it.

The later patch "fix leak of pending XDP_TX frames when going down"
seems to make this worse. It frees the page but still leaves is_xdp_tx
and page set.

Would it help to clear the pad swbd fully, for example with a memset
before enetc_map_tx_pad(), or by clearing is_xdp_tx, page, is_dma_page
and dma explicitly?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261015105536.1692348-1-vladimir.oltean%40nxp.com

Re: [PATCH v4 net 7/8] net: enetc: fix leak of pending XDP_TX frames when going down

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · 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 new XDP_TX branch in enetc_free_tx_frame()
  (drivers/net/ethernet/freescale/enetc/enetc.c:171-178) decrements…

--

net: enetc: fix leak of pending XDP_TX frames when going down

This patch changes enetc_free_tx_frame() so that XDP_TX buffers still in
the TX ring at teardown are handled. It decrements the RX ring's
xdp_tx_in_flight counter and frees the page, so the RX ring is no longer
left waiting for buffers that will never come back.

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
> index d456b9b488acf..218065b9a3777 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.c
> @@ -168,6 +168,14 @@ static void enetc_free_tx_frame(struct enetc_bdr *tx_ring,
>  
>  	if (tx_swbd->dma)
>  		enetc_unmap_tx_buff(tx_ring, tx_swbd);
> +	if (tx_swbd->is_xdp_tx) {
> +		struct enetc_ndev_priv *priv = netdev_priv(tx_ring->ndev);
> +		struct enetc_bdr *rx_ring;
> +
> +		rx_ring = enetc_rx_ring_from_xdp_tx_ring(priv, tx_ring);
> +		rx_ring->xdp.xdp_tx_in_flight--;
> +		__free_page(tx_swbd->page);
> +	}

[Severity: High]
Should tx_swbd->is_xdp_tx and tx_swbd->page be cleared here, or the whole
swbd memset, after the page is freed?

As written, the swbd still has is_xdp_tx set and still points at the page
that was just freed. enetc_unmap_tx_buff() clears only ->dma, and the code
after this block clears only ->xdp_frame or ->skb. Before this patch,
calling enetc_free_tx_frame() twice on the same swbd did no harm. With
this patch, the second call frees the same page again.

The other release paths do scrub their descriptors. enetc_clean_tx_ring()
does this after enetc_recycle_xdp_tx_buff():

    memset(tx_swbd, 0, sizeof(*tx_swbd));

and enetc_free_rx_ring() sets rx_swbd->page = NULL.

The tx_swbd arrays can be reused after enetc_free_tx_ring() on the error
path of enetc_reconfigure():

    enetc_stop(priv->ndev);
    enetc_free_rxtx_rings(priv);

    /* Interface is down, run optional callback now */
    if (cb) {
        err = cb(priv, ctx);
        if (err)
            goto out_restart;
    }
    [ ... ]
out_restart:
    enetc_setup_bdrs(priv, extended);
    enetc_start(priv->ndev);

Here is one way to reach that path. Detaching an XDP program goes through
enetc_setup_xdp_prog()->enetc_reconfigure()->enetc_reconfigure_xdp_cb().
netif_set_real_num_tx_queues() can fail there with -ENOMEM when the stack
TX queue count grows. The callback then restores the old program, and
enetc_reconfigure() takes out_restart.

The interface then restarts on the old tx_swbd arrays. Any XDP_TX slot
that hardware had sent but NAPI never cleaned before enetc_stop() still
has is_xdp_tx=1 and a pointer to a freed page.

enetc_setup_txbdr() reloads next_to_use and next_to_clean from
TBPIR/TBCIR, so these stale slots can end up outside the active window.
They are only overwritten when enetc_xdp_map_tx_buff() copies a whole new
swbd into them.

At the next enetc_close() or enetc_reconfigure(), enetc_free_tx_ring()
walks every slot:

    for (i = 0; i < tx_ring->bd_count; i++) {
        struct enetc_tx_swbd *tx_swbd = &tx_ring->tx_swbd[i];

        enetc_free_tx_frame(tx_ring, tx_swbd);
    }

Would that call __free_page() a second time on the already freed page,
and decrement xdp_tx_in_flight a second time?

Nothing resets xdp_tx_in_flight. Once it goes negative, this check in
enetc_clean_rx_ring_xdp() is always true:

    if (cleaned_cnt > rx_ring->xdp.xdp_tx_in_flight)
        enetc_refill_rx_ring(rx_ring, enetc_bd_unused(rx_ring) -
                             rx_ring->xdp.xdp_tx_in_flight);

Can that ask for more buffers than there are free RX BDs, and overwrite
BDs the hardware still owns?

There is a second variant. If a short XDP frame's pad BD lands in a stale
slot, enetc_map_tx_pad() sets only len, is_eof, do_twostep_tstamp and
check_wb. In that case is_xdp_tx and page are left as they were. When
that BD completes, enetc_clean_tx_ring() would call
enetc_recycle_xdp_tx_buff() on the freed page with dma == 0.

Could that put a freed page back into the RX ring for device DMA, and
decrement xdp_tx_in_flight once more?

Also, on rev1, if PI != CI at restart, the hardware would resume the
stale BDs, which now point at unmapped DMA addresses.

>  
>  	if (xdp_frame) {
>  		xdp_return_frame(tx_swbd->xdp_frame);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261015105536.1692348-1-vladimir.oltean%40nxp.com

Re: [PATCH v4 net 8/8] net: enetc: ensure enetc_xdp_xmit() calls enetc_update_tx_ring_tail()

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 (1) · 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] In enetc_xdp_xmit()
  (drivers/net/ethernet/freescale/enetc/enetc.c), the doorbell condition…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it comes from
  the earlier commit in the same series, 'net: enetc: serialize access…

--

net: enetc: ensure enetc_xdp_xmit() calls enetc_update_tx_ring_tail()

This commit changes the doorbell condition in enetc_xdp_xmit(). The TX ring
tail is now updated only when at least one frame was sent and either
XDP_XMIT_FLUSH was requested or the frames array could not be fully
enqueued. The redundant xdp_tx_frm_cnt variable is removed in favour of k.

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
> index 218065b9a3777..3003a415418ba 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.c

[ ... ]

> @@ -1928,22 +1928,20 @@ int enetc_xdp_xmit(struct net_device *ndev, int num_frames,
>  						    &xdp_redirect_arr[i]);
>  			break;
>  		}
> -
> -		xdp_tx_frm_cnt++;
>  	}
>  
> -	if (unlikely((flags & XDP_XMIT_FLUSH) || k != xdp_tx_frm_cnt))
> +	if (unlikely(k && ((flags & XDP_XMIT_FLUSH) || k < num_frames)))
>  		enetc_update_tx_ring_tail(tx_ring);

[Severity: High]

Does the new k && term skip the doorbell when an XDP_XMIT_FLUSH call
enqueues zero frames?

The old condition rang on every FLUSH call, because k != xdp_tx_frm_cnt was
always false. XDP_XMIT_FLUSH asks the driver to publish everything queued so
far. That includes frames from earlier ndo_xdp_xmit() calls made with
flags == 0.

devmap makes calls like that when the bulk queue fills up:

kernel/bpf/devmap.c:bq_enqueue() {
    ...
	if (unlikely(bq->count == DEV_MAP_BULK_SIZE))
		bq_xmit_all(bq, 0);
    ...
}

Only the later __dev_flush() passes XDP_XMIT_FLUSH:

kernel/bpf/devmap.c:__dev_flush() {
    ...
		bq_xmit_all(bq, XDP_XMIT_FLUSH);
    ...
}

The flags == 0 call advances tx_ring->next_to_use without writing TPIR. If
the first frame of the next FLUSH batch fails, k is 0 and
enetc_update_tx_ring_tail() is never called.

The first frame can fail in these cases:

  - enetc_xdp_tx() finds too few free BDs. This is likely right after the
    previous batch used up the ring.
  - The head buffer is shorter than ENETC_MIN_BUFF_SIZE.
  - The frame fails the ENETC_MAX_SKB_FRAGS check.
  - DMA mapping fails.

On a ring reserved for XDP, the only other TPIR writer is the XDP_TX tail
update in enetc_clean_rx_ring_xdp(). It rings only when it has enqueued
XDP_TX frames itself:

	if (xdp_tx_frm_cnt) {
		spin_lock(&tx_ring->xdp_lock);
		enetc_update_tx_ring_tail(tx_ring);
		spin_unlock(&tx_ring->xdp_lock);
	}

So the stranded BDs stay unpublished until a later call both succeeds and
flushes. If traffic stops, they never go out.

If they fill the ring, every later enqueue fails on its first frame. TX
completion cannot reclaim BDs the hardware never saw. Can the XDP TX queue
then stall permanently, with the xdp_frames and pages still pinned?

Other drivers ring the tail on XDP_XMIT_FLUSH no matter how many frames
that call sent. For example, i40e_xdp_xmit():

	if (unlikely(flags & XDP_XMIT_FLUSH))
		i40e_xdp_ring_update_tail(xdp_ring);

Would something like this be closer to the intent?

	if (unlikely((flags & XDP_XMIT_FLUSH) || (k && k < num_frames)))

The commit message says:

    The correct condition on which we should ring the doorbell is when at
    least one frame was sent, and either the caller required us to flush, or
    we couldn't enqueue the entire passed array.

The subject says the patch ensures the tail update happens, and the patch
has a Fixes: tag. Even so, FLUSH calls with k == 0 no longer ring the
doorbell, and the commit message does not mention this change.

>  
>  	enetc_unlock_mdio();
>  
>  out:
> -	tx_ring->stats.xdp_tx += xdp_tx_frm_cnt;
> -	tx_ring->stats.xdp_tx_drops += num_frames - xdp_tx_frm_cnt;
> +	tx_ring->stats.xdp_tx += k;
> +	tx_ring->stats.xdp_tx_drops += num_frames - k;
>  
>  	spin_unlock(&tx_ring->xdp_lock);

[Severity: High]

This isn't a bug introduced by this patch. It comes from the earlier patch
in this series, "net: enetc: serialize access to the same TX ring from NAPI
XDP_TX and ndo_xdp_xmit()".

Can tx_ring->xdp_lock and enetc_mdio_lock deadlock on PREEMPT_RT when the
ERR050089 workaround (enetc_has_err050089) is enabled?

enetc_xdp_xmit() takes xdp_lock first and then the MDIO read lock:

	spin_lock(&tx_ring->xdp_lock);
	...
	enetc_lock_mdio();

enetc_clean_rx_ring_xdp() drops xdp_lock at the out: label but keeps the
MDIO read lock. It then takes xdp_lock again, so the order is reversed:

out:
	spin_unlock(&tx_ring->xdp_lock);
	...
	if (xdp_tx_frm_cnt) {
		spin_lock(&tx_ring->xdp_lock);
		enetc_update_tx_ring_tail(tx_ring);
		spin_unlock(&tx_ring->xdp_lock);
	}
	...
	enetc_unlock_mdio();

On PREEMPT_RT, rwbase_write_lock() takes the rtmutex and removes
READER_BIAS. New readers then block behind a pending writer. That would
allow this cycle:

T1 (RX NAPI, enetc_clean_rx_ring_xdp())
  holds the MDIO read lock after out:

T3 (_enetc_rd_mdio_reg_wa())
  write_lock_irqsave(&enetc_mdio_lock, flags);
  waits for T1 to drop the read lock

T2 (enetc_xdp_xmit() on another CPU, targeting the same XDP TX ring,
    e.g. after an IRQ affinity change)
  spin_lock(&tx_ring->xdp_lock);
  enetc_lock_mdio();          <- blocks behind T3

T1
  spin_lock(&tx_ring->xdp_lock);  <- blocks behind T2

Non-RT testing would not catch this. On non-RT, qrwlock readers in
interrupt context ignore a waiting writer, and lockdep treats this
read_lock as recursive.

Would it help to use one lock order in the RX tail update path? For
example, the MDIO read lock could be dropped before re-taking xdp_lock and
then taken again inside it.

>  
> -	return err ?: xdp_tx_frm_cnt;
> +	return err ?: k;
>  }
>  EXPORT_SYMBOL_GPL(enetc_xdp_xmit);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261015105536.1692348-1-vladimir.oltean%40nxp.com