Re: [PATCH net-next 5/9] net: sparx5/lan969x: size the FDMA buffers from the port MTUs

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: sparx5/lan969x: size the FDMA buffers from the port MTUs

This commit stops using a fixed single page for the Sparx5 and lan969x
FDMA buffers. They are now sized from the largest port MTU plus the
Ethernet header, FCS, IFH and two VLAN tags. On Sparx5 the TX buffers can
now be smaller than a page, so sparx5_fdma_xmit() drops frames that do
not fit; on lan969x the page_pool page order comes from the max frame
size.

> diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c b/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
> index 7ac5887abe6ee..3a74a305d55d4 100644
> --- a/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c
> +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_fdma.c

[ ... ]

> @@ -218,6 +217,10 @@ int sparx5_fdma_xmit(struct sparx5 *sparx5, u32 *ifh, struct sk_buff *skb,
>  	struct fdma *fdma = &tx->fdma;
>  	void *virt_addr;
>  
> +	/* Drop frames that do not fit in the TX buffers */
> +	if (unlikely(IFH_LEN * 4 + skb->len + ETH_FCS_LEN > fdma->db_size))
> +		return -EMSGSIZE;
> +

[Severity: High]
This is a pre-existing issue, but this new -EMSGSIZE return gives it a new
trigger. What happens to a two-step PTP skb that gets dropped here?

By the time sparx5_fdma_xmit() runs, sparx5_port_xmit_impl() has already
called sparx5_ptp_txtstamp_request(). That function puts the skb on
port->tx_skbs without taking a reference and increments ptp_skbs:

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

Any negative return other than -EBUSY goes to the drop label. The drop
label frees the skb but does not do the cleanup that the busy label does:

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

Doesn't this leave a freed skb linked on port->tx_skbs? Three later
accesses would touch it:

- On the next two-step TX, sparx5_ptp_txtstamp_old_release() reads
  SPARX5_SKB_CB(skb)->jiffies from it, then calls __skb_unlink() and
  dev_kfree_skb_any() on it a second time.

- The two-step timestamp IRQ handler walks the same list and reads
  SPARX5_SKB_CB(skb)->ts_id from it.

- Unlinking a neighbouring skb writes into the freed skb's next/prev
  pointers.

Each of these drops also seems to leak one ptp_skbs slot. After
SPARX5_MAX_PTP_ID leaked slots, sparx5_ptp_txtstamp_request() returns
-EBUSY for every request, and two-step PTP TX stalls with NETDEV_TX_BUSY.

The broken drop-path cleanup is older than this patch. The existing
-EINVAL return just below (TX DB not done) and errors from
sparx5_inject() already reach the same drop label.

What this patch changes is the frame size that triggers it. At MTU 1500
the TX DB shrinks from 4096 bytes to ALIGN(1562, 128) = 1664. A two-step
PTP skb longer than 1624 bytes now takes this path. Before the patch,
frames up to about 4060 bytes were sent normally.

The later commit "net: sparx5/lan969x: add support for changing the MTU"
leaves sparx5_port_xmit_impl() and sparx5_fdma_xmit() as they are. In that
commit, lowering the MTU shrinks db_size through sparx5_fdma_resize(),
while skbs sized for the old MTU may still be queued.

Would it make sense to do the size check before
sparx5_ptp_txtstamp_request() is called? Another option is to call
sparx5_ptp_txtstamp_release() for two-step skbs on the drop path, as the
busy path already does.

>  	if (!fdma_db_is_done(fdma_db_get(fdma, fdma->dcb_index, 0)))
>  		return -EINVAL;
>  

[ ... ]