[PATCH net] net: sparx5: don't reuse the last DCB in fdma xmit

[PATCH net] net: sparx5: don't reuse the last DCB in fdma xmit

[Daniel Machon]

sparx5_fdma_xmit() advances dcb_index to the next tx DCB and, if the
FDMA is done with it, adds it to the end of the chain, which makes it
the new fdma->last_dcb.

Before this patch, sparx5_fdma_xmit() can pick last_dcb itself. A
failed transmit advances dcb_index without adding a DCB, so dcb_index
no longer follows last_dcb, and after a burst of failures it can land
on it. Re-adding last_dcb loses the frame, because the FDMA has already
passed the tail of the chain.

Also reject the DCB when it is the last one in the chain, using the
same !fdma_is_last() guard that lan966x and lan969x use to pick their
next tx DCB, so the tail is never re-added.

Found by an AI code review agent and confirmed by code inspection; not
reproduced. Tested on sparx5 that traffic still passes, with ping floods
in both directions.

Fixes: f4aa7e361ae2 ("net: sparx5: use FDMA library for adding DCB's in the tx path")
Signed-off-by: Daniel Machon <daniel.machon@microchip.com>
---
 drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c b/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
index dbe86f937b21..221b2f1932f6 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
@@ -218,7 +218,8 @@ int sparx5_fdma_xmit(struct sparx5 *sparx5, u32 *ifh, struct sk_buff *skb,
 	void *virt_addr;
 
 	fdma_dcb_advance(fdma);
-	if (!fdma_db_is_done(fdma_db_get(fdma, fdma->dcb_index, 0)))
+	if (!fdma_db_is_done(fdma_db_get(fdma, fdma->dcb_index, 0)) ||
+	    fdma_is_last(fdma, fdma_dcb_next_get(fdma)))
 		return -EINVAL;
 
 	/* Get the virtual address of the dataptr for the next DB */

---
base-commit: 07bd391eb9572e74f073c49acef327611234f9bb
change-id: 20261019-sparx5-fdma-last-dcb-e2e8491b7d86

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

Re: [PATCH net] net: sparx5: don't reuse the last DCB in fdma xmit

[netdev-bot+sashiko]

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

Critical: 0 (1) · High: 1 · Medium: 0 · 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).

- [High] net: sparx5: unguarded fdma_dcb_advance() in sparx5_fdma_xmit()
  breaks tx ring

Pre-existing issues:
- [Critical] This problem wasn't introduced by this patch, but the patch
  adds a new way to reach it.

--

net: sparx5: don't reuse the last DCB in fdma xmit

This patch adds a check to sparx5_fdma_xmit() so it rejects the next tx
DCB when that DCB is fdma->last_dcb. This uses the same !fdma_is_last()
guard as lan966x and lan969x, so the tail of the chain is never re-added
and the frame is not lost.

> diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c b/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
> index dbe86f937b21..221b2f1932f6 100644
> --- a/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
> +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
> @@ -218,7 +218,8 @@ int sparx5_fdma_xmit(struct sparx5 *sparx5, u32 *ifh, struct sk_buff *skb,
>  	void *virt_addr;
>  
>  	fdma_dcb_advance(fdma);
> -	if (!fdma_db_is_done(fdma_db_get(fdma, fdma->dcb_index, 0)))
> +	if (!fdma_db_is_done(fdma_db_get(fdma, fdma->dcb_index, 0)) ||
> +	    fdma_is_last(fdma, fdma_dcb_next_get(fdma)))
>  		return -EINVAL;

[Severity: High]
Does this fully fix the drift described in the commit message? The
underlying problem looks like it is still there: fdma_dcb_advance() runs
before the done check, and fdma->dcb_index is not restored when
sparx5_fdma_xmit() returns -EINVAL. The new fdma_is_last() test only
covers the moment dcb_index lands exactly on last_dcb.

When the ring is full, a failed xmit leaves dcb_index on the oldest
in-flight DCB. The next call advances again before checking anything,
so if the FDMA has completed that DCB in the meantime it is skipped and
we test a newer DCB that is still in flight. Each failure moves
dcb_index one step further, so a burst can keep dropping frames while
completed DCBs sit unused behind dcb_index.

If dcb_index ends up just behind last_dcb and the FDMA then drains the
chain, every DCB is done and the ring is idle. The next xmit advances
onto last_dcb, hits the new fdma_is_last() check and returns -EINVAL,
which sparx5_port_xmit_impl() treats as a drop:

	if (ret == -EBUSY)
		goto busy;
	if (ret < 0)
		goto drop;

The lost frame becomes a counted drop, but it is still a drop on an
empty ring. lan966x_fdma_get_next_dcb() and lan969x_fdma_get_next_dcb()
don't hit this because they use !fdma_is_last() inside a loop over all
DCBs and pick another free one, rather than failing on a single index.

The previous tail is also still unsafe. Say the FDMA has halted on
last_dcb at index L and dcb_index is at L - 2. The first frame advances
to L - 1, which is done and not last, so it is linked from
dcbs[L].nextptr and becomes the new last_dcb. __fdma_dcb_add() does not
clear the done bit on dcbs[L], so if a second frame comes in before the
FDMA has fetched dcbs[L].nextptr after the reload, it advances onto L,
which is still done and no longer last. Re-adding it rewrites
dcbs[L].nextptr to FDMA_DCB_INVALID_DATA, so dcbs[L - 1] is cut out of
the chain. Its frame is lost, and since the FDMA never visits it again
its done bit stays clear, so later xmits that land on it fail too.

The drift itself dates back to f4aa7e361ae2, but since this patch is
meant to fix it, would it be cleaner to fix it at the source, by only
committing the dcb_index advance once the DCB has been accepted (or
rolling it back on failure)? Then dcb_index always follows last_dcb,
the next DCB is always the oldest one, and the fdma_is_last() check
should not be needed. Alternatively, sparx5 could search the ring the
way lan969x_fdma_get_next_dcb() does.

[Severity: Critical]
This is a pre-existing issue, but this patch adds a new way to reach it.
What happens to a two-step PTP skb when this returns -EINVAL?

For a two-step PTP skb, sparx5_port_xmit_impl() first calls
sparx5_ptp_txtstamp_request(). That function links the skb onto
port->tx_skbs and does not take an extra reference:

drivers/net/ethernet/microchip/sparx5/sparx5_ptp.c:sparx5_ptp_txtstamp_request() {
    ...
	skb_queue_tail(&port->tx_skbs, skb);
	SPARX5_SKB_CB(skb)->ts_id = port->ts_id;
	SPARX5_SKB_CB(skb)->jiffies = jiffies;

	sparx5->ptp_skbs++;
    ...
}

When ops->fdma_xmit() returns -EINVAL, the caller jumps to the drop label.
Only the busy label calls sparx5_ptp_txtstamp_release():

drivers/net/ethernet/microchip/sparx5/sparx5_packet.c:sparx5_port_xmit_impl() {
    ...
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;
}

That leaves the freed skb linked on port->tx_skbs. On the next two-step
request, sparx5_ptp_txtstamp_old_release() reads the freed skb's cb and
then frees it a second time:

drivers/net/ethernet/microchip/sparx5/sparx5_ptp.c:sparx5_ptp_txtstamp_old_release() {
    ...
		if time_after(SPARX5_SKB_CB(skb)->jiffies + SPARX5_PTP_TIMEOUT,
			      jiffies)
			break;

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

sparx5_ptp_irq_handler() walks the same queue. It also reads
SPARX5_SKB_CB(skb)->ts_id from the freed skb. On this path the ptp_skbs
slot is never given back either.

The drop path has been like this since commit 121c6672b0191. Until now it
could only be reached through the !fdma_db_is_done() (ring full) case.
Before this patch, the case where dcb_index landed on last_dcb returned
NETDEV_TX_OK, so the PTP skb stayed queued safely. With the new
fdma_is_last() rejection, that case now goes through the same drop path.

Could the drop path call sparx5_ptp_txtstamp_release() for two-step PTP
skbs, the same way the busy path does?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261020-sparx5-fdma-last-dcb-v1-1-fb9a956196dd%40microchip.com

Re: [PATCH net] net: sparx5: don't reuse the last DCB in fdma xmit

[Daniel Machon]

> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 (1) · High: 1 · Medium: 0 · 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).
> 
> - [High] net: sparx5: unguarded fdma_dcb_advance() in sparx5_fdma_xmit()
>   breaks tx ring
> 
> Pre-existing issues:
> - [Critical] This problem wasn't introduced by this patch, but the patch
>   adds a new way to reach it.
> 
> --
> 
> net: sparx5: don't reuse the last DCB in fdma xmit
> 
> This patch adds a check to sparx5_fdma_xmit() so it rejects the next tx
> DCB when that DCB is fdma->last_dcb. This uses the same !fdma_is_last()
> guard as lan966x and lan969x, so the tail of the chain is never re-added
> and the frame is not lost.
> 
> > diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c b/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
> > index dbe86f937b21..221b2f1932f6 100644
> > --- a/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
> > +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
> > @@ -218,7 +218,8 @@ int sparx5_fdma_xmit(struct sparx5 *sparx5, u32 *ifh, struct sk_buff *skb,
> >       void *virt_addr;
> >
> >       fdma_dcb_advance(fdma);
> > -     if (!fdma_db_is_done(fdma_db_get(fdma, fdma->dcb_index, 0)))
> > +     if (!fdma_db_is_done(fdma_db_get(fdma, fdma->dcb_index, 0)) ||
> > +         fdma_is_last(fdma, fdma_dcb_next_get(fdma)))
> >               return -EINVAL;
> 
> [Severity: High]
> Does this fully fix the drift described in the commit message? The
> underlying problem looks like it is still there: fdma_dcb_advance() runs
> before the done check, and fdma->dcb_index is not restored when
> sparx5_fdma_xmit() returns -EINVAL. The new fdma_is_last() test only
> covers the moment dcb_index lands exactly on last_dcb.
> 
> When the ring is full, a failed xmit leaves dcb_index on the oldest
> in-flight DCB. The next call advances again before checking anything,
> so if the FDMA has completed that DCB in the meantime it is skipped and
> we test a newer DCB that is still in flight. Each failure moves
> dcb_index one step further, so a burst can keep dropping frames while
> completed DCBs sit unused behind dcb_index.
> 
> If dcb_index ends up just behind last_dcb and the FDMA then drains the
> chain, every DCB is done and the ring is idle. The next xmit advances
> onto last_dcb, hits the new fdma_is_last() check and returns -EINVAL,
> which sparx5_port_xmit_impl() treats as a drop:
> 
>         if (ret == -EBUSY)
>                 goto busy;
>         if (ret < 0)
>                 goto drop;
> 
> The lost frame becomes a counted drop, but it is still a drop on an
> empty ring. lan966x_fdma_get_next_dcb() and lan969x_fdma_get_next_dcb()
> don't hit this because they use !fdma_is_last() inside a loop over all
> DCBs and pick another free one, rather than failing on a single index.
> 
> The previous tail is also still unsafe. Say the FDMA has halted on
> last_dcb at index L and dcb_index is at L - 2. The first frame advances
> to L - 1, which is done and not last, so it is linked from
> dcbs[L].nextptr and becomes the new last_dcb. __fdma_dcb_add() does not
> clear the done bit on dcbs[L], so if a second frame comes in before the
> FDMA has fetched dcbs[L].nextptr after the reload, it advances onto L,
> which is still done and no longer last. Re-adding it rewrites
> dcbs[L].nextptr to FDMA_DCB_INVALID_DATA, so dcbs[L - 1] is cut out of
> the chain. Its frame is lost, and since the FDMA never visits it again
> its done bit stays clear, so later xmits that land on it fail too.
> 
> The drift itself dates back to f4aa7e361ae2, but since this patch is
> meant to fix it, would it be cleaner to fix it at the source, by only
> committing the dcb_index advance once the DCB has been accepted (or
> rolling it back on failure)? Then dcb_index always follows last_dcb,
> the next DCB is always the oldest one, and the fdma_is_last() check
> should not be needed. Alternatively, sparx5 could search the ring the
> way lan969x_fdma_get_next_dcb() does.
>

The drift is pre-existing and not what I intended to fix. That said, I think I
can fix both with a simple change where the ring index is computed from the last
index instead.

pw-bot: cr