RE: [PATCH iwl-net v2] ice: do not report success for a VF setting that was not applied

[Loktionov, Aleksandr]

> -----Original Message-----
> From: Linkui Xiao <xiaolinkui@xxxxxxx>
> Sent: Wednesday, October 21, 2026 10:13 AM
> To: Keller, Jacob E <jacob.e.keller@xxxxxxxxx>; Nguyen, Anthony L
> <anthony.l.nguyen@xxxxxxxxx>; Kitszel, Przemyslaw
> <przemyslaw.kitszel@xxxxxxxxx>; andrew+netdev@xxxxxxx;
> davem@xxxxxxxxxxxxx; kuba@xxxxxxxxxx; pabeni@xxxxxxxxxx
> Cc: intel-wired-lan@xxxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; Linkui Xiao <xiaolinkui@xxxxxxxxxx>
> Subject: [PATCH iwl-net v2] ice: do not report success for a VF setting
> that was not applied
> 
> From: Linkui Xiao <xiaolinkui@xxxxxxxxxx>
> 
> __ice_set_vf_mac(), ice_set_vf_trust() and ice_set_vf_port_vlan() store
> the new setting in the VF structure and then call ice_reset_vf() to push
> it into hardware. The result of ice_reset_vf() is not looked at, and it
> could not be: it returns 0 both when the reset ran and when it bailed
> out because the VF is disabled.
> 
> Whether the reset is skipped is decided by ice_is_vf_disabled(), while
> the readiness check that guards these handlers runs before cfg_lock is
> taken:
> 
> 	ret = ice_check_vf_ready_for_cfg(vf);
> 	if (ret)
> 		goto out_put_vf;
> 
> 	mutex_lock(&vf->cfg_lock);
> 
> A PF-wide reset can therefore start in between. ice_reset_all_vfs()
> raises ICE_VF_DIS in pf->state, rebuilds every VF under its cfg_lock and
> only clears the bit again once the last one is done. A handler that
> passed the check just before that still gets cfg_lock, and two
> interleavings follow:
> 
>  - It wins the lock before the rebuild loop reaches this VF. It stores
>    the setting, its own reset is skipped, and ice_reset_all_vfs() then
>    rebuilds the VF and replays the stored setting through
>    ice_vf_rebuild_host_cfg(). Nothing is lost.
> 
>  - It gets the lock after this VF has already been rebuilt but before
>    ICE_VF_DIS is cleared. ice_vf_set_initialized() has run, so the VF
>    looks fully initialised and only the PF-wide bit betrays the reset
>    still in flight. The setting is stored, the reset is skipped, and
>    ice_reset_all_vfs() is done with this VF, so nothing replays it.
> 
> For a MAC address that leaves the switch without a filter for the new
> vf->hw_lan_addr, while ip link and the devlink port function already
> report it, and vf->pf_set_mac makes ice_vc_can_add_mac() refuse the
> address the VF would otherwise choose for itself. The VF stays
> unreachable until something else resets it. vf->trusted and
> vf->port_vlan_info are stored the same way without being applied.
> 
> Repeat the readiness check with cfg_lock held, so that the second
> interleaving fails with -EINVAL instead of reporting a setting that
> hardware never got. The caller can retry once the reset is over. Only
> the flag tests can move inside: ice_check_vf_ready_for_cfg() starts with
> ice_wait_on_vf_reset(), which sleeps up to 40 x 20 ms polling for
> ICE_VF_STATE_INIT, and the resets that clear that bit set it again from
> ice_vf_post_vsi_rebuild() with cfg_lock held. Waiting under cfg_lock
> could never observe that transition, it would only stall the reset and
> the VF mailbox, which ice_vc_process_vf_msg() serves under the same
> lock. The wait therefore stays outside and the flag tests are repeated
> inside.
> 
> Changing what ice_reset_vf() returns on the skipped path is not an
> option, too many callers rely on 0 meaning "nothing to do".
> 
> The race was found by code inspection of the ndo VF handlers, it was not
> reproduced at runtime and no error message was observed.
> 
> The change has not been tested on real hardware, it was only build
> tested.
> 
> Fixes: e6ba5273d4ed ("ice: Fix race conditions between virtchnl handling
> and VF ndo ops")
> Signed-off-by: Linkui Xiao <xiaolinkui@xxxxxxxxxx>
> ---
> v2:
>  - Rebased: ice_set_vf_trust() changed under the patch, see 0a01e1df137f
>    ("ice: skip unnecessary VF reset when setting trust").
>  - Move the readiness check inside the mutex as suggested. Only the flag
>    tests can move there: ice_check_vf_ready_for_cfg() starts with the
>    sleeping ice_wait_on_vf_reset(), which polls for ICE_VF_STATE_INIT, a
> bit
>    the reset paths set again with cfg_lock held. Under cfg_lock it would
>    always time out and stall the reset and the VF mailbox for the full
>    800 ms before failing with -EINVAL anyway. The wait stays outside the
>    lock and the flag tests are repeated inside it by the new
>    ice_check_vf_ready_for_cfg_locked(), which replaces the three ad-hoc
>    ice_is_vf_disabled() tests of v1.
>  - Extend the changelog with both interleavings, including the one where
>    ice_reset_all_vfs() replays the stored setting after the skipped
> reset.
> 
> v1:
>  - Link: https://lore.kernel.org/netdev/20261012082910.283229-1-
> xiaolinkui@xxxxxxx/
>  drivers/net/ethernet/intel/ice/ice_sriov.c | 41 ++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.c
> b/drivers/net/ethernet/intel/ice/ice_sriov.c
> index f893dff39aac..6c5d5b332d09 100644
> --- a/drivers/net/ethernet/intel/ice/ice_sriov.c
> +++ b/drivers/net/ethernet/intel/ice/ice_sriov.c
> @@ -1289,6 +1289,30 @@ ice_get_vf_cfg(struct net_device *netdev, int
> vf_id, struct ifla_vf_info *ivi)
>  	return ret;
>  }
> 
> +/**
> + * ice_check_vf_ready_for_cfg_locked - re-check VF readiness under
> +cfg_lock
> + * @vf: VF to check
> + *
> + * The flag tests of ice_check_vf_ready_for_cfg(), repeated with
> +vf->cfg_lock
> + * held. ice_wait_on_vf_reset() is left out on purpose: it sleeps while
> +it
> + * polls for ICE_VF_STATE_INIT, and the resets that clear that bit set
> +it
> + * again from ice_vf_post_vsi_rebuild() with cfg_lock held, so the wait
> +could
> + * never observe the transition and would only hold the lock for its
> +full
> + * timeout.
> + *
> + * Return: 0 if the VF can still be configured, -EINVAL if it is
> +disabled or
> + * -EBUSY if it is in reset.
> + */
> +static int ice_check_vf_ready_for_cfg_locked(struct ice_vf *vf) {
> +	lockdep_assert_held(&vf->cfg_lock);
> +
> +	if (ice_is_vf_disabled(vf))
> +		return -EINVAL;
> +
> +	return ice_check_vf_init(vf);
> +}
> +
>  /**
>   * __ice_set_vf_mac - program VF MAC address
>   * @pf: PF to be configure
> @@ -1327,6 +1351,10 @@ int __ice_set_vf_mac(struct ice_pf *pf, u16
> vf_id, const u8 *mac)
> 
>  	mutex_lock(&vf->cfg_lock);
> 
> +	ret = ice_check_vf_ready_for_cfg_locked(vf);
> +	if (ret)
> +		goto out_unlock;
> +
>  	/* VF is notified of its new MAC via the PF's response to the
>  	 * VIRTCHNL_OP_GET_VF_RESOURCES message after the VF has been
> reset
>  	 */
> @@ -1345,6 +1373,8 @@ int __ice_set_vf_mac(struct ice_pf *pf, u16 vf_id,
> const u8 *mac)
>  	}
> 
>  	ice_reset_vf(vf, ICE_VF_RESET_NOTIFY);
> +
> +out_unlock:
>  	mutex_unlock(&vf->cfg_lock);
> 
>  out_put_vf:
> @@ -1415,6 +1445,10 @@ int ice_set_vf_trust(struct net_device *netdev,
> int vf_id, bool trusted)
> 
>  	mutex_lock(&vf->cfg_lock);
> 
> +	ret = ice_check_vf_ready_for_cfg_locked(vf);
> +	if (ret)
> +		goto out_unlock;
> +
>  	/* Reset only if revoking trust and VF has advanced features
> configured */
>  	if (!trusted &&
>  	    (vf->num_mac_lldp > 0 ||
> @@ -1431,6 +1465,7 @@ int ice_set_vf_trust(struct net_device *netdev,
> int vf_id, bool trusted)
>  	dev_info(ice_pf_to_dev(pf), "VF %u is now %strusted\n",
>  		 vf_id, trusted ? "" : "un");
> 
> +out_unlock:
>  	mutex_unlock(&vf->cfg_lock);
> 
>  out_put_vf:
> @@ -1743,6 +1778,10 @@ ice_set_vf_port_vlan(struct net_device *netdev,
> int vf_id, u16 vlan_id, u8 qos,
> 
>  	mutex_lock(&vf->cfg_lock);
> 
> +	ret = ice_check_vf_ready_for_cfg_locked(vf);
> +	if (ret)
> +		goto out_unlock;
> +
>  	vf->port_vlan_info = ICE_VLAN(local_vlan_proto, vlan_id, qos);
>  	if (ice_vf_is_port_vlan_ena(vf))
>  		dev_info(dev, "Setting VLAN %u, QoS %u, TPID 0x%04x on VF
> %d\n", @@ -1751,6 +1790,8 @@ ice_set_vf_port_vlan(struct net_device
> *netdev, int vf_id, u16 vlan_id, u8 qos,
>  		dev_info(dev, "Clearing port VLAN on VF %d\n", vf_id);
> 
>  	ice_reset_vf(vf, ICE_VF_RESET_NOTIFY);
> +
> +out_unlock:
>  	mutex_unlock(&vf->cfg_lock);
> 
>  out_put_vf:
> --
> 2.25.1

Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@xxxxxxxxx>