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

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

[Linkui Xiao]

From: Linkui Xiao <xiaolinkui@kylinos.cn>

__ice_set_vf_mac(), ice_set_vf_trust() and ice_set_vf_port_vlan() all
check that the VF is ready for configuration, take vf->cfg_lock and then
call ice_reset_vf() to push the new setting 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.

The check runs before the lock is taken, so a reset can disable the VF
in between. ice_reset_all_vfs() raises ICE_VF_DIS in pf->state before it
walks the VF table, rebuilds every VSI and clears the bit again. An
ndo_set_vf_mac() that passed ice_check_vf_ready_for_cfg() just before
that then waits for cfg_lock, stores the new address in vf->dev_lan_addr
and vf->hw_lan_addr and calls ice_reset_vf(), which sees the VF
disabled, stops its queues and returns 0 without reprogramming anything.
__ice_set_vf_mac() reports success, so ip link and the devlink port
function now show a MAC address the hardware never got, and the switch
keeps enforcing the previous one until something else resets the VF.

ice_set_vf_trust() and ice_set_vf_port_vlan() have the same shape: the
port VLAN lands in vf->port_vlan_info and the trust flag in vf->trusted
before the reset is skipped.

Re-check ice_is_vf_disabled() once cfg_lock is held and fail with
-EINVAL, the same code ice_check_vf_ready_for_cfg() returns for a
disabled VF. The VF is being reset or removed at that point, so the
caller should retry rather than be told the setting is in place.
Changing what ice_reset_vf() returns on that path is not an option, too
many callers rely on it 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.

Fixes: e6ba5273d4ed ("ice: Fix race conditions between virtchnl handling and VF ndo ops")
Signed-off-by: Linkui Xiao <xiaolinkui@kylinos.cn>
---
 drivers/net/ethernet/intel/ice/ice_sriov.c | 30 ++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.c b/drivers/net/ethernet/intel/ice/ice_sriov.c
index e04de0215596..de0631673149 100644
--- a/drivers/net/ethernet/intel/ice/ice_sriov.c
+++ b/drivers/net/ethernet/intel/ice/ice_sriov.c
@@ -1327,6 +1327,15 @@ int __ice_set_vf_mac(struct ice_pf *pf, u16 vf_id, const u8 *mac)
 
 	mutex_lock(&vf->cfg_lock);
 
+	/* The VF can have been disabled by a reset after the check above, and
+	 * ice_reset_vf() then skips the reprogram and still returns 0. Fail
+	 * here instead of reporting a MAC address that hardware never got.
+	 */
+	if (ice_is_vf_disabled(vf)) {
+		ret = -EINVAL;
+		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 +1354,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:
@@ -1401,6 +1412,14 @@ int ice_set_vf_trust(struct net_device *netdev, int vf_id, bool trusted)
 
 	mutex_lock(&vf->cfg_lock);
 
+	/* See __ice_set_vf_mac(), the VF can have been disabled after the
+	 * check above and ice_reset_vf() would skip the reprogram.
+	 */
+	if (ice_is_vf_disabled(vf)) {
+		ret = -EINVAL;
+		goto out_unlock;
+	}
+
 	while (!trusted && vf->num_mac_lldp)
 		ice_vf_update_mac_lldp_num(vf, ice_get_vf_vsi(vf), false);
 
@@ -1409,6 +1428,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:
@@ -1721,6 +1741,14 @@ ice_set_vf_port_vlan(struct net_device *netdev, int vf_id, u16 vlan_id, u8 qos,
 
 	mutex_lock(&vf->cfg_lock);
 
+	/* See __ice_set_vf_mac(), the VF can have been disabled after the
+	 * check above and ice_reset_vf() would skip the reprogram.
+	 */
+	if (ice_is_vf_disabled(vf)) {
+		ret = -EINVAL;
+		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",
@@ -1729,6 +1757,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

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

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

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

[Linkui Xiao]

On 2026/10/12 16:34, netdev-bot+sinfo@kernel.org wrote:
> Hi!
> 
> This is an automated message. This series looks like a fix, but its
> commit messages seem to be missing some information:
> 
>   - What hardware the change was tested on. For driver fixes please
>     mention the device (and if relevant firmware version) used for
>     testing, or say that the change was not tested on real hardware.
> 
> Please do not repost the series just to address the above. Instead,
> reply to this email with the missing information, so that reviewers
> can take it into account. If the series needs another revision for
> other reasons, please include the information in the commit messages
> then.
> 
> The evaluation is done by an LLM so it may be wrong, if you think
> that is the case please reply and explain.

Hi,
Thanks for the note. The automated check is correct; the commit message is
missing the hardware-testing information. Here it is.

This change was not tested on real hardware, and there is no Tested-by tag
for exactly that reason. The race was found by code inspection of the ndo VF
handlers; it was not reproduced at runtime.

What I verified is limited to build and static checks:

- ice_sriov.o was build tested only, with the native kbuild command line of
   an existing v7.3-rc2 x86_64 build tree (gcc 9.3.0), both with the normal
   -Wall set and with -Wextra added. The diagnostics for the baseline file
   and for the patched file are identical (zero), so the patch adds no new
   warnings. The only new undefined symbol in the object is
   ice_is_vf_disabled(), the helper the patch starts calling.
- checkpatch.pl --strict: 0 errors, 0 warnings, 0 checks.
- The patch applies to e04de0215596 with no offset, and at the object level
   only __ice_set_vf_mac(), ice_set_vf_trust() and ice_set_vf_port_vlan()
   gain an ice_is_vf_disabled() call; no other function in the file is
   affected.

I could not run it on hardware: reaching the window needs an ice PF with
SR-IOV enabled and VFs assigned, plus a PF-wide VF reset racing with a VF
configuration request. The reset side is the ice_reset_all_vfs() path, i.e.
a PF-wide reset: a PFR (ethtool --reset <pf> pfr is one way to ask for one)
or a CORER/GLOBR/EMPR completion. The other side is an
"ip link set dev <pf> vf <n> mac|trust|vlan ..." request. I do not have
access to such a system.

For anyone who does have one: on the unpatched baseline the visible symptom
is that "ip link show <pf>" and the VF's devlink port report the new setting
while the hardware and the switch keep using the old one. With the patch the
request should come back with -EINVAL instead of reporting success. I am
happy to run any specific test that is suggested.

No repost was made, as requested. If a v2 is needed for other reasons, I'll
include this information in the commit messages.

Thanks,
Linkui