Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 5 Apr 2023 16:40:33 +0200
From:   Maciej Fijalkowski <maciej.fijalkowski@intel.com>
To:     John Hickey <jjh@daedalian.us>
CC:     <anthony.l.nguyen@intel.com>,
        Jesse Brandeburg <jesse.brandeburg@intel.com>,
        "David S. Miller" <davem@davemloft.net>,
        "Eric Dumazet" <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Jesper Dangaard Brouer <hawk@kernel.org>,
        "John Fastabend" <john.fastabend@gmail.com>,
        Shujin Li <lishujin@kuaishou.com>,
        Jason Xing <xingwanli@kuaishou.com>,
        <intel-wired-lan@lists.osuosl.org>, <netdev@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <bpf@vger.kernel.org>
Subject: Re: [PATCH net v3] ixgbe: Panic during XDP_TX with > 64 CPUs
Message-ID: <ZC2IYWgTUFCnlKc9@boxer>
References: <20230308220756.587317-1-jjh@daedalian.us>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20230308220756.587317-1-jjh@daedalian.us>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?aD2o26c/MJiAFf9okQr6gjr7TbtX10kgn/QPf2+Scg2ayLq4RV/Yrse5cbYo?=
 =?us-ascii?Q?5yu0XTM3K21VIHNvCGUNJHFkF5liiBXTBEyRE5ZzftNb8kPh8CaaypinB+YZ?=
 =?us-ascii?Q?lKhIyUxjEQ6+vcNLiolj5+R0THH2I2gTC1uAFJSKCdeKbMzrROa4soyrxJlh?=
 =?us-ascii?Q?j9uocnLAb4EA3It+O7LawZrtfr2VUdh0TKkaRrDUehfCEN3JfMB/fskVpB75?=
 =?us-ascii?Q?AmPmrmNn9sa6jxdruRtGv1LEa2F3mPNp4zr44Fxa5uIoIw/hlAuyAfFIzywq?=
 =?us-ascii?Q?wPDZjcma5ulTHCwji6GvJ3s8I6B6RDlIQMedXLdRaBGnfPeAKk64nGEqK1Pq?=
 =?us-ascii?Q?Zx6r2oxjb63yzr81imiFzLoyvkQLwXhBhYvTHcnBAc44lJfb90z4St6VHnoj?=
 =?us-ascii?Q?RbBb9lgFEu4gTF8m//HqgHtwcxwxXcvWNFmzADxK2p4ErCrmUS3r97Uly5aZ?=
 =?us-ascii?Q?NTM2BcQSW+2t+TTfvewUjEmhK3DAyQkAbUGHmmSL3/aVjZKBTjljBfuk8H2Z?=
 =?us-ascii?Q?+tsGy5rEPWqM+kwUZzMOLnb//NNU6d3u646MkSlMQXC9eyWUgZ2BLo4lq2VP?=
 =?us-ascii?Q?pXRNAQzVEq7yMytFAU8lEjUDK01qRFKx0zhquo07Kd2bwG6ZzHZjwF9eY5H/?=
 =?us-ascii?Q?1t3oXBoIpzlZe2A6VxN425r82iehKsnJXkk/6i2lD5qhvxzeBaUMw1cDovyY?=
 =?us-ascii?Q?0cWJFN0KqbG27zg2jH4eCDDEZozF+p5KhrX01Neg2dmNbs7564KBqzYVQrR9?=
 =?us-ascii?Q?ue6986LcPagPzzdUm3T4hcNShzNyUteAonp8DAYG383FoL70ViFiO2wAacgg?=
 =?us-ascii?Q?S5Gmxx1uEf/8cfFQXavUo5ytkeJ13DtJ8wdbL9qiYXS5FRomQwWE1E80ssIo?=
 =?us-ascii?Q?oBsq6ZdATHLeotTKuCnAgZ+O5lEywX8PSSDKIjeNx2uuVvndvJgew4kLKi0f?=
 =?us-ascii?Q?aN0CbiPceINqZdisGzvwAT4+sU5LNlJ4oxDaWbNLkSkj/4oy/HgppOcSLnpi?=
 =?us-ascii?Q?Qys2HguohUxUKmZpkA8daZA5rdLbGJfzi4eIL9sCk68ut75pF523d2QVV9ss?=
 =?us-ascii?Q?DgU5oCs62I5qtQJz0F5hFY/Vb2ZBAf+imuUo2Tk8ccDwvn3DmUVR7qr3JaRq?=
 =?us-ascii?Q?1rmOYCNZqU3YdOHP9J7DbZciFhk0hBPYa/GoFQHP8fgKwGdN/xBrTLIiIN6O?=
 =?us-ascii?Q?nRCX5s9MFcC9zC82Jv0WAE8JrJ/l37FsH++iPrZVM8eoh4mJSFCs9+iU61uE?=
 =?us-ascii?Q?5Njxt4otLwlsD7gswRGU+YKo7vpn9CyZfjSWnpf20+/YFtGS3Y20X9V0C5ES?=
 =?us-ascii?Q?x8l84BirLTs/3q7DdtUsuvP0pZo08IT6JQbx3Ht8AXBvdi+VTztH7usFL88j?=
 =?us-ascii?Q?FzbyzmkF0c16YCejaIxZitIfy2o0N1t2BY3Ql+zoEefPCI0LnMkKttcIV5qx?=
 =?us-ascii?Q?zPTrCpH+/zTWsrFloh8iue4eUiz1x3dgKvN/9XDnvLk5Vd8BG/rrNDgDgNLu?=
 =?us-ascii?Q?fp3JtYaD9HbEEb9L717bbyhc/2jJ/kvZBfLf5AzOX2W6Jhu/EJfw2QaK04Yf?=
 =?us-ascii?Q?MYpTroUE5mMAtnA8jcScOa+9oGmCLk0fcWoU2BupLVEt9N55X5L80Ad1NAFh?=
 =?us-ascii?Q?Sw=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 23860a01-a2b2-4ef9-8f34-08db35e3be77
X-MS-Exchange-CrossTenant-AuthSource: DM4PR11MB6117.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Apr 2023 14:40:47.5037
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: E+hsyuogyTOeilJV3hnmjqyrEfOwkGiXIyjNbKEAOL5gUAQgJN9jbtUPQ/xKI/3TNWQT+wAgDXXfCagsA8TVuE4eszR/0GG/7d5efDtd4pg=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR11MB4642
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.5 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,SPF_HELO_PASS,
        SPF_NONE autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Wed, Mar 08, 2023 at 02:07:57PM -0800, John Hickey wrote:
> 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:

i am assuming that in-tree driver is also affected so the info about OOT
driver is irrelevant here :)

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

From technical POV i think the contents here are fine. Maybe we could even
move this check to probe path because as you said this is static. I saw
that others had some wording/style comments, so if you decide to fix it
and push a v4, then you can add:

Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>

> 
> Fixes: 4fe815850bdc ("ixgbe: let the xdpdrv work with more than 64 cpus")
> Signed-off-by: John Hickey <jjh@daedalian.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);
> -- 
> 2.37.2
>