[PATCH net] net: lan966x: save the RX state after quiescing the NAPI in a reload

[PATCH net] net: lan966x: save the RX state after quiescing the NAPI in a reload

[Daniel Machon]

lan966x_fdma_reload() replaces the RX ring when the MTU or the XDP
program changes. It copies rx->page[] and the RX FDMA state, allocates
the new ring, and on success returns the old pages to the old page pool.
If the allocation fails, it restores the old state instead.

Before this patch, the copies are taken before the NAPI is disabled. A
poll still running at that point refills the ring and advances the DCB
index and the last DCB pointer after the copy. The success path then
returns a page that the stack still owns to the old pool and never
returns the page that replaced it, and the restore path puts back a
stale DCB index and last DCB pointer.

Disable the NAPI and stop the queues before taking the copies. The copy
of rx->page[] is still allocated first, so a failed allocation returns
before anything has to be undone.

Found by an AI code review agent. The race was not reproduced, as its
window is narrow. The reworked reload was tested on a lan9668 board with
repeated MTU and XDP program changes under RX traffic, with no page pool
or UAF splat.

Fixes: 59c3d55a946c ("net: lan966x: fix use-after-free and leak in lan966x_fdma_reload()")
Signed-off-by: Daniel Machon <daniel.machon@microchip.com>
---
 drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
index 21668bbcc9063..2dbc25c098fec 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
@@ -819,21 +819,21 @@ static int lan966x_fdma_reload(struct lan966x *lan966x, int new_mtu)
 	int page_order, max_mtu;
 	int err, i, j;
 
-	old_pages = kmemdup(lan966x->rx.page, sizeof(lan966x->rx.page),
-			   GFP_KERNEL);
+	old_pages = kmalloc_objs(*lan966x->rx.page, FDMA_DCB_MAX);
 	if (!old_pages)
 		return -ENOMEM;
 
+	napi_synchronize(&lan966x->napi);
+	napi_disable(&lan966x->napi);
+	lan966x_fdma_stop_netdev(lan966x);
+
 	/* Store these for later to free them */
+	memcpy(old_pages, lan966x->rx.page, sizeof(lan966x->rx.page));
 	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);
-	lan966x_fdma_stop_netdev(lan966x);
-
 	lan966x_fdma_rx_disable(&lan966x->rx);
 	lan966x->rx.page_order = round_up(new_mtu, PAGE_SIZE) / PAGE_SIZE - 1;
 	lan966x->rx.max_mtu = new_mtu;

---
base-commit: 829a09df899dcce860dcf5f70e09c1b2d7d425ab
change-id: 20261028-lan966x-fdma-reload-quiesce-8fb5c277e284

Best regards,
-- 
Daniel Machon <daniel.machon@microchip.com>

Re: [PATCH net] net: lan966x: save the RX state after quiescing the NAPI in a reload

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 (4) · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The patch does not fully fix the restore path, even though the
  commit message says it does.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but
  lan966x_fdma_rx_alloc()…
- [High] This problem wasn't introduced by this patch, but
  lan966x_fdma_reload() swaps lan966x->rx.page_pool and destroys the old…
- [High] This problem wasn't introduced by this patch, but it produces
  the same outcome the commit message describes: a page the stack still…
- [High] This problem wasn't introduced by this patch, but
  lan966x_fdma_reload() runs lan966x_fdma_rx_start() and…

--

net: lan966x: save the RX state after quiescing the NAPI in a reload

This patch moves napi_synchronize(), napi_disable() and
lan966x_fdma_stop_netdev() in lan966x_fdma_reload() so they run before
the copies of rx->page[] and the RX FDMA state are taken. A NAPI poll
can then no longer advance the ring after the snapshot. The old_pages
buffer is still allocated first, so an allocation failure returns
before anything needs to be undone.

> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
> index 21668bbcc9063..2dbc25c098fec 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_fdma.c
> @@ -819,21 +819,21 @@ static int lan966x_fdma_reload(struct lan966x *lan966x, int new_mtu)
>  	int page_order, max_mtu;
>  	int err, i, j;
>  
> -	old_pages = kmemdup(lan966x->rx.page, sizeof(lan966x->rx.page),
> -			   GFP_KERNEL);
> +	old_pages = kmalloc_objs(*lan966x->rx.page, FDMA_DCB_MAX);
>  	if (!old_pages)
>  		return -ENOMEM;
>  
> +	napi_synchronize(&lan966x->napi);
> +	napi_disable(&lan966x->napi);
> +	lan966x_fdma_stop_netdev(lan966x);

[Severity: High]
This is a pre-existing issue, but both the success path and the restore
path of lan966x_fdma_reload() restart things in this order:

	lan966x_fdma_rx_start(&lan966x->rx);
	[ ... ]
	lan966x_fdma_wakeup_netdev(lan966x);
	napi_enable(&lan966x->napi);

An FDMA DB interrupt can fire while NAPI is still disabled. Examples are
a TX completion after netif_wake_queue() on another CPU, or ndo_xdp_xmit
traffic, which ignores queue state. In that case lan966x_fdma_irq_handler()
does:

		lan_wr(0, lan966x, FDMA_INTR_DB_ENA);
		lan_wr(db, lan966x, FDMA_INTR_DB);

		napi_schedule(&lan966x->napi);

The napi_schedule() does nothing, because napi_disable() holds
NAPI_STATE_SCHED. napi_enable() does not schedule a poll either.
FDMA_INTR_DB_ENA is only re-armed by the poll, by rx_start, or by the
first TX activation.

Could this leave all DB interrupts masked, with RX and TX completion
dead on every port until the next reload?

There is also a narrower window between napi_disable() and rx_start().
The read-modify-write in rx_start only sets the RX channel bit, so a TX
bit cleared by the handler stays cleared until an RX poll writes 0xff.

lan966x_fdma_pci_reload() documents this hazard and calls napi_enable()
first:

	/* Order matters: napi_enable() must precede the wakes, or a TX that
	 * completes first clears FDMA_INTR_DB_ENA with nothing scheduled to
	 * restore it, leaving RX dead until the next reload.
	 */

Should the platform reload use the same ordering?

> +
>  	/* Store these for later to free them */
> +	memcpy(old_pages, lan966x->rx.page, sizeof(lan966x->rx.page));

[Severity: High]
This isn't a bug introduced by this patch, but a refill failure in
lan966x_fdma_napi_poll() seems to lead to the outcome the commit message
describes, a page the stack still owns being returned to the old pool:

allocate_new:
	while (dcb_reload != fdma->dcb_index) {
		[ ... ]
		fdma_dcb_add(fdma, old_dcb, FDMA_DCB_INFO_DATAL(fdma->db_size),
			     FDMA_DCB_STATUS_INTR);
		[ ... ]
	}

The return value of fdma_dcb_add() is ignored. If
page_pool_dev_alloc_pages() fails, lan966x_fdma_rx_dataptr_cb() returns
-ENOMEM without updating rx->page[old_dcb][0]. The slot still points at
the page that was already passed to the stack, placed in the TX ring,
redirected, or recycled.

The DCB is not relinked either. Software eventually stops at that DCB,
so the stale pointer stays in place.

The next reload copies that pointer into old_pages here and calls
page_pool_put_full_page() on it before page_pool_destroy(). Can this
cause a use-after-free or double free when the skb or xdp_frame owner
later frees the page? lan966x_fdma_rx_free_pages() at teardown seems to
have the same exposure.

>  	memcpy(&fdma_rx_old, &lan966x->rx.fdma, sizeof(struct fdma));

[Severity: High]
The commit message says:

  "...and the restore path puts back a stale DCB index and last DCB
  pointer."

With the snapshot now taken after napi_disable(), the restore path puts
back the current dcb_index (say k) and last_dcb (DCB k-1). Does the
hardware agree with that position? The restore path does:

restore:
	[ ... ]
	memcpy(&lan966x->rx.fdma, &fdma_rx_old, sizeof(struct fdma));
	[ ... ]
	lan966x_fdma_llp_configure(lan966x, lan966x->rx.fdma.dma,
				   lan966x->rx.fdma.channel_id);

	lan966x_fdma_rx_start(&lan966x->rx);

