[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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] enetc: Use-After-Free in enetc_xdp_xmit() racing with enetc_close() teardown
- [High] enetc: OOB array access if CPUs exceed hardware TX rings
- [High] enetc: TX ring corruption when XDP_REDIRECT overlaps with network stack
--

commit 805311ddad67da52149365adfc7772abd1f8ca5b
Author: vladimir.oltean@nxp.com <vladimir.oltean@nxp.com>

net: enetc: consistently track dropped frames in enetc_xdp_xmit()
    
This patch updates enetc_xdp_xmit() to consistently increment the xdp_tx_drops
counter for all silently dropped frames. It routes early failures and batch
failures to a common exit path to ensure statistics are updated accurately.

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
> index 80f0082f6c633..26abc5fafd188 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.c
> @@ -1814,15 +1814,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()];

[Severity: High]
Can tx_ring point to out-of-bounds memory here if the system has more
possible CPUs than hardware TX rings?

During initialization, the driver configures:

    first_xdp_tx_ring = priv->num_tx_rings - num_possible_cpus();
    priv->xdp_tx_ring = &priv->tx_ring[first_xdp_tx_ring];

If num_possible_cpus() is greater than priv->num_tx_rings,
first_xdp_tx_ring becomes negative, leaving priv->xdp_tx_ring pointing
out-of-bounds.

Because this patch moves the tx_ring assignment before the ENETDOWN check
and adds an unconditional dereference of tx_ring->stats in the goto out
path, an XDP_REDIRECT to this interface will now dereference this invalid
pointer and crash, whereas previously it would safely return -ENETDOWN.

[Severity: High]
Does this setup cause concurrent, lockless modifications of a TX ring
that is already in use by the network stack?

When no local XDP program is loaded, enetc_num_stack_tx_queues() allocates
all priv->num_tx_rings to the network stack. However, the driver also
unconditionally assigns priv->xdp_tx_ring to point to the uppermost
hardware queues.

If another interface uses XDP_REDIRECT to send frames to this device,
enetc_xdp_xmit() executes locklessly here, concurrently modifying the
same TX ring that the standard network stack is using under the Qdisc
__netif_tx_lock, leading to queue corruption.

>  
>  	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));

[ ... ]

> @@ -1853,11 +1855,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;

[Severity: High]
Will this result in a use-after-free if it races with the interface teardown?

In the teardown path, enetc_close() does not wait for RCU or hold a
synchronization lock against remote XDP_REDIRECT threads before freeing
the TX rings:

enetc_close()
    enetc_stop()
        set_bit(ENETC_TX_DOWN, &priv->flags);
    ...
    enetc_assign_tx_resources(priv, NULL);

Since this patch changes the early ENETDOWN check to jump to the out
label instead of returning immediately, the code will now unconditionally
dereference tx_ring->stats here even if the interface is going down
and the resources have already been freed.

> +	tx_ring->stats.xdp_tx_drops += num_frames - xdp_tx_frm_cnt;
> +
> +	return err ?: xdp_tx_frm_cnt;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015105536.1692348-1-vladimir.oltean@nxp.com?part=1

[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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] net/enetc: AB-BA deadlock between tx_ring->xdp_lock and enetc_mdio_lock
- [High] net/enetc: concurrent TX ring corruption from stack and XDP_REDIRECT
- [High] net: enetc: out-of-bounds array access in enetc_xdp_xmit()
--

commit 129585c95d671cf1c40cb76a4f76a3a478c00776
Author: vladimir.oltean@nxp.com <vladimir.oltean@nxp.com>

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

This commit introduces a spin lock to serialize access to the transmit ring
between the napi XDP_TX path and ndo_xdp_xmit(). It acquires the new
xdp_lock in both the transmit and receive paths.

> 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]
Could this lead to an out-of-bounds array access if the hardware provides
fewer transmit rings than num_possible_cpus()?

If priv->num_tx_rings < num_possible_cpus(), enetc_alloc_msix() evaluates
first_xdp_tx_ring to a negative index. This causes priv->xdp_tx_ring to
point to memory preceding the priv->tx_ring array.

While enetc_setup_xdp_prog() refuses to attach a local xdp program when
resources are insufficient, the destination device's ndo_xdp_xmit() can
still be invoked independently via cross-device XDP_REDIRECT from another
interface. Under those conditions, this unconditionally reads garbage
memory and crashes when acquiring tx_ring->xdp_lock.

[Severity: High]
Is there a risk of concurrent transmit ring corruption from the network stack?

If no local xdp program is attached to the interface,
enetc_num_stack_tx_queues() exposes all transmit rings (including the ones
pointed to by priv->xdp_tx_ring) to the core network stack. The stack
queues packets to these rings via enetc_start_xmit() using __netif_tx_lock,
rather than tx_ring->xdp_lock.

