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

[Petr Oros <poros@redhat.com>]

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@redhat.com>
>> ---
>>   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,
> Marcin
>
>> +
>>   	if (test_bit(ICE_FLAG_RSS_ENA, pf->flags))
>>   		ice_rss_clean(vsi);
>>   
>