Re: [PATCH net] net: hsr: remove seqnr_lock

[Simon Horman <horms@kernel.org>]

On Wed, Sep 04, 2024 at 01:37:25PM +0000, Eric Dumazet wrote:
> syzbot found a new splat [1].
> 
> Instead of adding yet another spin_lock_bh(&hsr->seqnr_lock) /
> spin_unlock_bh(&hsr->seqnr_lock) pair, remove seqnr_lock
> and use atomic_t for hsr->sequence_nr and hsr->sup_sequence_nr.
> 
> This also avoid a race in hsr_fill_info().
> 
> Also remove interlink_sequence_nr which is unused.
> 
> [1]
>  WARNING: CPU: 1 PID: 9723 at net/hsr/hsr_forward.c:602 handle_std_frame+0x247/0x2c0 net/hsr/hsr_forward.c:602
> Modules linked in:
> CPU: 1 UID: 0 PID: 9723 Comm: syz.0.1657 Not tainted 6.11.0-rc6-syzkaller-00026-g88fac17500f4 #0
> Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-debian-1.16.3-2~bpo12+1 04/01/2014
>  RIP: 0010:handle_std_frame+0x247/0x2c0 net/hsr/hsr_forward.c:602
> Code: 49 8d bd b0 01 00 00 be ff ff ff ff e8 e2 58 25 00 31 ff 89 c5 89 c6 e8 47 53 a8 f6 85 ed 0f 85 5a ff ff ff e8 fa 50 a8 f6 90 <0f> 0b 90 e9 4c ff ff ff e8 cc e7 06 f7 e9 8f fe ff ff e8 52 e8 06
> RSP: 0018:ffffc90000598598 EFLAGS: 00010246
> RAX: 0000000000000000 RBX: ffffc90000598670 RCX: ffffffff8ae2c919
> RDX: ffff888024e94880 RSI: ffffffff8ae2c926 RDI: 0000000000000005
> RBP: 0000000000000000 R08: 0000000000000005 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000003
> R13: ffff8880627a8cc0 R14: 0000000000000000 R15: ffff888012b03c3a
> FS:  0000000000000000(0000) GS:ffff88802b700000(0063) knlGS:00000000f5696b40
> CS:  0010 DS: 002b ES: 002b CR0: 0000000080050033
> CR2: 0000000020010000 CR3: 00000000768b4000 CR4: 0000000000350ef0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
>  <IRQ>
>   hsr_fill_frame_info+0x2c8/0x360 net/hsr/hsr_forward.c:630
>   fill_frame_info net/hsr/hsr_forward.c:700 [inline]
>   hsr_forward_skb+0x7df/0x25c0 net/hsr/hsr_forward.c:715
>   hsr_handle_frame+0x603/0x850 net/hsr/hsr_slave.c:70
>   __netif_receive_skb_core.constprop.0+0xa3d/0x4330 net/core/dev.c:5555
>   __netif_receive_skb_list_core+0x357/0x950 net/core/dev.c:5737
>   __netif_receive_skb_list net/core/dev.c:5804 [inline]
>   netif_receive_skb_list_internal+0x753/0xda0 net/core/dev.c:5896
>   gro_normal_list include/net/gro.h:515 [inline]
>   gro_normal_list include/net/gro.h:511 [inline]
>   napi_complete_done+0x23f/0x9a0 net/core/dev.c:6247
>   gro_cell_poll+0x162/0x210 net/core/gro_cells.c:66
>   __napi_poll.constprop.0+0xb7/0x550 net/core/dev.c:6772
>   napi_poll net/core/dev.c:6841 [inline]
>   net_rx_action+0xa92/0x1010 net/core/dev.c:6963
>   handle_softirqs+0x216/0x8f0 kernel/softirq.c:554
>   do_softirq kernel/softirq.c:455 [inline]
>   do_softirq+0xb2/0xf0 kernel/softirq.c:442
>  </IRQ>
>  <TASK>
> 
> Fixes: 06afd2c31d33 ("hsr: Synchronize sending frames to have always incremented outgoing seq nr.")
> Fixes: f421436a591d ("net/hsr: Add support for the High-availability Seamless Redundancy protocol (HSRv0)")
> Reported-by: syzbot <syzkaller@googlegroups.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Thanks Eric,

I see that;

1) The stack trace above is for a call path where
   hsr_forward_skb(), and in turn handle_std_frame(),
   is called without hsr->seqnr_lock held, which explains the splat.

2) Access to hsr->sequence_nr in hsr_fill_info() is not synchronised
   with other accesses to it.

3) hsr->interlink_sequence_nr is set but otherwise unknown

And that this patch addresses all of the above.

Reviewed-by: Simon Horman <horms@kernel.org>