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

[Linkui Xiao <xiaolinkui@126.com>]

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.

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.

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,
Linkui