Re: [PATCH iwl-net v2 2/2] ice: preserve uplink DFLT Rx rule on switchdev release

[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
> ---
> ice: preserve uplink DFLT Rx rule on switchdev release
>
> This commit preserves the default Rx rule on the uplink VSI when releasing
> the switchdev environment if the interface is in promiscuous mode. This
> prevents the catch-all rule from being removed while the netdev flags still
> request promiscuity.
>
>> diff --git a/drivers/net/ethernet/intel/ice/ice_eswitch.c b/drivers/net/ethernet/intel/ice/ice_eswitch.c
>> index 2e4f0969035f7..48273ef9f69dc 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_eswitch.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_eswitch.c
> [ ... ]
>> @@ -278,8 +280,16 @@ static void ice_eswitch_release_env(struct ice_pf *pf)
>>   	vlan_ops->ena_rx_filtering(uplink_vsi);
> [Severity: High]
> Could this lockless read of netdev->flags cause a regression due to a TOCTOU
> race?
Both reads key on the same live netdev->flags & IFF_PROMISC (the pruning
read just above included); it is a single word, so no torn read, and a
flip in the two-statement window is transient and reconciled by the next
ice_vsi_sync_fltr() after switchdev stops. I kept the read live on purpose
so the preserved rule and the pruning decision key on the same source.
> The ena_rx_filtering() call eventually reaches ice_cfg_vlan_pruning(),
> which reads uplink_vsi->netdev->flags & IFF_PROMISC locklessly to decide
> whether to enable VLAN pruning.
>
>>   	ice_cfg_dflt_vsi(uplink_vsi->port_info, uplink_vsi->idx, false,
>>   			 ICE_FLTR_TX);
>> -	ice_cfg_dflt_vsi(uplink_vsi->port_info, uplink_vsi->idx, false,
>> -			 ICE_FLTR_RX);
>> +
>> +	/* Keep the Rx DFLT rule if the uplink is promiscuous; it must outlive
>> +	 * the session. Test the live netdev->flags, the same value
>> +	 * ena_rx_filtering() -> ice_cfg_vlan_pruning() above keys its decision
>> +	 * on, so the preserved DFLT rule and the pruning state stay consistent.
>> +	 */
>> +	if (!(uplink_vsi->netdev->flags & IFF_PROMISC))
> ... and then the exact same flags are read locklessly again right here.
>
> Because setup/teardown paths are protected by devl_lock rather than
> rtnl_lock, could a concurrent dev_change_flags() holding rtnl_lock toggle
> IFF_PROMISC between the two checks?
>
> If IFF_PROMISC changes from 0 to 1 in this window, the code would incorrectly
> enable VLAN pruning (seeing 0) and then incorrectly retain the DFLT_RX rule
> (seeing 1), leaving the hardware with VLAN pruning enabled during promiscuous
> mode.
>
> [Severity: High]
> Does preserving the Rx DFLT rule here cause a regression by permanently
> breaking multicast promiscuity configuration if IFF_PROMISC was set?
Correct, the DFLT rule is unicast only. v3 will fix this in patch 1:
ice_set_promisc(ICE_MCAST_PROMISC_BITS) will run whenever the netdev is
promiscuous, including when ice_is_dflt_vsi_in_use() is already true.

>
> During the switchdev session, ice_remove_vsi_fltr() removes the
> ICE_SW_LKUP_PROMISC rules. By manually preserving the DFLT rule here, any
> future synchronization via ice_vsi_sync_fltr() will evaluate
> !ice_is_dflt_vsi_in_use(vsi->port_info) as false:
>
> ice_main.c:ice_vsi_sync_fltr() {
>      ...
>      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);
>      }
>      ...
> }
>
> Because it is skipped, ICE_MCAST_PROMISC_BITS is never configured, leaving
> the VSI in unicast promiscuous mode but silently dropping multicast traffic
> destined for other MAC addresses.
>
>> +		ice_cfg_dflt_vsi(uplink_vsi->port_info, uplink_vsi->idx,
>> +				 false, ICE_FLTR_RX);
>> +
>>   	ice_fltr_add_mac_and_broadcast(uplink_vsi,
>>   				       uplink_vsi->port_info->mac.perm_addr,
>>   				       ICE_FWD_TO_VSI);
> [Severity: High]
> This is a pre-existing issue, but does this teardown function fail to
> synchronize hardware state for previously joined multicast groups and
> secondary unicast addresses?
Pre-existing (rx_mode is gated during switchdev). This patch only gates
the DFLT Rx removal in ice_eswitch_release_env(); the multicast-promisc
half is covered by the v3 patch 1 change above, and replaying the full
deferred uc/mc list is a wider item left out here.

Petr
>
> When entering switchdev mode, __dev_uc_unsync() and __dev_mc_unsync() clear
> the synchronization state of OS-level MAC addresses, and
> ice_remove_vsi_fltr() removes the actual hardware filters. While switchdev
> runs, calls to ndo_set_rx_mode return early and do not update hardware.
>
> When returning to legacy mode, this function restores only the primary MAC
> and broadcast addresses, but doesn't seem to invoke netif_set_rx_mode() or
> equivalent to resynchronize the deferred Rx filters from the netdev to the
> hardware. Could this cause the hardware to silently drop packets for
> previously joined groups until the interface is bounced?
>