> -----Original Message-----
> From: Linkui Xiao <[email protected]>
> Sent: Wednesday, October 21, 2026 10:13 AM
> To: Keller, Jacob E <[email protected]>; Nguyen, Anthony L
> <[email protected]>; Kitszel, Przemyslaw
> <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected]
> Cc: [email protected]; [email protected]; linux-
> [email protected]; Linkui Xiao <[email protected]>
> Subject: [PATCH iwl-net v2] ice: do not report success for a VF setting
> that was not applied
>
> From: Linkui Xiao <[email protected]>
>
> __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 <[email protected]>
> ---
> 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-
> [email protected]/
> 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 <[email protected]>
