[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <d08eeee9-1178-4f33-b22a-a32d6f3dc086@intel.com>
Date: Wed, 24 Jul 2024 11:27:16 -0700
From: Jacob Keller <jacob.e.keller@...el.com>
To: Larysa Zaremba <larysa.zaremba@...el.com>,
<intel-wired-lan@...ts.osuosl.org>
CC: Tony Nguyen <anthony.l.nguyen@...el.com>, "David S. Miller"
<davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski
<kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Alexei Starovoitov
<ast@...nel.org>, Daniel Borkmann <daniel@...earbox.net>, "Jesper Dangaard
Brouer" <hawk@...nel.org>, John Fastabend <john.fastabend@...il.com>, "Maciej
Fijalkowski" <maciej.fijalkowski@...el.com>, <netdev@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <bpf@...r.kernel.org>,
<magnus.karlsson@...el.com>, Michal Kubiak <michal.kubiak@...el.com>,
Wojciech Drewek <wojciech.drewek@...el.com>, Amritha Nambiar
<amritha.nambiar@...el.com>
Subject: Re: [PATCH iwl-net v2 4/6] ice: check ICE_VSI_DOWN under rtnl_lock
when preparing for reset
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@...el.com>
> Signed-off-by: Larysa Zaremba <larysa.zaremba@...el.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@...el.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);
> }
> }
Powered by blists - more mailing lists