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

[Linkui Xiao <xiaolinkui@126.com>]

From: Linkui Xiao <xiaolinkui@kylinos.cn>

__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@kylinos.cn>
---
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@126.com/
 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