Re: [PATCH net-next v3 04/13] net: lan966x: add FDMA LLP register write helper

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing FDMA LLP register initialization in MTU reload error path.
- [High] Use-After-Free of XDP page pool on MTU change failure.
--

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

net: lan966x: add FDMA LLP register write helper

The FDMA Link List Pointer (LLP) register points to the first DCB in the
chain and must be written before the channel is activated. This tells
the FDMA engine where to begin DMA transfers. Move the LLP register writes
from the channel start/activate functions into the allocation functions.

> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
> index f8ce735a7fc09..6c5761e886d47 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
[ ... ]
> @@ -127,6 +134,9 @@ static int lan966x_fdma_rx_alloc(struct lan966x_rx *rx)
>  	fdma_dcbs_init(fdma, FDMA_DCB_INFO_DATAL(fdma->db_size),
>  		       FDMA_DCB_STATUS_INTR);
>  
> +	lan966x_fdma_llp_configure(lan966x, (u64)fdma->dma,
> +				   fdma->channel_id);
> +
>  	return 0;
>  }

This isn't a bug introduced by this patch, but there seems to be a pre-existing
flaw in the MTU reload error path related to how lan966x_fdma_rx_alloc() handles
the XDP page pool.

During lan966x_fdma_reload(), lan966x_fdma_rx_alloc() first calls
lan966x_fdma_rx_alloc_page_pool(), which unregisters the old XDP memory model
and registers the newly allocated page pool.

If fdma_alloc_coherent() subsequently fails, the error path destroys the
new page pool and lan966x_fdma_reload() jumps to the restore block:

lan966x_fdma_reload() {
    ...
	err = lan966x_fdma_rx_alloc(&lan966x->rx);
	if (err)
		goto restore;
    ...
restore:
	lan966x->rx.page_pool = page_pool;
	memcpy(&lan966x->rx.fdma, &fdma_rx_old, sizeof(struct fdma));
	lan966x_fdma_rx_start(&lan966x->rx);
}

Since the old page pool is never re-registered to port->xdp_rxq, does this
leave the XDP subsystem holding a dangling pointer to the destroyed new page
pool, potentially causing a use-after-free when incoming packets attempt to
manage or return pages?

[ ... ]
> @@ -136,14 +146,6 @@ static void lan966x_fdma_rx_start(struct lan966x_rx *rx)
>  	struct fdma *fdma = &rx->fdma;
>  	u32 mask;
>  
> -	/* When activating a channel, first is required to write the first DCB
> -	 * address and then to activate it
> -	 */
> -	lan_wr(lower_32_bits((u64)fdma->dma), lan966x,
> -	       FDMA_DCB_LLP(fdma->channel_id));
> -	lan_wr(upper_32_bits((u64)fdma->dma), lan966x,
> -	       FDMA_DCB_LLP1(fdma->channel_id));
> -
>  	lan_wr(FDMA_CH_CFG_CH_DCB_DB_CNT_SET(fdma->n_dbs) |
>  	       FDMA_CH_CFG_CH_INTR_DB_EOF_ONLY_SET(1) |
>  	       FDMA_CH_CFG_CH_INJ_PORT_SET(0) |

By removing the LLP initialization from lan966x_fdma_rx_start(), what happens in
the MTU reload error path?

If an MTU reallocation fails (e.g., due to memory pressure) in
lan966x_fdma_reload(), the code jumps to the restore block, copies back the old
FDMA state, and restarts the channel via lan966x_fdma_rx_start():

lan966x_fdma_reload() {
    ...
restore:
	lan966x->rx.page_pool = page_pool;
	memcpy(&lan966x->rx.fdma, &fdma_rx_old, sizeof(struct fdma));
	lan966x_fdma_rx_start(&lan966x->rx);
    ...
}

Because lan966x_fdma_rx_start() no longer writes the FDMA_DCB_LLP register,
will the hardware channel be reactivated without its DMA pointer being reset
to the beginning of the old ring (fdma_rx_old.dma)?

Could this cause the DMA engine to fetch from a stale or undefined address
and break RX functionality after a failed MTU change? Note that this issue
does not appear to be fixed by subsequent patches in the series.

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