[PATCH net] net: lan966x: release the in-flight slot of a stale TX skb

[PATCH net] net: lan966x: release the in-flight slot of a stale TX skb

[Daniel Machon]

lan966x_ptp_txtstamp_request() counts every two step TX skb waiting for
its timestamp in lan966x->ptp_skbs, and refuses new ones once
LAN966X_MAX_PTP_ID are in flight. lan966x_ptp_txtstamp_old_release()
frees the skbs that have waited longer than LAN966X_PTP_TIMEOUT.

Before this patch, old_release() does not decrement ptp_skbs, so every
skb whose timestamp never arrives, for example because the switch
discarded the frame, keeps its slot forever. Once LAN966X_MAX_PTP_ID
slots are lost, two step frames fail to transmit on every port of the
switch.

Return the number of released skbs from old_release() and subtract it
from ptp_skbs in txtstamp_request(), under the ptp_ts_id_lock it already
takes.

This fixes the accounting only. A port's stale skbs are still aged out
only when that port sends a two step frame, so a port that stops sending
them keeps its stale skbs, and their slots, until it sends one again.
That behaviour is unchanged by this patch.

Found by an AI code review agent. Reproduced on a lan9668 board.

Fixes: 77eecf25bd9d ("net: lan966x: Update extraction/injection for timestamping")
Signed-off-by: Daniel Machon <daniel.machon@microchip.com>
---
 drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c b/drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c
index 8c40db90ee8f6..9be2cfe6feeb7 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c
@@ -376,9 +376,10 @@ static void lan966x_ptp_classify(struct lan966x_port *port, struct sk_buff *skb,
 	*rew_op = IFH_REW_OP_TWO_STEP_PTP;
 }
 
-static void lan966x_ptp_txtstamp_old_release(struct lan966x_port *port)
+static unsigned int lan966x_ptp_txtstamp_old_release(struct lan966x_port *port)
 {
 	struct sk_buff *skb, *skb_tmp;
+	unsigned int purged = 0;
 	unsigned long flags;
 
 	spin_lock_irqsave(&port->tx_skbs.lock, flags);
@@ -389,14 +390,18 @@ static void lan966x_ptp_txtstamp_old_release(struct lan966x_port *port)
 
 		__skb_unlink(skb, &port->tx_skbs);
 		dev_kfree_skb_any(skb);
+		purged++;
 	}
 	spin_unlock_irqrestore(&port->tx_skbs.lock, flags);
