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

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

[Petr Oros]

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);
+
 	if (test_bit(ICE_FLAG_RSS_ENA, pf->flags))
 		ice_rss_clean(vsi);
 
-- 
2.53.0

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

[Marcin Szycik]

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?

Thanks,
Marcin

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

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

[Petr Oros]

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);
>>   
>

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@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 for the writeup, sounds reasonable.

Reviewed-by: Marcin Szycik <marcin.szycik@linux.intel.com>

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

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

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
ice: clear the default forwarding VSI rule when releasing a VSI

This patch restores the cleanup of the default forwarding VSI rule
in ice_vsi_release(), clearing it when the VSI owns it. This prevents
a rule leak if a trusted VF enters promiscuous mode and is then destroyed.

> diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
> --- a/drivers/net/ethernet/intel/ice/ice_lib.c
> +++ b/drivers/net/ethernet/intel/ice/ice_lib.c
> @@ -2871,6 +2871,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);
> +

[Severity: High]
Is there a time-of-check to time-of-use race here?

ice_is_vsi_dflt_vsi() acquires and releases the filt_rule_lock to check if
the current VSI is the default. If true, ice_clear_dflt_vsi() is called,
which acquires the lock again during rule removal.

If the lock is dropped between these two calls, could another thread
establish a new default VSI rule before ice_clear_dflt_vsi() runs?

When ice_clear_dflt_vsi() executes, it appears the rule search only matches
by l_data and flag:

ice_switch.c:ice_find_rule_entry() {
	if (!memcmp(&f_info->l_data, &list_itr->fltr_info.l_data,
		    sizeof(f_info->l_data)) &&
	    f_info->flag == list_itr->fltr_info.flag) {
        ...
}

For the default VSI rule, l_data is empty, so it might match any newly
established default rule.

And since ice_remove_rule_internal() does not appear to validate the
vsi_handle for non-list rules:

ice_switch.c:ice_remove_rule_internal() {
	if (list_elem->fltr_info.fltr_act != ICE_FWD_TO_VSI_LIST) {
		remove_rule = true;
        ...
}

Could this blindly remove the default forwarding configuration for a
completely unrelated VSI?

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

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

[Petr Oros]

On 6/23/26 12:29, Simon Horman wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> Full review at: https://sashiko.dev
> ---
> ice: clear the default forwarding VSI rule when releasing a VSI
>
> This patch restores the cleanup of the default forwarding VSI rule
> in ice_vsi_release(), clearing it when the VSI owns it. This prevents
> a rule leak if a trusted VF enters promiscuous mode and is then destroyed.
>
>> diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
>> --- a/drivers/net/ethernet/intel/ice/ice_lib.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_lib.c
>> @@ -2871,6 +2871,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);
>> +
> [Severity: High]
> Is there a time-of-check to time-of-use race here?
>
> ice_is_vsi_dflt_vsi() acquires and releases the filt_rule_lock to check if
> the current VSI is the default. If true, ice_clear_dflt_vsi() is called,
> which acquires the lock again during rule removal.
>
> If the lock is dropped between these two calls, could another thread
> establish a new default VSI rule before ice_clear_dflt_vsi() runs?
>
> When ice_clear_dflt_vsi() executes, it appears the rule search only matches
> by l_data and flag:
>
> ice_switch.c:ice_find_rule_entry() {
> 	if (!memcmp(&f_info->l_data, &list_itr->fltr_info.l_data,
> 		    sizeof(f_info->l_data)) &&
> 	    f_info->flag == list_itr->fltr_info.flag) {
>          ...
> }
>
> For the default VSI rule, l_data is empty, so it might match any newly
> established default rule.
>
> And since ice_remove_rule_internal() does not appear to validate the
> vsi_handle for non-list rules:
>
> ice_switch.c:ice_remove_rule_internal() {
> 	if (list_elem->fltr_info.fltr_act != ICE_FWD_TO_VSI_LIST) {
> 		remove_rule = true;
>          ...
> }
>
> Could this blindly remove the default forwarding configuration for a
> completely unrelated VSI?
  The dropped lock isn't new here. The whole dflt_vsi API is check-then-act.
  ice_vsi_sync_fltr() does the same if (ice_is_vsi_dflt_vsi(vsi))
  ice_clear_dflt_vsi(vsi), and this path runs under vf->cfg_lock, the same
  domain as the ice_vf_clear_all_promisc_modes() cleanup it restores. There
  is at most one DFLT rule per direction, because a second default VSI folds
  both into one ICE_FWD_TO_VSI_LIST, which is the leak this fixes, so the
  empty l_data match is unambiguous. In that list case removal honors the
  handle via ice_rem_update_vsi_list() and drops only the requested VSI. The
  unvalidated whole rule branch is only the single VSI case where that 
VSI is
  the sole default, so removing it is intended. An unrelated removal would
  require another context to clear this VSI and install a different sole
  default in the gap, but those flows are serialized per context with rtnl,
  vf->cfg_lock and ICE_CFG_BUSY.

Regards,

Petr


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