Concurrently, if an XDP_REDIRECT from another interface targets this one,
enetc_xdp_xmit() will modify the exact same transmit ring using
tx_ring->xdp_lock.

Will this mismatch in locking strategies lead to unsynchronized access to
the producer variables (next_to_use, tx_swbd[]), causing ring corruption?

[ ... ]

> @@ -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;
>  	rx_ring->stats.bytes += rx_byte_cnt;
>  
>  	if (xdp_redirect_frm_cnt) {
>  		enetc_unlock_mdio();
>  		xdp_do_flush();
>  		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 create an AB-BA deadlock between tx_ring->xdp_lock and
enetc_mdio_lock?

In enetc_xdp_xmit(), the code establishes the lock order tx_ring->xdp_lock
followed by enetc_mdio_lock:

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

However, here in enetc_clean_rx_ring_xdp(), tx_ring->xdp_lock is released
at the out: label, but the enetc_mdio_lock remains held (or is re-acquired
after xdp_do_flush()). Then, if xdp_tx_frm_cnt > 0, it attempts to
re-acquire tx_ring->xdp_lock while enetc_mdio_lock is held.

Since enetc_mdio_lock is an rwlock_t, a pending writer (like an ethtool phy
command) will block new readers. A cpu holding xdp_lock will block waiting
for mdio_lock, while the cpu holding mdio_lock (read) spins indefinitely
for xdp_lock, creating a hard deadlock.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015105536.1692348-1-vladimir.oltean@nxp.com?part=2

[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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] net: enetc: HW LSO bypasses software padding defense causing info leak
- [High] net: enetc: under-reserved TX ring capacity for VLAN TSO packets
- [High] net/enetc: TX BD hardware limit violation from pad BD
- [High] net: enetc: tx_swbd state corruption after DMA map failure unwind
--

commit 3dad676559c79da08025ef8636033dd30717a901
Author: vladimir.oltean@nxp.com <vladimir.oltean@nxp.com>

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

This commit changes the driver to pad all short frames to ETH_ZLEN in
software instead of relying on the hardware MAC to pad them, avoiding
potential information leaks of uninitialized SoC memory. For GSO packets,
it appends a zero-padding BD to segments that are shorter than ETH_ZLEN.

> 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
[ ... ]
> @@ -958,8 +981,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);

[Severity: High]
Can a DMA map failure here cause state corruption on subsequent
transmissions?

When enetc_map_tx_tso_data() fails, the error path unwinds the mapped
buffers via enetc_unwind_tx_frame(). However, enetc_unwind_tx_frame() does
not clear the tx_swbd state:

drivers/net/ethernet/freescale/enetc/enetc.c:enetc_unwind_tx_frame() {
    while (count--) {
        struct enetc_tx_swbd *tx_swbd = &tx_ring->tx_swbd[i];
        enetc_free_tx_frame(tx_ring, tx_swbd);
        ...
    }
}

When these slots are subsequently reused, enetc_map_tx_tso_data() populates
the dma and len fields but does not explicitly clear bitfields like
is_dma_page and is_eof from the unwound frame. Will premature observation
of the leftover is_eof == 1 flag corrupt the transmit ring cleanup process?

>  			if (err) {
>  				if (i == 0)
> @@ -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 this unconditional padding BD cause the frame to violate the 15-BD
hardware limit?

The LS1028A hardware restricts chained TX BDs to a maximum of 15 per frame.
The driver enforces a max_frags limit of 13 data fragments. In
enetc_map_tx_tso_buffs(), the fragment processing loop terminates normally
if the segment reaches exactly 13 fragments and data_len drops to 0:

drivers/net/ethernet/freescale/enetc/enetc.c:enetc_map_tx_tso_buffs() {
    ...
    if (unlikely(bd_data_num >= priv->max_frags && data_len))
        goto err_chained_bd;
}

After the loop, the padding BD is unconditionally appended. If a VLAN tag
is present, the frame will consist of 1 header BD + 1 VLAN extension BD +
13 data BDs + 1 padding BD = 16 BDs. Will this violate the 15-BD limit
and cause a MAC stall or DMA error?

> +
>  		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: High]
Does this code still allow hardware LSO to bypass the software padding
defense?

The patch introduces software padding to prevent the MAC from inserting
uninitialized SoC memory into short frames. However, for GSO packets, if
hardware LSO is enabled:

drivers/net/ethernet/freescale/enetc/enetc.c:enetc_start_xmit() {
    if (priv->active_offloads & ENETC_F_LSO && ...) {
        count = enetc_lso_hw_offload(tx_ring, skb);
    }
}

