Re: [PATCH net-next v3 05/13] net: lan966x: export FDMA helpers for reuse

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Data race and Use-After-Free during driver teardown due to lockless state modification and incorrect ordering
- [High] Unsafe DMA teardown due to ignored timeout in lan966x_fdma_tx_disable() and lan966x_fdma_rx_disable()
--

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

net: lan966x: export FDMA helpers for reuse

Make shared FDMA helpers non-static, so they can be reused by the PCIe
FDMA implementation.

> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
> index 6c5761e886d47..25e673bdf0841 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
[ ... ]
> @@ -171,7 +171,7 @@ static void lan966x_fdma_rx_start(struct lan966x_rx *rx)
>  		lan966x, FDMA_CH_ACTIVATE);
>  }
>  
> -static void lan966x_fdma_rx_disable(struct lan966x_rx *rx)
> +void lan966x_fdma_rx_disable(struct lan966x_rx *rx)
>  {
>  	struct lan966x *lan966x = rx->lan966x;
>  	struct fdma *fdma = &rx->fdma;

This isn't a bug introduced in this patch, but exporting this function
exposes an existing issue.

Does this code handle hardware timeouts safely? Looking at the implementation
of lan966x_fdma_rx_disable(), the return value of readx_poll_timeout_atomic()
is ignored:

    readx_poll_timeout_atomic(lan966x_fdma_channel_active, lan966x,
                              val, !(val & BIT(fdma->channel_id)),
                              READL_SLEEP_US, READL_TIMEOUT_US);

If the hardware is unresponsive and the poll times out, the driver might
falsely assume the channel is halted and proceed to alter hardware state.

If the caller later frees the coherent DMA rings while the DMA engine is
still active, could this lead to memory corruption or IOMMU faults?

[ ... ]
> @@ -265,7 +265,7 @@ static void lan966x_fdma_tx_activate(struct lan966x_tx *tx)
>  		lan966x, FDMA_CH_ACTIVATE);
>  }
>  
> -static void lan966x_fdma_tx_disable(struct lan966x_tx *tx)
> +void lan966x_fdma_tx_disable(struct lan966x_tx *tx)
>  {
>  	struct lan966x *lan966x = tx->lan966x;
>  	struct fdma *fdma = &tx->fdma;

This is another pre-existing issue exposed by exporting these helpers.

The same ignored timeout issue from readx_poll_timeout_atomic() exists
here in lan966x_fdma_tx_disable().

Additionally, is it safe to modify tx->activated without holding the
lan966x->tx_lock?

Looking at the driver teardown path, lan966x_remove() appears to call
lan966x->ops->fdma_deinit() (which calls lan966x_fdma_tx_disable() and frees
the DMA rings) before lan966x_cleanup_ports() unregisters the netdev.

If the network interface is up and actively transmitting during module
removal, could the networking stack concurrently call lan966x_port_xmit(),
acquire tx_lock, and write to tx->fdma.dcbs while it is being freed,
resulting in a use-after-free?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260504-lan966x-pci-fdma-v3-0-a56f5740d870@microchip.com?part=5