Re: [PATCH net-next 9/9] net: sparx5/lan969x: add support for changing the MTU

[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: add support for changing the MTU

This commit adds an ndo_change_mtu callback for sparx5 and lan969x ports.
It programs the MAC maximum frame length, resizes the shared FDMA buffers,
and updates the queue watermarks and the TAS guard band. MTU changes are
only allowed when the FDMA is used, and max_mtu is raised to 13982 in that
case.

> The MTU of a port is currently fixed to 1500, as the MAC maximum frame
> length, the FDMA buffers, the queue watermarks and the TAS guard band
> are all sized for a 1518 byte frame.

[Severity: Low]
Is "fixed to 1500" accurate? Before this patch, sparx5_netdev.c had no
ndo_change_mtu. The netdevs come from devm_alloc_etherdev_mqs(), so
ether_setup() gives min_mtu 68 and max_mtu 1500, and __netif_set_mtu()
just does:

net/core/dev.c:__netif_set_mtu() {
    ...
	if (ops->ndo_change_mtu)
		return ops->ndo_change_mtu(dev, new_mtu);

	/* Pairs with all the lockless reads of dev->mtu in the stack */
	WRITE_ONCE(dev->mtu, new_mtu);
    ...
}

So every board accepted any MTU from 68 to 1500.

With the new sparx5_port_change_mtu(), every system using register based
extraction now gets -EOPNOTSUPP for every MTU change. That includes
decreases such as "ip link set ethX mtu 1400", which used to work. These
systems are:

  - Sparx5 rev 0, because sparx5_frame_io_init() only requests the FDMA
    IRQ when GCB_CHIP_ID_REV_ID_GET(chip_id) > 0 || !is_sparx5()
  - boards without an fdma IRQ
  - boards where FDMA init failed

Could the commit message describe this as a removed capability, and give
the actual reason for it (see the sparx5_xtr_grp() comment below)?

> diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_netdev.c b/drivers/net/ethernet/microchip/sparx5/sparx5_netdev.c
> index b3725cf2dd44c..095925752f5f6 100644
> --- a/drivers/net/ethernet/microchip/sparx5/sparx5_netdev.c
> +++ b/drivers/net/ethernet/microchip/sparx5/sparx5_netdev.c
> @@ -250,6 +250,44 @@ static int sparx5_port_hwtstamp_set(struct net_device *dev,
>  	return sparx5_ptp_hwtstamp_set(sparx5_port, cfg, extack);
>  }
>  
> +static int sparx5_port_change_mtu(struct net_device *dev, int new_mtu)
> +{
> +	struct sparx5_port *port = netdev_priv(dev);
> +	struct sparx5 *sparx5 = port->sparx5;
> +	bool resize = new_mtu > dev->mtu;
> +	int err;
> +
> +	/* Register based extraction sizes its skbs from the MTU at the time
> +	 * a frame is read, so it cannot handle frames queued before an MTU
> +	 * decrease.
> +	 */
> +	if (sparx5->fdma_irq <= 0)
> +		return -EOPNOTSUPP;

[Severity: High]
This isn't a bug introduced by this patch, but this check also quietly
closes a heap overflow in register based extraction. That overflow is
still present in current and stable kernels.

sparx5_xtr_grp() in sparx5_packet.c sizes the skb from the current MTU. It
then copies extraction words until EOF without checking tailroom:

drivers/net/ethernet/microchip/sparx5/sparx5_packet.c:sparx5_xtr_grp() {
    ...
	skb = netdev_alloc_skb(netdev, netdev->mtu + ETH_HLEN);
    ...
		default:
			*rxbuf = val;
			byte_cnt += 4;
			rxbuf++;
    ...
	skb_put(skb, byte_cnt - ETH_FCS_LEN);
    ...
}

Without this patch, sparx5_port_init() programs MAC_MAXLEN_CFG with the
fixed ETH_MAXLEN (1518), whatever dev->mtu is set to. There is also no
ndo_change_mtu, so "ip link set ethX mtu 68" succeeds.

After that, a normal 1518 byte frame to the CPU (larger with VLAN tags)
would be copied into an skb with only about 256 bytes of tailroom. Wouldn't
that overrun skb_shared_info and the neighbouring slab objects before
skb_put() reaches skb_over_panic() and BUG() in the xtr IRQ handler?

The commit message presents this check as a feature limit ("frames queued
before an MTU decrease would not fit"). There is no Fixes: tag, so stable
kernels remain exposed.

Would a separate net fix with a Fixes: tag make sense, for example
bounding the copy loop in sparx5_xtr_grp() against the skb tailroom? The
loop itself is unchanged here, so it is still only safe while the MAC
maxlen fits within the slack of an skb sized from dev->mtu.

> +
> +	/* Never let the MAC accept frames larger than the RX buffers */
> +	if (!resize)
> +		sparx5_port_max_len_set(sparx5, port, SPX5_HW_MTU(new_mtu));
> +
> +	err = sparx5->data->ops->fdma_resize(sparx5, port, new_mtu);

[Severity: Medium]
Can an MTU change leak the two-step PTP state on lan969x?

When sparx5_port_change_mtu() resizes the buffers,
lan969x_fdma_resize() pauses and resets the FDMA. Then
lan969x_fdma_tx_reset() walks the TX descriptors that are still marked
used after lan969x_fdma_tx_clear_buf(). Those frames were never
injected, but for a two-step PTP frame the loop only does:

	if (!db->ptp)
		dev_kfree_skb_any(db->skb);

	db->used = false;

The skb was queued on port->tx_skbs by sparx5_ptp_txtstamp_request(),
which also incremented sparx5->ptp_skbs. As the frame never reached the
rewriter, sparx5_ptp_irq_handler() will never get a timestamp for it,
so nothing releases that state.

The skb itself is eventually freed by
sparx5_ptp_txtstamp_old_release() once SPARX5_PTP_TIMEOUT has passed
and another two-step frame is sent. That function does not decrement
sparx5->ptp_skbs, though. So every MTU change under two-step PTP load
can leak slots. Once ptp_skbs reaches SPARX5_MAX_PTP_ID, every further
two-step timestamp request fails with -EBUSY until the driver is
reloaded.

sparx5_ptp_txtstamp_release() fixes up ptp_skbs and unlinks the skb,
but it also decrements port->ts_id. That is only correct for the most
recently requested skb. How should the dropped PTP descriptors be
released here? Would it work to unlink them from port->tx_skbs,
decrement ptp_skbs under ptp_ts_id_lock and then free them? Freeing
them without the unlink would leave a dangling skb on port->tx_skbs.

[Severity: High]
Can the resize path double free an RX page on lan969x?

With ndo_change_mtu, lan969x_fdma_resize() can now run at any time,
and it goes through lan969x_fdma_rx_free() and
lan969x_fdma_free_pages(). That function returns every non-NULL
rx->page[i][j] to rx->page_pool with page_pool_put_full_page().

To make that safe, lan969x_fdma_rx_get_frame() clears
rx->page[fdma->dcb_index][fdma->db_index] before it returns the skb.
The free_page error path does not:

	free_page:
		page_pool_recycle_direct(rx->page_pool, page);

		return NULL;

That path is taken when build_skb() fails under memory pressure, or
when the IFH carries an invalid source port. The page goes back to the
pool, but the slot still points to it. lan969x_fdma_napi_poll() then
breaks out of its loop without advancing fdma->dcb_index, so the DB is
not refilled and the stale pointer stays in place.

A later MTU change on any port, or lan969x_fdma_deinit() on teardown,
would then call page_pool_put_full_page() on a page the pool may
already have handed out again, to another DB or to an skb that is still
in flight in the stack. Wouldn't that corrupt the page_pool and lead to
a use-after-free?

Simply clearing the slot in the free_page path doesn't look sufficient
either. If I read lan969x_fdma_napi_poll() correctly, the next poll
calls lan969x_fdma_rx_get_frame() on the same DB again. It would then
pass a NULL page to page_address(). As the code stands, it re-reads the
page that was just recycled. Should the error path drop the frame,
advance past the DB and let the refill loop replace the page? That way
each slot is always either owned by the ring or NULL.

[Severity: Medium]
Can this resize allocate the whole RX ring with GFP_ATOMIC from sleepable
context?

sparx5_port_change_mtu() runs under RTNL. On Sparx5 the call chain is:

sparx5_port_change_mtu()
  sparx5_fdma_resize()
    sparx5_fdma_rx_alloc(rx_new)
      fdma_dcbs_init()
        sparx5_fdma_rx_dataptr_cb()

For each of the 64 x 15 = 960 buffers, sparx5_fdma_rx_dataptr_cb() does:

	skb = __netdev_alloc_skb(rx->ndev, fdma->db_size, GFP_ATOMIC);

With the new max_mtu of 13982, db_size becomes 14080 bytes. Each skb head
is then an order-2 kmalloc allocation, so the ring needs roughly 15 MB of
GFP_ATOMIC memory in total, and the old ring is still allocated while this
happens.

Without __GFP_DIRECT_RECLAIM this can neither reclaim nor compact. It could
drain the atomic reserves that NAPI refills rely on. It could also fail
with -ENOMEM on fragmented memory where GFP_KERNEL would have succeeded.

Could the RX allocation used for resize take a gfp argument, so that
GFP_KERNEL is used here?

The TX ring in the same path comes from fdma_alloc_phys(), which does a
kzalloc(GFP_KERNEL) of about 900 KB (order-8) without __GFP_NOWARN. A
failure there would also print an allocation failure warning.

> +	if (err) {
> +		if (resize)
> +			return err;
> +
> +		/* The current, larger, buffers still fit all frames */
> +		netdev_warn(dev, "Failed to resize FDMA buffers: %d\n", err);
> +	}

[ ... ]

> @@ -306,7 +345,14 @@ int sparx5_register_netdevs(struct sparx5 *sparx5)
>  
>  	for (portno = 0; portno < sparx5->data->consts->n_ports; portno++)
>  		if (sparx5->ports[portno]) {
> -			err = register_netdev(sparx5->ports[portno]->ndev);
> +			struct net_device *ndev = sparx5->ports[portno]->ndev;
> +
> +			/* The MTU can only be changed when FDMA is used */
> +			if (sparx5->fdma_irq > 0)
> +				ndev->max_mtu = SPX5_MAX_FRAME_LEN - ETH_HLEN -
> +						ETH_FCS_LEN;

[Severity: High]
This isn't a bug introduced by this patch, but raising max_mtu here makes
an existing RX refill problem much more likely to trigger.

sparx5_fdma_napi_callback() ignores the return value of fdma_dcb_add():

	fdma_dcb_add(fdma, fdma->dcb_index,
		     FDMA_DCB_INFO_DATAL(fdma->db_size),
		     FDMA_DCB_STATUS_INTR);
	fdma_db_reset(fdma);
	fdma_dcb_advance(fdma);

lan969x_fdma_napi_poll() does the same in its refill loop.

If the dataptr callback fails for any DB, __fdma_dcb_add() returns early:

	for (i = 0; i < fdma->n_dbs; i++) {
		err = __fdma_db_add(fdma, dcb_idx, i, status, db_cb);
		if (unlikely(err))
			return err;
	}

At that point the DB's status has already been reset (DONE cleared), but
the DCB has not been linked after fdma->last_dcb. The next successful
fdma_dcb_add() then links DCB k-1 directly to DCB k+1, so the hardware
never visits DCB k again.

Once the software dcb_index wraps back to k, wouldn't
sparx5_fdma_rx_get_frame() (or fdma_has_frames() on lan969x) keep
returning false? That would stop CPU RX on the shared FDMA for all ports
until another resize or a driver reload.

Before this series, a Sparx5 refill was roughly a 4 KB request and a
lan969x refill was order-0. With max_mtu at 13982, each Sparx5 refill
becomes an order-2 GFP_ATOMIC allocation, and lan969x page_pool pages
become order-2. Those allocations can realistically fail in NAPI context
on a fragmented system.

Should refill failures be handled, for example by retrying the refill
later, before jumbo MTUs are allowed?

> +
> +			err = register_netdev(ndev);
>  			if (err) {
>  				dev_err(sparx5->dev,
>  					"port: %02u: netdev registration failed\n",