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

[Jacob Keller <jacob.e.keller@intel.com>]

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 <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.
>>>
>> 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/20261021081248.261762-1-xiaolinkui@126.com/
> 

Thanks for educating me :)

> Thanks,
> Linkui
>