If the hardware LSO engine produces short segments (e.g., small gso_size
or a short final remainder segment), the LSO engine emits frames shorter
than ETH_ZLEN. Since ENETC_PM0_CMD_TXP remains set in the MAC, won't the
MAC automatically pad these short LSO segments with uninitialized memory,
continuing to leak SoC memory?

>  		} 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) {

[Severity: High]
Does this capacity check under-reserve descriptors for VLAN TSO packets?

The generic tso_count_descs() calculates gso_segs * 2 + nr_frags. The
updated check adds skb_shinfo(skb)->gso_segs to account for the new padding
BD, resulting in a total reservation of 3 * gso_segs + nr_frags. 

However, if a VLAN tag is present, enetc_map_tx_tso_hdr() allocates an
additional hardware-specific Extension BD for every segment:

drivers/net/ethernet/freescale/enetc/enetc.c:enetc_map_tx_tso_hdr() {
    if (flags & ENETC_TXBD_FLAGS_EX) {
        enetc_bdr_idx_inc(tx_ring, i);
        ...
        count++;
    }
}

This means the actual worst-case BD consumption is 4 * gso_segs + nr_frags.
Could transmitting a highly segmented software TSO packet with a VLAN tag
overflow the TX ring buffer when the ring is nearly full, silently
overwriting active descriptors?

>  				netif_stop_subqueue(ndev, tx_ring->index);
>  				return NETDEV_TX_BUSY;
>  			}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015105536.1692348-1-vladimir.oltean@nxp.com?part=4

[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

Re: [PATCH v4 net 5/8] net: enetc: ensure head buffer has at least 16 octets

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] enetc: missing lock release on dropped one-step tstamp packets
--

commit 9392e5d35a9d4208a5ea777cd38ae2adf7ac8c86
Author: vladimir.oltean@nxp.com <vladimir.oltean@nxp.com>
net: enetc: ensure head buffer has at least 16 octets

This commit addresses a hardware issue in ENETC devices where transmitting
frames with a head buffer under 16 bytes causes an interrupt storm. It adds
a check in enetc_start_xmit to pull at least 16 bytes into the linear skb
header, dropping the packet if that fails.

Closes: https://lore.kernel.org/netdev/b3d9136c-2803-4203-b1ea-1f9e62de80a1@gmail.com/

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
> index 707c17fe4a4c5..89b47accbac84 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.c
> @@ -1079,6 +1079,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;
> +		}
> +

[Severity: High]
Does this error path miss releasing the bit lock for one-step timestamp
packets?

Earlier in enetc_start_xmit(), the bit lock is acquired for packets with
the ENETC_F_TX_ONESTEP_SYNC_TSTAMP flag:

        if (enetc_cb->flag & ENETC_F_TX_ONESTEP_SYNC_TSTAMP) {
                if (test_and_set_bit_lock(ENETC_TX_ONESTEP_TSTAMP_IN_PROGRESS,
                                          &priv->flags)) {
                        skb_queue_tail(&priv->tx_skbs, skb);
                        return NETDEV_TX_OK;
                }
        }

If the newly added pskb_may_pull() check fails, the packet is freed and the
function returns NETDEV_TX_OK without clearing the previously acquired lock.

Could this leave the ENETC_TX_ONESTEP_TSTAMP_IN_PROGRESS bit permanently set,
causing subsequent one-step timestamp packets to queue infinitely to tx_skbs
without ever being dequeued?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015105536.1692348-1-vladimir.oltean@nxp.com?part=5

[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 8/8] net: enetc: ensure enetc_xdp_xmit() calls enetc_update_tx_ring_tail()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] net: enetc: XDP_XMIT_FLUSH is ignored when k == 0 causing TX ring stalls
--

commit a3de2d9d87edd1ec648a1023c638ce2d70118f57
Author: vladimir.oltean@nxp.com <vladimir.oltean@nxp.com>

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

This commit changes the loop exit condition that triggers the TX doorbell
in enetc_xdp_xmit(). It replaces the redundant xdp_tx_frm_cnt variable with
the loop index k and updates the logic determining when to flush the TX
ring.

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
> --- 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]
If a caller batches XDP frames by making multiple ndo_xdp_xmit() calls
with flags = 0, and then makes a final call with flags = XDP_XMIT_FLUSH,
does this new condition safely handle the case where the final call fails
to enqueue any frames?

If the caller passes num_frames == 0, or if the first frame fails to map
(for example, due to a full TX ring or DMA error), the loop exits with
k = 0.

With the addition of the "k &&" guard, the entire condition evaluates to
false.

Could this skip enetc_update_tx_ring_tail() and leave previously queued
packets stalled in the TX ring without a doorbell ring, violating the
XDP_XMIT_FLUSH contract?

>  	enetc_unlock_mdio();

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015105536.1692348-1-vladimir.oltean@nxp.com?part=8