[PATCH v3 net 0/7] Fix short frame transmission in enetc

[PATCH v3 net 0/7] 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 (patches 4, 5) were focused on handling the invalid
frame geometries. However, review pointed out further issues in annex
code. The other patches handle those.

The original patch is 4/7, which Zefir already tested (in a simpler
form) and confirmed working in the thread from February. The code path
should be identical for his issue, so I've preserved the tag despite the
additions.

I've CCed the BPF/XDP folks for the new xdp_frame_pad() helper in patch
5/7.

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/

Change log in individual patches.

Vladimir Oltean (7):
  net: enetc: consistenly track dropped frames in enetc_xdp_xmit()
  net: enetc: ensure enetc_xdp_xmit() calls enetc_update_tx_ring_tail()
  net: enetc: fix bogus TX ring consumer index after reinitialization
  net: enetc: pad short frames in software
  net: enetc: pad short XDP frames coming from devmap
  net: enetc: linearize PTP event packets with one-step TX timestamping
  net: enetc: drain and cancel one-step TX tstamp queue when going down

 drivers/net/ethernet/freescale/enetc/enetc.c | 50 +++++++++++++++-----
 drivers/net/ethernet/freescale/enetc/enetc.h |  2 +
 include/net/xdp.h                            | 23 +++++++++
 3 files changed, 64 insertions(+), 11 deletions(-)

-- 
2.43.0

[PATCH v3 net 1/7] net: enetc: consistenly track dropped frames in enetc_xdp_xmit()

[vladimir.oltean]

2 distinct classes of problems:
(a) failures in enetc_xdp_frame_to_xdp_tx_swbd(), as well as frames
    split across too many buffers, should be tracked by some drop
    counters, as they will not be transmitted. This is similar to how
    enetc_xdp_tx() increments xdp_tx_drops.
(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 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>
---
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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
index 80f0082f6c63..e7ecdf8ef67c 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc.c
@@ -1843,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;
 		}
 
