* Re: [PATCH net] net: sparx5/lan969x: release the in-flight slot of a stale TX skb
2026-10-29 11:48 [PATCH net] net: sparx5/lan969x: release the in-flight slot of a stale TX skb Daniel Machon
@ 2026-10-30 22:37 ` netdev-bot+sashiko
0 siblings, 0 replies; 2+ messages in thread
From: netdev-bot+sashiko @ 2026-10-30 22:37 UTC (permalink / raw)
To: daniel.machon
Cc: andrew+netdev, davem, edumazet, kuba, pabeni, Steen.Hegelund,
UNGLinuxDriver, richardcochran, horatiu.vultur, netdev,
linux-arm-kernel, linux-kernel
Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.
Critical: 0 (1) · High: 0 (2) · Medium: 1 (1) · Low: 0 (1)
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [Medium] This patch adds a second ptp_skbs decrement path that is not
coordinated with sparx5_ptp_txtstamp_release().
Pre-existing issues:
- [Critical] Double free and use-after-free of two-step PTP skbs that go
through the drop path of sparx5_port_xmit_impl().
- [High] Use-after-free in lan969x_fdma_tx_clear_buf()
(lan969x/lan969x_fdma.c:70-77) for two-step PTP skbs.
- [High] sparx5_ptp_txtstamp_request() adds the skb to port->tx_skbs with
skb_queue_tail() (sparx5_ptp.c:255), which takes and releases…
- [Medium] Stale skbs, and their global ptp_skbs slots, are aged only
when the same port issues another two-step request.
- [Low] The 10 ms SPARX5_PTP_TIMEOUT used by
sparx5_ptp_txtstamp_old_release() treats a frame held in an egress…
--
net: sparx5/lan969x: release the in-flight slot of a stale TX skb
This patch makes sparx5_ptp_txtstamp_old_release() return how many stale
two step TX skbs it freed. sparx5_ptp_txtstamp_request() then subtracts
that number from sparx5->ptp_skbs under ptp_ts_id_lock. Without this, an
skb whose timestamp never arrived kept one of the SPARX5_MAX_PTP_ID
in-flight slots forever.
> diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_ptp.c b/drivers/net/ethernet/microchip/sparx5/sparx5_ptp.c
> index 2826e48af1033..9f61692b243b7 100644
> --- a/drivers/net/ethernet/microchip/sparx5/sparx5_ptp.c
> +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_ptp.c
[ ... ]
> @@ -217,8 +218,11 @@ static void sparx5_ptp_txtstamp_old_release(struct sparx5_port *port)
>
> __skb_unlink(skb, &port->tx_skbs);
> dev_kfree_skb_any(skb);
> + purged++;
> }
[Severity: Critical]
This isn't a bug introduced by this patch, but can this walk free a two
step skb a second time?
In sparx5_port_xmit_impl(), sparx5_ptp_txtstamp_request() has already
queued the skb on port->tx_skbs and incremented ptp_skbs. If the backend
then fails with anything other than -EBUSY, the drop label only frees the
skb:
drivers/net/ethernet/microchip/sparx5/sparx5_packet.c:sparx5_port_xmit_impl() {
...
if (ret == -EBUSY)
goto busy;
if (ret < 0)
goto drop;
...
drop:
stats->tx_dropped++;
sparx5->tx.dropped++;
dev_kfree_skb_any(skb);
return NETDEV_TX_OK;
busy:
if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP &&
SPARX5_SKB_CB(skb)->rew_op == IFH_REW_OP_TWO_STEP_PTP)
sparx5_ptp_txtstamp_release(port, skb);
return NETDEV_TX_BUSY;
}
Only the busy path calls sparx5_ptp_txtstamp_release(). On the drop path
the freed skb stays linked on port->tx_skbs.
On sparx5, sparx5_fdma_xmit() returns -EINVAL when the TX ring is full:
if (!fdma_db_is_done(fdma_db_get(fdma, fdma->dcb_index, 0)))
return -EINVAL;
On lan969x, lan969x_fdma_xmit() can return -ENOMEM from
pskb_expand_head(GFP_ATOMIC) or on dma_mapping_error().
The next two step xmit on that port, at least SPARX5_PTP_TIMEOUT later,
would reach sparx5_ptp_txtstamp_old_release(). It would read
SPARX5_SKB_CB(skb)->jiffies and skb->next from freed memory, call
__skb_unlink() on it, and call dev_kfree_skb_any() on it again. The PTP
IRQ handlers also walk this list and read cb->ts_id of the freed skb.
With this patch the double-freed entry is also counted in purged, so its
slot gets reclaimed through the double free.
Should the drop path call sparx5_ptp_txtstamp_release() for two step skbs,
as the busy path does?
[Severity: High]
This is a pre-existing issue, but on lan969x, can the FDMA TX ring still
reference the skb freed here?
lan969x_fdma_xmit() stores the two step skb in the ring with
db_buf->skb = skb and db_buf->ptp = true, and the PTP code is responsible
for freeing it. This function and lan969x_ptp_irq_handler() both free the
skb without clearing db->skb or checking whether the DCB has completed.
lan969x_fdma_tx_clear_buf() then dereferences db->skb before it checks
db->ptp:
drivers/net/ethernet/microchip/sparx5/lan969x/lan969x_fdma.c:lan969x_fdma_tx_clear_buf() {
...
db->dev->stats.tx_bytes += db->skb->len;
db->dev->stats.tx_packets++;
sparx5->tx.packets++;
dma_unmap_single(sparx5->dev,
db->dma_addr,
db->skb->len,
DMA_TO_DEVICE);
if (!db->ptp)
napi_consume_skb(db->skb, weight);
...
}
The PTP IRQ is a separate threaded IRQ with no ordering against NAPI. A
delayed NAPI poll could therefore read skb->len from freed memory.
If old_release frees an skb whose DCB has not completed yet, for example
because the FDMA stalled for more than 10 ms, wouldn't the device still
DMA-read the freed buffer?
[Severity: Low]
This is a pre-existing issue, but is a fixed 10 ms SPARX5_PTP_TIMEOUT long
enough here?
A frame held in an egress queue for more than 10 ms, for example behind a
closed TAS gate or by pause frames, would be freed here as lost. When it
later egresses, sparx5_ptp_irq_handler() and lan969x_ptp_irq_handler()
find no matching skb and hit:
if (WARN_ON(!skb_match))
continue;
The continue skips the ptp_skbs decrement, so the counter itself stays
consistent with port->tx_skbs.
[ ... ]
> @@ -236,9 +241,10 @@ int sparx5_ptp_txtstamp_request(struct sparx5_port *port,
> if (rew_op != IFH_REW_OP_TWO_STEP_PTP)
> return 0;
>
> - sparx5_ptp_txtstamp_old_release(port);
> + purged = sparx5_ptp_txtstamp_old_release(port);
[Severity: Medium]
This is a pre-existing issue, and the commit message already mentions it.
Stale skbs, and their global ptp_skbs slots, are only aged when the same
port sends another two step frame.
sparx5_ptp_hwtstamp_set() with HWTSTAMP_TX_OFF does not purge
port->tx_skbs, and neither does port stop. Only sparx5_ptp_deinit() does.
A port that stops sending two step frames, because the link went down or
TX timestamping was turned off, keeps any lost skbs that had not expired
at its last request.
If these leftovers add up to SPARX5_MAX_PTP_ID across ports, two step
xmit on every other port gets -EBUSY. sparx5_port_xmit_impl() turns that
into NETDEV_TX_BUSY.
Could port->tx_skbs also be purged from a path that does not depend on the
port's own traffic, such as HWTSTAMP_TX_OFF or port stop?
>
> spin_lock_irqsave(&sparx5->ptp_ts_id_lock, flags);
> + sparx5->ptp_skbs -= purged;
[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.
> if (sparx5->ptp_skbs == SPARX5_MAX_PTP_ID) {
> spin_unlock_irqrestore(&sparx5->ptp_ts_id_lock, flags);
> return -EBUSY;
[Severity: High]
This isn't a bug introduced by this patch, but a little further down in
sparx5_ptp_txtstamp_request() the skb is added to port->tx_skbs before its
cb fields are written:
skb_queue_tail(&port->tx_skbs, skb);
SPARX5_SKB_CB(skb)->ts_id = port->ts_id;
SPARX5_SKB_CB(skb)->jiffies = jiffies;
skb_queue_tail() takes and releases tx_skbs.lock, and the cb writes happen
under ptp_ts_id_lock only. sparx5_ptp_txtstamp_old_release() and both PTP
IRQ handlers walk the list holding only tx_skbs.lock.
With several TX queues per port, another CPU could run old_release on the
same port and read X's cb->jiffies before it is set. That field still
holds leftover bytes from earlier layers such as the qdisc. If the value
looks expired, would old_release free X while CPU0 is still filling in the
cb and passing X to the injector or FDMA? The IRQ handlers could also
match a stale cb->ts_id the same way.
Would writing ts_id and jiffies before skb_queue_tail() close this?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261029-sparx5-ptp-stale-tx-slot-v1-1-ec15f391bbb7%40microchip.com
^ permalink raw reply [flat|nested] 2+ messages in thread