RE: [PATCH 6.18.y] net: fec: do not release NULL pages when RX buffer allocation fails

> fec_enet_alloc_rxq_buffers() leaves the loop as soon as
> page_pool_dev_alloc_pages() returns NULL and jumps to err_alloc, which
> calls fec_enet_free_buffers(). That helper walks the whole ring and
> hands every rx_skb_info[i].page to page_pool_put_full_page(), including
> the entries the allocation loop never reached. Those are still NULL,
> because the queue was allocated with kzalloc(), and
> page_pool_put_full_page() dereferences the page, so an open that runs
> out of memory oopses instead of returning -ENOMEM:
> 
>   Unable to handle kernel NULL pointer dereference at virtual address
> 00000014 when read
>   Internal error: Oops: 5 [#1] SMP ARM
>   CPU: 0 PID: 384 Comm: connmand Not tainted 6.18.43 #1
>   Hardware name: Freescale Vybrid VF5xx/VF6xx (Device Tree)
>   PC is at fec_enet_free_buffers+0xb0/0x2a8
>   Call trace:
>    fec_enet_free_buffers from fec_enet_open+0x1e0/0x504
>    fec_enet_open from __dev_open+0x114/0x238
>    __dev_open from __dev_change_flags+0x190/0x208
>    __dev_change_flags from netif_change_flags+0x1c/0x58
>    netif_change_flags from dev_change_flags+0x44/0x74
>    dev_change_flags from devinet_ioctl+0x3a4/0x768
> 
> Seen on a Colibri VF50, 128 MiB of RAM, on the first ifup after boot.
> 
> Skip the entries that hold no page, and clear the ones that do after
> releasing them, so that a later failed open cannot release the same page
> a second time.
> 
> Mainline is not affected. Commit a2ae70c0efe4 ("net: fec: add
> fec_alloc_rxq_buffers_pp() to allocate buffers from page pool") replaced
> this loop with fec_free_rxq_buffers(), which skips and clears the empty
> entries. That commit is part of the XDP zero copy series and is not a
> stable candidate, so this is the equivalent minimal fix for 6.18.y.
> 
> Fixes: 95698ff6177b ("net: fec: using page pool to manage RX buffers")
> Cc: [email protected]
> Signed-off-by: Mehmet Fide <[email protected]>
> ---
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -3341,8 +3341,15 @@
> 
>         for (q = 0; q < fep->num_rx_queues; q++) {
>                 rxq = fep->rx_queue[q];
> -               for (i = 0; i < rxq->bd.ring_size; i++)
> -                       page_pool_put_full_page(rxq->page_pool,
> rxq->rx_skb_info[i].page, false);
> +               for (i = 0; i < rxq->bd.ring_size; i++) {
> +                       struct page *page = rxq->rx_skb_info[i].page;
> +
> +                       if (!page)
> +                               continue;
> +
> +                       page_pool_put_full_page(rxq->page_pool, page,
> false);
> +                       rxq->rx_skb_info[i].page = NULL;
> +               }
> 
>                 for (i = 0; i < XDP_STATS_TOTAL; i++)
>                         rxq->stats[i] = 0;
> --
> 2.43.0

It looks good to me, thanks.

Reviewed-by: Wei Fang <[email protected]>