netdev - RE: [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] [thread-next>] [day] [month] [year] [list]
Message-ID: <IA1PR11MB6194461AB832D0F17B044AA2DD9D2@IA1PR11MB6194.namprd11.prod.outlook.com>
Date: Thu, 5 Sep 2024 18:29:47 +0000
From: "Ertman, David M" <david.m.ertman@...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>, Daniel Machon <daniel.machon@...rochip.com>
CC: poros <poros@...hat.com>, Michal Swiatkowski
	<michal.swiatkowski@...ux.intel.com>, "intel-wired-lan@...ts.osuosl.org"
	<intel-wired-lan@...ts.osuosl.org>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: RE: [PATCH iwl-net v2] ice: fix VSI lists confusion when adding VLANs

> -----Original Message-----
> From: Michal Schmidt <mschmidt@...hat.com>
> Sent: Wednesday, September 4, 2024 2: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: poros <poros@...hat.com>; Michal Swiatkowski
> <michal.swiatkowski@...ux.intel.com>; intel-wired-lan@...ts.osuosl.org;
> netdev@...r.kernel.org; linux-kernel@...r.kernel.org
> Subject: [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

I did a couple of quick tests, and it does not look like this patch interferes
with the creation/cleanup of prune lists for the LAG feature.  So, I don't
see a problem with this patch.  Thanks for catching this!

DaveE

Reviewed-by: Dave Ertman <David.m.ertman@...el.com>