Re: [Intel-wired-lan] [PATCH iwl-net] ice: clear the default forwarding VSI rule when releasing a VSI

[Marcin Szycik]

On 22.06.2026 17:30, Petr Oros wrote:
> 
> On 6/22/26 15:52, Marcin Szycik wrote:
>>
>> On 22/06/2026 10:10, Petr Oros wrote:
>>> When a VSI is configured as the switch's default forwarding VSI
>>> (ICE_SW_LKUP_DFLT) and is then torn down, the rule is left behind in
>>> the switch. ice_vsi_release() no longer removes it, and the SR-IOV VF
>>> free path (ice_free_vfs() -> ice_free_vf_res() -> ice_vf_vsi_release()
>>> -> ice_vsi_release()) does not disable promiscuous mode either, which
>>> only happens on VF reset in ice_vf_clear_all_promisc_modes().
>>>
>>> A trusted VF that enters unicast promiscuous mode becomes the default
>>> forwarding VSI (this is the default mode, when the PF does not have VF
>>> true-promiscuous mode enabled). If the VFs are then destroyed without
>>> the VF first leaving promiscuous mode, the ICE_SW_LKUP_DFLT rule for
>>> the now-freed VSI is leaked. When VFs are recreated, a VSI reuses the
>>> freed hw_vsi_id. If it is assigned a different VSI handle than the
>>> leaked rule holds, ice_set_dflt_vsi() does not recognize it as
>>> already-default, and ice_add_update_vsi_list() folds the dangling
>>> (freed) handle into a VSI list, which the firmware rejects. The VSI
>>> handle assigned on re-creation varies, so the failure is intermittent
>>> rather than every cycle.
>>>
>>> Reproduce by repeatedly running the cycle below on the two ports of the
>>> same card, where $VF0 and $VF1 are the netdevs of vf 15 once they
>>> appear. The VF must be brought up so iavf actually pushes the unicast
>>> promiscuous request, and the rule must settle before the VFs are torn
>>> down again:
>>>
>>>    echo 16 > /sys/class/net/$PF0/device/sriov_numvfs
>>>    echo 16 > /sys/class/net/$PF1/device/sriov_numvfs
>>>    ip link set $PF0 vf 15 trust on
>>>    ip link set $PF1 vf 15 trust on
>>>    ip link set $VF0 up
>>>    ip link set $VF1 up
>>>    ip link set $VF0 promisc on
>>>    ip link set $VF1 promisc on
>>>    sleep 1
>>>    echo 0 > /sys/class/net/$PF0/device/sriov_numvfs
>>>    echo 0 > /sys/class/net/$PF1/device/sriov_numvfs
>>>
>>> Within a few cycles the ice PF and iavf VF log:
>>>
>>>    Failed to set VSI 25 as the default forwarding VSI, error -22
>>>    Turning on/off promiscuous mode for VF 63 failed, error: -22
>>>    PF returned error -53 (IAVF_ERR_ADMIN_QUEUE_ERROR) to our request 14
>>>
>>> This cleanup used to live in ice_vsi_release() but was dropped by the
>>> referenced refactor. Restore it. Clear the default forwarding VSI rule
>>> in ice_vsi_release() when this VSI owns it, which covers every teardown
>>> path.
>>>
>>> Fixes: 6624e780a577 ("ice: split ice_vsi_setup into smaller functions")
>>> Signed-off-by: Petr Oros <poros@xxxxxxxxxx>
>>> ---
>>>   drivers/net/ethernet/intel/ice/ice_lib.c | 3 +++
>>>   1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
>>> index 2717cc31bff8fe..408464434506ef 100644
>>> --- a/drivers/net/ethernet/intel/ice/ice_lib.c
>>> +++ b/drivers/net/ethernet/intel/ice/ice_lib.c
>>> @@ -2872,6 +2872,9 @@ int ice_vsi_release(struct ice_vsi *vsi)
>>>           return -ENODEV;
>>>       pf = vsi->back;
>>>   +    if (ice_is_vsi_dflt_vsi(vsi))
>>> +        ice_clear_dflt_vsi(vsi);
>> In the referenced commit, the chunk of code that contained these missing 2 lines
>> was moved to ice_vsi_decfg(). It also sounds like a good place for them and will
>> be called from ice_vsi_release(). Are you sure we should place them directly in
>> ice_vsi_release() instead?
> No, ice_vsi_decfg() is not a good place for them because it is not
> release only. It also runs on the rebuild and reconfig paths
> (ice_vsi_rebuild(), ice_vf_reconfig_vsi(), the ice_vsi_cfg() error
> path), where the VSI is reconfigured in place and stays alive, so it
> can still be the default VSI afterwards.
> 
> Before the refactor the release-path clear lived only in
> ice_vsi_release() and the old ice_vsi_rebuild() never cleared it.
> Putting it in ice_vsi_decfg() would also clear the default VSI whenever
> the default VSI itself is reset or reconfigured, which the original
> code never did. ice_vsi_release() keeps it to the case where the owning
> VSI is actually torn down, and the ice_is_vsi_dflt_vsi() guard makes it
> a no-op everywhere else.
> 
> So I would prefer to keep it in ice_vsi_release().
> 
> Regards,
> 
> Petr

Thanks for the writeup, sounds reasonable.

Reviewed-by: Marcin Szycik <marcin.szycik@xxxxxxxxxxxxxxx>

> 
>> Thanks,
>> Marcin
>>
>>> +
>>>       if (test_bit(ICE_FLAG_RSS_ENA, pf->flags))
>>>           ice_rss_clean(vsi);
>>>   
>>
>