| Message ID | 20240724164840.2536605-5-larysa.zaremba@intel.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Anthony Nguyen |
| Headers | show |
| Series | ice: fix synchronization between .ndo_bpf() and reset | expand |
On 7/24/2024 9:48 AM, Larysa Zaremba wrote: > Consider the following scenario: > > .ndo_bpf() | ice_prepare_for_reset() | > ________________________|_______________________________________| > rtnl_lock() | | > ice_down() | | > | test_bit(ICE_VSI_DOWN) - true | > | ice_dis_vsi() returns | > ice_up() | | > | proceeds to rebuild a running VSI | > > .ndo_bpf() is not the only rtnl-locked callback that toggles the interface > to apply new configuration. Another example is .set_channels(). > > To avoid the race condition above, act only after reading ICE_VSI_DOWN > under rtnl_lock. > > Fixes: 0f9d5027a749 ("ice: Refactor VSI allocation, deletion and rebuild flow") > Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com> > Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com> > --- > drivers/net/ethernet/intel/ice/ice_lib.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c > index f9852f1a136e..b773078ad81a 100644 > --- a/drivers/net/ethernet/intel/ice/ice_lib.c > +++ b/drivers/net/ethernet/intel/ice/ice_lib.c > @@ -2665,8 +2665,7 @@ int ice_ena_vsi(struct ice_vsi *vsi, bool locked) > */ > void ice_dis_vsi(struct ice_vsi *vsi, bool locked) > { > - if (test_bit(ICE_VSI_DOWN, vsi->state)) > - return; > + bool already_down = test_bit(ICE_VSI_DOWN, vsi->state); > Do we need to initialize already_down? I guess its because the other initialization happens inside the conditional which may not be executed in every flow. Ok. Reviewed-by: Jacob Keller <jacob.e.keller@intel.com> > set_bit(ICE_VSI_NEEDS_RESTART, vsi->state); > > @@ -2674,15 +2673,16 @@ void ice_dis_vsi(struct ice_vsi *vsi, bool locked) > if (netif_running(vsi->netdev)) { > if (!locked) > rtnl_lock(); > - > - ice_vsi_close(vsi); > + already_down = test_bit(ICE_VSI_DOWN, vsi->state); > + if (!already_down) > + ice_vsi_close(vsi); > > if (!locked) > rtnl_unlock(); > - } else { > + } else if (!already_down) { > ice_vsi_close(vsi); > } > - } else if (vsi->type == ICE_VSI_CTRL) { > + } else if (vsi->type == ICE_VSI_CTRL && !already_down) { > ice_vsi_close(vsi); > } > }
>-----Original Message----- >From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of >Zaremba, Larysa >Sent: Wednesday, July 24, 2024 10:19 PM >To: intel-wired-lan@lists.osuosl.org >Cc: Drewek, Wojciech <wojciech.drewek@intel.com>; Fijalkowski, Maciej ><maciej.fijalkowski@intel.com>; Jesper Dangaard Brouer <hawk@kernel.org>; >Daniel Borkmann <daniel@iogearbox.net>; Zaremba, Larysa ><larysa.zaremba@intel.com>; netdev@vger.kernel.org; John Fastabend ><john.fastabend@gmail.com>; Alexei Starovoitov <ast@kernel.org>; linux- >kernel@vger.kernel.org; Eric Dumazet <edumazet@google.com>; Kubiak, >Michal <michal.kubiak@intel.com>; Nguyen, Anthony L ><anthony.l.nguyen@intel.com>; Nambiar, Amritha ><amritha.nambiar@intel.com>; Keller, Jacob E <jacob.e.keller@intel.com>; >Jakub Kicinski <kuba@kernel.org>; bpf@vger.kernel.org; Paolo Abeni ><pabeni@redhat.com>; David S. Miller <davem@davemloft.net>; Karlsson, >Magnus <magnus.karlsson@intel.com> >Subject: [Intel-wired-lan] [PATCH iwl-net v2 4/6] ice: check ICE_VSI_DOWN >under rtnl_lock when preparing for reset > >Consider the following scenario: > >.ndo_bpf() | ice_prepare_for_reset() | >________________________|_______________________________________| >rtnl_lock() | | >ice_down() | | > | test_bit(ICE_VSI_DOWN) - true | > | ice_dis_vsi() returns | >ice_up() | | > | proceeds to rebuild a running VSI | > >.ndo_bpf() is not the only rtnl-locked callback that toggles the interface to apply >new configuration. Another example is .set_channels(). > >To avoid the race condition above, act only after reading ICE_VSI_DOWN under >rtnl_lock. > >Fixes: 0f9d5027a749 ("ice: Refactor VSI allocation, deletion and rebuild flow") >Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com> >Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com> >--- > drivers/net/ethernet/intel/ice/ice_lib.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > Tested-by: Chandan Kumar Rout <chandanx.rout@intel.com> (A Contingent Worker at Intel)
diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c index f9852f1a136e..b773078ad81a 100644 --- a/drivers/net/ethernet/intel/ice/ice_lib.c +++ b/drivers/net/ethernet/intel/ice/ice_lib.c @@ -2665,8 +2665,7 @@ int ice_ena_vsi(struct ice_vsi *vsi, bool locked) */ void ice_dis_vsi(struct ice_vsi *vsi, bool locked) { - if (test_bit(ICE_VSI_DOWN, vsi->state)) - return; + bool already_down = test_bit(ICE_VSI_DOWN, vsi->state); set_bit(ICE_VSI_NEEDS_RESTART, vsi->state); @@ -2674,15 +2673,16 @@ void ice_dis_vsi(struct ice_vsi *vsi, bool locked) if (netif_running(vsi->netdev)) { if (!locked) rtnl_lock(); - - ice_vsi_close(vsi); + already_down = test_bit(ICE_VSI_DOWN, vsi->state); + if (!already_down) + ice_vsi_close(vsi); if (!locked) rtnl_unlock(); - } else { + } else if (!already_down) { ice_vsi_close(vsi); } - } else if (vsi->type == ICE_VSI_CTRL) { + } else if (vsi->type == ICE_VSI_CTRL && !already_down) { ice_vsi_close(vsi); } }
Consider the following scenario: .ndo_bpf() | ice_prepare_for_reset() | ________________________|_______________________________________| rtnl_lock() | | ice_down() | | | test_bit(ICE_VSI_DOWN) - true | | ice_dis_vsi() returns | ice_up() | | | proceeds to rebuild a running VSI | .ndo_bpf() is not the only rtnl-locked callback that toggles the interface to apply new configuration. Another example is .set_channels(). To avoid the race condition above, act only after reading ICE_VSI_DOWN under rtnl_lock. Fixes: 0f9d5027a749 ("ice: Refactor VSI allocation, deletion and rebuild flow") Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com> Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com> --- drivers/net/ethernet/intel/ice/ice_lib.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)