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

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

[Mehmet Fide]

From: Mehmet Fide <mehmet.fide@screeningeagle.com>

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: stable@vger.kernel.org
Signed-off-by: Mehmet Fide <mehmet.fide@screeningeagle.com>
---
--- 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

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

[Mehmet Fide]

A note on the trees this applies to, since the subject names only 6.18.y.

The loop this patch fixes has been unchanged since 95698ff6177b ("net: fec:
using page pool to manage RX buffers") in v6.1, so every stable tree from
6.1.y onwards carries it. I checked 6.12.y in particular: the code there is
byte identical to 6.18.y, so the patch applies without any change, and the
crash is reachable on the same terms.

Please take it into whichever of those trees you consider appropriate, not
6.18.y alone. I only tagged 6.18.y because that is the tree I reproduced and
tested it on.

Thanks,
Mehmet

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

[Sasha Levin]

> 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.

Hm... I think that the patch makes sense.

Wei, Shenwei, Clark - could one of you ack this? It is a stable-only fix
with no upstream commit to point at, so I would rather not queue it on
my own judgement.

-- 
Thanks,
Sasha

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

[Wei Fang]

> 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: stable@vger.kernel.org
> Signed-off-by: Mehmet Fide <mehmet.fide@screeningeagle.com>
> ---
> --- 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 <wei.fang@nxp.com>