Re: [PATCH v4] r8169: migrate Rx path to page_pool, prepare for XDP

[Simon Horman <horms@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
r8169: migrate Rx path to page_pool, prepare for XDP

This commit migrates the Rx path to page_pool on MACs newer than
RTL_GIGA_MAC_VER_06, reserves XDP_PACKET_HEADROOM in each buffer, and
decouples the Rx consumption loop (cur_rx) from a new refill loop
(dirty_rx) that reallocates only after napi_build_skb() takes
ownership of a page.

> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index ec4fc21fa21f..5b13880c7ed8 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c

[ ... ]

> @@ -4161,21 +4169,14 @@ static void rtl8169_mark_to_asic(struct RxDesc *desc)
>  static struct page *rtl8169_alloc_rx_data(struct rtl8169_private *tp,
>  					  struct RxDesc *desc)
>  {
> -	struct device *d = tp_to_dev(tp);
> -	int node = dev_to_node(d);
>  	dma_addr_t mapping;
>  	struct page *data;
>  
> -	data = alloc_pages_node(node, GFP_KERNEL, get_order(R8169_RX_BUF_SIZE));
> +	data = page_pool_dev_alloc_pages(tp->rx_pool);
>  	if (!data)
>  		return NULL;
>  
> -	mapping = dma_map_page(d, data, 0, R8169_RX_BUF_SIZE, DMA_FROM_DEVICE);
> -	if (unlikely(dma_mapping_error(d, mapping))) {
> -		netdev_err(tp->dev, "Failed to map RX DMA!\n");
> -		__free_pages(data, get_order(R8169_RX_BUF_SIZE));
> -		return NULL;
> -	}
> +	mapping = page_pool_get_dma_addr(data) + tp->rx_headroom;
>  
>  	desc->addr = cpu_to_le64(mapping);
>  	rtl8169_mark_to_asic(desc);

[Critical]
Can the NIC DMA past the end of the allocated page here?  The
descriptor's addr is offset by tp->rx_headroom (XDP_PACKET_HEADROOM,
256) inside a page whose backing allocation is order-2 (16384 bytes,
since R8169_RX_BUF_SIZE is SZ_16K - 1 = 16383):

	mapping = page_pool_get_dma_addr(data) + tp->rx_headroom;

but rtl8169_mark_to_asic() still programs the full R8169_RX_BUF_SIZE
into the descriptor's writable-size field:

	WRITE_ONCE(desc->opts1,
		   cpu_to_le32(DescOwn | eor | R8169_RX_BUF_SIZE));

So the NIC's writable window becomes [page + 256, page + 16639], which
overshoots the 16384-byte allocation by up to 255 bytes.  page_pool's
own accounting has already been narrowed via params.max_len =
R8169_RX_BUF_SIZE - tp->rx_headroom, but that value never propagates
to opts1.

On RTL8125/8126, rtl_jumbo_max() returns JUMBO_16K and rtl_set_rx_max_size()
programs RxMaxSize = R8169_RX_BUF_SIZE + 1, so on-wire frames close to
16 KiB are accepted, exactly the range that drives the overrun.

The same mismatch appears on the zero-copy skb path.  napi_build_skb()
is called with frag_size = R8169_RX_BUF_SIZE:

	skb = napi_build_skb(rx_buf, R8169_RX_BUF_SIZE);
	if (likely(skb)) {
		skb_reserve(skb, tp->rx_headroom);
		skb_put(skb, pkt_size);

After skb_reserve(256), tailroom is around 16383 - 256 - overhead,
while pkt_size comes straight from the descriptor's 14-bit length
field (up to 16383, or 16379 after the ETH_FCS_LEN strip).  A large
enough pkt_size would trip skb_over_panic() inside skb_put() → BUG().

Should the descriptor size and napi_build_skb() frag_size use
R8169_RX_BUF_SIZE - tp->rx_headroom (or params.max_len) instead of
R8169_RX_BUF_SIZE?

Also, the commit message states:

    The pool is locked to order-2 (SZ_16K) allocations on all MACs,
    matching the existing R8169_RX_BUF_SIZE, so this migration
    introduces no change in per-descriptor allocation size or
    jumbo-frame behavior relative to the current tree.

Given that rx_headroom is now consuming part of that 16 KiB on newer
MACs while the descriptor size field is unchanged, is the
"no change in ... jumbo-frame behavior" claim still accurate?

> @@ -4188,14 +4189,18 @@ static void rtl8169_rx_clear(struct rtl8169_private *tp)
>  	int i;
>  
>  	for (i = 0; i < NUM_RX_DESC && tp->Rx_databuff[i]; i++) {
> -		dma_unmap_page(tp_to_dev(tp),
> -			       le64_to_cpu(tp->RxDescArray[i].addr),
> -			       R8169_RX_BUF_SIZE, DMA_FROM_DEVICE);
> -		__free_pages(tp->Rx_databuff[i], get_order(R8169_RX_BUF_SIZE));
> +		page_pool_put_full_page(tp->rx_pool, tp->Rx_databuff[i], false);
>  		tp->Rx_databuff[i] = NULL;
>  		tp->RxDescArray[i].addr = 0;
>  		tp->RxDescArray[i].opts1 = 0;
>  	}
> +
> +	if (tp->rx_pool) {
> +		if (xdp_rxq_info_is_reg(&tp->xdp_rxq))
> +			xdp_rxq_info_unreg(&tp->xdp_rxq);
> +		page_pool_destroy(tp->rx_pool);
> +		tp->rx_pool = NULL;
> +	}
>  }

[High]
Can this loop leak Rx_databuff pages and orphan the page_pool?  The
loop condition terminates at the first NULL entry:

	for (i = 0; i < NUM_RX_DESC && tp->Rx_databuff[i]; i++) {

Prior to this patch, Rx_databuff[] was uniformly non-NULL between
fill and close, so early termination was safe.  With this patch, two
paths create NULL entries interspersed with live pages:

  - rl_rx() sets tp->Rx_databuff[entry] = NULL when napi_build_skb()
    succeeds and takes ownership of the page.
  - rtl8169_rx_refill() breaks on the first
    page_pool_dev_alloc_pages() failure, leaving that slot NULL while
    later slots still hold live pages.

If a close happens with any NULL entry in front of a still-populated
one, the loop exits early, page_pool_put_full_page() is never called
on the remaining pages, and page_pool_destroy() then sees non-zero
in-flight pages and schedules the deferred release worker.  With
tp->rx_pool set to NULL and the next rtl8169_init_ring() clearing
Rx_databuff via memset(), the surviving pointers are permanently
lost.

Should this iterate over all NUM_RX_DESC entries and null-check
inside the body instead of using the pointer as a loop condition?

[High]
A related concern that isn't in this diff, but is created by the new
Rx_databuff NULL semantics: rtl_reset_work() calls rtl8169_cleanup()
(which zeroes cur_rx and dirty_rx via rtl8169_init_ring_indexes())
and then unconditionally re-arms every descriptor:

drivers/net/ethernet/realtek/r8169_main.c:rtl_reset_work() {
	...
	rtl8169_cleanup(tp);

	for (i = 0; i < NUM_RX_DESC; i++)
		rtl8169_mark_to_asic(tp->RxDescArray + i);

	napi_enable(&tp->napi);
	rtl_hw_start(tp);
}

After this patch, Rx_databuff[k] can be NULL for slots whose pages
were consumed by napi_build_skb(), and the descriptor's addr for
those slots still points at a page that has been returned to the
page_pool (and may be recycled to another buffer).  Once
rtl_hw_start() re-enables the NIC, it can DMA incoming frames into
those stale addresses.

On top of that, because dirty_rx == cur_rx == 0 after the reset,
rtl8169_rx_refill() has nothing to advance over, and rl_rx's
guard fires without progressing state:

	if (unlikely(!tp->Rx_databuff[entry]))
		break;

so the NULL slots never get repopulated.

Should rtl_reset_work() refill the NULL slots (or do a full
rx_clear/rx_fill) before marking descriptors to ASIC?