Re: [Intel-wired-lan] [PATCH iwl-net] ice: fix LAG and VF lock dependency in ice_reset_vf()

[Przemek Kitszel <przemyslaw.kitszel@intel.com>]

On 4/9/24 01:03, Jacob Keller wrote:
> 9f74a3dfcf83 ("ice: Fix VF Reset paths when interface in a failed over
> aggregate"), the ice driver has acquired the LAG mutex in ice_reset_vf().
> The commit placed this lock acquisition just prior to the acquisition of
> the VF configuration lock.
> 
> If ice_reset_vf() acquires the configuration lock via the ICE_VF_RESET_LOCK
> flag, this could deadlock with ice_vc_cfg_qs_msg() because it always
> acquires the locks in the order of the VF configuration lock and then the
> LAG mutex.
> 
> Lockdep reports this violation almost immediately on creating and then
> removing 2 VF:
> 

[...]

> To avoid deadlock, we must acquire the LAG mutex only after acquiring the
> VF configuration lock. Fix the ice_reset_vf() to acquire the LAG mutex only
> after we either acquire or check that the VF configuration lock is held.
> 
> Fixes: 9f74a3dfcf83 ("ice: Fix VF Reset paths when interface in a failed over aggregate")
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> Reviewed-by: Dave Ertman <david.m.ertman@intel.com>
> ---
>   drivers/net/ethernet/intel/ice/ice_vf_lib.c | 16 ++++++++--------
>   1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib.c b/drivers/net/ethernet/intel/ice/ice_vf_lib.c
> index 21d26e19338a..d10a4be965b5 100644
> --- a/drivers/net/ethernet/intel/ice/ice_vf_lib.c
> +++ b/drivers/net/ethernet/intel/ice/ice_vf_lib.c
> @@ -856,6 +856,11 @@ int ice_reset_vf(struct ice_vf *vf, u32 flags)
>   		return 0;
>   	}
>   
> +	if (flags & ICE_VF_RESET_LOCK)
> +		mutex_lock(&vf->cfg_lock);
> +	else
> +		lockdep_assert_held(&vf->cfg_lock);
> +
>   	lag = pf->lag;
>   	mutex_lock(&pf->lag_mutex);
>   	if (lag && lag->bonded && lag->primary) {
> @@ -867,11 +872,6 @@ int ice_reset_vf(struct ice_vf *vf, u32 flags)
>   			act_prt = ICE_LAG_INVALID_PORT;
>   	}
>   
> -	if (flags & ICE_VF_RESET_LOCK)
> -		mutex_lock(&vf->cfg_lock);
> -	else
> -		lockdep_assert_held(&vf->cfg_lock);
> -
>   	if (ice_is_vf_disabled(vf)) {
>   		vsi = ice_get_vf_vsi(vf);
>   		if (!vsi) {
> @@ -956,14 +956,14 @@ int ice_reset_vf(struct ice_vf *vf, u32 flags)
>   	ice_mbx_clear_malvf(&vf->mbx_info);
>   
>   out_unlock:
> -	if (flags & ICE_VF_RESET_LOCK)
> -		mutex_unlock(&vf->cfg_lock);
> -
>   	if (lag && lag->bonded && lag->primary &&
>   	    act_prt != ICE_LAG_INVALID_PORT)
>   		ice_lag_move_vf_nodes_cfg(lag, pri_prt, act_prt);
>   	mutex_unlock(&pf->lag_mutex);
>   
> +	if (flags & ICE_VF_RESET_LOCK)
> +		mutex_unlock(&vf->cfg_lock);
> +
>   	return err;
>   }
>   
> 
> base-commit: 3ca3256cde596573d060eda8c477996435c6d63f

Thanks,
Tested-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>