Re: [PATCH net-next v8 15/15] misc: lan966x-pci: dts: add fdma interrupt to overlay

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 2 · 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] PCIe FDMA init races with a concurrent MTU change on netdevices
  that are already registered.
- [High] Removing a PCIe lan966x can deadlock against a concurrent MTU
  change.

--

misc: lan966x-pci: dts: add fdma interrupt to overlay

This patch adds the fdma interrupt (OIC interrupt 14) to the lan966x PCI
device tree overlay. With the "fdma" named interrupt present,
platform_get_irq_byname() succeeds and lan966x->fdma becomes true for the
PCIe card, which turns on the new PCIe FDMA init, resize and deinit paths.

> diff --git a/drivers/misc/lan966x_pci.dtso b/drivers/misc/lan966x_pci.dtso
> index 7bb726550cafd..5bb12dbc08431 100644
> --- a/drivers/misc/lan966x_pci.dtso
> +++ b/drivers/misc/lan966x_pci.dtso
> @@ -141,8 +141,9 @@ switch: switch@e0000000 {
>  
>  					interrupt-parent = <&oic>;
>  					interrupts = <12 IRQ_TYPE_LEVEL_HIGH>,
> +						     <14 IRQ_TYPE_LEVEL_HIGH>,
>  						     <9 IRQ_TYPE_LEVEL_HIGH>;
> -					interrupt-names = "xtr", "ana";
> +					interrupt-names = "xtr", "fdma", "ana";

[Severity: High]
This overlay change makes the PCIe FDMA path reachable. Can
lan966x_fdma_pci_init() race with an MTU change on a port that is already
registered?

In lan966x_probe(), the port netdevs are registered and NAPI is enabled
from inside the port loop:

lan966x_probe()
  lan966x_probe_port()
    register_netdev(dev)
  lan966x_port_init()
    lan966x_fdma_netdev_init()

Only after that loop does probe call lan966x->ops->fdma_init(), and it
does so without RTNL.

The only thing guarding the resize path is a plain pointer check in
lan966x_fdma_pci_resize():

	if (!lan966x->rx.lan966x)
		return 0;

However, lan966x_fdma_pci_init() sets that pointer first, before
rx.max_mtu, before tx.lan966x and before the rings are allocated:

	lan966x->rx.lan966x = lan966x;
	lan966x->rx.max_mtu = lan966x_fdma_get_max_frame(lan966x);
	...
	lan966x->tx.lan966x = lan966x;
	...
	err = lan966x_fdma_pci_rx_alloc(&lan966x->rx);
	...
	err = lan966x_fdma_pci_tx_alloc(&lan966x->tx);

If an RTM_SETLINK MTU change lands in that window (for example a network
manager configuring the new netdev), it would take this path:

lan966x_port_change_mtu()
  lan966x_fdma_pci_resize()
    __lan966x_fdma_pci_reload()
      lan966x_fdma_pci_reload()

lan966x_fdma_pci_reload() snapshots the half-built rings into
rx_fdma_old/tx_fdma_old. It then disables NAPI and the channels, and
allocates and frees rings in the same lan966x->rx.fdma/tx.fdma fields that
probe is still filling in.

If probe has not yet set tx.lan966x, lan966x_fdma_tx_disable() would pass
a NULL lan966x to lan_rmw(). If probe is past that point, both threads
would allocate, overwrite and free the same ring and ATU region fields at
the same time. Could that leak the rings and ATU windows, or free memory
that probe or the FDMA engine is still using?

The commit that adds PCIe FDMA MTU change support says the resize is
skipped "until lan966x_fdma_pci_init() has built the rings". The flag,
though, is set at the start of init rather than after the rings exist.
Would it be better to set it last, or to serialize init against
ndo_change_mtu, for example by holding RTNL?

[Severity: High]
Also enabled by this change: can lan966x_remove() deadlock against a
concurrent MTU change?

lan966x_remove() tears down the FDMA before the netdevs are unregistered.
It does not take RTNL and does not call netif_device_detach():

	lan966x->ops->fdma_deinit(lan966x);
	lan966x_cleanup_ports(lan966x);

lan966x_fdma_pci_deinit() disables NAPI and frees the rings. It leaves
rx.lan966x set:

	if (lan966x->fdma_ndev)
		napi_disable(&lan966x->napi);
	...
	fdma_free_coherent_and_unmap(lan966x->dma_dev, &lan966x->rx.fdma);
	fdma_free_coherent_and_unmap(lan966x->dma_dev, &lan966x->tx.fdma);

The ports stay registered and present until each unregister_netdev() in
lan966x_cleanup_ports() finishes. During that time, an MTU change that
alters the max frame size would still pass the check in
lan966x_fdma_pci_resize() and reach lan966x_fdma_pci_reload(), which
calls napi_disable() a second time with RTNL held:

	napi_disable(&lan966x->napi);

The first disable leaves NAPIF_STATE_SCHED set. napi_disable_locked()
therefore waits forever:

	while (val & (NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC)) {
		usleep_range(20, 200);
		val = READ_ONCE(n->state);
	}

In that case the remove task would then block in unregister_netdev() on
rtnl_lock(), and so would every other RTNL user. If the second
napi_disable() did return, the reload would go on to free
rx_fdma_old/tx_fdma_old, which deinit has already freed.

lan966x_shutdown() in this series already guards against this. It takes
rtnl_lock() and calls netif_device_detach() on each port. The shutdown
commit says this is done "so ndo_open/ndo_change_mtu cannot re-enter the
FDMA against a disabled NAPI".

Does lan966x_remove() need the same protection before calling
fdma_deinit(), or should deinit clear rx.lan966x so that later resizes
are skipped?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260924-lan966x-pci-fdma-v8-0-201c7b707d8b%40microchip.com