Re: [PATCH v5] 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.
https://sashiko.dev/#/patchset/20260713035049.25310-1-atharvapotdar07@gmail.com?part=1
---
--- Patch [1]: [PATCH v5] r8169: migrate Rx path to page_pool, prepare for XDP ---
commit db5a368e94d087883d45d81ba30f59c05de881cf
Author: Atharva Potdar <atharvapotdar07@gmail.com>

r8169: migrate Rx path to page_pool, prepare for XDP

Replace the alloc_pages() + dma_map_page() + skb_copy() Rx model with
a page_pool-backed napi_build_skb() path for zero-copy delivery.

> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index ec4fc21fa21fb..c548e7e8db874 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> @@ -4311,8 +4363,20 @@ static void rtl_reset_work(struct rtl8169_private *tp)
>  
>  	rtl8169_cleanup(tp);
>  
> -	for (i = 0; i < NUM_RX_DESC; i++)
> -		rtl8169_mark_to_asic(tp->RxDescArray + i);
> +	for (i = 0; i < NUM_RX_DESC; i++) {
> +		if (tp->Rx_databuff[i])
> +			continue;
> +		tp->Rx_databuff[i] =
> +			rtl8169_alloc_rx_data(tp, tp->RxDescArray + i);
> +		if (!tp->Rx_databuff[i])
> +			break;
> +	}

[Severity: High]
If memory allocation fails under pressure during an interface reset,
this loop breaks and leaves tp->Rx_databuff[i] as NULL. When NAPI runs,
could this lead to a permanent deadlock in rtl_rx()?

If a descriptor has a NULL buffer, rtl_rx() currently does:

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

This breaks out of the Rx loop without advancing tp->cur_rx. Because
cur_rx didn't advance, the subsequent call to rtl8169_rx_refill() sees
tp->dirty_rx == tp->cur_rx and will not attempt to allocate memory.
Would it be better to advance cur_rx and drop the packet, or handle
the allocation failure without stalling the entire ring?

> @@ -4820,21 +4907,40 @@ static int rtl_rx(struct net_device *dev, struct rtl8169_private *tp, int budget
>  			goto release_descriptor;
>  		}
>  
> -		skb = napi_alloc_skb(&tp->napi, pkt_size);
> -		if (unlikely(!skb)) {
> -			dev->stats.rx_dropped++;
> -			goto release_descriptor;
> -		}
> +		if (unlikely(!tp->Rx_databuff[entry]))
> +			break;
>  
>  		addr = le64_to_cpu(desc->addr);
>  		rx_buf = page_address(tp->Rx_databuff[entry]);
>  
>  		dma_sync_single_for_cpu(d, addr, pkt_size, DMA_FROM_DEVICE);
> -		prefetch(rx_buf);
> -		skb_copy_to_linear_data(skb, rx_buf, pkt_size);
> -		skb->tail += pkt_size;
> -		skb->len = pkt_size;
> -		dma_sync_single_for_device(d, addr, pkt_size, DMA_FROM_DEVICE);
> +		prefetch(rx_buf + tp->rx_headroom);
> +
> +		if (unlikely(tp->rx_headroom == 0)) {
> +			skb = napi_alloc_skb(&tp->napi, pkt_size);
> +			if (likely(skb)) {
> +				skb_copy_to_linear_data(skb, rx_buf, pkt_size);
> +				skb_put(skb, pkt_size);

[Severity: Critical]
This is a pre-existing issue, but earlier in rtl_rx() there is an
integer underflow on runt frames:

    pkt_size = status & GENMASK(13, 0);
    if (likely(!(dev->features & NETIF_F_RXFCS)))
        pkt_size -= ETH_FCS_LEN;

If NETIF_F_RXALL is enabled, the hardware can pass runt frames smaller
than 4 bytes. In this case, pkt_size underflows to a massive unsigned
value. 

Since this code passes pkt_size directly into dma_sync_single_for_device()
and skb_put(), could this massive value trigger an skb_over_panic() and
crash the kernel when processing runts?