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

[Simon Horman <horms@kernel.org>]

On Mon, Nov 02, 2026 at 10:14:49AM +0100, Daniel Machon wrote:

...

> > [Severity: Medium]
> > Can this decrement race with sparx5_ptp_txtstamp_release() and count the
> > same skb down twice?
> > 
> > Each port netdev has SPX5_PRIOS TX queues and there is no
> > ndo_select_queue, so two CPUs can run sparx5_port_xmit_impl() for the same
> > port at the same time. sparx5_ptp_txtstamp_old_release() unlinks while
> > holding only tx_skbs.lock. The busy path decrements without checking
> > whether the skb is still on the list:
> > 
> > drivers/net/ethernet/microchip/sparx5/sparx5_ptp.c:sparx5_ptp_txtstamp_release() {
> >     ...
> >         spin_lock_irqsave(&sparx5->ptp_ts_id_lock, flags);
> >         port->ts_id--;
> >         sparx5->ptp_skbs--;
> >         skb_unlink(skb, &port->tx_skbs);
> >         spin_unlock_irqrestore(&sparx5->ptp_ts_id_lock, flags);
> > }
> > 
> > A possible sequence:
> > 
> > CPU0
> > sparx5_ptp_txtstamp_request()
> >     skb_queue_tail(&port->tx_skbs, X)
> >     sparx5->ptp_skbs++
> > spin_lock(&sparx5->tx_lock)   /* waits */
> > 
> > CPU1
> > sparx5_ptp_txtstamp_request()
> >     sparx5_ptp_txtstamp_old_release()
> >         /* X looks older than SPARX5_PTP_TIMEOUT */
> >         __skb_unlink(X); dev_kfree_skb_any(X); purged++
> >     sparx5->ptp_skbs -= purged
> > 
> > CPU0
> > ops->fdma_xmit() or sparx5_inject() returns -EBUSY
> > sparx5_ptp_txtstamp_release(port, X)
> >     sparx5->ptp_skbs--
> > 
> > At HZ=100, SPARX5_PTP_TIMEOUT is one jiffy, so crossing a single tick is
> > enough for X to look expired.
> > 
> > Before this patch the purge path did not decrement, so X was counted down
> > only once. ptp_skbs is a u16 and the limit check only tests for equality.
> > 
> > If the counter wraps to 65535, does the SPARX5_MAX_PTP_ID limit stop
> > working, so that ts_id values can be reused while they are still
> > outstanding?
> > 
> > The same window already lets CPU0 keep using X after CPU1 has freed it.
> > That part predates this patch; the double decrement is new.
> 
> This fixes a leak seen in normal operation. Each lost timestamp keeps
> an in-flight slot, and after SPARX5_MAX_PTP_ID losses, two step
> timestamping stops on all ports. The lan966x twin (074caa30a627) is
> already in net.
> 
> AFAICT, the wrong count can never be used. The second decrement is in the busy
> path, which on the very next line unlinks an skb that a purge on
> another TX queue has already freed. That use-after-free is
> pre-existing and crashes before the count matters.
> 
> I am working on a net-next series that reworks PTP TX timestamping and fixes
> this and several related issues.

Thanks Daniel,

I agree that is a good approach.

Reviewed-by: Simon Horman <horms@kernel.org>

...