[PATCH net] net: gianfar: clear the pending Rx skb when freeing the Rx ring

[PATCH net] net: gianfar: clear the pending Rx skb when freeing the Rx ring

[Anthony Pighin]

gfar_clean_rx_ring() keeps a frame that spans several Rx buffers in
rx_queue->skb between NAPI polls. When the interface is stopped (close,
MTU change, reset), free_skb_rx_queue() frees that skb but leaves the
pointer set, and nothing clears it when the rings are set up again.

After the restart, gfar_clean_rx_ring() picks up the stale pointer. The
first buffer it sees has RXBD_FIRST set, so the overrun handling frees
the skb a second time. By then the skb has usually been reallocated, so
the second free releases another owner's skb and data while they are
still in use.

Clear rx_queue->skb after freeing it.

Fixes: 75354148ce69 ("gianfar: Add paged allocation and Rx S/G")
Cc: stable@vger.kernel.org
Signed-off-by: Anthony Pighin <anthony.pighin@nokia.com>
---
 drivers/net/ethernet/freescale/gianfar.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
index cf636fc5aafa..19010e08fc55 100644
--- a/drivers/net/ethernet/freescale/gianfar.c
+++ b/drivers/net/ethernet/freescale/gianfar.c
@@ -1092,6 +1092,7 @@ static void free_skb_rx_queue(struct gfar_priv_rx_q *rx_queue)
 	struct rxbd8 *rxbdp = rx_queue->rx_bd_base;
 
 	dev_kfree_skb(rx_queue->skb);
+	rx_queue->skb = NULL;
 
 	for (i = 0; i < rx_queue->rx_ring_size; i++) {
 		struct	gfar_rx_buff *rxb = &rx_queue->rx_buff[i];
-- 
2.43.0

Re: [PATCH net] net: gianfar: clear the pending Rx skb when freeing the Rx ring

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by LLM and not proven at runtime). If it was triggered
   please include the symptoms, like the stack trace or kernel messages.

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net] net: gianfar: clear the pending Rx skb when freeing the Rx ring

[Anthony Pighin]

> - How the issue was discovered, e.g. hit in production, hit during
>   development, syzbot report, manual code inspection, LLM or static
>   analysis tool scan.

Hit during development, as intermittent kernel panics in our automated
regression on an NXP LS1021A board (eTSEC), in an L2 Ethernet traffic
test that shuts and re-enables ports. It was seen on 6.6, 6.12 and
6.18 based kernels, crashing in unrelated places such as
kmem_cache_alloc() called from build_skb() in gfar_poll_rx_sq().

> - Whether the issue was actually triggered, or is only theoretical
>   (e.g. found by LLM and not proven at runtime). If it was triggered
>   please include the symptoms, like the stack trace or kernel messages.

Triggered and reproduced. Bouncing an eTSEC port while a traffic
generator sends 9000 byte frames into it at line rate, an unpatched
6.18.62 based kernel panicked within 50 bounces. A temporary WARN in
free_skb_rx_queue() showed a pending skb being freed on nearly every
bounce. The bad PTE values below look like packet contents:

  BUG: Bad page map in process udevd  pte:c40cffffffffffff
  BUG: Bad page map in process udevd  pte:450008c193dd7a
  BUG: Bad page map in process udevd  pte:1140000000004801
  Internal error: : 221 [#1] SMP ARM
  PC is at folio_remove_rmap_ptes+0x3c/0x118
  LR is at unmap_page_range+0x5ec/0xde0
  Kernel panic - not syncing: Fatal exception

With slub_debug=FZU it panicked on the second bounce, in usbnet's
rx_complete() using an skb that SLUB reports as free, last allocated
by gianfar and then freed by the IPv6 stack. That is, the stale free
had released usbnet's skb while usbnet still owned it:

  Unable to handle kernel paging request at virtual address 00040035 when read
  PC is at rx_complete+0x260/0x37c
  Register r6 information: slab skbuff_head_cache start c1e86dc0 data offset 64 pointer offset 0 size 184 allocated at build_skb+0x30/0xac
      build_skb+0x30/0xac
      gfar_poll_rx_sq+0x19c/0x7f0
   Free path:
      kmem_cache_free+0x284/0x438
      kfree_skbmem+0xac/0xb0
      sk_skb_reason_drop+0x68/0x134
      ip6_rcv_core+0x46c/0x830
  ...
   rx_complete from __usb_hcd_giveback_urb+0xa4/0x12c
  Kernel panic - not syncing: Fatal exception in interrupt

> - What hardware the change was tested on. For driver fixes please
>   mention the device (and if relevant firmware version) used for
>   testing, or say that the change was not tested on real hardware.

NXP LS1021A board (eTSEC) on a 6.18.62 based kernel. With the fix, the
same test ran 90 bounces with no warnings or crashes. On top of the net
tree, the patch was only build tested (ARM multi_v7_defconfig, W=1).