Message-ID: <f586237d-66be-8968-d980-f911b83d11ca@molgen.mpg.de>
Date:   Sun, 26 Mar 2023 17:10:06 +0200
From:   Paul Menzel <pmenzel@...gen.mpg.de>
To:     John Hickey <jjh@...dalian.us>
Cc:     Shujin Li <lishujin@...ishou.com>,
        Jesper Dangaard Brouer <hawk@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>, netdev@...r.kernel.org,
        John Fastabend <john.fastabend@...il.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Jesse Brandeburg <jesse.brandeburg@...el.com>,
        Eric Dumazet <edumazet@...gle.com>,
        Tony Nguyen <anthony.l.nguyen@...el.com>,
        Jakub Kicinski <kuba@...nel.org>,
        intel-wired-lan@...ts.osuosl.org, bpf@...r.kernel.org,
        Paolo Abeni <pabeni@...hat.com>,
        "David S. Miller" <davem@...emloft.net>,
        linux-kernel@...r.kernel.org, Jason Xing <xingwanli@...ishou.com>
Subject: Re: [Intel-wired-lan] [PATCH net v3] ixgbe: Panic during XDP_TX with
 > 64 CPUs

[Cc: Remove undeliverable <xingwanli@...ishou.com>]

Am 26.03.23 um 17:03 schrieb Paul Menzel:
> Dear John,
> 
> 
> Thank you for your patch.
> 
> I’d recommend, to use a statement in the git commit message/summary by 
> adding a verb (in imperative mood). Maybe:
> 
> Fix panic during XDP_TX with > 64 CPUs
> 
> Am 08.03.23 um 23:07 schrieb John Hickey:
>> In commit 'ixgbe: let the xdpdrv work with more than 64 cpus'
>> (4fe815850bdc), support was added to allow XDP programs to run on systems
> 
> I think it’s more common to write it like:
> 
> In commit 4fe815850bdc (ixgbe: let the xdpdrv work with more than 64 
> cpus) …
> 
> Even shorter
> 
> Commit 4fe815850bdc (ixgbe: let the xdpdrv work with more than 64 cpus) 
> adds support to allow XDP programs …
> 
>> 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
> 
> Upon trying this patch out via …
> 
>> 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:
> 
> Please add, that you have UBSAN  enabled, or does it happen without?
> 
>>
>>   
>> ==========================================================================
>>   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
> 
> If you do not it yet, `scripts/decode_stacktrace.sh` helps decoding 
> these traces.
> 
>>   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.
> 
> … with more 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@...dalian.us>
>> ---
>> 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(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c 
>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c
>> index f8156fe4b1dc..0ee943db3dc9 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c
>> @@ -1035,9 +1035,6 @@ static void ixgbe_free_q_vector(struct 
>> ixgbe_adapter *adapter, int v_idx)
>>       adapter->q_vector[v_idx] = NULL;
>>       __netif_napi_del(&q_vector->napi);
>> -    if (static_key_enabled(&ixgbe_xdp_locking_key))
>> -        static_branch_dec(&ixgbe_xdp_locking_key);
>> -
>>       /*
>>        * after a call to __netif_napi_del() napi may still be used and
>>        * ixgbe_get_stats64() might access the rings on this vector,
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c 
>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> index ab8370c413f3..cd2fb72c67be 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> @@ -10283,7 +10283,7 @@ static int ixgbe_xdp_setup(struct net_device 
>> *dev, struct bpf_prog *prog)
>>       if (nr_cpu_ids > IXGBE_MAX_XDP_QS * 2)
>>           return -ENOMEM;
>>       else if (nr_cpu_ids > IXGBE_MAX_XDP_QS)
>> -        static_branch_inc(&ixgbe_xdp_locking_key);
>> +        static_branch_enable(&ixgbe_xdp_locking_key);
>>       old_prog = xchg(&adapter->xdp_prog, prog);
>>       need_reset = (!!prog != !!old_prog);
> 
> 
> Kind regards,
> 
> Paul

Powered by blists - more mailing lists