RE: [PATCH net v3] ixgbe: Panic during XDP_TX with > 64 CPUs

[Rout, ChandanX]

>-----Original Message-----
>From: John Hickey <jjh@xxxxxxxxxxxx>
>Sent: 09 March 2023 03:38
>To: Nguyen, Anthony L <anthony.l.nguyen@xxxxxxxxx>
>Cc: John Hickey <jjh@xxxxxxxxxxxx>; Brandeburg, Jesse
><jesse.brandeburg@xxxxxxxxx>; David S. Miller <davem@xxxxxxxxxxxxx>;
>Eric Dumazet <edumazet@xxxxxxxxxx>; Jakub Kicinski <kuba@xxxxxxxxxx>;
>Paolo Abeni <pabeni@xxxxxxxxxx>; Alexei Starovoitov <ast@xxxxxxxxxx>;
>Daniel Borkmann <daniel@xxxxxxxxxxxxx>; Jesper Dangaard Brouer
><hawk@xxxxxxxxxx>; John Fastabend <john.fastabend@xxxxxxxxx>; Shujin Li
><lishujin@xxxxxxxxxxxx>; Xing, Wanli <xingwanli@xxxxxxxxxxxx>; intel-
>wired-lan@xxxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; linux-
>kernel@xxxxxxxxxxxxxxx; bpf@xxxxxxxxxxxxxxx
>Subject: [PATCH net v3] ixgbe: Panic during XDP_TX with > 64 CPUs
>
>In commit 'ixgbe: let the xdpdrv work with more than 64 cpus'
>(4fe815850bdc), support was added to allow XDP programs to run on systems
>with more than 64 CPUs by locking the XDP TX rings and indexing them using
>cpu % 64 (IXGBE_MAX_XDP_QS).
>
>Upon trying this out patch via the Intel 5.18.6 out of tree driver on a system
>with more than 64 cores, the kernel paniced with an array-index-out-of-
>bounds at the return in ixgbe_determine_xdp_ring in ixgbe.h, which means
>ixgbe_determine_xdp_q_idx was just returning the cpu instead of cpu %
>IXGBE_MAX_XDP_QS.  An example splat:
>
>
>===========================================================
>===============
> UBSAN: array-index-out-of-bounds in
> /var/lib/dkms/ixgbe/5.18.6+focal-1/build/src/ixgbe.h:1147:26
> index 65 is out of range for type 'ixgbe_ring *[64]'
>
>===========================================================
>===============
> BUG: kernel NULL pointer dereference, address: 0000000000000058
> #PF: supervisor read access in kernel mode
> #PF: error_code(0x0000) - not-present page  PGD 0 P4D 0
> Oops: 0000 [#1] SMP NOPTI
> CPU: 65 PID: 408 Comm: ksoftirqd/65
> Tainted: G          IOE     5.15.0-48-generic #54~20.04.1-Ubuntu
> Hardware name: Dell Inc. PowerEdge R640/0W23H8, BIOS 2.5.4 01/13/2020
> RIP: 0010:ixgbe_xmit_xdp_ring+0x1b/0x1c0 [ixgbe]
> Code: 3b 52 d4 cf e9 42 f2 ff ff 66 0f 1f 44 00 00 0f 1f 44 00 00 55 b9
> 00 00 00 00 48 89 e5 41 57 41 56 41 55 41 54 53 48 83 ec 08 <44> 0f b7
> 47 58 0f b7 47 5a 0f b7 57 54 44 0f b7 76 08 66 41 39 c0
> RSP: 0018:ffffbc3fcd88fcb0 EFLAGS: 00010282
> RAX: ffff92a253260980 RBX: ffffbc3fe68b00a0 RCX: 0000000000000000
> RDX: ffff928b5f659000 RSI: ffff928b5f659000 RDI: 0000000000000000
> RBP: ffffbc3fcd88fce0 R08: ffff92b9dfc20580 R09: 0000000000000001
> R10: 3d3d3d3d3d3d3d3d R11: 3d3d3d3d3d3d3d3d R12: 0000000000000000
> R13: ffff928b2f0fa8c0 R14: ffff928b9be20050 R15: 000000000000003c
> FS:  0000000000000000(0000) GS:ffff92b9dfc00000(0000)
> knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000000000058 CR3: 000000011dd6a002 CR4: 00000000007706e0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> PKRU: 55555554
> Call Trace:
>  <TASK>
>  ixgbe_poll+0x103e/0x1280 [ixgbe]
>  ? sched_clock_cpu+0x12/0xe0
>  __napi_poll+0x30/0x160
>  net_rx_action+0x11c/0x270
>  __do_softirq+0xda/0x2ee
>  run_ksoftirqd+0x2f/0x50
>  smpboot_thread_fn+0xb7/0x150
>  ? sort_range+0x30/0x30
>  kthread+0x127/0x150
>  ? set_kthread_struct+0x50/0x50
>  ret_from_fork+0x1f/0x30
>  </TASK>
>
>I think this is how it happens:
>
>Upon loading the first XDP program on a system with more than 64 CPUs,
>ixgbe_xdp_locking_key is incremented in ixgbe_xdp_setup.  However,
>immediately after this, the rings are reconfigured by ixgbe_setup_tc.
>ixgbe_setup_tc calls ixgbe_clear_interrupt_scheme which calls
>ixgbe_free_q_vectors which calls ixgbe_free_q_vector in a loop.
>ixgbe_free_q_vector decrements ixgbe_xdp_locking_key once per call if it is
>non-zero.  Commenting out the decrement in ixgbe_free_q_vector stopped
>my system from panicing.
>
>I suspect to make the original patch work, I would need to load an XDP
>program and then replace it in order to get ixgbe_xdp_locking_key back
>above 0 since ixgbe_setup_tc is only called when transitioning between XDP
>and non-XDP ring configurations, while ixgbe_xdp_locking_key is
>incremented every time ixgbe_xdp_setup is called.
>
>Also, ixgbe_setup_tc can be called via ethtool --set-channels, so this becomes
>another path to decrement ixgbe_xdp_locking_key to 0 on systems with
>greater than 64 CPUs.
>
>For this patch, I have changed static_branch_inc to static_branch_enable in
>ixgbe_setup_xdp.  We weren't counting references.  The
>ixgbe_xdp_locking_key only protects code in the XDP_TX path, which is not
>run when an XDP program is loaded.  The other condition for setting it on is
>the number of CPUs, which I assume is static.
>
>Fixes: 4fe815850bdc ("ixgbe: let the xdpdrv work with more than 64 cpus")
>Signed-off-by: John Hickey <jjh@xxxxxxxxxxxx>
>---
>v1 -> v2:
>	Added Fixes and net tag.  No code changes.
>v2 -> v3:
>	Added splat.  Slight clarification as to why ixgbe_xdp_locking_key
>	is not turned off.  Based on feedback from Maciej Fijalkowski.
>---
> drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c  | 3 ---
>drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 2 +-
> 2 files changed, 1 insertion(+), 4 deletions(-)
>

Tested-by: Chandan Kumar Rout <chandanx.rout@xxxxxxxxx> (A Contingent Worker at Intel)