netdev - RE: [Intel-wired-lan] [PATCH iwl-net v2] ice: fix VSI lists confusion when adding VLANs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <SJ0PR11MB58654F137DB3E2F3098A03D38F9E2@SJ0PR11MB5865.namprd11.prod.outlook.com>
Date: Fri, 6 Sep 2024 10:52:37 +0000
From: "Romanowski, Rafal" <rafal.romanowski@...el.com>
To: mschmidt <mschmidt@...hat.com>, "Drewek, Wojciech"
	<wojciech.drewek@...el.com>, "Szycik, Marcin" <marcin.szycik@...el.com>,
	"Miskell, Timothy" <timothy.miskell@...el.com>, "Nguyen, Anthony L"
	<anthony.l.nguyen@...el.com>, "Kitszel, Przemyslaw"
	<przemyslaw.kitszel@...el.com>, "David S. Miller" <davem@...emloft.net>,
	"Eric Dumazet" <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
	"Paolo Abeni" <pabeni@...hat.com>, "Ertman, David M"
	<david.m.ertman@...el.com>, Daniel Machon <daniel.machon@...rochip.com>
CC: Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>,
	"intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [Intel-wired-lan] [PATCH iwl-net v2] ice: fix VSI lists confusion
 when adding VLANs

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@...osl.org> On Behalf Of Michal
> Schmidt
> Sent: Wednesday, September 4, 2024 11:39 AM
> To: Drewek, Wojciech <wojciech.drewek@...el.com>; Szycik, Marcin
> <marcin.szycik@...el.com>; Miskell, Timothy <timothy.miskell@...el.com>;
> Nguyen, Anthony L <anthony.l.nguyen@...el.com>; Kitszel, Przemyslaw
> <przemyslaw.kitszel@...el.com>; David S. Miller <davem@...emloft.net>; Eric
> Dumazet <edumazet@...gle.com>; Jakub Kicinski <kuba@...nel.org>; Paolo
> Abeni <pabeni@...hat.com>; Ertman, David M <david.m.ertman@...el.com>;
> Daniel Machon <daniel.machon@...rochip.com>
> Cc: Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>; intel-wired-
> lan@...ts.osuosl.org; linux-kernel@...r.kernel.org; netdev@...r.kernel.org
> 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 <michal.swiatkowski@...ux.intel.com>
> Signed-off-by: Michal Schmidt <mschmidt@...hat.com>
> ---
> 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 <rafal.romanowski@...el.com>