On 10/21/2026 1:20 AM, Linkui Xiao wrote: > Hi Jacob, > > thanks for the review. v2 follows your suggestion, with one adjustment. > > On 2026/10/21 03:10, Jacob Keller wrote: >> On 10/12/2026 1:29 AM, Linkui Xiao wrote: >>> From: Linkui Xiao <[email protected]> >>> >>> __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. >>> >> Hm. My recollection is that we should set something on the PF side that >> stores the new address so that the VF would get that value when next >> configured.. But I guess that isn't how ice works. > > It is how ice works, and that store is why one of the two interleavings > is harmless. __ice_set_vf_mac() already keeps the address in vf- >>dev_lan_addr / vf->hw_lan_addr, and every rebuild replays it through > ice_vf_rebuild_host_mac_cfg(). The bug needs a window where nothing > configures the VF again: ice_reset_all_vfs() rebuilds VF n under its > cfg_lock, drops the lock, moves on, and only clears ICE_VF_DIS after the > last VF. If the ndo handler takes cfg_lock in that gap, it stores the > setting, its own reset is skipped, and the PF-wide reset is already past > this VF, so no rebuild ever replays it. If the handler instead wins the > lock before the rebuild loop reaches this VF, the follow-up rebuild does > replay the store. That is why v2 fails the request rather than trying to > re-arm a reset of its own.
Makes sense. > > Worth noting the ICE_VF_STATE_INIT half is not what catches this window: > ice_vf_set_initialized() has already run for the VF by then, only the > PF-wide ICE_VF_DIS bit shows the reset is still in flight. > > Two things I would like your opinion on: > > -EINVAL is what ice_check_vf_ready_for_cfg() returns for a disabled VF, > so v2 keeps it, but -EBUSY would say “retry” more clearly. Easy to > switch if you prefer. > > ice_repr_ethtool_reset() has the same shape and also reports success > when the reset was skipped. It stores no setting, so I left it out of > this patch, but happy to fold it in. >> >> Why not just move ice_check_vf_ready_for_cfg() inside the mutex though? > > v2 moves the flag tests inside cfg_lock, but not the sleeping half. > ice_check_vf_ready_for_cfg() starts with ice_wait_on_vf_reset(), 40 x 20 > ms of msleep 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. > Under the lock the wait could never observe the transition, it would > burn the full 800 ms and still return -EINVAL, while stalling the reset > and the VF mailbox that ice_vc_process_vf_msg() serves under the same > lock. So the wait stays outside, and the flag tests are repeated inside > by ice_check_vf_ready_for_cfg_locked(), which replaces the three ad-hoc > ice_is_vf_disabled() tests of v1. > Oh. I didn't realize the check_vf_ready_for_cfg() would sleep! Should have read it more carefully :\ Do we even need the checks outside the loop then? I guess they aren't harmful. > v2 is posted as “[PATCH iwl-net v2] ice: do not report success for a VF > setting that was not applied”, rebased on top of 0a01e1df137f (“ice: > skip unnecessary VF reset when setting trust”): > > https://lore.kernel.org/netdev/[email protected]/ > Thanks for educating me :) > Thanks, > Linkui >
