Re: [PATCH iwl-net v2 1/2] ice: skip per-VLAN promisc rules when default VSI Rx rule is set

[Petr Oros <poros@redhat.com>]

On 6/23/26 18:25, 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
> ---
> Subject: ice: skip per-VLAN promisc rules when default VSI Rx rule is set
>
> This patch optimizes the number of hardware switch rules used when an ice
> port is part of a vlan-filtering bridge. It avoids installing per-VLAN
> promiscuous rules if the broad default VSI Rx rule is already catching
> all traffic.
>
>> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
>> index e2fbe111f8496..efb1f536de6c7 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_main.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> [ ... ]
>> @@ -442,17 +496,12 @@ static int ice_vsi_sync_fltr(struct ice_vsi *vsi)
> [Severity: High]
> This is a pre-existing issue, but does MAC filter overflow fail to force
> promiscuous mode, resulting in silent packet drops?
Pre-existing; the promisc_forced_on fallback is not changed by this patch.

>
> When the hardware MAC filter limit is reached (LIBIE_AQ_RC_ENOSPC), the driver
> attempts to fall back to promiscuous mode by setting promisc_forced_on = true.
>
> However, the execution flow then reaches a check for IFF_PROMISC:
>
>      if (vsi->current_netdev_flags & IFF_PROMISC) {
>          /* Apply Rx filter rule to get traffic from wire */
>          ...
>      } else {
>          /* Clear Rx filter to remove traffic from wire */
>
> Since the user never explicitly requested IFF_PROMISC, this evaluates to false,
> causing the driver to take the else branch. The else branch actively disables
> the default VSI rule and clears promiscuous mode instead of enabling it. The
> fallback feature appears to be non-functional, leading to lost traffic for
> newly added MACs.
>
> [Severity: High]
> This is a pre-existing issue, but could this function prevent retries of
> incomplete configurations and break multicast promiscuity if any default
> VSI is already active?
v3 will address the multicast half: ice_set_promisc(ICE_MCAST_PROMISC_BITS)
will be issued whenever the netdev is promiscuous, not only inside the
!ice_is_dflt_vsi_in_use() branch.

>
> In ice_vsi_sync_fltr(), the setup of multicast promiscuity and disabling of
> Rx filtering is gated:
>
>      if (!ice_is_dflt_vsi_in_use(vsi->port_info)) {
>          err = ice_set_dflt_vsi(vsi);
>          ...
>          err = ice_set_promisc(vsi, ICE_MCAST_PROMISC_BITS);
>          if (err)
>              goto out_promisc;
>      }
>
> If ice_set_dflt_vsi() succeeds but the subsequent ice_set_promisc() fails,
> the function jumps to out_promisc to trigger a retry. On the next cycle,
> ice_is_dflt_vsi_in_use() returns true because this VSI is now the default,
> causing the entire block to be skipped and leaving the VSI misconfigured.
>
> Furthermore, if another VSI (such as a VF) is already the default VSI,
> the PF VSI entirely skips setting multicast promiscuity, breaking multicast
> reception in promiscuous mode.
>
>>   		} else {
>>   			/* Clear Rx filter to remove traffic from wire */
>>   			if (ice_is_vsi_dflt_vsi(vsi)) {
>> -				err = ice_clear_dflt_vsi(vsi);
>> +				err = ice_vsi_exit_dflt_promisc(vsi);
>>   				if (err) {
>> -					netdev_err(netdev, "Error %d clearing default VSI %i Rx rule\n",
>> -						   err, vsi->vsi_num);
>>   					vsi->current_netdev_flags |=
>>   						IFF_PROMISC;
>>   					goto out_promisc;
>>   				}
>> -				if (vsi->netdev->features &
>> -				    NETIF_F_HW_VLAN_CTAG_FILTER)
>> -					vlan_ops->ena_rx_filtering(vsi);
>>   			}
> [ ... ]
>> @@ -3675,10 +3724,9 @@ int ice_vlan_rx_add_vid(struct net_device *netdev, __be16 proto, u16 vid)
>>   	while (test_and_set_bit(ICE_CFG_BUSY, vsi->state))
>>   		usleep_range(1000, 2000);
> [Severity: High]
> This is a pre-existing issue, but isn't there a risk of TOCTOU races with
> this ad-hoc lock pattern lacking memory barriers?
  Pre-existing; the ICE_CFG_BUSY locking is not changed by this patch.

>
> The driver spins on ICE_CFG_BUSY using test_and_set_bit() and usleep_range()
> to guard VSI filter synchronization. The lock is subsequently dropped using
> clear_bit(ICE_CFG_BUSY, vsi->state) at the exit label.
>
> Because clear_bit() lacks a memory release barrier, the CPU or compiler can
> reorder memory stores from the critical section to occur after the flag is
> cleared, leading to data races and corruption of shared filter state on
> weakly ordered architectures.
>
>> -	/* Add multicast promisc rule for the VLAN ID to be added if
>> -	 * all-multicast is currently enabled.
>> -	 */
>> -	if (vsi->current_netdev_flags & IFF_ALLMULTI) {
>> +	/* skip the per-VID rule when the DFLT Rx rule already covers this VID */
>> +	if ((vsi->current_netdev_flags & IFF_ALLMULTI) &&
>> +	    !ice_is_vsi_dflt_vsi(vsi)) {
>>   		ret = ice_fltr_set_vsi_promisc(&vsi->back->hw, vsi->idx,
>>   					       ICE_MCAST_VLAN_PROMISC_BITS,
>>   					       vid);
>> @@ -3696,11 +3744,12 @@ int ice_vlan_rx_add_vid(struct net_device *netdev, __be16 proto, u16 vid)
>>   	if (ret)
>>   		goto finish;
> [Severity: High]
> This is a pre-existing issue, but what happens if vlan_ops->add_vlan()
> fails here? Is the hardware switch rule leaked?
Pre-existing; the add_vlan() error unwind is not changed by this patch.

>
> When adding a VLAN to an interface with IFF_ALLMULTI enabled, this function
> first allocates a promiscuous switch rule in hardware via
> ice_fltr_set_vsi_promisc().
>
> If this succeeds but the subsequent call to vlan_ops->add_vlan() fails
> (e.g., due to ENOSPC or memory allocation failure), the function returns
> early without rolling back the promiscuous rule. Because the network stack
> considers the VLAN addition to have failed, ice_vlan_rx_kill_vid() will
> never be called for this VID, permanently leaking a Flow Lookup Unit (FLU).
>
>> -	/* If all-multicast is currently enabled and this VLAN ID is only one
>> -	 * besides VLAN-0 we have to update look-up type of multicast promisc
>> -	 * rule for VLAN-0 from ICE_SW_LKUP_PROMISC to ICE_SW_LKUP_PROMISC_VLAN.
>> +	/* On the first non-zero VLAN, promote the VLAN-0 multicast promisc
>> +	 * rule from ICE_SW_LKUP_PROMISC to ICE_SW_LKUP_PROMISC_VLAN. Skip when
>> +	 * the DFLT Rx rule is installed; it already covers every VID.
>>   	 */
>>   	if ((vsi->current_netdev_flags & IFF_ALLMULTI) &&
>> +	    !ice_is_vsi_dflt_vsi(vsi) &&
>>   	    ice_vsi_num_non_zero_vlans(vsi) == 1) {
> [ ... ]
>> @@ -3763,11 +3812,12 @@ int ice_vlan_rx_kill_vid(struct net_device *netdev, __be16 proto, u16 vid)
>>   					   ICE_MCAST_VLAN_PROMISC_BITS, vid);
>>   
>>   	if (!ice_vsi_has_non_zero_vlans(vsi)) {
>> -		/* Update look-up type of multicast promisc rule for VLAN 0
>> -		 * from ICE_SW_LKUP_PROMISC_VLAN to ICE_SW_LKUP_PROMISC when
>> -		 * all-multicast is enabled and VLAN 0 is the only VLAN rule.
>> +		/* Last non-zero VLAN gone: demote the VLAN-0 multicast promisc
>> +		 * rule back to ICE_SW_LKUP_PROMISC. Skip when the DFLT Rx rule
>> +		 * is installed; no recipe swap is needed.
>>   		 */
>> -		if (vsi->current_netdev_flags & IFF_ALLMULTI) {
>> +		if ((vsi->current_netdev_flags & IFF_ALLMULTI) &&
>> +		    !ice_is_vsi_dflt_vsi(vsi)) {
> [Severity: High]
> Will this modification cause a stale switch rule (ICE_SW_LKUP_PROMISC_VLAN for
> vid=0) to be permanently leaked if the last VLAN is removed while the DFLT VSI
> rule is active?
Real, and introduced by v1/v2. v3 will drop the two vid=0 recipe-swap
guards, so the rule is demoted back to ICE_SW_LKUP_PROMISC when the last
VLAN is removed and never stranded.

Petr
>
> This patch skips demoting the vid=0 multicast promiscuous rule from
> ICE_SW_LKUP_PROMISC_VLAN back to ICE_SW_LKUP_PROMISC when the last VLAN is
> removed, if ice_is_vsi_dflt_vsi(vsi) is true.
>
> However, if the interface later drops IFF_PROMISC (removing the DFLT VSI rule)
> and then drops IFF_ALLMULTI, ice_clear_promisc() is called. Because the
> interface now has zero VLANs, ice_clear_promisc() executes its else branch:
>
>      } else {
>          status = ice_fltr_clear_vsi_promisc(&vsi->back->hw, vsi->idx,
>                                              promisc_m, 0);
>      }
>
> This only searches and clears rules from the ICE_SW_LKUP_PROMISC recipe. The
> stranded rule in the ICE_SW_LKUP_PROMISC_VLAN recipe is never cleared,
> resulting in a permanent hardware switch rule leak.
>
>>   			ice_fltr_clear_vsi_promisc(&vsi->back->hw, vsi->idx,
>>   						   ICE_MCAST_VLAN_PROMISC_BITS,
>>   						   0);