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@xxxxxxxxx>
> Signed-off-by: Larysa Zaremba <larysa.zaremba@xxxxxxxxx>
> ---
> 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@xxxxxxxxx>
> 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);
> }
> }
