| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
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