Re: [PATCH v2] net: lan966x: restore RX state on reload failure

[sashiko-bot@kernel.org]

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

New issues:
- [High] The return value of xdp_rxq_info_reg_mem_model() is ignored, leaving XDP queues without a valid memory model if registration fails.

Pre-existing issues:
- [High] lan966x_fdma_rx_alloc() ignores the return value of fdma_dcbs_init(), leading to hardware DMA into uninitialized memory if allocation fails.
- [High] lan966x_fdma_reload() fails to update rx->fdma.db_size during MTU changes, causing frames to be truncated.
- [High] Snapshotting the RX ring state before disabling NAPI creates a race condition leading to Use-After-Free and memory leaks.
--

commit 8b6a1ed08092bd9d26dff38bd133b141c326226a
Author: Guangshuo Li <lgs201920130244@gmail.com>

net: lan966x: restore RX state on reload failure

This commit saves and restores rx->page_order, rx->max_mtu, and per-port
XDP RXQ mem model registrations during a failed RX MTU reload. It attempts
to keep the restored RX state consistent when lan966x_fdma_rx_alloc() fails.

> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
> index f8ce735a7fc09..8272ad0851508 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
> @@ -815,6 +815,7 @@ static int lan966x_fdma_reload(struct lan966x *lan966x, int new_mtu)
>  	struct page *(*old_pages)[FDMA_RX_DCB_MAX_DBS];
>  	struct page_pool *page_pool;
>  	struct fdma fdma_rx_old;
> +	int page_order, max_mtu;
>  	int err, i, j;
>  
>  	old_pages = kmemdup(lan966x->rx.page, sizeof(lan966x->rx.page),
> @@ -825,6 +826,8 @@ static int lan966x_fdma_reload(struct lan966x *lan966x, int new_mtu)
>  	/* Store these for later to free them */
>  	memcpy(&fdma_rx_old, &lan966x->rx.fdma, sizeof(struct fdma));
>  	page_pool = lan966x->rx.page_pool;
> +	page_order = lan966x->rx.page_order;
> +	max_mtu = lan966x->rx.max_mtu;
>  
>  	napi_synchronize(&lan966x->napi);
>  	napi_disable(&lan966x->napi);

[Severity: High]
This is a pre-existing issue, but does snapshotting the RX ring state before
disabling NAPI create a race condition?

If the NAPI poll loop runs concurrently on another CPU during this window, it
can process packets, pass some of the old pages to the network stack, and
allocate new pages into rx->page.

Later, the driver iterates over the stale old_pages snapshot and frees them
to the page pool. This could double-free the pages that were consumed by the
network stack. Additionally, any new pages allocated by NAPI after the
snapshot might be completely absent from old_pages and thus leaked.

[ ... ]

> @@ -854,7 +857,24 @@ static int lan966x_fdma_reload(struct lan966x *lan966x, int new_mtu)
>  	return 0;
>  restore:
>  	lan966x->rx.page_pool = page_pool;
> +	lan966x->rx.page_order = page_order;
> +	lan966x->rx.max_mtu = max_mtu;

[Severity: High]
This isn't a bug introduced by this patch, but does lan966x_fdma_reload() fail
to update rx->fdma.db_size during MTU changes?

When changing the MTU, the driver updates rx->page_order and rx->max_mtu, but
fails to update rx->fdma.db_size (which was initially calculated in
lan966x_fdma_init() as PAGE_SIZE << rx->page_order).

Consequently, the subsequent call to fdma_dcbs_init() configures the hardware
descriptors with the old db_size. If the user increases the MTU, the hardware
will still restrict DMA to the old buffer size, which could silently truncate
larger frames.

[Severity: High]
This is also a pre-existing issue, but does lan966x_fdma_rx_alloc() ignore the
return value of fdma_dcbs_init()?

drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c:lan966x_fdma_rx_alloc() {
    ...
	fdma_dcbs_init(fdma, FDMA_DCB_INFO_DATAL(fdma->db_size),
		       FDMA_DCB_STATUS_INTR);

	return 0;
}

If page allocation fails within fdma_dcbs_init(), it aborts and returns
-ENOMEM. However, lan966x_fdma_rx_alloc() ignores this return value and
returns 0. The hardware RX ring is then started with uninitialized DMA
descriptors (containing physical address 0), which could cause silent memory
corruption when the hardware DMAs packets into address 0.

>  	memcpy(&lan966x->rx.fdma, &fdma_rx_old, sizeof(struct fdma));
> +	/*
> +	 * lan966x_fdma_rx_alloc_page_pool() registered the new pool with
> +	 * each port's XDP RXQ before the allocation failed. The new pool is
> +	 * destroyed by lan966x_fdma_rx_alloc(), so restore the old pool's
> +	 * registration before restarting RX.
> +	 */
> +	for (i = 0; i < lan966x->num_phys_ports; i++) {
> +		if (!lan966x->ports[i])
> +			continue;
> +
> +		xdp_rxq_info_unreg_mem_model(&lan966x->ports[i]->xdp_rxq);
> +		xdp_rxq_info_reg_mem_model(&lan966x->ports[i]->xdp_rxq,
> +					   MEM_TYPE_PAGE_POOL, page_pool);

[Severity: High]
Is it safe to ignore the return value of xdp_rxq_info_reg_mem_model() here?

If xdp_rxq_info_reg_mem_model() fails (e.g., due to -ENOMEM from kzalloc), the
XDP RX queue is left unregistered. When subsequent packets are processed, XDP
might treat the page_pool buffers as standard shared pages and call put_page()
instead of page_pool_put_page(), which could corrupt the page pool allocator.

This same issue also appears to be present in the forward path within
lan966x_fdma_rx_alloc_page_pool():

drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c:lan966x_fdma_rx_alloc_page_pool() {
    ...
		xdp_rxq_info_unreg_mem_model(&port->xdp_rxq);
		xdp_rxq_info_reg_mem_model(&port->xdp_rxq, MEM_TYPE_PAGE_POOL,
					   rx->page_pool);
    ...
}

> +	}
> +
>  	lan966x_fdma_rx_start(&lan966x->rx);
>  
>  	lan966x_fdma_wakeup_netdev(lan966x);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260607145747.1494514-1-lgs201920130244@gmail.com?part=1