Re: [PATCH net-next v6 09/14] net: lan966x: add shutdown callback to stop FDMA on reboot

[sashiko-bot@kernel.org]

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

New issues:
- [High] Early return in `lan966x_shutdown` bypasses required `ANA_ANAINTR` masking for non-FDMA configurations.
- [High] Concurrent XDP redirect operations can re-arm the FDMA TX channel after `lan966x_shutdown` has disabled it.

Pre-existing issues:
- [High] Missing `dma_wmb()` and incorrect ordering when appending a DCB to the FDMA ring.
- [High] Teardown race in `lan966x_remove` causing use-after-free and double-free with concurrent `ndo_change_mtu`.
--

commit 3c2ffa8799eedb58249177a7fb0968ccfc34255a
Author: Daniel Machon <daniel.machon@microchip.com>

net: lan966x: add shutdown callback to stop FDMA on reboot

This commit adds a shutdown callback to disable NAPI, stop TX queues,
and disable RX/TX channels. It also masks interrupts to prevent the
FDMA engine from asserting the shared PCIe INTx line across a host reboot.

> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> index 11094a381ec2a..d6ce1e3e373fd 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
[ ... ]
> @@ -1324,9 +1324,35 @@ static void lan966x_remove(struct platform_device *pdev)
>  	debugfs_remove_recursive(lan966x->debugfs_root);
>  }

[Severity: High]
This is a pre-existing issue, but does this teardown sequence in
lan966x_remove() contain a use-after-free or double-free race?

It appears that lan966x->ops->fdma_deinit(lan966x) is called to free the DMA
memory before lan966x_cleanup_ports() unregisters the netdevs, all without
holding rtnl_lock().

Can a concurrent "ip link set mtu" trigger ndo_change_mtu() and race to free
and reallocate the exact same coherent DMA memory?

> +static void lan966x_shutdown(struct platform_device *pdev)
> +{
> +	struct lan966x *lan966x = platform_get_drvdata(pdev);
> +
> +	if (!lan966x->fdma)
> +		return;
> +

[Severity: High]
Does this early return bypass the required ANA_ANAINTR masking for non-FDMA
configurations?

If the switch is used on PCIe without FDMA (register-based I/O), this will
return early because !lan966x->fdma, failing to mask ANA_ANAINTR which is
armed unconditionally.

Will the shared INTx line remain asserted across a warm reboot, causing a host
interrupt storm?

> +	/* The reload paths disable this NAPI under rtnl; serialize with them. */
> +	rtnl_lock();
> +
> +	if (lan966x->fdma_ndev)
> +		napi_disable(&lan966x->napi);
> +
> +	lan966x_fdma_tx_disable_netdev(lan966x);
> +
> +	lan966x_fdma_rx_disable(&lan966x->rx);
> +	lan966x_fdma_tx_disable(&lan966x->tx);
> +
> +	lan_wr(0, lan966x, FDMA_INTR_ENA);
> +	lan_wr(0, lan966x, FDMA_INTR_DB_ENA);

[Severity: High]
Can concurrent XDP redirect operations re-arm the FDMA TX channel after it
has been disabled here?

lan966x_shutdown() disables the TX channel and clears tx->activated. However,
if a concurrent XDP_REDIRECT from another device calls ndo_xdp_xmit() ->
lan966x_fdma_xmit_xdpf() -> lan966x_fdma_tx_start(), it will see tx->activated
is false and take the activation path:

drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c:lan966x_fdma_tx_start() {
	if (likely(lan966x->tx.activated)) {
		lan966x_fdma_tx_reload(tx);
	} else {
		/* Because it is first time, then just activate */
		lan966x->tx.activated = true;
		lan966x_fdma_tx_activate(tx);
	}
}

Since ndo_xdp_xmit() runs without rtnl_lock() and ignores stopped netdev
queues, will it actively re-arm FDMA_INTR_DB_ENA and FDMA_CH_ACTIVATE
post-shutdown, leaving hardware channels active across a warm reboot and
causing memory corruption?

> +	lan_wr(0, lan966x, ANA_ANAINTR);
> +
> +	rtnl_unlock();
> +}

[Severity: High]
This isn't a bug introduced by this patch, but is there a missing dma_wmb()
and incorrect ordering when appending a DCB to the FDMA ring?

In __fdma_dcb_add(), the new DCB is linked into the active hardware ring
before its struct fields are initialized:

drivers/net/ethernet/microchip/fdma/fdma_api.c:__fdma_dcb_add() {
	err = dcb_cb(fdma, dcb_idx, &fdma->last_dcb->nextptr);
	if (unlikely(err))
		return err;

	fdma->last_dcb = dcb;

	dcb->nextptr = FDMA_DCB_INVALID_DATA;
	dcb->info = info;
}

Because dcb_cb() writes to the previous DCB's nextptr without a memory
barrier, can the hardware engine immediately follow the updated nextptr and
read the uninitialized new DCB, leading to DMA faults or memory corruption?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909-lan966x-pci-fdma-v6-0-6f48dab9d671@microchip.com?part=9