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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Moving LLP register initialization out of `lan966x_fdma_rx_start()` breaks the MTU change error recovery path.
- [High] The return value of `fdma_dcbs_init()` is ignored in `lan966x_fdma_rx_alloc()`, causing page leaks and the activation of uninitialized DMA descriptors on failure.
- [High] Destroying the page pool on the error path in `lan966x_fdma_rx_alloc()` without unregistering it from the XDP RX queues leaves dangling pointers.
- [High] The `restore:` error path in `lan966x_fdma_reload()` contains a severe hardware/software ring desync bug.
--

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

net: lan966x: add FDMA LLP register write helper

The patch introduces a helper lan966x_fdma_llp_configure() and moves the
FDMA Link List Pointer (LLP) register initialization from the 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 does lan966x_fdma_rx_alloc()
leak pages and activate uninitialized DMA descriptors on failure?

During lan966x_fdma_rx_alloc(), fdma_dcbs_init() invokes the callback
lan966x_fdma_rx_dataptr_cb() to populate the DMA buffer rings. This callback
dynamically allocates pages using page_pool_dev_alloc_pages() and returns
-ENOMEM on failure, which causes fdma_dcbs_init() to return -ENOMEM.

However, lan966x_fdma_rx_alloc() completely ignores this return value and
returns 0.

Will this cause all successfully allocated pages up to the failure point to
be leaked, and trick the caller into activating the hardware DMA engine with
partially uninitialized DCBs, leading to IOMMU faults or memory corruption?

This isn't a bug introduced by this patch either, but does the error path in
lan966x_fdma_rx_alloc() leave dangling pointers in the XDP RX queues?

In lan966x_fdma_rx_alloc(), lan966x_fdma_rx_alloc_page_pool() registers the
newly created page pool with the XDP memory model across all physical ports
via xdp_rxq_info_reg_mem_model(), overwriting the old registration.

If the subsequent fdma_alloc_coherent() fails, the error path invokes
page_pool_destroy(rx->page_pool) but fails to restore the old page pool
registration:

	err = fdma_alloc_coherent(lan966x->dev, fdma);
	if (err) {
		page_pool_destroy(rx->page_pool);
		return err;
	}

When lan966x_fdma_reload() falls back to the restore: path, it resumes
network operations.

Could this leave a dangling pointer to the destroyed page pool in the
xdp_rxq structures, leading to a use-after-free when XDP processes the next
packet?

> @@ -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));
> -

Will moving the LLP register configuration into lan966x_fdma_rx_alloc() break
the MTU change error recovery path?

In lan966x_fdma_reload(), if the new lan966x_fdma_rx_alloc() fails, the code
jumps to the restore: label, copies back the old software state, and calls
lan966x_fdma_rx_start() to reactivate the channel:

drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c: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);
    ...
}

Because lan966x_fdma_rx_alloc() is bypassed for the restored state, the
hardware channel is activated without configuring the LLP.

Since the LLP must be written before the channel is activated, won't
activating the channel after a disable and discard cycle without
reprogramming the LLP result in a DMA fault or system hang?

This isn't a bug introduced by this patch, but does the restore: error path
in lan966x_fdma_reload() contain a hardware/software ring desync issue?

When MTU allocation fails, the driver restores the fdma_rx_old software
state, which contains the old non-zero dcb_index and db_index from when the
ring was active.

However, before this patch, lan966x_fdma_rx_start() unconditionally set the
hardware LLP register to fdma->dma (index 0).

Does this cause the hardware DMA engine to start writing at DCB 0, while the
software's napi_poll routine resumes polling from the restored non-zero
dcb_index? Could this complete desync between hardware and software ring
indices lead to a dead RX path or the processing of stale memory?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260508-lan966x-pci-fdma-v4-0-14e0c89d8d63@microchip.com?part=4