Message-ID: <ZnwEYJjG64PeJHaE@boxer>
Date: Wed, 26 Jun 2024 14:06:56 +0200
From: Maciej Fijalkowski <maciej.fijalkowski@...el.com>
To: Michal Kubiak <michal.kubiak@...el.com>
CC: <intel-wired-lan@...ts.osuosl.org>, <netdev@...r.kernel.org>,
	<wojciech.drewek@...el.com>, <george.kuruvinakunnel@...el.com>,
	<jacob.e.keller@...el.com>, <kuba@...nel.org>
Subject: Re: [PATCH iwl-net v2] i40e: Fix XDP program unloading while
 removing the driver

On Mon, Jun 24, 2024 at 05:12:13PM +0200, Michal Kubiak wrote:
> The commit 6533e558c650 ("i40e: Fix reset path while removing
> the driver") introduced a new PF state "__I40E_IN_REMOVE" to block
> modifying the XDP program while the driver is being removed.
> Unfortunately, such a change is useful only if the ".ndo_bpf()"
> callback was called out of the rmmod context because unloading the
> existing XDP program is also a part of driver removing procedure.
> In other words, from the rmmod context the driver is expected to
> unload the XDP program without reporting any errors. Otherwise,
> the kernel warning with callstack is printed out to dmesg.
> 
> Example failing scenario:
>  1. Load the i40e driver.
>  2. Load the XDP program.
>  3. Unload the i40e driver (using "rmmod" command).
> 
> The example kernel warning log:
> 
> [  +0.004646] WARNING: CPU: 94 PID: 10395 at net/core/dev.c:9290 unregister_netdevice_many_notify+0x7a9/0x870
> [...]
> [  +0.010959] RIP: 0010:unregister_netdevice_many_notify+0x7a9/0x870
> [...]
> [  +0.002726] Call Trace:
> [  +0.002457]  <TASK>
> [  +0.002119]  ? __warn+0x80/0x120
> [  +0.003245]  ? unregister_netdevice_many_notify+0x7a9/0x870
> [  +0.005586]  ? report_bug+0x164/0x190
> [  +0.003678]  ? handle_bug+0x3c/0x80
> [  +0.003503]  ? exc_invalid_op+0x17/0x70
> [  +0.003846]  ? asm_exc_invalid_op+0x1a/0x20
> [  +0.004200]  ? unregister_netdevice_many_notify+0x7a9/0x870
> [  +0.005579]  ? unregister_netdevice_many_notify+0x3cc/0x870
> [  +0.005586]  unregister_netdevice_queue+0xf7/0x140
> [  +0.004806]  unregister_netdev+0x1c/0x30
> [  +0.003933]  i40e_vsi_release+0x87/0x2f0 [i40e]
> [  +0.004604]  i40e_remove+0x1a1/0x420 [i40e]
> [  +0.004220]  pci_device_remove+0x3f/0xb0
> [  +0.003943]  device_release_driver_internal+0x19f/0x200
> [  +0.005243]  driver_detach+0x48/0x90
> [  +0.003586]  bus_remove_driver+0x6d/0xf0
> [  +0.003939]  pci_unregister_driver+0x2e/0xb0
> [  +0.004278]  i40e_exit_module+0x10/0x5f0 [i40e]
> [  +0.004570]  __do_sys_delete_module.isra.0+0x197/0x310
> [  +0.005153]  do_syscall_64+0x85/0x170
> [  +0.003684]  ? syscall_exit_to_user_mode+0x69/0x220
> [  +0.004886]  ? do_syscall_64+0x95/0x170
> [  +0.003851]  ? exc_page_fault+0x7e/0x180
> [  +0.003932]  entry_SYSCALL_64_after_hwframe+0x71/0x79
> [  +0.005064] RIP: 0033:0x7f59dc9347cb
> [  +0.003648] Code: 73 01 c3 48 8b 0d 65 16 0c 00 f7 d8 64 89 01 48 83
> c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa b8 b0 00 00 00 0f
> 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 35 16 0c 00 f7 d8 64 89 01 48
> [  +0.018753] RSP: 002b:00007ffffac99048 EFLAGS: 00000206 ORIG_RAX: 00000000000000b0
> [  +0.007577] RAX: ffffffffffffffda RBX: 0000559b9bb2f6e0 RCX: 00007f59dc9347cb
> [  +0.007140] RDX: 0000000000000000 RSI: 0000000000000800 RDI: 0000559b9bb2f748
> [  +0.007146] RBP: 00007ffffac99070 R08: 1999999999999999 R09: 0000000000000000
> [  +0.007133] R10: 00007f59dc9a5ac0 R11: 0000000000000206 R12: 0000000000000000
> [  +0.007141] R13: 00007ffffac992d8 R14: 0000559b9bb2f6e0 R15: 0000000000000000
> [  +0.007151]  </TASK>
> [  +0.002204] ---[ end trace 0000000000000000 ]---
> 
> Fix this by checking if the XDP program is being loaded or unloaded.
> Then, block only loading a new program while "__I40E_IN_REMOVE" is set.
> Also, move testing "__I40E_IN_REMOVE" flag to the beginning of XDP_SETUP
> callback to avoid unnecessary operations and checks.
> 
> Fixes: 6533e558c650 ("i40e: Fix reset path while removing the driver")
> Signed-off-by: Michal Kubiak <michal.kubiak@...el.com>
> 
> ---
> 
> v1 -> v2 changes:
>  - simplify the fix according to Kuba's suggestions, i.e. remove
>    checking 'NETREG_UNREGISTERING' flag directly from ndo_bpf
>    and remove a separate handling for 'unregister' context.
>  - update the commit message accordingly
>  - add an example of the kernel warning for the issue being fixed.
> 
> See:
> v1: <https://lore.kernel.org/netdev/20240528-net-2024-05-28-intel-net-fixes-v1-4-dc8593d2bbc6@intel.com/>
> ---
>  drivers/net/ethernet/intel/i40e/i40e_main.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index 284c3fad5a6e..310513d9321b 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -13293,6 +13293,10 @@ static int i40e_xdp_setup(struct i40e_vsi *vsi, struct bpf_prog *prog,
>  	bool need_reset;
>  	int i;
>  
> +	/* VSI shall be deleted in a moment, block loading new programs */
> +	if (prog && test_bit(__I40E_IN_REMOVE, pf->state))
> +		return -EINVAL;
> +
>  	/* Don't allow frames that span over multiple buffers */
>  	if (vsi->netdev->mtu > frame_size - I40E_PACKET_HDR_PAD) {
>  		NL_SET_ERR_MSG_MOD(extack, "MTU too large for linear frames and XDP prog does not support frags");
> @@ -13301,14 +13305,9 @@ static int i40e_xdp_setup(struct i40e_vsi *vsi, struct bpf_prog *prog,
>  
>  	/* When turning XDP on->off/off->on we reset and rebuild the rings. */
>  	need_reset = (i40e_enabled_xdp_vsi(vsi) != !!prog);
> -
>  	if (need_reset)
>  		i40e_prep_for_reset(pf);

i40e_reset_and_rebuild() is decorated with this __I40E_IN_REMOVE flag
check and bails out early but not i40e_prep_for_reset() from what I see.
__I40E_RESET_RECOVERY_PENDING is set in i40e_remove() and that's what
i40e_prep_for_reset() checks. So after all, looks like flow you are
addressing will take care of BPF resources only, which is what we wanted.

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

one side note is that these flags are a nightmare, but let's not touch it
in this change.

>  
> -	/* VSI shall be deleted in a moment, just return EINVAL */
> -	if (test_bit(__I40E_IN_REMOVE, pf->state))
> -		return -EINVAL;
> -
>  	old_prog = xchg(&vsi->xdp_prog, prog);
>  
>  	if (need_reset) {
> -- 
> 2.33.1
> 

Powered by blists - more mailing lists