+
+	return purged;
 }
 
 int lan966x_ptp_txtstamp_request(struct lan966x_port *port,
 				 struct sk_buff *skb)
 {
 	struct lan966x *lan966x = port->lan966x;
+	unsigned int purged;
 	unsigned long flags;
 	u8 pdu_type;
 	u8 rew_op;
@@ -408,9 +413,10 @@ int lan966x_ptp_txtstamp_request(struct lan966x_port *port,
 	if (rew_op != IFH_REW_OP_TWO_STEP_PTP)
 		return 0;
 
-	lan966x_ptp_txtstamp_old_release(port);
+	purged = lan966x_ptp_txtstamp_old_release(port);
 
 	spin_lock_irqsave(&lan966x->ptp_ts_id_lock, flags);
+	lan966x->ptp_skbs -= purged;
 	if (lan966x->ptp_skbs == LAN966X_MAX_PTP_ID) {
 		spin_unlock_irqrestore(&lan966x->ptp_ts_id_lock, flags);
 		return -EBUSY;

---
base-commit: 6430fb7f1cc4e329e386521b7b208df443de92f2
change-id: 20261023-lan966x-ptp-stale-tx-slot-95dffa0df0dd

Best regards,
-- 
Daniel Machon <daniel.machon@microchip.com>

Re: [PATCH net] net: lan966x: release the in-flight slot of a stale TX skb

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] After this patch, slots are reclaimed only by
  lan966x_ptp_txtstamp_old_release(), which ages only the queue of the…

Pre-existing issues:
- [Critical] This problem wasn't introduced by this patch, but when the
  shared 512-entry FDMA TX ring is full, lan966x_fdma_xmit() returns…
- [High] This problem wasn't introduced by this patch, but the
  sparx5/lan969x driver has the same leak this patch fixes for lan966x.
- [High] This problem wasn't introduced by this patch, but
  lan966x_ptp_txtstamp_request() makes the skb visible on port->tx_skbs…
- [High] This problem wasn't introduced by this patch, but when
  lan966x_ptp_txtstamp_request() returns -EBUSY, lan966x_port_xmit()…
- [High] This problem wasn't introduced by this patch, but in FDMA mode
  lan966x_fdma_xmit() leaves a two-step skb DMA-mapped and referenced by…
- [High] This problem wasn't introduced by this patch, but
  lan966x_fdma_xmit() can free a two-step skb that is still on…
- [Medium] This problem wasn't introduced by this patch, but
  lan966x_ptp_txtstamp_old_release() frees two-step skbs after…

--

net: lan966x: release the in-flight slot of a stale TX skb

lan966x_ptp_txtstamp_old_release() now returns how many stale two-step TX
skbs it freed. lan966x_ptp_txtstamp_request() subtracts that count from
lan966x->ptp_skbs under ptp_ts_id_lock. Before this, a slot whose
timestamp never arrived was never released, so two-step TX eventually
failed on every port.

> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c b/drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c
> index 8c40db90ee8f..9be2cfe6feeb 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c

[ ... ]

> @@ -389,14 +390,18 @@ static void lan966x_ptp_txtstamp_old_release(struct lan966x_port *port)
>  
>  		__skb_unlink(skb, &port->tx_skbs);
>  		dev_kfree_skb_any(skb);
> +		purged++;
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but can this free a two-step
skb that the FDMA engine is still reading?

In FDMA mode, lan966x_fdma_xmit() leaves the skb DMA-mapped and
referenced by the shared TX ring, and hands its lifetime to the PTP code:

drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c:lan966x_fdma_xmit() {
    ...
	next_dcb_buf->data.skb = skb;
    ...
	if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP &&
	    LAN966X_SKB_CB(skb)->rew_op == IFH_REW_OP_TWO_STEP_PTP)
		next_dcb_buf->ptp = true;
    ...
}

On completion, lan966x_fdma_tx_clear_buf() only unmaps it:

			if (!dcb_buf->ptp)
				napi_consume_skb(dcb_buf->data.skb, weight);

lan966x_ptp_txtstamp_old_release() frees by age alone.
LAN966X_PTP_TIMEOUT is msecs_to_jiffies(10), which is a single jiffy at
HZ=100. Nothing checks whether the FDMA descriptor has completed.

The PTP frame can sit behind other frames in the 512-entry ring that all
ports share, or egress can be slow (10/100 link, PAUSE, shapers). In
either case, the next two-step request on the same port can hand the skb
data back to the allocator while the device can still DMA-read it.

Could the device then send freed or reused memory onto the wire? And
would the late stamp then hit WARN_ON(!skb_match)?

[Severity: Medium]
This is also a pre-existing issue, but because aging here is based only
on time, what happens to a stamp that arrives after its skb has been
aged out?

lan966x_ptp_irq_handler() matches FIFO entries only by the per-port
0..511 id:

		skb_queue_walk_safe(&port->tx_skbs, skb, skb_tmp) {
			if (LAN966X_SKB_CB(skb)->ts_id != id)
				continue;
		...
		if (WARN_ON(!skb_match))
			continue;

A late stamp either hits the WARN_ON(), or it gets attributed to a newer
skb that reused the id, if the port's ts_id has wrapped in the meantime.

Can userspace then receive a wrong TX hardware timestamp, with the newer
skb's own stamp tripping the WARN later? Frames held in egress past the
timeout would trigger this, for example with taprio/tbf/cbs offload,
PAUSE or a slow link.

ptp_skbs never prevented id reuse, so this patch does not change the
behaviour.

>  	spin_unlock_irqrestore(&port->tx_skbs.lock, flags);
> +
> +	return purged;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does sparx5/lan969x need
the same fix? sparx5_ptp_txtstamp_old_release() in
drivers/net/ethernet/microchip/sparx5/sparx5_ptp.c matches the pre-patch
lan966x version, and it never decrements sparx5->ptp_skbs:

	skb_queue_walk_safe(&port->tx_skbs, skb, skb_tmp) {
		if time_after(SPARX5_SKB_CB(skb)->jiffies + SPARX5_PTP_TIMEOUT,
			      jiffies)
			break;

		__skb_unlink(skb, &port->tx_skbs);
		dev_kfree_skb_any(skb);
	}

There are only two other decrements. The IRQ handlers in sparx5_ptp.c
and lan969x.c decrement it for a matched skb, and
sparx5_ptp_txtstamp_release() does so on the xmit busy path.

After SPARX5_MAX_PTP_ID (512) lost stamps,
sparx5_ptp_txtstamp_request() rejects every two-step request:

	if (sparx5->ptp_skbs == SPARX5_MAX_PTP_ID) {
		spin_unlock_irqrestore(&sparx5->ptp_ts_id_lock, flags);
		return -EBUSY;
	}

sparx5_port_xmit_impl() then returns NETDEV_TX_BUSY without stopping the
queue:

		if (sparx5_ptp_txtstamp_request(port, skb) < 0)
			return NETDEV_TX_BUSY;

Wouldn't the qdisc then requeue and retry the same PTP skb forever, and
block all traffic behind it on that TX queue?

A companion patch for sparx5 seems needed. It would return the purged
count and subtract it under ptp_ts_id_lock.

[ ... ]

> @@ -408,9 +413,10 @@ int lan966x_ptp_txtstamp_request(struct lan966x_port *port,
>  	if (rew_op != IFH_REW_OP_TWO_STEP_PTP)
>  		return 0;
>  
> -	lan966x_ptp_txtstamp_old_release(port);
> +	purged = lan966x_ptp_txtstamp_old_release(port);
>  
>  	spin_lock_irqsave(&lan966x->ptp_ts_id_lock, flags);
> +	lan966x->ptp_skbs -= purged;
>  	if (lan966x->ptp_skbs == LAN966X_MAX_PTP_ID) {

[Severity: Medium]
The ptp_skbs limit is switch-wide. Slots are only reclaimed by aging the
transmitting port's queue, and only when that port sends a two-step
frame. Can stale skbs on one port still use up the limit for every other
port?

The commit message says idle ports keep their stale skbs. Nothing else
releases them either. lan966x_port_stop() does not purge port->tx_skbs,
and HWTSTAMP_TX_OFF in lan966x_ptp_hwtstamp_set() only does:

	case HWTSTAMP_TX_OFF:
		port->ptp_tx_cmd = IFH_REW_OP_NOOP;
		break;

After that the port can never send a two-step frame again, so its stale
skbs are never aged. Only lan966x_ptp_deinit() purges tx_skbs.

Lost stamps can pile up on a port that then goes quiet: ifdown, TX
timestamping turned off, ptp4l stopped, or a boundary clock port in
passive state. Two-step TX on the other ports is then throttled, and
returns -EBUSY once the total reaches LAN966X_MAX_PTP_ID. That is the
failure this patch sets out to fix.

Would any of these close the gap: aging all ports before rejecting,
aging from a timer or worker, or purging and uncounting on ndo_stop and
HWTSTAMP_TX_OFF?

>  		spin_unlock_irqrestore(&lan966x->ptp_ts_id_lock, flags);
>  		return -EBUSY;

[Severity: High]
This isn't a bug introduced by this patch, but does this -EBUSY path
leak the skb? lan966x_port_xmit() returns the errno directly as its
netdev_tx_t:

		err = lan966x_ptp_txtstamp_request(port, skb);
		if (err)
			return err;

dev_xmit_complete() treats any rc < NET_XMIT_MASK, negative values
included, as meaning the driver consumed the skb. The core therefore
neither requeues nor frees it. The skb is not on port->tx_skbs either,
because this return happens before skb_queue_tail().

So every two-step frame sent while the slot table is full would leak an
skb, and its sk_wmem_alloc charge would keep the sending socket pinned.

Once an admin has enabled HW TX timestamping on a port, could an
unprivileged user fill the table and keep leaking memory? Sending UDP to
port 319 with SOF_TIMESTAMPING_TX_HARDWARE would be enough.

[Severity: High]
This is a pre-existing issue, but further down in
lan966x_ptp_txtstamp_request() the skb is published on port->tx_skbs
before its control block is filled in:

	skb_queue_tail(&port->tx_skbs, skb);
	LAN966X_SKB_CB(skb)->ts_id = port->ts_id;
	LAN966X_SKB_CB(skb)->jiffies = jiffies;

Only ptp_ts_id_lock covers these writes. lan966x_ptp_irq_handler() walks
the list holding only tx_skbs.lock and matches on
LAN966X_SKB_CB(skb)->ts_id.

On PREEMPT_RT, ptp_ts_id_lock is a sleeping lock and the PTP handler is
a threaded IRQ (devm_request_threaded_irq() in lan966x_probe()). The
handler can therefore preempt the sender right after skb_queue_tail().

Suppose the FIFO holds an unmatched stamp, for example a late one for an
aged-out skb, whose id equals the leftover ts_id bytes in the cb. The
handler then unlinks the new skb, and once request() drops the lock it
timestamps and frees it.

Wouldn't lan966x_port_xmit() then keep reading the cb and transmit the
freed skb? Setting ts_id and jiffies before skb_queue_tail() would avoid
this.

[Severity: High]
This is another pre-existing issue. After request() has queued and
counted the skb, lan966x_fdma_xmit() can free it without unlinking it
from port->tx_skbs:

drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c:lan966x_fdma_xmit() {
    ...
	if (skb_put_padto(skb, ETH_ZLEN)) {
		dev->stats.tx_dropped++;
		return NETDEV_TX_OK;
	}
    ...
}

skb_put_padto() passes free_on_error=true, so __skb_pad() frees the skb
when pskb_expand_head(GFP_ATOMIC) or skb_linearize() fails. This return
also skips the lan966x_ptp_txtstamp_release() call that the release:
label makes.

L2 PTP event messages are 58 bytes, which is below ETH_ZLEN. They are
cloned when an AF_PACKET tap is active, and that forces
pskb_expand_head().

Under memory pressure, could a later old_release() or IRQ handler walk
read the freed skb, __skb_unlink() it, and call dev_kfree_skb_any() on
it a second time?

[Severity: Critical]
This isn't introduced by this patch either, but the ring-full path in
lan966x_fdma_xmit() also returns without undoing the reservation made
here:

	next_to_use = lan966x_fdma_get_next_dcb(tx);
	if (next_to_use < 0) {
		netif_stop_queue(dev);
		return NETDEV_TX_BUSY;
	}

The register injection path does undo it, in lan966x_port_ifh_xmit():

err:
	if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP &&
	    LAN966X_SKB_CB(skb)->rew_op == IFH_REW_OP_TWO_STEP_PTP)
		lan966x_ptp_txtstamp_release(port, skb);

	return NETDEV_TX_BUSY;

On NETDEV_TX_BUSY, dev_hard_start_xmit() overwrites skb->next, and
dev_requeue_skb() links the skb into q->gso_skb. At that point the skb
is still linked on port->tx_skbs and still counted in ptp_skbs.

Can this corrupt port->tx_skbs? A later old_release() or IRQ walk could
follow the overwritten pointers, or free an skb the stack still owns.

When the qdisc retries, request() calls skb_queue_tail() on the same skb
again. If that skb was the tail, skb->next ends up pointing to itself,
and skb_queue_walk_safe() in lan966x_ptp_irq_handler() would loop
forever with tx_skbs.lock held.

The 512-entry ring is shared by all ports, and netif_stop_queue() only
stops TX queue 0 of the current netdev. Ordinary TX congestion plus
two-step traffic on another port or queue looks like enough to reach
this.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261026-lan966x-ptp-stale-tx-slot-v1-1-9f7c895eb1ff%40microchip.com

Re: [PATCH net] net: lan966x: release the in-flight slot of a stale TX skb

[Daniel Machon]

> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 (1) · High: 0 (5) · Medium: 1 (1) · Low: 0
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [Medium] After this patch, slots are reclaimed only by
>   lan966x_ptp_txtstamp_old_release(), which ages only the queue of the…
> 
> Pre-existing issues:
> - [Critical] This problem wasn't introduced by this patch, but when the
>   shared 512-entry FDMA TX ring is full, lan966x_fdma_xmit() returns…
> - [High] This problem wasn't introduced by this patch, but the
>   sparx5/lan969x driver has the same leak this patch fixes for lan966x.
> - [High] This problem wasn't introduced by this patch, but
>   lan966x_ptp_txtstamp_request() makes the skb visible on port->tx_skbs…
> - [High] This problem wasn't introduced by this patch, but when
>   lan966x_ptp_txtstamp_request() returns -EBUSY, lan966x_port_xmit()…
> - [High] This problem wasn't introduced by this patch, but in FDMA mode
>   lan966x_fdma_xmit() leaves a two-step skb DMA-mapped and referenced by…
> - [High] This problem wasn't introduced by this patch, but
>   lan966x_fdma_xmit() can free a two-step skb that is still on…
> - [Medium] This problem wasn't introduced by this patch, but
>   lan966x_ptp_txtstamp_old_release() frees two-step skbs after…
> 
> --
> 
> net: lan966x: release the in-flight slot of a stale TX skb
> 
> lan966x_ptp_txtstamp_old_release() now returns how many stale two-step TX
> skbs it freed. lan966x_ptp_txtstamp_request() subtracts that count from
> lan966x->ptp_skbs under ptp_ts_id_lock. Before this, a slot whose
> timestamp never arrived was never released, so two-step TX eventually
> failed on every port.
> 
> > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c b/drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c
> > index 8c40db90ee8f..9be2cfe6feeb 100644
> > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c
> > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c
> 
> [ ... ]
> 
> > @@ -389,14 +390,18 @@ static void lan966x_ptp_txtstamp_old_release(struct lan966x_port *port)
> >
> >               __skb_unlink(skb, &port->tx_skbs);
> >               dev_kfree_skb_any(skb);
> > +             purged++;
> >       }
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but can this free a two-step
> skb that the FDMA engine is still reading?
> 
> In FDMA mode, lan966x_fdma_xmit() leaves the skb DMA-mapped and
> referenced by the shared TX ring, and hands its lifetime to the PTP code:
> 
> drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c:lan966x_fdma_xmit() {
>     ...
>         next_dcb_buf->data.skb = skb;
>     ...
>         if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP &&
>             LAN966X_SKB_CB(skb)->rew_op == IFH_REW_OP_TWO_STEP_PTP)
>                 next_dcb_buf->ptp = true;
>     ...
> }
> 
> On completion, lan966x_fdma_tx_clear_buf() only unmaps it:
> 
>                         if (!dcb_buf->ptp)
>                                 napi_consume_skb(dcb_buf->data.skb, weight);
> 
> lan966x_ptp_txtstamp_old_release() frees by age alone.
> LAN966X_PTP_TIMEOUT is msecs_to_jiffies(10), which is a single jiffy at
> HZ=100. Nothing checks whether the FDMA descriptor has completed.
> 
> The PTP frame can sit behind other frames in the 512-entry ring that all
> ports share, or egress can be slow (10/100 link, PAUSE, shapers). In
> either case, the next two-step request on the same port can hand the skb
> data back to the allocator while the device can still DMA-read it.
> 
> Could the device then send freed or reused memory onto the wire? And
> would the late stamp then hit WARN_ON(!skb_match)?
> 
> [Severity: Medium]
> This is also a pre-existing issue, but because aging here is based only
> on time, what happens to a stamp that arrives after its skb has been
> aged out?
> 
> lan966x_ptp_irq_handler() matches FIFO entries only by the per-port
> 0..511 id:
> 
>                 skb_queue_walk_safe(&port->tx_skbs, skb, skb_tmp) {
>                         if (LAN966X_SKB_CB(skb)->ts_id != id)
>                                 continue;
>                 ...
>                 if (WARN_ON(!skb_match))
>                         continue;
> 
> A late stamp either hits the WARN_ON(), or it gets attributed to a newer
> skb that reused the id, if the port's ts_id has wrapped in the meantime.
> 
> Can userspace then receive a wrong TX hardware timestamp, with the newer
> skb's own stamp tripping the WARN later? Frames held in egress past the
> timeout would trigger this, for example with taprio/tbf/cbs offload,
> PAUSE or a slow link.
> 
> ptp_skbs never prevented id reuse, so this patch does not change the
> behaviour.
> 
> >       spin_unlock_irqrestore(&port->tx_skbs.lock, flags);
> > +
> > +     return purged;
> >  }
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but does sparx5/lan969x need
> the same fix? sparx5_ptp_txtstamp_old_release() in
> drivers/net/ethernet/microchip/sparx5/sparx5_ptp.c matches the pre-patch
> lan966x version, and it never decrements sparx5->ptp_skbs:
> 
>         skb_queue_walk_safe(&port->tx_skbs, skb, skb_tmp) {
>                 if time_after(SPARX5_SKB_CB(skb)->jiffies + SPARX5_PTP_TIMEOUT,
>                               jiffies)
>                         break;
> 
>                 __skb_unlink(skb, &port->tx_skbs);
>                 dev_kfree_skb_any(skb);
>         }
> 
> There are only two other decrements. The IRQ handlers in sparx5_ptp.c
> and lan969x.c decrement it for a matched skb, and
> sparx5_ptp_txtstamp_release() does so on the xmit busy path.
> 
> After SPARX5_MAX_PTP_ID (512) lost stamps,
> sparx5_ptp_txtstamp_request() rejects every two-step request:
> 
>         if (sparx5->ptp_skbs == SPARX5_MAX_PTP_ID) {
>                 spin_unlock_irqrestore(&sparx5->ptp_ts_id_lock, flags);
>                 return -EBUSY;
>         }
> 
> sparx5_port_xmit_impl() then returns NETDEV_TX_BUSY without stopping the
> queue:
> 
>                 if (sparx5_ptp_txtstamp_request(port, skb) < 0)
>                         return NETDEV_TX_BUSY;
> 
> Wouldn't the qdisc then requeue and retry the same PTP skb forever, and
> block all traffic behind it on that TX queue?
> 
> A companion patch for sparx5 seems needed. It would return the purged
> count and subtract it under ptp_ts_id_lock.
> 
> [ ... ]
> 
> > @@ -408,9 +413,10 @@ int lan966x_ptp_txtstamp_request(struct lan966x_port *port,
> >       if (rew_op != IFH_REW_OP_TWO_STEP_PTP)
> >               return 0;
> >
> > -     lan966x_ptp_txtstamp_old_release(port);
> > +     purged = lan966x_ptp_txtstamp_old_release(port);
> >
> >       spin_lock_irqsave(&lan966x->ptp_ts_id_lock, flags);
> > +     lan966x->ptp_skbs -= purged;
> >       if (lan966x->ptp_skbs == LAN966X_MAX_PTP_ID) {
> 
> [Severity: Medium]
> The ptp_skbs limit is switch-wide. Slots are only reclaimed by aging the
> transmitting port's queue, and only when that port sends a two-step
> frame. Can stale skbs on one port still use up the limit for every other
> port?

Yes, but this is a separate, pre-existing limitation, and the commit message
even says so.

> 
> The commit message says idle ports keep their stale skbs. Nothing else
> releases them either. lan966x_port_stop() does not purge port->tx_skbs,
> and HWTSTAMP_TX_OFF in lan966x_ptp_hwtstamp_set() only does:
> 
>         case HWTSTAMP_TX_OFF:
>                 port->ptp_tx_cmd = IFH_REW_OP_NOOP;
>                 break;
> 
> After that the port can never send a two-step frame again, so its stale
> skbs are never aged. Only lan966x_ptp_deinit() purges tx_skbs.
> 
> Lost stamps can pile up on a port that then goes quiet: ifdown, TX
> timestamping turned off, ptp4l stopped, or a boundary clock port in
> passive state. Two-step TX on the other ports is then throttled, and
> returns -EBUSY once the total reaches LAN966X_MAX_PTP_ID. That is the
> failure this patch sets out to fix.
> 
> Would any of these close the gap: aging all ports before rejecting,
> aging from a timer or worker, or purging and uncounting on ndo_stop and
> HWTSTAMP_TX_OFF?

The bug fixed here is that every lost timestamp leaked its slot for good, so the
limit was eventually hit on any system that lost timestamps.  Aging the queues
of all ports on each request, so an idle port cannot hold slots, I believe is a
change for net-next.

> 
> >               spin_unlock_irqrestore(&lan966x->ptp_ts_id_lock, flags);
> >               return -EBUSY;
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but does this -EBUSY path
> leak the skb? lan966x_port_xmit() returns the errno directly as its
> netdev_tx_t:
> 
>                 err = lan966x_ptp_txtstamp_request(port, skb);
>                 if (err)
>                         return err;
> 
> dev_xmit_complete() treats any rc < NET_XMIT_MASK, negative values
> included, as meaning the driver consumed the skb. The core therefore
> neither requeues nor frees it. The skb is not on port->tx_skbs either,
> because this return happens before skb_queue_tail().
> 
> So every two-step frame sent while the slot table is full would leak an
> skb, and its sk_wmem_alloc charge would keep the sending socket pinned.
> 
> Once an admin has enabled HW TX timestamping on a port, could an
> unprivileged user fill the table and keep leaking memory? Sending UDP to
> port 319 with SOF_TIMESTAMPING_TX_HARDWARE would be enough.
> 
> [Severity: High]
> This is a pre-existing issue, but further down in
> lan966x_ptp_txtstamp_request() the skb is published on port->tx_skbs
> before its control block is filled in:
> 
>         skb_queue_tail(&port->tx_skbs, skb);
>         LAN966X_SKB_CB(skb)->ts_id = port->ts_id;
>         LAN966X_SKB_CB(skb)->jiffies = jiffies;
> 
> Only ptp_ts_id_lock covers these writes. lan966x_ptp_irq_handler() walks
> the list holding only tx_skbs.lock and matches on
> LAN966X_SKB_CB(skb)->ts_id.
> 
> On PREEMPT_RT, ptp_ts_id_lock is a sleeping lock and the PTP handler is
> a threaded IRQ (devm_request_threaded_irq() in lan966x_probe()). The
> handler can therefore preempt the sender right after skb_queue_tail().
> 
> Suppose the FIFO holds an unmatched stamp, for example a late one for an
> aged-out skb, whose id equals the leftover ts_id bytes in the cb. The
> handler then unlinks the new skb, and once request() drops the lock it
> timestamps and frees it.
> 
> Wouldn't lan966x_port_xmit() then keep reading the cb and transmit the
> freed skb? Setting ts_id and jiffies before skb_queue_tail() would avoid
> this.
> 
> [Severity: High]
> This is another pre-existing issue. After request() has queued and
> counted the skb, lan966x_fdma_xmit() can free it without unlinking it
> from port->tx_skbs:
> 
> drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c:lan966x_fdma_xmit() {
>     ...
>         if (skb_put_padto(skb, ETH_ZLEN)) {
>                 dev->stats.tx_dropped++;
>                 return NETDEV_TX_OK;
>         }
>     ...
> }
> 
> skb_put_padto() passes free_on_error=true, so __skb_pad() frees the skb
> when pskb_expand_head(GFP_ATOMIC) or skb_linearize() fails. This return
> also skips the lan966x_ptp_txtstamp_release() call that the release:
> label makes.
> 
> L2 PTP event messages are 58 bytes, which is below ETH_ZLEN. They are
> cloned when an AF_PACKET tap is active, and that forces
> pskb_expand_head().
> 
> Under memory pressure, could a later old_release() or IRQ handler walk
> read the freed skb, __skb_unlink() it, and call dev_kfree_skb_any() on
> it a second time?
> 
> [Severity: Critical]
> This isn't introduced by this patch either, but the ring-full path in
> lan966x_fdma_xmit() also returns without undoing the reservation made
> here:
> 
>         next_to_use = lan966x_fdma_get_next_dcb(tx);
>         if (next_to_use < 0) {
>                 netif_stop_queue(dev);
>                 return NETDEV_TX_BUSY;
>         }
> 
> The register injection path does undo it, in lan966x_port_ifh_xmit():
> 
> err:
>         if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP &&
>             LAN966X_SKB_CB(skb)->rew_op == IFH_REW_OP_TWO_STEP_PTP)
>                 lan966x_ptp_txtstamp_release(port, skb);
> 
>         return NETDEV_TX_BUSY;
> 
> On NETDEV_TX_BUSY, dev_hard_start_xmit() overwrites skb->next, and
> dev_requeue_skb() links the skb into q->gso_skb. At that point the skb
> is still linked on port->tx_skbs and still counted in ptp_skbs.
> 
> Can this corrupt port->tx_skbs? A later old_release() or IRQ walk could
> follow the overwritten pointers, or free an skb the stack still owns.
> 
> When the qdisc retries, request() calls skb_queue_tail() on the same skb
> again. If that skb was the tail, skb->next ends up pointing to itself,
> and skb_queue_walk_safe() in lan966x_ptp_irq_handler() would loop
> forever with tx_skbs.lock held.
> 
> The 512-entry ring is shared by all ports, and netif_stop_queue() only
> stops TX queue 0 of the current netdev. Ordinary TX congestion plus
> two-step traffic on another port or queue looks like enough to reach
> this.
> 
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261026-lan966x-ptp-stale-tx-slot-v1-1-9f7c895eb1ff%40microchip.com

Re: [PATCH net] net: lan966x: release the in-flight slot of a stale TX skb

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Mon, 26 Oct 2026 14:46:54 +0100 you wrote:
> lan966x_ptp_txtstamp_request() counts every two step TX skb waiting for
> its timestamp in lan966x->ptp_skbs, and refuses new ones once
> LAN966X_MAX_PTP_ID are in flight. lan966x_ptp_txtstamp_old_release()
> frees the skbs that have waited longer than LAN966X_PTP_TIMEOUT.
> 
> Before this patch, old_release() does not decrement ptp_skbs, so every
> skb whose timestamp never arrives, for example because the switch
> discarded the frame, keeps its slot forever. Once LAN966X_MAX_PTP_ID
> slots are lost, two step frames fail to transmit on every port of the
> switch.
> 
> [...]

Here is the summary with links:
  - [net] net: lan966x: release the in-flight slot of a stale TX skb
    https://git.kernel.org/netdev/net/c/074caa30a627

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html