@@ -1854,6 +1853,7 @@ int enetc_xdp_xmit(struct net_device *ndev, int num_frames,
 		enetc_update_tx_ring_tail(tx_ring);
 
 	tx_ring->stats.xdp_tx += xdp_tx_frm_cnt;
+	tx_ring->stats.xdp_tx_drops += num_frames - xdp_tx_frm_cnt;
 
 	enetc_unlock_mdio();
 
-- 
2.43.0

RE: [PATCH v3 net 1/7] net: enetc: consistenly track dropped frames in enetc_xdp_xmit()

[Wei Fang]

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c
> b/drivers/net/ethernet/freescale/enetc/enetc.c
> index 80f0082f6c63..e7ecdf8ef67c 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.c
> @@ -1843,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;
>  		}
> 
> @@ -1854,6 +1853,7 @@ int enetc_xdp_xmit(struct net_device *ndev, int
> num_frames,
>  		enetc_update_tx_ring_tail(tx_ring);
> 
>  	tx_ring->stats.xdp_tx += xdp_tx_frm_cnt;
> +	tx_ring->stats.xdp_tx_drops += num_frames - xdp_tx_frm_cnt;
> 
>  	enetc_unlock_mdio();
> 
> --
> 2.43.0

Reviewed-by: Wei Fang <wei.fang@nxp.com>

Re: [PATCH v3 net 1/7] net: enetc: consistenly track dropped frames in enetc_xdp_xmit()

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 (1) · Low: 0

- [Medium] enetc_xdp_xmit()
  (drivers/net/ethernet/freescale/enetc/enetc.c:1817-1820) still returns…

Pre-existing issues:
- [Medium] The accumulation added by this patch,
  `tx_ring->stats.xdp_tx_drops += num_frames - xdp_tx_frm_cnt;`…

--

net: enetc: consistenly track dropped frames in enetc_xdp_xmit()

This makes enetc_xdp_xmit() account for every frame in the batch that was
not handed to hardware, instead of only counting a single drop on
enetc_xdp_tx() failure.  The drop counter is now bumped by the remainder
num_frames - xdp_tx_frm_cnt, which also covers the too-many-fragments and
enetc_xdp_frame_to_xdp_tx_swbd() failure cases.

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
> index 80f0082f6c633..e7ecdf8ef67c8 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.c

[ ... ]

> @@ -1854,6 +1853,7 @@ int enetc_xdp_xmit(struct net_device *ndev, int num_frames,
>  		enetc_update_tx_ring_tail(tx_ring);
>  
>  	tx_ring->stats.xdp_tx += xdp_tx_frm_cnt;
> +	tx_ring->stats.xdp_tx_drops += num_frames - xdp_tx_frm_cnt;

[Severity: Medium]
Should the early exit at the top of enetc_xdp_xmit() also be counted here?
That path returns before this new accumulation runs:

drivers/net/ethernet/freescale/enetc/enetc.c:enetc_xdp_xmit() {
	if (unlikely(test_bit(ENETC_TX_DOWN, &priv->flags) ||
		     !netif_carrier_ok(ndev)))
		return -ENETDOWN;
	...
}

On a negative return, bq_xmit_all() in kernel/bpf/devmap.c discards the
whole batch:

	sent = dev->netdev_ops->ndo_xdp_xmit(dev, to_send, bq->q, flags);
	if (sent < 0) {
		err = sent;
		sent = 0;
	}
	for (i = sent; unlikely(i < to_send); i++)
		xdp_return_frame_rx_napi(bq->q[i]);

So all num_frames frames are dropped, but "Tx ring N XDP drops" in
ethtool -S stays unchanged.

The XDP_TX producer of the same counter in enetc_clean_rx_ring_xdp() does
count the identical condition:

		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++;
				break;
			}

Given that the commit message says frames "will not be transmitted" should
be tracked by drop counters, does leaving the -ENETDOWN case uncounted give
the same counter two different meanings depending on which in-driver path
produced the drop?  tx_ring is derivable there as
priv->xdp_tx_ring[smp_processor_id()], so the early return could bump the
counter by num_frames before returning.

[Severity: Medium]
This isn't a bug introduced by this patch, but the new accumulation inherits
an unsynchronized read-modify-write on tx_ring->stats.xdp_tx_drops, the same
way the existing xdp_tx += line does.  The fields in struct
enetc_ring_stats are plain unsigned long, with no per-CPU separation and no
u64_stats_sync.

The same ring's counters are also written from NAPI in
enetc_clean_rx_ring_xdp():

			tx_ring = priv->xdp_tx_ring[rx_ring->index];
			...
				tx_ring->stats.xdp_tx_drops++;

enetc_xdp_xmit() picks the ring with smp_processor_id() while XDP_TX picks it
with rx_ring->index, so both can target the same tx_ring from different CPUs
once RX IRQ affinity is not the identity mapping (enetc_setup_irqs() only
installs a hint via irq_set_affinity_hint(), and userspace can rewrite
/proc/irq/*/smp_affinity).

The only lock held is enetc_lock_mdio(), which is a shared read_lock() and
compiles away unless the errata static branch is enabled:

drivers/net/ethernet/freescale/enetc/enetc_hw.h:enetc_lock_mdio() {
	if (static_branch_unlikely(&enetc_has_err050089))
		read_lock(&enetc_mdio_lock);
}

Can increments be lost this way, and is it worth converting these ring stats
to u64_stats_sync while touching the accounting?

>  
>  	enetc_unlock_mdio();
>

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

[PATCH v3 net 2/7] 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>
---
v2->v3: delete redundant xdp_tx_frm_cnt variable
v1->v2: patch is new
---
 drivers/net/ethernet/freescale/enetc/enetc.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
index e7ecdf8ef67c..c9c23e994f6d 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc.c
@@ -1813,7 +1813,6 @@ int enetc_xdp_xmit(struct net_device *ndev, int num_frames,
 	struct skb_shared_info *shinfo;
 	struct enetc_bdr *tx_ring;
 	int xdp_tx_bd_cnt, i, k;
-	int xdp_tx_frm_cnt = 0;
 
 	if (unlikely(test_bit(ENETC_TX_DOWN, &priv->flags) ||
 		     !netif_carrier_ok(ndev)))
@@ -1845,19 +1844,17 @@ 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);
 
-	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;
 
 	enetc_unlock_mdio();
 
-	return xdp_tx_frm_cnt;
+	return k;
 }
 EXPORT_SYMBOL_GPL(enetc_xdp_xmit);
 
-- 
2.43.0

RE: [PATCH v3 net 2/7] net: enetc: ensure enetc_xdp_xmit() calls enetc_update_tx_ring_tail()

[Wei Fang]

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c
> b/drivers/net/ethernet/freescale/enetc/enetc.c
> index e7ecdf8ef67c..c9c23e994f6d 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.c
> @@ -1813,7 +1813,6 @@ int enetc_xdp_xmit(struct net_device *ndev, int
> num_frames,
>  	struct skb_shared_info *shinfo;
>  	struct enetc_bdr *tx_ring;
>  	int xdp_tx_bd_cnt, i, k;
> -	int xdp_tx_frm_cnt = 0;
> 
>  	if (unlikely(test_bit(ENETC_TX_DOWN, &priv->flags) ||
>  		     !netif_carrier_ok(ndev)))
> @@ -1845,19 +1844,17 @@ 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);
> 
> -	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;
> 
>  	enetc_unlock_mdio();
> 
> -	return xdp_tx_frm_cnt;
> +	return k;
>  }
>  EXPORT_SYMBOL_GPL(enetc_xdp_xmit);
> 
> --
> 2.43.0

Reviewed-by: Wei Fang <wei.fang@nxp.com>

Re: [PATCH v3 net 2/7] net: enetc: ensure enetc_xdp_xmit() calls enetc_update_tx_ring_tail()

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 (1) · Medium: 0 · Low: 1

- [High] drivers/net/ethernet/freescale/enetc/enetc.c, enetc_xdp_xmit():
  the new doorbell condition `if (unlikely(k && ((flags &…
- [Low] The commit message of "net: enetc: ensure enetc_xdp_xmit() calls
  enetc_update_tx_ring_tail()" carries a Fixes: 9d2b68cc108d tag (making…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the function
  it modifies contains an unsynchronised shared-TX-ring race.

--

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

This changes the condition under which enetc_xdp_xmit() rings the TX
doorbell, since the old "k != xdp_tx_frm_cnt" test could never be true,
and removes the now redundant xdp_tx_frm_cnt variable in favour of the
loop index k.

A couple of questions below.

> 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.

[Severity: Low]
The patch carries a Fixes: tag but does not describe any user-visible
symptom of the old code.  Was one observed?

Looking at bq_enqueue() in kernel/bpf/devmap.c, the current frame is always
stored right after the bulk transmit:

	if (unlikely(bq->count == DEV_MAP_BULK_SIZE))
		bq_xmit_all(bq, 0);
	...
	bq->q[bq->count++] = xdpf;

so bq->count >= 1 whenever __dev_flush() runs, and bq_xmit_all() does not
take its "if (unlikely(!cnt)) return;" early exit.  The pre-patch code then
rang the doorbell unconditionally on that XDP_XMIT_FLUSH call in the same
NAPI poll, which would make the dead "k != xdp_tx_frm_cnt" condition a
latency issue rather than a lost-frame issue.

Could the commit message say what the practical effect was, so that stable
backporters can judge the risk?

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
> index e7ecdf8ef67c8..c9c23e994f6d1 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.c

[ ... ]

> @@ -1845,19 +1844,17 @@ 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]
Can the new "k &&" term drop a doorbell that the caller explicitly asked
for with XDP_XMIT_FLUSH?

enetc_xdp_tx() only writes the BDs and advances tx_ring->next_to_use.  The
hardware producer index is written only here:

static void enetc_update_tx_ring_tail(struct enetc_bdr *tx_ring)
{
	/* includes wmb() */
	enetc_wr_reg_hot(tx_ring->tpir, tx_ring->next_to_use);
}

Now consider the devmap bulk path in kernel/bpf/devmap.c:

bq_enqueue()
	if (unlikely(bq->count == DEV_MAP_BULK_SIZE))
		bq_xmit_all(bq, 0);		/* flags == 0, no doorbell */

__dev_flush()
	bq_xmit_all(bq, XDP_XMIT_FLUSH);	/* the only flush */
	bq->dev_rx = NULL;
	__list_del_clearprev(&bq->flush_node);

The flags == 0 call enqueues all its frames (k == num_frames) and
legitimately defers the doorbell.  If the following XDP_XMIT_FLUSH call
breaks at the very first frame, k == 0 and enetc_update_tx_ring_tail() is
now skipped, so the BDs written by the earlier call are never announced to
hardware.  The bq and its flush-list entry are cleared right afterwards, so
there is no retry.

k == 0 on that flush call looks reachable through any of the loop's early
breaks:

	if (unlikely((shinfo->nr_frags + 1) > ENETC_MAX_SKB_FRAGS))
		break;
	...
	if (unlikely(xdp_tx_bd_cnt < 0))		/* DMA map error */
		break;
	...
	if (unlikely(!enetc_xdp_tx(tx_ring, xdp_redirect_arr,
				   xdp_tx_bd_cnt))) {		/* ring full */

The ring-full variant looks self-perpetuating: the un-announced BDs keep
enetc_bd_unused() low, every later call fails at frame 0, and the doorbell
is skipped again.  enetc_clean_tx_ring() cannot release those xdp_frames or
their DMA mappings because hardware never completes descriptors it was not
told about, and the dedicated per-CPU XDP TX ring is outside
real_num_tx_queues so the TX watchdog does not cover it.  The XDP_TX path
has the same shape:

enetc_clean_rx_ring_xdp()
	if (xdp_tx_frm_cnt)
		enetc_update_tx_ring_tail(tx_ring);

so it does not re-synchronise TPIR either.

Since writing TPIR with an unchanged next_to_use is idempotent, is there
anything gained by the "k &&" term?  Would keeping the flush branch
unconditional be preferable, e.g.

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

The condition is unchanged at the end of the series (381447d90b1f).

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

[Severity: High]
This isn't a bug introduced by this patch, but while touching
enetc_xdp_xmit() it may be worth looking at what serialises this ring
against the stack TX path.

enetc_xdp_xmit() picks the ring per-CPU and holds no TX queue lock:

	tx_ring = priv->xdp_tx_ring[smp_processor_id()];

The only lock taken is enetc_lock_mdio(), which in enetc_hw.h is

	if (static_branch_unlikely(&enetc_has_err050089))
		read_lock(&enetc_mdio_lock);

i.e. a reader lock for an unrelated MDIO erratum, so it gives no mutual
exclusion between two producers.

The XDP rings are a slice of the same array the stack uses:

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

and they are only hidden from the stack when a program is attached:

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

	return num_tx_rings;

enetc_pf_netdev_setup() advertises NETDEV_XDP_ACT_NDO_XMIT unconditionally,
and devmap only tests that flag, so ndo_xdp_xmit() can be called into this
device with priv->xdp_prog == NULL.  In that case every ring, including the
xdp_tx_ring range, is also reachable from

enetc_start_xmit()
	tx_ring = priv->tx_ring[skb->queue_mapping];

under only that queue's __netif_tx_lock.  Can the two producers then race on
the same next_to_use and tx_swbd slot, overwriting bookkeeping (leaking the
previous skb and its DMA mapping) and leaving enetc_clean_tx_ring() freeing
entries that do not match what hardware consumed?

The overlap looks unavoidable when num_tx_rings == num_possible_cpus():
first_xdp_tx_ring becomes 0, and enetc_setup_xdp_prog()'s check

	if (priv->min_num_stack_tx_queues + num_xdp_tx_queues >
	    priv->num_tx_rings) {

prevents attaching a program at all, yet ndo_xdp_xmit() still targets rings
0..nr_cpus-1, which are pure stack queues.

i40e and ice return -ENXIO from ndo_xdp_xmit when XDP TX rings are not
reserved.  Would a similar check fit here?

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

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

[PATCH v3 net 3/7] 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->v3: 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 c9c23e994f6d..0216f7d08e19 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc.c
@@ -2627,7 +2627,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 v3 net 4/7] net: enetc: pad short frames in software

[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 is out of the driver's control, and the network
stack can actually send packets with skb->len smaller than that. The
result is unpleasant, as will be explained below, so for simplicity
sake, we just pad anything shorter than ETH_ZLEN.

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.

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.

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/
Tested-by: Zefir Kurtisi <zefir.kurtisi@westermo.com>
Reviewed-by: Wei Fang <wei.fang@nxp.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v1->v3: none
---
 drivers/net/ethernet/freescale/enetc/enetc.c | 13 +++++++++++++
 drivers/net/ethernet/freescale/enetc/enetc.h |  2 ++
 2 files changed, 15 insertions(+)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
index 0216f7d08e19..bbad942041f5 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc.c
@@ -1077,6 +1077,19 @@ netdev_tx_t enetc_xmit(struct sk_buff *skb, struct net_device *ndev)
 	u8 udp, msgtype, twostep;
 	u16 offset1, offset2;
 
+	/* Hardware does not support transmit buffer descriptors with a total
+	 * length of less than 16 bytes, or a first buffer size of less than
+	 * 16 bytes.
+	 */
+	if (unlikely(skb_headlen(skb) < ENETC_MIN_BUFF_SIZE &&
+		     skb_linearize(skb))) {
+		dev_kfree_skb_any(skb);
+		return NETDEV_TX_OK;
+	}
+
+	if (eth_skb_pad(skb))
+		return NETDEV_TX_OK;
+
 	/* Mark tx timestamp type on enetc_cb->flag if requires */
 	if ((skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) &&
 	    (priv->active_offloads & ENETC_F_TX_TSTAMP_MASK))
diff --git a/drivers/net/ethernet/freescale/enetc/enetc.h b/drivers/net/ethernet/freescale/enetc/enetc.h
index d1e9d9130057..a4e76060e94b 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 v3 net 4/7] net: enetc: pad short frames in software

[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 (1)

- [High] enetc_xmit() (drivers/net/ethernet/freescale/enetc/enetc.c:1090)
  calls eth_skb_pad(skb) unconditionally, i.e. also for GSO skbs.

Pre-existing issues:
- [Low] Incomplete enforcement of the 16-byte TX BD minimum: at this
  commit the new ENETC_MIN_BUFF_SIZE check exists only in enetc_xmit()…

--

net: enetc: pad short frames in software

ENETC hardware rejects TX buffer descriptors whose FRM_LEN or first
BUFF_LEN is below 16 bytes, which leaves the TX ring consumer index
stuck and amplifies into an MSI-X interrupt storm. The patch linearizes
skbs with a short head and pads anything shorter than ETH_ZLEN in
enetc_xmit().

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 0216f7d08e197..bbad942041f5a 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.c
> @@ -1077,6 +1077,19 @@ netdev_tx_t enetc_xmit(struct sk_buff *skb, struct net_device *ndev)
>  	u8 udp, msgtype, twostep;
>  	u16 offset1, offset2;
>  
> +	/* Hardware does not support transmit buffer descriptors with a total
> +	 * length of less than 16 bytes, or a first buffer size of less than
> +	 * 16 bytes.
> +	 */
> +	if (unlikely(skb_headlen(skb) < ENETC_MIN_BUFF_SIZE &&
> +		     skb_linearize(skb))) {
> +		dev_kfree_skb_any(skb);
> +		return NETDEV_TX_OK;
> +	}
> +
> +	if (eth_skb_pad(skb))
> +		return NETDEV_TX_OK;

[Severity: High]
Should the pad be skipped for GSO skbs?

enetc advertises NETIF_F_GSO_UDP_L4 in enetc_pf_common.c and enetc_vf.c,
so a UDP segmentation skb reaches ndo_start_xmit still unsegmented, and
such an skb can have skb->len < ETH_ZLEN. udp_send_skb() marks the skb
SKB_GSO_UDP_L4 whenever datalen > cork->gso_size, and UDP_SEGMENT accepts
any value in 0..USHRT_MAX:

net/ipv4/udp.c:udp_send_skb() {
    ...
	if (datalen > cork->gso_size) {
		skb_shinfo(skb)->gso_size = cork->gso_size;
		skb_shinfo(skb)->gso_type = SKB_GSO_UDP_L4;
    ...
}

With UDP_SEGMENT=1 and a 2-byte sendmsg(), skb->len is 14 + 20 + 8 + 2 =
44 with gso_size 1 and gso_segs 2. eth_skb_pad() -> skb_put_padto(skb,
ETH_ZLEN) then appends 16 zero bytes and sets skb->len to 60, but leaves
gso_size and gso_segs alone.

The skb is still GSO, so enetc_start_xmit() takes the skb_is_gso() branch,
and both segmentation back-ends derive the payload length from skb->len:

drivers/net/ethernet/freescale/enetc/enetc.c:enetc_lso_start() {
	lso->lso_seg_size = skb_shinfo(skb)->gso_size;
    ...
	lso->total_len = skb->len - lso->hdr_len;
}

drivers/net/ethernet/freescale/enetc/enetc.c:enetc_map_tx_tso_buffs() {
	hdr_len = tso_start(skb, &tso);
	total_len = skb->len - hdr_len;
    ...
		data_len = min_t(int, skb_shinfo(skb)->gso_size, total_len);
}

Does that make the hardware LSO engine and the software TSO loop segment
18 bytes instead of 2, putting 18 one-byte UDP datagrams on the wire, 16
of them made up of pad zeros? tso_build_hdr() rewrites IP tot_len and UDP
len per segment, so the extra datagrams would look well formed to the
receiver, and nothing is reported back to the sender.

The software TSO path always emits header and data BDs of at least
hdr_len bytes, so the 16-byte first-BD and frame-length limits do not seem
to apply to GSO skbs. Would guarding the pad with !skb_is_gso(skb) work
here? This is still present at the end of the series (381447d90b1f), where
the pad in enetc_xmit() remains unconditional.

[Severity: Low]
This isn't a bug introduced by this patch, but the 16-byte minimum is
only enforced on the skb path here, while the XDP descriptor producers
still program the BD length verbatim:

enetc_xdp_xmit() -> enetc_xdp_frame_to_xdp_tx_swbd() {
	int len = xdp_frame->len;
    ...
	xdp_tx_swbd->len = len;
}

enetc_xdp_map_tx_buff() {
	txbd->buf_len = cpu_to_le16(tx_swbd->len);
	txbd->frm_len = cpu_to_le16(frm_len);
}

Can an XDP_REDIRECT of a 14 or 15-byte frame into ndo_xdp_xmit still
program an illegal first BD and reproduce the stuck TBaCIR / TBaSR[BUSY]
condition and the MSI-X re-arm storm described in the commit message?
bpf_xdp_adjust_tail() only enforces an ETH_HLEN floor.

The XDP_TX case does not look affected, since
enetc_rx_swbd_to_xdp_tx_swbd() copies rx_swbd->len, i.e. the length
recorded at reception.

This gap is closed by the next patch in the same series, "net: enetc: pad
short XDP frames coming from devmap", which adds the check in
enetc_xdp_xmit():

		if (unlikely(xdp_frame_pad(xdpf) ||
			     xdpf->len < ENETC_MIN_BUFF_SIZE)) {
			tx_ring->stats.xdp_tx_drops++;
			break;
		}

so nothing remains at the end of the series (381447d90b1f); the
underlying XDP exposure predates this patch (the follow-up carries Fixes:
9d2b68cc108d, "net: enetc: add support for XDP_REDIRECT"). Would it be
worth ordering the XDP patch first so the bisect window does not contain a
tree where only half of the constraint is enforced?

[ ... ]

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

Re: [PATCH v3 net 4/7] net: enetc: pad short frames in software

[David Laight]

On Wed, 16 Sep 2026 01:27:31 +0300
vladimir.oltean@nxp.com wrote:

> 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 is out of the driver's control, and the network
> stack can actually send packets with skb->len smaller than that. The
> result is unpleasant, as will be explained below, so for simplicity
> sake, we just pad anything shorter than ETH_ZLEN.
> 
> 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.
> 
> 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.
> 
> 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/
> Tested-by: Zefir Kurtisi <zefir.kurtisi@westermo.com>
> Reviewed-by: Wei Fang <wei.fang@nxp.com>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
> v1->v3: none
> ---
>  drivers/net/ethernet/freescale/enetc/enetc.c | 13 +++++++++++++
>  drivers/net/ethernet/freescale/enetc/enetc.h |  2 ++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
> index 0216f7d08e19..bbad942041f5 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.c
> @@ -1077,6 +1077,19 @@ netdev_tx_t enetc_xmit(struct sk_buff *skb, struct net_device *ndev)
>  	u8 udp, msgtype, twostep;
>  	u16 offset1, offset2;
>  
> +	/* Hardware does not support transmit buffer descriptors with a total
> +	 * length of less than 16 bytes, or a first buffer size of less than
> +	 * 16 bytes.
> +	 */
> +	if (unlikely(skb_headlen(skb) < ENETC_MIN_BUFF_SIZE &&
> +		     skb_linearize(skb))) {

Isn't it only necessary to pull a few bytes into the linear region?

> +		dev_kfree_skb_any(skb);
> +		return NETDEV_TX_OK;
> +	}
> +
> +	if (eth_skb_pad(skb))
> +		return NETDEV_TX_OK;

That could be inside the (skb_headlen(skb) < ENETC_MIN_BUFF_SIZE) test.

David

> +
>  	/* Mark tx timestamp type on enetc_cb->flag if requires */
>  	if ((skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) &&
>  	    (priv->active_offloads & ENETC_F_TX_TSTAMP_MASK))
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.h b/drivers/net/ethernet/freescale/enetc/enetc.h
> index d1e9d9130057..a4e76060e94b 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

[PATCH v3 net 5/7] net: enetc: pad short XDP frames coming from devmap

[vladimir.oltean]

Similar to the skb path issue explained in the previous change, ENETC
could end up transmitting short frames coming from XDP.

The way in which this could happen is a bit contrived, but it involves
XDP_REDIRECT from a veth interface pair.

As for enetc_xmit(), there are two separate limitations for the overall
FRM_LEN and for the head BUFF_LEN. For the head BUFF_LEN, we add a
direct restriction in enetc_xdp_xmit(), and for the overall FRM_LEN, we
introduce a xdp_frame_pad() best-effort generic helper which we call
from the same place.

This helper alters the frame, but that should be safe, because
ndo_xdp_xmit() is the hand-off function where the XDP frames become the
responsibility of the driver. AFAIU, struct xdp_frame doesn't have
multiple copies.

I say best-effort because xdp_frame_pad() can only expand the head
buffer of an XDP frame. It cannot expand the last fragment of a
multi-buffer XDP frame, because, unlike bpf_xdp_frags_increase_tail(),
it lacks access to the rxq->frag_size, aka the capacity of the chunk of
memory being pointed to by the fragment.

So, if the frame happens to be less than minimum Ethernet size, but
fragmented, callers of this function will have to drop it.

Fixes: 9d2b68cc108d ("net: enetc: add support for XDP_REDIRECT")
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v2->v3: none
v1->v2:
- handle multi-buffer frames instead of being unaware of their
  multi-buffer quality
- add separate restriction for BUFF_LEN
- increment drop counter
---
 drivers/net/ethernet/freescale/enetc/enetc.c | 14 +++++++++---
 include/net/xdp.h                            | 23 ++++++++++++++++++++
 2 files changed, 34 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
index bbad942041f5..8a9ba168eab1 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc.c
@@ -1838,15 +1838,23 @@ int enetc_xdp_xmit(struct net_device *ndev, int num_frames,
 	prefetchw(ENETC_TXBD(*tx_ring, tx_ring->next_to_use));
 
 	for (k = 0; k < num_frames; k++) {
-		if (xdp_frame_has_frags(frames[k])) {
-			shinfo = xdp_get_shared_info_from_frame(frames[k]);
+		struct xdp_frame *xdpf = frames[k];
+
+		if (xdp_frame_has_frags(xdpf)) {
+			shinfo = xdp_get_shared_info_from_frame(xdpf);
 			if (unlikely((shinfo->nr_frags + 1) > ENETC_MAX_SKB_FRAGS))
 				break;
 		}
 
+		if (unlikely(xdp_frame_pad(xdpf) ||
+			     xdpf->len < ENETC_MIN_BUFF_SIZE)) {
+			tx_ring->stats.xdp_tx_drops++;
+			break;
+		}
+
 		xdp_tx_bd_cnt = enetc_xdp_frame_to_xdp_tx_swbd(tx_ring,
 							       xdp_redirect_arr,
-							       frames[k]);
+							       xdpf);
 		if (unlikely(xdp_tx_bd_cnt < 0))
 			break;
 
diff --git a/include/net/xdp.h b/include/net/xdp.h
index aa742f413c35..276afc9aa21d 100644
--- a/include/net/xdp.h
+++ b/include/net/xdp.h
@@ -477,6 +477,29 @@ xdp_get_frame_len(const struct xdp_frame *xdpf)
 	return len;
 }
 
+static inline int xdp_frame_pad(struct xdp_frame *xdpf)
+{
+	unsigned int total_len, pad;
+	void *sinfo;
+
+	total_len = xdp_get_frame_len(xdpf);
+	if (likely(total_len >= ETH_ZLEN))
+		return 0;
+
+	if (unlikely(xdp_frame_has_frags(xdpf)))
+		return -EOPNOTSUPP;
+
+	pad = ETH_ZLEN - total_len;
+	sinfo = xdp_get_shared_info_from_frame(xdpf);
+	if (unlikely(xdpf->data + xdpf->len + pad > sinfo))
+		return -ENOMEM;
+
+	memset(xdpf->data + xdpf->len, 0, pad);
+	xdpf->len += pad;
+
+	return 0;
+}
+
 int __xdp_rxq_info_reg(struct xdp_rxq_info *xdp_rxq,
 		       struct net_device *dev, u32 queue_index,
 		       unsigned int napi_id, u32 frag_size);
-- 
2.43.0

Re: [PATCH v3 net 5/7] net: enetc: pad short XDP frames coming from devmap

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] In enetc_xdp_xmit()
  (drivers/net/ethernet/freescale/enetc/enetc.c), the newly added short-…

--

net: enetc: pad short XDP frames coming from devmap

This adds a generic best-effort xdp_frame_pad() helper and calls it from
enetc_xdp_xmit(), together with a direct check of the head buffer length
against ENETC_MIN_BUFF_SIZE, so that short XDP frames redirected into
ENETC are padded or dropped instead of being transmitted.

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
> index bbad942041f5a..8a9ba168eab1e 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.c
> @@ -1838,15 +1838,23 @@ int enetc_xdp_xmit(struct net_device *ndev, int num_frames,
>  	prefetchw(ENETC_TXBD(*tx_ring, tx_ring->next_to_use));
>  
>  	for (k = 0; k < num_frames; k++) {
> -		if (xdp_frame_has_frags(frames[k])) {
> -			shinfo = xdp_get_shared_info_from_frame(frames[k]);
> +		struct xdp_frame *xdpf = frames[k];
> +
> +		if (xdp_frame_has_frags(xdpf)) {
> +			shinfo = xdp_get_shared_info_from_frame(xdpf);
>  			if (unlikely((shinfo->nr_frags + 1) > ENETC_MAX_SKB_FRAGS))
>  				break;
>  		}
>  
> +		if (unlikely(xdp_frame_pad(xdpf) ||
> +			     xdpf->len < ENETC_MIN_BUFF_SIZE)) {
> +			tx_ring->stats.xdp_tx_drops++;
> +			break;
> +		}

[Severity: Medium]
Does this count the rejected frame twice in tx_ring->stats.xdp_tx_drops?

The new branch increments the counter and then breaks with k still pointing
at the frame that was just rejected, and the epilogue of enetc_xdp_xmit()
charges everything from k onwards again:

	tx_ring->stats.xdp_tx += k;
	tx_ring->stats.xdp_tx_drops += num_frames - k;

so the counter grows by 1 + (num_frames - k) rather than num_frames - k.

The other three break sites in this loop (the shinfo->nr_frags overflow
check, the enetc_xdp_frame_to_xdp_tx_swbd() failure and the enetc_xdp_tx()
failure) rely only on the remainder-based accounting added earlier in this
series by "net: enetc: consistenly track dropped frames in
enetc_xdp_xmit()", whose message states that the counter should be
incremented "by the remainder, not just by 1".

Would dropping the explicit increment here keep this site consistent with
the other break paths?

The branch is reachable when xdp_frame_pad() returns -EOPNOTSUPP (frame
shorter than ETH_ZLEN and fragmented) or -ENOMEM (no tailroom before the
shared info), and also when the frame total is at least ETH_ZLEN but the
head buffer is shorter than ENETC_MIN_BUFF_SIZE, which is the
XDP_REDIRECT-from-veth case described in the commit message.

Since xdp_tx_drops is exported to userspace by enetc_get_ethtool_stats()
via ethtool -S, can the reported drop count end up larger than the number
of frames the core passed to ndo_xdp_xmit()?

> +
>  		xdp_tx_bd_cnt = enetc_xdp_frame_to_xdp_tx_swbd(tx_ring,
>  							       xdp_redirect_arr,
> -							       frames[k]);
> +							       xdpf);
>  		if (unlikely(xdp_tx_bd_cnt < 0))
>  			break;
>  

[ ... ]

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

[PATCH v3 net 6/7] net: enetc: linearize PTP event packets with one-step TX timestamping

[vladimir.oltean]

There are two distinct problems, solved with the same
skb_linearize_cow() call.

First, enetc_ptp_parse() uses ptp_parse_header(), which does not handle
fragmented headers, and expects the entire area between skb_mac_header()
and the end of the PTP header to be linear.

When the driver fails to parse a fragmented PTP frame to find the
offsets to the originTimestamp and correctionField, it falls back to
two-step timestamping, which is technically not what user space asked
for, and it may not be prepared to receive the timestamped packet
through the socket error queue.

The problem can be avoided relatively easily by linearizing packets with
one-step timestamping requests prior to calling enetc_ptp_parse(). These
are infrequent enough that this should not be a performance problem.

The second problem is that later in the TX path of these packets,
enetc_update_ptp_sync_msg() modifies them by writing to the
originTimestamp field of the PTP header. This is not safe from drivers
because they may not hold the only copy of the packet, unless something
along the lines of skb_unshare() is used.

Actually skb_linearize_cow() kills both birds with one stone, as it
ensures the skb is linear and writable.

Note that modifying ptp_parse_header() to tolerate nonlinear skbs was
considered, but it wouldn't have saved us of the need to make it
writable, so this is the simpler solution.

Fixes: 7294380c5211 ("enetc: support PTP Sync packet one-step timestamping")
Link: https://sashiko.dev/#/patchset/20260401172246.1075883-1-vladimir.oltean%40nxp.com
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v2->v3:
- replace skb_linearize() with skb_linearize_cow(),
- improve justification
v1->v2:
- patch is new
---
 drivers/net/ethernet/freescale/enetc/enetc.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
index 8a9ba168eab1..62cdcaab3f3f 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc.c
@@ -1099,6 +1099,11 @@ netdev_tx_t enetc_xmit(struct sk_buff *skb, struct net_device *ndev)
 
 	/* Fall back to two-step timestamp if not one-step Sync packet */
 	if (enetc_cb->flag & ENETC_F_TX_ONESTEP_SYNC_TSTAMP) {
+		if (unlikely(skb_linearize(skb))) {
+			dev_kfree_skb_any(skb);
+			return NETDEV_TX_OK;
+		}
+
 		if (enetc_ptp_parse(skb, &udp, &msgtype, &twostep,
 				    &offset1, &offset2) ||
 		    msgtype != PTP_MSGTYPE_SYNC || twostep != 0) {
-- 
2.43.0

RE: [PATCH v3 net 6/7] net: enetc: linearize PTP event packets with one-step TX timestamping

[Wei Fang]

> ---
> v2->v3:
> - replace skb_linearize() with skb_linearize_cow(),

This is not true, this patch still use skb_linearize().

> - improve justification
> v1->v2:
> - patch is new
> ---
>  drivers/net/ethernet/freescale/enetc/enetc.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c
> b/drivers/net/ethernet/freescale/enetc/enetc.c
> index 8a9ba168eab1..62cdcaab3f3f 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.c
> @@ -1099,6 +1099,11 @@ netdev_tx_t enetc_xmit(struct sk_buff *skb, struct
> net_device *ndev)
> 
>  	/* Fall back to two-step timestamp if not one-step Sync packet */
>  	if (enetc_cb->flag & ENETC_F_TX_ONESTEP_SYNC_TSTAMP) {
> +		if (unlikely(skb_linearize(skb))) {
> +			dev_kfree_skb_any(skb);
> +			return NETDEV_TX_OK;
> +		}
> +
>  		if (enetc_ptp_parse(skb, &udp, &msgtype, &twostep,
>  				    &offset1, &offset2) ||
>  		    msgtype != PTP_MSGTYPE_SYNC || twostep != 0) {
> --
> 2.43.0

Re: [PATCH v3 net 6/7] net: enetc: linearize PTP event packets with one-step TX timestamping

[Vladimir Oltean]

On Wed, Sep 16, 2026 at 04:59:47AM +0300, Wei Fang wrote:
> > ---
> > v2->v3:
> > - replace skb_linearize() with skb_linearize_cow(),
> 
> This is not true, this patch still use skb_linearize().

Yeah, sorry, I ran a bunch of tests in 'git rebase -i' to reproduce the
issues with this patch unapplied (this is why it's reordered to 6/7 in
this series while it was 2/6 in the previous one), and I then updated
the commit message with the conclusions, but apparently forgot to fold
the actual code change as well :-/

I'll make this change together with other feedback for v4.

Thanks for noticing.

Re: [PATCH v3 net 6/7] net: enetc: linearize PTP event packets with one-step TX timestamping

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 0 (1) · Low: 0

- [High] Incomplete fix / commit-message mismatch in enetc_xmit(): the
  patch adds plain skb_linearize() where skb_linearize_cow() is required.

Pre-existing issues:
- [Medium] Speculative / discarded as a pre-existing Medium: the 10-byte
  originTimestamp write in enetc_update_ptp_sync_msg() (enetc.c:336-338,…

--

net: enetc: linearize PTP event packets with one-step TX timestamping

The patch adds a linearization step in enetc_xmit() before
enetc_ptp_parse() is called for packets requesting one-step Sync TX
timestamping, so that ptp_parse_header() sees a linear header and so that
the later in-place write of originTimestamp operates on a buffer the
driver owns.

Link: https://sashiko.dev/#/patchset/20260401172246.1075883-1-vladimir.oltean%40nxp.com

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
> index 8a9ba168eab1e..62cdcaab3f3fb 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.c
> @@ -1099,6 +1099,11 @@ netdev_tx_t enetc_xmit(struct sk_buff *skb, struct net_device *ndev)
>  
>  	/* Fall back to two-step timestamp if not one-step Sync packet */
>  	if (enetc_cb->flag & ENETC_F_TX_ONESTEP_SYNC_TSTAMP) {
> +		if (unlikely(skb_linearize(skb))) {
> +			dev_kfree_skb_any(skb);
> +			return NETDEV_TX_OK;
> +		}
> +

[Severity: High]
The commit message says:

    Actually skb_linearize_cow() kills both birds with one stone, as it
    ensures the skb is linear and writable.

and the v2->v3 changelog says "replace skb_linearize() with
skb_linearize_cow()", but the code added here calls skb_linearize().
Was the intended helper missed in this respin?

The two helpers differ exactly in the clone test:

include/linux/skbuff.h
    static inline int skb_linearize(struct sk_buff *skb)
    {
        return skb_is_nonlinear(skb) ? __skb_linearize(skb) : 0;
    }

    static inline int skb_linearize_cow(struct sk_buff *skb)
    {
        return skb_is_nonlinear(skb) || skb_cloned(skb) ?
               __skb_linearize(skb) : 0;
    }

So for an skb that is already linear but cloned, skb_linearize() returns 0
without doing anything, ENETC_F_TX_ONESTEP_SYNC_TSTAMP stays set, and the
packet reaches the write path:

enetc_start_xmit() -> enetc_map_tx_buffs() -> enetc_update_ptp_sync_msg()

where enetc_update_ptp_sync_msg() stores into the shared head buffer:

    *(__be16 *)(data + tstamp_off) = new_sec_h;
    *(__be32 *)(data + tstamp_off + 2) = new_sec_l;
    *(__be32 *)(data + tstamp_off + 6) = new_nsec;

and, in the UDP path without checksum offload, also rewrites
udp_hdr(skb)->check via inet_proto_csum_replace*().

Linear clones of a TX skb are ordinary: dev_queue_xmit_nit() does
skb_clone() for every AF_PACKET tap (tcpdump) before ndo_start_xmit(), and
tc mirred, bonding and team clone as well. With a tap running on the enetc
port, can these stores corrupt the frame bytes seen by the other holder of
the same data area, given that Documentation/networking/driver.rst states
"An ndo_start_xmit method must not modify the shared parts of a cloned
SKB"?

If skb_linearize_cow() is used instead, the clone case takes
__skb_linearize() and the write-side half of the problem described in the
commit message is covered too.

>  		if (enetc_ptp_parse(skb, &udp, &msgtype, &twostep,
>  				    &offset1, &offset2) ||
>  		    msgtype != PTP_MSGTYPE_SYNC || twostep != 0) {

[Severity: Medium]
This is a pre-existing issue and not something this patch introduces, but
while looking at this acceptance test: is the length of the Sync message
body checked anywhere before the 10-byte originTimestamp write?

enetc_ptp_parse() computes the body offset as the byte just past the common
header:

    *body_offset = (u8 *)hdr + sizeof(struct ptp_header) - base;

and ptp_parse_header() only validates the common header:

net/core/ptp_classifier.c:ptp_parse_header() {
    ...
	/* Ensure that the entire header is present in this packet. */
	if (ptr + sizeof(struct ptp_header) > skb->data + skb->len)
		return NULL;
    ...
}

A UDP-encapsulated frame that ends exactly at the end of the PTP header
(14 + 20 + 8 + 34 = 76 bytes for IPv4, so above ETH_ZLEN and not padded by
eth_skb_pad()) passes ptp_classify_raw() and ptp_parse_header(), has
msgtype == PTP_MSGTYPE_SYNC and twostep == 0, and then
enetc_update_ptp_sync_msg() reads and writes bytes at
data + origin_tstamp_off .. + 9, past skb->len.

Note this appears to land in the skb's own unused tailroom for frames in
that 76..85 byte range, since __alloc_skb() rounds the allocation up, and
dma_map_single() uses skb_headlen(), so no case where the write crosses
skb->end into skb_shared_info was constructed; the visible effect is a
bogus UDP checksum on an already malformed frame. A body length check in
enetc_ptp_parse() before accepting the packet for one-step processing would
make this explicit.

>  			enetc_cb->flag = ENETC_F_TX_TSTAMP;
>  		} else {

Checked that no later commit in this series changes the added call; the
only other enetc change is the cancel_work_sync()/skb_queue_purge() hunk in
enetc_stop().

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

[PATCH v3 net 7/7] net: enetc: drain and cancel one-step TX tstamp queue when going down

[vladimir.oltean]

The driver uses a work item on the system workqueue
(priv->tx_onestep_tstamp) for deferred transmission of packets with
one-step TX timestamping requests. The reason is that the MAC supports
a single such packet in flight, but we cannot block the rate at which
user space enqueues them.

The problem is that the skb queue is never explicitly drained, and it
can hold packets even after the interface goes down or (worse) the
driver is unbound from the device. Especially the last point is
critical, because the work item will attempt to use freed data
structures of the netdev.

The priv->tx_onestep_tstamp work item (enetc_tx_onestep_tstamp)
processes one item from the priv->tx_skbs queue at a time, and gets
rescheduled on each one-step PTP packet TX completion.

If we cancelled the work item while NAPI was still enabled, there would
be no guarantee that NAPI would not reenable it. So the cancellation
needs to be after napi_disable().

Cancelling the work item waits for enetc_tx_onestep_tstamp() to finish
sending the current packet if already scheduled. The packet will be put
in the disabled TX BD ring, where nothing will happen with it until
enetc_free_rxtx_rings() later reclaims its memory (*).

However, priv->tx_skbs may contain more packets than just this one, and
because NAPI is disabled, enetc_clean_tx_ring() is unable to take care
of the rest. So we still have to clean up the remainder from the queue
and reset the ENETC_TX_ONESTEP_TSTAMP_IN_PROGRESS flag back for use.

On driver unbind, the problem should be solved by virtue of the fact
that unregister_netdev() calls netif_close_many() and that triggers this
same code path.

(*) Even if we add a check for ENETC_TX_DOWN in enetc_tx_onestep_tstamp(),
it is unavoidable that racing one-step PTP packets will be enqueued in a
disabled TX ring. This is because the work item runs asynchronously and
can miss that flag getting set. Think below:

CPU A                                CPU B
enetc_tx_onestep_tstamp()
-> test_bit(ENETC_TX_DOWN)
   // says not down
                                     enetc_stop()
                                     -> set_bit(ENETC_TX_DOWN)
                                     -> enetc_wait_bdrs()
                                        // waits for the BDs in the
                                        // ring to be transmitted,
                                        // but the PTP frame is
                                        // still queued in software
                                     -> enetc_disable_tx_bdrs()
-> enetc_start_xmit()

So I don't see any point in adding an ENETC_TX_DOWN test in the work
item.

Fixes: 7294380c5211 ("enetc: support PTP Sync packet one-step timestamping")
Reported-by: Wei Fang <wei.fang@nxp.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
v2->v3: patch is new
---
 drivers/net/ethernet/freescale/enetc/enetc.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
index 62cdcaab3f3f..892490ff1ebe 100644
--- a/drivers/net/ethernet/freescale/enetc/enetc.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc.c
@@ -3110,6 +3110,10 @@ void enetc_stop(struct net_device *ndev)
 		napi_disable(&priv->int_vector[i]->napi);
 	}
 
+	cancel_work_sync(&priv->tx_onestep_tstamp);
+	skb_queue_purge(&priv->tx_skbs);
+	clear_bit_unlock(ENETC_TX_ONESTEP_TSTAMP_IN_PROGRESS, &priv->flags);
+
 	enetc_clear_interrupts(priv);
 }
 EXPORT_SYMBOL_GPL(enetc_stop);
-- 
2.43.0

Re: [PATCH v3 net 7/7] net: enetc: drain and cancel one-step TX tstamp queue when going down

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 (1) · Medium: 4 · Low: 0

- [Medium] The drain/cancel/flag-reset is added to enetc_stop(), which is
  not only the "going down" path but also the generic "pause traffic"…
- [Medium] The new cancel_work_sync() + skb_queue_purge(&priv->tx_skbs) +
  clear_bit_unlock(ENETC_TX_ONESTEP_TSTAMP_IN_PROGRESS) sequence in…
- [Medium] The patch's justification footnote accepts that
  enetc_tx_onestep_tstamp() can call enetc_start_xmit() after…
- [Medium] The new unconditional
  clear_bit_unlock(ENETC_TX_ONESTEP_TSTAMP_IN_PROGRESS) in enetc_stop()…

Pre-existing issues:
- [High] enetc_tx_onestep_tstamp() (enetc.c:2996-2998) dequeues an skb
  from priv->tx_skbs and calls enetc_start_xmit() while discarding the…

--

net: enetc: drain and cancel one-step TX tstamp queue when going down

The patch cancels the priv->tx_onestep_tstamp work item, purges the
priv->tx_skbs queue and resets ENETC_TX_ONESTEP_TSTAMP_IN_PROGRESS in
enetc_stop(), after napi_disable(), so that no deferred one-step PTP Sync
packet survives ifdown or driver unbind.

> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c
> index 62cdcaab3f3fb..892490ff1ebee 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.c
> @@ -3110,6 +3110,10 @@ void enetc_stop(struct net_device *ndev)
>  		napi_disable(&priv->int_vector[i]->napi);
>  	}
>  
> +	cancel_work_sync(&priv->tx_onestep_tstamp);

[Severity: Medium]
The commit message says the late frame submitted by the work item "will be
put in the disabled TX BD ring, where nothing will happen with it until
enetc_free_rxtx_rings() later reclaims its memory".  Does that cover the
hardware producer index as well?

enetc_map_tx_buffs() ends in enetc_update_tx_ring_tail():

	/* includes wmb() */
	enetc_wr_reg_hot(tx_ring->tpir, tx_ring->next_to_use);

so a submission that lands after enetc_disable_tx_bdrs() advances
ENETC_TBPIR and tx_ring->next_to_use.  Neither enetc_stop(),
enetc_free_tx_ring() nor enetc_assign_tx_resource() rolls those back, and
enetc_setup_txbdr() adopts the register values instead of clearing them on
ENETC v1:

	/* For ENETC v1, clearing PI/CI registers for Tx not supported,
	 * 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) &
				 ENETC_TBCIR_IDX_MASK;

	if (tx_ring->next_to_use != tx_ring->next_to_clean &&
	    !is_enetc_rev1(si)) {

After enetc_reconfigure() (enetc_stop(); enetc_free_rxtx_rings(); new
resources; enetc_setup_bdrs(); enetc_start();) an ENETC v1 part would come
back up with TBPIR != TBCIR on a freshly allocated, zero-filled BD ring, and
the driver's next_to_clean would point at a scrubbed swbd whose skb and DMA
mapping were already released by enetc_free_tx_frame().  Should the stop or
teardown path reconcile TBPIR with next_to_use, or keep the one-step frame
out of the disabled ring in the first place?

> +	skb_queue_purge(&priv->tx_skbs);

[Severity: Medium]
enetc_stop() is also the generic "pause traffic" helper, not only the going
down path.  It has three callers: enetc_close(), enetc_reconfigure() and
enetc_set_coalesce().  In the last two the interface stays administratively
up, so is the unconditional purge here dropping one-step Sync packets for
which the driver already returned NETDEV_TX_OK in enetc_start_xmit()?

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

That would make ethtool -C, XDP program attach/detach and hwtstamp
reconfiguration silently discard accepted PTP Sync frames.

Related to the same call sites, the footnote's reclaim argument does not seem
to hold for enetc_set_coalesce(), which is just:

	if (netif_running(ndev) && changed) {
		/* reconfigure the operation mode of h/w interrupts,
		 * traffic needs to be paused in the process
		 */
		enetc_stop(ndev);
		enetc_start(ndev);
	}

There is no enetc_free_rxtx_rings() in between, so a descriptor left in the
ring by the flushed work item is not reclaimed but transmitted once the ring
is re-enabled.  Should the drain live in enetc_close() instead, or be
conditional on the interface actually going down?

[Severity: High]
This isn't a bug introduced by this patch, but it limits what the new purge
can reclaim.  enetc_tx_onestep_tstamp() discards the return value of
enetc_start_xmit():

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

enetc_start_xmit() has NETDEV_TX_BUSY exits (the LSO and TSO paths, plus the
linear one):

		if (enetc_bd_unused(tx_ring) < ENETC_TXBDS_NEEDED(count)) {
			netif_stop_subqueue(ndev, tx_ring->index);
			return NETDEV_TX_BUSY;
		}

which neither transmit nor free the skb, because only a qdisc caller can
requeue it.  Does the work item leak that skb, given the dequeued reference
is the only one left?

The one-step branch has already run test_and_set_bit_lock() before those
returns, so does ENETC_TX_ONESTEP_TSTAMP_IN_PROGRESS also stay set with no
packet in flight?  The work item is only rescheduled from
enetc_clean_tx_ring() on a one-step TX completion, and that schedule_work()
happens inside the cleanup loop while tx_ring->next_to_clean is published
only after the loop, so the worker can observe a stale, nearly-full ring:

enetc_poll() -> enetc_clean_tx_ring()
	schedule_work(&priv->tx_onestep_tstamp);   /* inside the loop */
	...
	tx_ring->next_to_clean = i;                /* after the loop */

Such an abandoned skb is neither on priv->tx_skbs for the new
skb_queue_purge() nor in a TX ring for enetc_free_rxtx_rings(), so it would
survive ifdown and unbind too.

> +	clear_bit_unlock(ENETC_TX_ONESTEP_TSTAMP_IN_PROGRESS, &priv->flags);

[Severity: Medium]
The peer consumer touches exactly this state under the TX lock:

enetc_tx_onestep_tstamp()
	netif_tx_lock_bh(priv->ndev);
	clear_bit_unlock(ENETC_TX_ONESTEP_TSTAMP_IN_PROGRESS, &priv->flags);
	skb = skb_dequeue(&priv->tx_skbs);

Should the new drain and flag reset take netif_tx_lock_bh() as well?
netif_tx_stop_all_queues() only sets QUEUE_STATE_DRV_XOFF, it is not
netif_tx_disable() and takes no per-queue TX lock, so an ndo_start_xmit that
already passed the netif_xmit_frozen_or_stopped() check inside
HARD_TX_LOCK can still be running enetc_start_xmit() concurrently, and
enetc_start_xmit() has no ENETC_TX_DOWN test.

For the ndo_stop path this looks harmless, since __dev_close_many() calls
dev_deactivate_many(head, true) and waits for some_qdisc_is_busy() before
ndo_stop.  But enetc_set_coalesce() and enetc_reconfigure() call enetc_stop()
on a live interface with no qdisc deactivation, so:

CPU A                                  CPU B
enetc_start_xmit()                     enetc_stop()
                                       -> skb_queue_purge(&priv->tx_skbs)
                                       -> clear_bit_unlock(IN_PROGRESS)
-> test_and_set_bit_lock(IN_PROGRESS)
-> skb_queue_tail(&priv->tx_skbs, skb)

In the enetc_reconfigure() case the racing frame is released by
enetc_free_rxtx_rings() -> enetc_free_tx_frame(), which produces no TX
completion, so nothing calls schedule_work(&priv->tx_onestep_tstamp) again.
enetc_start() clears only ENETC_TX_DOWN and enetc_tx_onestep_tstamp_init() is
just INIT_WORK plus skb_queue_head_init:

	INIT_WORK(&priv->tx_onestep_tstamp, enetc_tx_onestep_tstamp);
	skb_queue_head_init(&priv->tx_skbs);

so can the IN_PROGRESS bit stay set for the rest of the up cycle, with every
subsequent one-step Sync appended to priv->tx_skbs and never sent?

[Severity: Medium]
This clear also releases the gate that enforces the MAC's "one single-step
frame in flight" rule, and the commit message accepts that cancel_work_sync()
may join the work item only after it pushed its frame into the disabled ring.

enetc_set_coalesce() then re-enables the same ring without reinitializing it,
via enetc_start() -> enetc_enable_txbdr():

	tbmr = enetc_txbdr_rd(hw, idx, ENETC_TBMR);
	tbmr |= ENETC_TBMR_EN;
	enetc_txbdr_wr(hw, idx, ENETC_TBMR, tbmr);

so the stale one-step descriptor is fetched and transmitted at a point where
the gate is already clear.  A new one-step Sync accepted right after
enetc_start() reprograms the single global single-step register:

	val |= ENETC_SET_SINGLE_STEP_OFFSET(offset);
	if (udp)
		val |= ENETC_PM0_SINGLE_STEP_CH;
	enetc_port_mac_wr(si, ENETC_PM0_SINGLE_STEP, val);

Can the correction field offset and the UDP checksum selector then be applied
to the wrong frame?  And when the stale frame finally completes, its
schedule_work() makes enetc_tx_onestep_tstamp() clear a gate that now belongs
to the newer frame, so does the one-in-flight invariant stay broken for
subsequent frames as well?

> +
>  	enetc_clear_interrupts(priv);
>  }
>  EXPORT_SYMBOL_GPL(enetc_stop);

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