Re: [PATCH net-next v9 12/15] net: lan966x: add PCIe FDMA MTU change support

[Daniel Machon]

> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 (1) · Medium: 0 · Low: 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).
> 
> - [High] lan966x_fdma_pci_resize() uses `lan966x->rx.lan966x` to decide
>   whether the rings exist, and that field does not track ring state.
> - [Low] lan966x_fdma_pci_reload() frees and ATU-unmaps the old rings and
>   reprograms LLP right after lan966x_fdma_rx_disable() and…
> 
> Pre-existing issues:
> - [High] This problem wasn't introduced by this patch, but the patch
>   explicitly acknowledges the hazard and fixes only the PCIe side.
> 
> --
> 
> net: lan966x: add PCIe FDMA MTU change support
> 
> This adds MTU change support to the lan966x PCIe FDMA path. On an MTU
> change, the contiguous ATU-mapped RX and TX rings are reallocated at the
> new size, and the old buffers are kept if allocation fails. The PCIe DCB
> ring is capped at 256, FDMA_PCI_MAX_MTU is advertised as max_mtu when the
> FDMA is in use, and the resize is skipped until lan966x_fdma_pci_init()
> has built the rings.
> 
> > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma_pci.c b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma_pci.c
> > index f511e70613141..758554c951c52 100644
> > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma_pci.c
> > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma_pci.c
> 
> [ ... ]
> 
> > +static int lan966x_fdma_pci_reload(struct lan966x *lan966x, int new_mtu)
> > +{
> > +	struct fdma tx_fdma_old = lan966x->tx.fdma;
> > +	struct fdma rx_fdma_old = lan966x->rx.fdma;
> > +	u32 old_mtu = lan966x->rx.max_mtu;
> > +	int err;
> > +
> > +	napi_disable(&lan966x->napi);
> > +	lan966x_fdma_tx_disable_netdev(lan966x);
> > +	lan966x_fdma_rx_disable(&lan966x->rx);
> > +	lan966x_fdma_tx_disable(&lan966x->tx);
> 
> [Severity: Low]
> lan966x_fdma_rx_disable() and lan966x_fdma_tx_disable() both throw away
> the result of the FDMA_CH_ACTIVE poll:
> 
> 	readx_poll_timeout_atomic(lan966x_fdma_channel_active, lan966x,
> 				  val, !(val & BIT(fdma->channel_id)),
> 				  READL_SLEEP_US, READL_TIMEOUT_US);
> 
> __lan966x_fdma_pci_reload() also ignores the result of the CPU queue drain
> poll. After that, the old rings are freed and ATU-unmapped straight away.
> 
> If a channel is still active when the poll times out, could it follow the
> old DCB chain into memory that has already been unmapped and freed?
> 
> The timeout is READL_TIMEOUT_US (100 seconds), so this would need wedged
> hardware. The platform lan966x_fdma_reload() uses the same sequence. It
> might still be worth checking the poll results and bailing out before
> freeing the old rings.
> 
> > +
> > +	lan966x->rx.max_mtu = new_mtu;
> 
> [ ... ]
> 
> > +	/* Free and unmap old memory. */
> > +	fdma_free_coherent_and_unmap(lan966x->dma_dev, &rx_fdma_old);
> > +	fdma_free_coherent_and_unmap(lan966x->dma_dev, &tx_fdma_old);
> 
> [ ... ]
> 
> >  static int lan966x_fdma_pci_resize(struct lan966x *lan966x)
> >  {
> > -	return -EOPNOTSUPP;
> > +	int max_mtu;
> > +
> > +	/* Nothing to resize until fdma_pci_init() has built the rings; it
> > +	 * sizes them from DEV_MAC_MAXLEN_CFG, which the caller already set.
> > +	 */
> > +	if (!lan966x->rx.lan966x)
> > +		return 0;
> 
> [Severity: High]
> Does rx.lan966x actually show whether the rings have been built?
> 
> In lan966x_fdma_pci_init() it is set before either ring is allocated or
> ATU-mapped:
> 
> 	lan966x->rx.lan966x = lan966x;
> 	lan966x->rx.max_mtu = lan966x_fdma_get_max_frame(lan966x);
> 	...
> 	err = lan966x_fdma_pci_rx_alloc(&lan966x->rx);
> 	if (err)
> 		return err;
> 
> It is not cleared on the init error paths or in
> lan966x_fdma_pci_deinit(). Init and deinit both run without RTNL while the
> port netdevs are registered, so ndo_change_mtu can reach this check during
> those windows.
> 
> First, lan966x_remove() tears down the FDMA before the netdevs are
> unregistered:
> 
> 	lan966x->ops->fdma_deinit(lan966x);
> 	lan966x_cleanup_ports(lan966x);
> 
> lan966x_fdma_pci_deinit() calls napi_disable() and frees both rings. An
> MTU change in that window passes this check and reaches
> lan966x_fdma_pci_reload(), which calls napi_disable() a second time.
> 
> Would napi_disable_locked() then spin forever on NAPIF_STATE_SCHED |
> NAPIF_STATE_NPSVC with RTNL held? That would leave unregister_netdev() and
> every other RTNL user blocked. The probe unwind at cleanup_fdma has the
> same ordering.
> 
> Second, if lan966x_fdma_pci_tx_alloc() fails in init, the RX ring is freed.
> However, rx.fdma.dcbs still points at it and rx.lan966x stays set. The
> probe unwind jumps to cleanup_ptp and skips fdma_deinit, so the netdevs
> stay registered until lan966x_cleanup_ports(). An MTU change in that
> window runs lan966x_fdma_pci_reload(), which calls:
> 
> 	fdma_free_coherent_and_unmap(lan966x->dma_dev, &rx_fdma_old);
> 
> Can this free the stale RX buffer a second time? The newly allocated,
> ATU-mapped rings would then leak while the FDMA runs on them.
> 
> The same thing looks possible when the ATU map fails inside
> lan966x_fdma_pci_rx_alloc(), because fdma_alloc_coherent_and_map() frees
> dcbs without clearing the pointer.
> 
> Third, lan966x_probe() registers every port netdev in lan966x_probe_port()
> before ops->fdma_init() runs. A concurrent MTU change, for example udev
> applying MTUBytes, could pass this check while init is still sleeping in
> dma_alloc_coherent() inside rx_alloc or tx_alloc.
> 
> Reload would then snapshot a half-built struct fdma, clear atu_region, and
> rewrite db_size and size while init is still filling them in. Could that
> leak or double-map ATU regions, or leave LLP pointing at freed memory?
> 
> The commit message says:
> 
>   Skip the resize until lan966x_fdma_pci_init() has built the rings;
> 
> The check does not seem to do that. The resize, init and deinit paths are
> also unchanged at the end of the series.

I dismissed this issue in v7, and Simon gave his RB. In short, all these windows
come from the probe init order and the netdev registration.

> 
> > +
> > +	max_mtu = lan966x_fdma_get_max_frame(lan966x);
> > +	if (max_mtu == lan966x->rx.max_mtu)
> > +		return 0;
> > +
> > +	return __lan966x_fdma_pci_reload(lan966x, max_mtu);
> >  }
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but the platform backend has
> the same pre-init window, and the guard added here only covers PCIe.
> 
> lan966x_fdma_change_mtu() in lan966x_fdma.c has no equivalent check:
> 
> 	max_mtu = lan966x_fdma_get_max_frame(lan966x);
> 	if (max_mtu == lan966x->rx.max_mtu)
> 		return 0;
> 
> 	return __lan966x_fdma_reload(lan966x, max_mtu);
> 
> rx.max_mtu is still 0 before lan966x_fdma_init() runs. So an MTU change
> after register_netdev() in lan966x_probe_port() always continues down:
> 
> lan966x_port_change_mtu()->lan966x_fdma_change_mtu()->
>     __lan966x_fdma_reload()->lan966x_fdma_reload()
> 
> >From there, lan966x_fdma_rx_disable() does:
> 
> 	struct lan966x *lan966x = rx->lan966x;
> 	...
> 	lan_rmw(FDMA_CH_DISABLE_CH_DISABLE_SET(BIT(fdma->channel_id)),
> 		FDMA_CH_DISABLE_CH_DISABLE,
> 		lan966x, FDMA_CH_DISABLE);
> 
> rx->lan966x is still NULL at that point. Can this oops under RTNL?
> 
> Would a check in the common caller, or an equivalent guard in
> lan966x_fdma_change_mtu(), cover both backends?
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928-lan966x-pci-fdma-v9-0-a17caf60f89a%40microchip.com