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

[Petr Oros <poros@redhat.com>]

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