> -----Original Message----- > From: Intel-wired-lan <[email protected]> On Behalf Of Michal > Schmidt > Sent: Wednesday, September 4, 2024 11:39 AM > To: Drewek, Wojciech <[email protected]>; Szycik, Marcin > <[email protected]>; Miskell, Timothy <[email protected]>; > Nguyen, Anthony L <[email protected]>; Kitszel, Przemyslaw > <[email protected]>; David S. Miller <[email protected]>; Eric > Dumazet <[email protected]>; Jakub Kicinski <[email protected]>; Paolo > Abeni <[email protected]>; Ertman, David M <[email protected]>; > Daniel Machon <[email protected]> > Cc: Michal Swiatkowski <[email protected]>; intel-wired- > [email protected]; [email protected]; [email protected] > Subject: [Intel-wired-lan] [PATCH iwl-net v2] ice: fix VSI lists confusion > when > adding VLANs > > The description of function ice_find_vsi_list_entry says: > Search VSI list map with VSI count 1 > > However, since the blamed commit (see Fixes below), the function no longer > checks vsi_count. This causes a problem in ice_add_vlan_internal, where the > decision to share VSI lists between filter rules relies on the vsi_count of > the found > existing VSI list being 1. > > The reproducing steps: > 1. Have a PF and two VFs. > There will be a filter rule for VLAN 0, referring to a VSI list > containing VSIs: 0 (PF), 2 (VF#0), 3 (VF#1). > 2. Add VLAN 1234 to VF#0. > ice will make the wrong decision to share the VSI list with the new > rule. The wrong behavior may not be immediately apparent, but it can > be observed with debug prints. > 3. Add VLAN 1234 to VF#1. > ice will unshare the VSI list for the VLAN 1234 rule. Due to the > earlier bad decision, the newly created VSI list will contain > VSIs 0 (PF) and 3 (VF#1), instead of expected 2 (VF#0) and 3 (VF#1). > 4. Try pinging a network peer over the VLAN interface on VF#0. > This fails. > > Reproducer script at: > https://gitlab.com/mschmidt2/repro/-/blob/master/RHEL-46814/test-vlan-vsi- > list-confusion.sh > Commented debug trace: > https://gitlab.com/mschmidt2/repro/-/blob/master/RHEL-46814/ice-vlan-vsi- > lists-debug.txt > Patch adding the debug prints: > https://gitlab.com/mschmidt2/linux/- > /commit/f8a8814623944a45091a77c6094c40bfe726bfdb > (Unsafe, by the way. Lacks rule_lock when dumping in ice_remove_vlan.) > > Michal Swiatkowski added to the explanation that the bug is caused by reusing > a > VSI list created for VLAN 0. All created VFs' VSIs are added to VLAN 0 > filter. When > a non-zero VLAN is created on a VF which is already in VLAN 0 (normal case), > the > VSI list from VLAN 0 is reused. > It leads to a problem because all VFs (VSIs to be specific) that are > subscribed to > VLAN 0 will now receive a new VLAN tag traffic. This is one bug, another is > the > bug described above. Removing filters from one VF will remove VLAN filter from > the previous VF. It happens a VF is reset. Example: > - creation of 3 VFs > - we have VSI list (used for VLAN 0) [0 (pf), 2 (vf1), 3 (vf2), 4 (vf3)] > - we are adding VLAN 100 on VF1, we are reusing the previous list > because 2 is there > - VLAN traffic works fine, but VLAN 100 tagged traffic can be received > on all VSIs from the list (for example broadcast or unicast) > - trust is turning on on VF2, VF2 is resetting, all filters from VF2 are > removed; the VLAN 100 filter is also removed because 3 is on the list > - VLAN traffic to VF1 isn't working anymore, there is a need to recreate > VLAN interface to readd VLAN filter > > One thing I'm not certain about is the implications for the LAG feature, > which is > another caller of ice_find_vsi_list_entry. I don't have a LAG-capable card at > hand > to test. > > Fixes: 23ccae5ce15f ("ice: changes to the interface with the HW and FW for > SRIOV_VF+LAG") > Reviewed-by: Michal Swiatkowski <[email protected]> > Signed-off-by: Michal Schmidt <[email protected]> > --- > v2: Corrected the Fixes commit ID (the ID in v1 was of a centos-stream-9 > backport accidentally). > Added the extended explanation from Michal Swiatkowski. > Fixed some typos. > --- > drivers/net/ethernet/intel/ice/ice_switch.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/intel/ice/ice_switch.c > b/drivers/net/ethernet/intel/ice/ice_switch.c > index fe8847184cb1..4e6e7af962bd 100644 > --- a/drivers/net/ethernet/intel/ice/ice_switch.c > +++ b/drivers/net/ethernet/intel/ice/ice_switch.c > @@ -3264,7 +3264,7 @@ ice_find_vsi_list_entry(struct ice_hw *hw, u8 recp_id, > u16 vsi_handle, > > list_head = &sw->recp_list[recp_id].filt_rules; > list_for_each_entry(list_itr, list_head, list_entry) { > - if (list_itr->vsi_list_info) { > + if (list_itr->vsi_count == 1 && list_itr->vsi_list_info) { > map_info = list_itr->vsi_list_info; > if (test_bit(vsi_handle, map_info->vsi_map)) { > *vsi_list_id = map_info->vsi_list_id; > -- > 2.45.2
Tested-by: Rafal Romanowski <[email protected]>
