| Message ID | 20240516164108.1482192-1-michal.kubiak@intel.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [iwl-net] i40e: Fix XDP program unloading while removing the driver | expand |
> -----Original Message----- > From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of Michal > Kubiak > Sent: Thursday, May 16, 2024 10:11 PM > To: intel-wired-lan@lists.osuosl.org > Cc: Drewek, Wojciech <wojciech.drewek@intel.com>; netdev@vger.kernel.org; > Kubiak, Michal <michal.kubiak@intel.com>; Fijalkowski, Maciej > <maciej.fijalkowski@intel.com> > Subject: [Intel-wired-lan] [PATCH iwl-net] i40e: Fix XDP program unloading while > removing the driver > > 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). > > Fix this by improving checks in ".ndo_bpf()" to determine if that callback was called > from the removing context and if the kernel wants to unload the XDP program. > Allow for unloading the XDP program in such a case. > > Fixes: 6533e558c650 ("i40e: Fix reset path while removing the driver") > Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com> > Signed-off-by: Michal Kubiak <michal.kubiak@intel.com> > --- > drivers/net/ethernet/intel/i40e/i40e_main.c | 19 ++++++++++++++----- > 1 file changed, 14 insertions(+), 5 deletions(-) > Tested-by: George Kuruvinakunnel <george.kuruvinakunnel@intel.com>
On Thu, May 16, 2024 at 06:41:08PM +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). > > Fix this by improving checks in ".ndo_bpf()" to determine if that > callback was called from the removing context and if the kernel > wants to unload the XDP program. Allow for unloading the XDP program > in such a case. > > Fixes: 6533e558c650 ("i40e: Fix reset path while removing the driver") > Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com> > Signed-off-by: Michal Kubiak <michal.kubiak@intel.com> Acked-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> > --- > drivers/net/ethernet/intel/i40e/i40e_main.c | 19 ++++++++++++++----- > 1 file changed, 14 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c > index ffb9f9f15c52..19fc043e351f 100644 > --- a/drivers/net/ethernet/intel/i40e/i40e_main.c > +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c > @@ -13264,6 +13264,20 @@ static int i40e_xdp_setup(struct i40e_vsi *vsi, struct bpf_prog *prog, > bool need_reset; > int i; > > + /* Called from netdev unregister context. Unload the XDP program. */ > + if (vsi->netdev->reg_state == NETREG_UNREGISTERING) { > + xdp_features_clear_redirect_target(vsi->netdev); > + old_prog = xchg(&vsi->xdp_prog, NULL); > + if (old_prog) > + bpf_prog_put(old_prog); > + > + return 0; > + } > + > + /* VSI shall be deleted in a moment, just return EINVAL */ > + if (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"); > @@ -13272,14 +13286,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); > > - /* 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 >
On Thu, May 16, 2024 at 06:41:08PM +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). > > Fix this by improving checks in ".ndo_bpf()" to determine if that > callback was called from the removing context and if the kernel > wants to unload the XDP program. Allow for unloading the XDP program > in such a case. > > Fixes: 6533e558c650 ("i40e: Fix reset path while removing the driver") > Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com> > Signed-off-by: Michal Kubiak <michal.kubiak@intel.com> Reviewed-by: Simon Horman <horms@kernel.org>
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c index ffb9f9f15c52..19fc043e351f 100644 --- a/drivers/net/ethernet/intel/i40e/i40e_main.c +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c @@ -13264,6 +13264,20 @@ static int i40e_xdp_setup(struct i40e_vsi *vsi, struct bpf_prog *prog, bool need_reset; int i; + /* Called from netdev unregister context. Unload the XDP program. */ + if (vsi->netdev->reg_state == NETREG_UNREGISTERING) { + xdp_features_clear_redirect_target(vsi->netdev); + old_prog = xchg(&vsi->xdp_prog, NULL); + if (old_prog) + bpf_prog_put(old_prog); + + return 0; + } + + /* VSI shall be deleted in a moment, just return EINVAL */ + if (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"); @@ -13272,14 +13286,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); - /* 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) {
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). Fix this by improving checks in ".ndo_bpf()" to determine if that callback was called from the removing context and if the kernel wants to unload the XDP program. Allow for unloading the XDP program in such a case. Fixes: 6533e558c650 ("i40e: Fix reset path while removing the driver") Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com> Signed-off-by: Michal Kubiak <michal.kubiak@intel.com> --- drivers/net/ethernet/intel/i40e/i40e_main.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-)