The LLP points at the ring base (DCB 0), and the channel is reactivated
without rebuilding the DCB chain.

After traffic, each NAPI refill through fdma_dcb_add()->__fdma_dcb_add()
appends the consumed DCB after last_dcb with an INVALID nextptr. The
chain is then k -> ... -> n-1 -> 0 -> ... -> k-1 (INVALID).

Starting at DCB 0, the hardware would fill DCBs 0..k-1 and stop at k-1.
lan966x_fdma_napi_poll() only checks fdma_has_frames() on DCB k. If
lan966x_fdma_rx_alloc() fails while k != 0, wouldn't RX stop on all
ports until the next successful reload?

On its restore path, lan966x_fdma_pci_reload() calls
lan966x_fdma_pci_reset_mem(). That runs fdma_dcbs_init() to reset the
indices and rebuild the chain before the LLP is programmed.

Does the platform restore path need to re-initialise the old ring in a
similar way, or program the LLP to fdma.dma + dcb_index *
sizeof(struct fdma_dcb)?

>  	page_pool = lan966x->rx.page_pool;

[Severity: High]
This is a pre-existing issue, but is it safe to destroy this page_pool
while XDP_TX pages from it can still be queued in the TX ring?

lan966x_fdma_xmit_xdpf() stores the RX page in the TX ring:

		next_dcb_buf->data.page = page;

These buffers are only reclaimed from NAPI, by
lan966x_fdma_tx_clear_buf(). That function recycles into whichever pool
is current:

			else
				page_pool_recycle_direct(rx->page_pool,
							 dcb_buf->data.page);

The TX channel is not drained during the reload. After napi_enable(),
these old-pool pages would be recycled into the new pool, and
__page_pool_put_page() does not check page->pp against the pool
argument.

Would the old pool's inflight count then never reach zero, so the
deferred page_pool_destroy() never completes? The new pool's inflight
accounting would also go negative.

When the MTU grows (page_order 0 -> 2), the new pool would DMA sync and
later unmap an order-0 page using its larger max_len and order. On
non-coherent systems, could that invalidate cache lines past the end of
the page, or let the device DMA past it? After XDP detach the dma_dir
would also mismatch (DMA_BIDIRECTIONAL vs DMA_FROM_DEVICE).

>  	page_order = lan966x->rx.page_order;
>  	max_mtu = lan966x->rx.max_mtu;
>  
> -	napi_synchronize(&lan966x->napi);
> -	napi_disable(&lan966x->napi);
> -	lan966x_fdma_stop_netdev(lan966x);
> -
>  	lan966x_fdma_rx_disable(&lan966x->rx);
>  	lan966x->rx.page_order = round_up(new_mtu, PAGE_SIZE) / PAGE_SIZE - 1;
>  	lan966x->rx.max_mtu = new_mtu;

[Severity: High]
This isn't a bug introduced by this patch, but lan966x_fdma_rx_alloc(),
called right after this, drops the return value of fdma_dcbs_init():

	fdma_dcbs_init(fdma, FDMA_DCB_INFO_DATAL(fdma->db_size),
		       FDMA_DCB_STATUS_INTR);

	lan966x_fdma_llp_configure(lan966x, fdma->dma, fdma->channel_id);

	return 0;

Suppose page_pool_dev_alloc_pages() fails at DCB k. This can happen
because it uses GFP_ATOMIC, with order > 0 for a jumbo MTU.
lan966x_fdma_rx_dataptr_cb() then returns -ENOMEM before writing
rx->page[k][0], and fdma_dcbs_init() stops there.

lan966x_fdma_reload() still takes the success path. It puts every
old_pages[i][j] back to the old pool and calls page_pool_destroy().
rx.page[k..n_dcbs-1] are never cleared, so they still point at those
old pages.

Could a later lan966x_fdma_rx_free_pages() on deinit, or the memcpy()
into old_pages on the next reload, put those freed pages again?

The new ring would also end at DCB k-1, so RX would stall once
dcb_index reaches k.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261029-lan966x-fdma-reload-quiesce-v1-1-daaeda5ca3aa%40microchip.com