[PATCH iwl-net 1/2] ice: Fix deinitializing VF in error path

[PATCH iwl-net 1/2] ice: Fix deinitializing VF in error path

[Marcin Szycik]

If ice_ena_vfs() fails after calling ice_create_vf_entries(), it frees
all VFs without removing them from snapshot PF-VF mailbox list, leading
to list corruption.

Reproducer:
  devlink dev eswitch set $PF1_PCI mode switchdev
  ip l s $PF1 up
  ip l s $PF1 promisc on
  sleep 1
  echo 1 > /sys/class/net/$PF1/device/sriov_numvfs
  sleep 1
  echo 1 > /sys/class/net/$PF1/device/sriov_numvfs

Trace (minimized):
  list_add corruption. next->prev should be prev (ffff8882e241c6f0), but was 0000000000000000. (next=ffff888455da1330).
  kernel BUG at lib/list_debug.c:29!
  RIP: 0010:__list_add_valid_or_report+0xa6/0x100
   ice_mbx_init_vf_info+0xa7/0x180 [ice]
   ice_initialize_vf_entry+0x1fa/0x250 [ice]
   ice_sriov_configure+0x8d7/0x1520 [ice]
   ? __percpu_ref_switch_mode+0x1b1/0x5d0
   ? __pfx_ice_sriov_configure+0x10/0x10 [ice]

Sometimes a KASAN report can be seen instead with a similar stack trace:
  BUG: KASAN: use-after-free in __list_add_valid_or_report+0xf1/0x100

VFs are added to this list in ice_mbx_init_vf_info(), but only removed
in ice_free_vfs(). Move the removing to ice_free_vf_entries(), which is
also being called in other places where VFs are being removed (including
ice_free_vfs() itself).

Fixes: 8cd8a6b17d27 ("ice: move VF overflow message count into struct ice_mbx_vf_info")
Reported-by: Sujai Buvaneswaran <sujai.buvaneswaran@intel.com>
Closes: https://lore.kernel.org/intel-wired-lan/PH0PR11MB50138B635F2E5CEB7075325D961F2@PH0PR11MB5013.namprd11.prod.outlook.com
Reviewed-by: Martyna Szapar-Mudlaw <martyna.szapar-mudlaw@linux.intel.com>
Signed-off-by: Marcin Szycik <marcin.szycik@linux.intel.com>
---
 drivers/net/ethernet/intel/ice/ice_sriov.c          | 5 +----
 drivers/net/ethernet/intel/ice/ice_vf_lib.c         | 8 ++++++++
 drivers/net/ethernet/intel/ice/ice_vf_lib_private.h | 1 +
 3 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.c b/drivers/net/ethernet/intel/ice/ice_sriov.c
index b83f99c01d91..8aabf7749aa5 100644
--- a/drivers/net/ethernet/intel/ice/ice_sriov.c
+++ b/drivers/net/ethernet/intel/ice/ice_sriov.c
@@ -36,6 +36,7 @@ static void ice_free_vf_entries(struct ice_pf *pf)
 
 	hash_for_each_safe(vfs->table, bkt, tmp, vf, entry) {
 		hash_del_rcu(&vf->entry);
+		ice_deinitialize_vf_entry(vf);
 		ice_put_vf(vf);
 	}
 }
@@ -193,10 +194,6 @@ void ice_free_vfs(struct ice_pf *pf)
 			wr32(hw, GLGEN_VFLRSTAT(reg_idx), BIT(bit_idx));
 		}
 
-		/* clear malicious info since the VF is getting released */
-		if (!ice_is_feature_supported(pf, ICE_F_MBX_LIMIT))
-			list_del(&vf->mbx_info.list_entry);
-
 		mutex_unlock(&vf->cfg_lock);
 	}
 
diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib.c b/drivers/net/ethernet/intel/ice/ice_vf_lib.c
index c7c0c2f50c26..815ad0bfe832 100644
--- a/drivers/net/ethernet/intel/ice/ice_vf_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_vf_lib.c
@@ -1036,6 +1036,14 @@ void ice_initialize_vf_entry(struct ice_vf *vf)
 	mutex_init(&vf->cfg_lock);
 }
 
+void ice_deinitialize_vf_entry(struct ice_vf *vf)
+{
+	struct ice_pf *pf = vf->pf;
+
+	if (!ice_is_feature_supported(pf, ICE_F_MBX_LIMIT))
+		list_del(&vf->mbx_info.list_entry);
+}
+
 /**
  * ice_dis_vf_qs - Disable the VF queues
  * @vf: pointer to the VF structure
diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib_private.h b/drivers/net/ethernet/intel/ice/ice_vf_lib_private.h
index 0c7e77c0a09f..5392b0404986 100644
--- a/drivers/net/ethernet/intel/ice/ice_vf_lib_private.h
+++ b/drivers/net/ethernet/intel/ice/ice_vf_lib_private.h
@@ -24,6 +24,7 @@
 #endif
 
 void ice_initialize_vf_entry(struct ice_vf *vf);
+void ice_deinitialize_vf_entry(struct ice_vf *vf);
 void ice_dis_vf_qs(struct ice_vf *vf);
 int ice_check_vf_init(struct ice_vf *vf);
 enum virtchnl_status_code ice_err_to_virt_err(int err);
-- 
2.45.0

[PATCH iwl-net 2/2] ice: Avoid setting default Rx VSI twice in switchdev setup

[Marcin Szycik]

As part of switchdev environment setup, uplink VSI is configured as
default for both Tx and Rx. Default Rx VSI is also used by promiscuous
mode. If promisc mode is enabled and an attempt to enter switchdev mode
is made, the setup will fail because Rx VSI is already configured as
default (rule exists).

Reproducer:
  devlink dev eswitch set $PF1_PCI mode switchdev
  ip l s $PF1 up
  ip l s $PF1 promisc on
  echo 1 > /sys/class/net/$PF1/device/sriov_numvfs

In switchdev setup, use ice_set_dflt_vsi() instead of plain
ice_cfg_dflt_vsi(), which avoids repeating setting default VSI for Rx if
it's already configured.

Fixes: 50d62022f455 ("ice: default Tx rule instead of to queue")
Reported-by: Sujai Buvaneswaran <sujai.buvaneswaran@intel.com>
Closes: https://lore.kernel.org/intel-wired-lan/PH0PR11MB50138B635F2E5CEB7075325D961F2@PH0PR11MB5013.namprd11.prod.outlook.com
Reviewed-by: Martyna Szapar-Mudlaw <martyna.szapar-mudlaw@linux.intel.com>
Signed-off-by: Marcin Szycik <marcin.szycik@linux.intel.com>
---
 drivers/net/ethernet/intel/ice/ice_eswitch.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_eswitch.c b/drivers/net/ethernet/intel/ice/ice_eswitch.c
index b44a375e6365..ed21d7f55ac1 100644
--- a/drivers/net/ethernet/intel/ice/ice_eswitch.c
+++ b/drivers/net/ethernet/intel/ice/ice_eswitch.c
@@ -38,8 +38,7 @@ static int ice_eswitch_setup_env(struct ice_pf *pf)
 	if (ice_vsi_add_vlan_zero(uplink_vsi))
 		goto err_vlan_zero;
 
-	if (ice_cfg_dflt_vsi(uplink_vsi->port_info, uplink_vsi->idx, true,
-			     ICE_FLTR_RX))
+	if (ice_set_dflt_vsi(uplink_vsi))
 		goto err_def_rx;
 
 	if (ice_cfg_dflt_vsi(uplink_vsi->port_info, uplink_vsi->idx, true,
-- 
2.45.0

Re: [PATCH iwl-net 2/2] ice: Avoid setting default Rx VSI twice in switchdev setup

[Simon Horman]

On Tue, Feb 11, 2025 at 06:43:22PM +0100, Marcin Szycik wrote:
> As part of switchdev environment setup, uplink VSI is configured as
> default for both Tx and Rx. Default Rx VSI is also used by promiscuous
> mode. If promisc mode is enabled and an attempt to enter switchdev mode
> is made, the setup will fail because Rx VSI is already configured as
> default (rule exists).
> 
> Reproducer:
>   devlink dev eswitch set $PF1_PCI mode switchdev
>   ip l s $PF1 up
>   ip l s $PF1 promisc on
>   echo 1 > /sys/class/net/$PF1/device/sriov_numvfs
> 
> In switchdev setup, use ice_set_dflt_vsi() instead of plain
> ice_cfg_dflt_vsi(), which avoids repeating setting default VSI for Rx if
> it's already configured.
> 
> Fixes: 50d62022f455 ("ice: default Tx rule instead of to queue")
> Reported-by: Sujai Buvaneswaran <sujai.buvaneswaran@intel.com>
> Closes: https://lore.kernel.org/intel-wired-lan/PH0PR11MB50138B635F2E5CEB7075325D961F2@PH0PR11MB5013.namprd11.prod.outlook.com
> Reviewed-by: Martyna Szapar-Mudlaw <martyna.szapar-mudlaw@linux.intel.com>
> Signed-off-by: Marcin Szycik <marcin.szycik@linux.intel.com>

Reviewed-by: Simon Horman <horms@kernel.org>

RE: [PATCH iwl-net 2/2] ice: Avoid setting default Rx VSI twice in switchdev setup

[Buvaneswaran, Sujai]

> -----Original Message-----
> From: Marcin Szycik <marcin.szycik@linux.intel.com>
> Sent: Tuesday, February 11, 2025 11:13 PM
> To: intel-wired-lan@lists.osuosl.org
> Cc: netdev@vger.kernel.org; michal.swiatkowski@linux.intel.com; Marcin
> Szycik <marcin.szycik@linux.intel.com>; Buvaneswaran, Sujai
> <sujai.buvaneswaran@intel.com>; Martyna Szapar-Mudlaw
> <martyna.szapar-mudlaw@linux.intel.com>
> Subject: [PATCH iwl-net 2/2] ice: Avoid setting default Rx VSI twice in
> switchdev setup
> 
> As part of switchdev environment setup, uplink VSI is configured as default
> for both Tx and Rx. Default Rx VSI is also used by promiscuous mode. If
> promisc mode is enabled and an attempt to enter switchdev mode is made,
> the setup will fail because Rx VSI is already configured as default (rule exists).
> 
> Reproducer:
>   devlink dev eswitch set $PF1_PCI mode switchdev
>   ip l s $PF1 up
>   ip l s $PF1 promisc on
>   echo 1 > /sys/class/net/$PF1/device/sriov_numvfs
> 
> In switchdev setup, use ice_set_dflt_vsi() instead of plain ice_cfg_dflt_vsi(),
> which avoids repeating setting default VSI for Rx if it's already configured.
> 
> Fixes: 50d62022f455 ("ice: default Tx rule instead of to queue")
> Reported-by: Sujai Buvaneswaran <sujai.buvaneswaran@intel.com>
> Closes: https://lore.kernel.org/intel-wired-
> lan/PH0PR11MB50138B635F2E5CEB7075325D961F2@PH0PR11MB5013.nam
> prd11.prod.outlook.com
> Reviewed-by: Martyna Szapar-Mudlaw <martyna.szapar-
> mudlaw@linux.intel.com>
> Signed-off-by: Marcin Szycik <marcin.szycik@linux.intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_eswitch.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
Tested-by: Sujai Buvaneswaran <sujai.buvaneswaran@intel.com>

Re: [PATCH iwl-net 1/2] ice: Fix deinitializing VF in error path

[Simon Horman]

On Tue, Feb 11, 2025 at 06:43:21PM +0100, Marcin Szycik wrote:
> If ice_ena_vfs() fails after calling ice_create_vf_entries(), it frees
> all VFs without removing them from snapshot PF-VF mailbox list, leading
> to list corruption.
> 
> Reproducer:
>   devlink dev eswitch set $PF1_PCI mode switchdev
>   ip l s $PF1 up
>   ip l s $PF1 promisc on
>   sleep 1
>   echo 1 > /sys/class/net/$PF1/device/sriov_numvfs

Should the line above be "echo 0" to remove the VFs before creating VFs
below (I'm looking at sriov_numvfs_store())?

>   sleep 1
>   echo 1 > /sys/class/net/$PF1/device/sriov_numvfs
> 
> Trace (minimized):
>   list_add corruption. next->prev should be prev (ffff8882e241c6f0), but was 0000000000000000. (next=ffff888455da1330).
>   kernel BUG at lib/list_debug.c:29!
>   RIP: 0010:__list_add_valid_or_report+0xa6/0x100
>    ice_mbx_init_vf_info+0xa7/0x180 [ice]
>    ice_initialize_vf_entry+0x1fa/0x250 [ice]
>    ice_sriov_configure+0x8d7/0x1520 [ice]
>    ? __percpu_ref_switch_mode+0x1b1/0x5d0
>    ? __pfx_ice_sriov_configure+0x10/0x10 [ice]
> 
> Sometimes a KASAN report can be seen instead with a similar stack trace:
>   BUG: KASAN: use-after-free in __list_add_valid_or_report+0xf1/0x100
> 
> VFs are added to this list in ice_mbx_init_vf_info(), but only removed
> in ice_free_vfs(). Move the removing to ice_free_vf_entries(), which is
> also being called in other places where VFs are being removed (including
> ice_free_vfs() itself).
> 
> Fixes: 8cd8a6b17d27 ("ice: move VF overflow message count into struct ice_mbx_vf_info")
> Reported-by: Sujai Buvaneswaran <sujai.buvaneswaran@intel.com>
> Closes: https://lore.kernel.org/intel-wired-lan/PH0PR11MB50138B635F2E5CEB7075325D961F2@PH0PR11MB5013.namprd11.prod.outlook.com
> Reviewed-by: Martyna Szapar-Mudlaw <martyna.szapar-mudlaw@linux.intel.com>
> Signed-off-by: Marcin Szycik <marcin.szycik@linux.intel.com>

The comment above notwithstanding, I agree that this addresses the
bug you have described.

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH iwl-net 1/2] ice: Fix deinitializing VF in error path

[Marcin Szycik]

On 13.02.2025 11:55, Simon Horman wrote:
> On Tue, Feb 11, 2025 at 06:43:21PM +0100, Marcin Szycik wrote:
>> If ice_ena_vfs() fails after calling ice_create_vf_entries(), it frees
>> all VFs without removing them from snapshot PF-VF mailbox list, leading
>> to list corruption.
>>
>> Reproducer:
>>   devlink dev eswitch set $PF1_PCI mode switchdev
>>   ip l s $PF1 up
>>   ip l s $PF1 promisc on
>>   sleep 1
>>   echo 1 > /sys/class/net/$PF1/device/sriov_numvfs
> 
> Should the line above be "echo 0" to remove the VFs before creating VFs
> below (I'm looking at sriov_numvfs_store())?

Both "echo 1" commands fail (I'm fixing it in patch 2/2), that's why there's
no "echo 0" in between. Also, in this minimal example I'm assuming no VFs
were initially present.

Thanks for reviewing!
Marcin

>>   sleep 1
>>   echo 1 > /sys/class/net/$PF1/device/sriov_numvfs
>>
>> Trace (minimized):
>>   list_add corruption. next->prev should be prev (ffff8882e241c6f0), but was 0000000000000000. (next=ffff888455da1330).
>>   kernel BUG at lib/list_debug.c:29!
>>   RIP: 0010:__list_add_valid_or_report+0xa6/0x100
>>    ice_mbx_init_vf_info+0xa7/0x180 [ice]
>>    ice_initialize_vf_entry+0x1fa/0x250 [ice]
>>    ice_sriov_configure+0x8d7/0x1520 [ice]
>>    ? __percpu_ref_switch_mode+0x1b1/0x5d0
>>    ? __pfx_ice_sriov_configure+0x10/0x10 [ice]
>>
>> Sometimes a KASAN report can be seen instead with a similar stack trace:
>>   BUG: KASAN: use-after-free in __list_add_valid_or_report+0xf1/0x100
>>
>> VFs are added to this list in ice_mbx_init_vf_info(), but only removed
>> in ice_free_vfs(). Move the removing to ice_free_vf_entries(), which is
>> also being called in other places where VFs are being removed (including
>> ice_free_vfs() itself).
>>
>> Fixes: 8cd8a6b17d27 ("ice: move VF overflow message count into struct ice_mbx_vf_info")
>> Reported-by: Sujai Buvaneswaran <sujai.buvaneswaran@intel.com>
>> Closes: https://lore.kernel.org/intel-wired-lan/PH0PR11MB50138B635F2E5CEB7075325D961F2@PH0PR11MB5013.namprd11.prod.outlook.com
>> Reviewed-by: Martyna Szapar-Mudlaw <martyna.szapar-mudlaw@linux.intel.com>
>> Signed-off-by: Marcin Szycik <marcin.szycik@linux.intel.com>
> 
> The comment above notwithstanding, I agree that this addresses the
> bug you have described.
> 
> Reviewed-by: Simon Horman <horms@kernel.org>
> 

Re: [PATCH iwl-net 1/2] ice: Fix deinitializing VF in error path

[Simon Horman]

On Thu, Feb 13, 2025 at 01:32:38PM +0100, Marcin Szycik wrote:
> 
> 
> On 13.02.2025 11:55, Simon Horman wrote:
> > On Tue, Feb 11, 2025 at 06:43:21PM +0100, Marcin Szycik wrote:
> >> If ice_ena_vfs() fails after calling ice_create_vf_entries(), it frees
> >> all VFs without removing them from snapshot PF-VF mailbox list, leading
> >> to list corruption.
> >>
> >> Reproducer:
> >>   devlink dev eswitch set $PF1_PCI mode switchdev
> >>   ip l s $PF1 up
> >>   ip l s $PF1 promisc on
> >>   sleep 1
> >>   echo 1 > /sys/class/net/$PF1/device/sriov_numvfs
> > 
> > Should the line above be "echo 0" to remove the VFs before creating VFs
> > below (I'm looking at sriov_numvfs_store())?
> 
> Both "echo 1" commands fail (I'm fixing it in patch 2/2), that's why there's
> no "echo 0" in between. Also, in this minimal example I'm assuming no VFs
> were initially present.
> 
> Thanks for reviewing!

Likewise, thanks for the clarification.

...

RE: [PATCH iwl-net 1/2] ice: Fix deinitializing VF in error path

[Buvaneswaran, Sujai]

> -----Original Message-----
> From: Marcin Szycik <marcin.szycik@linux.intel.com>
> Sent: Tuesday, February 11, 2025 11:13 PM
> To: intel-wired-lan@lists.osuosl.org
> Cc: netdev@vger.kernel.org; michal.swiatkowski@linux.intel.com; Marcin
> Szycik <marcin.szycik@linux.intel.com>; Buvaneswaran, Sujai
> <sujai.buvaneswaran@intel.com>; Martyna Szapar-Mudlaw
> <martyna.szapar-mudlaw@linux.intel.com>
> Subject: [PATCH iwl-net 1/2] ice: Fix deinitializing VF in error path
> 
> If ice_ena_vfs() fails after calling ice_create_vf_entries(), it frees all VFs
> without removing them from snapshot PF-VF mailbox list, leading to list
> corruption.
> 
> Reproducer:
>   devlink dev eswitch set $PF1_PCI mode switchdev
>   ip l s $PF1 up
>   ip l s $PF1 promisc on
>   sleep 1
>   echo 1 > /sys/class/net/$PF1/device/sriov_numvfs
>   sleep 1
>   echo 1 > /sys/class/net/$PF1/device/sriov_numvfs
> 
> Trace (minimized):
>   list_add corruption. next->prev should be prev (ffff8882e241c6f0), but was
> 0000000000000000. (next=ffff888455da1330).
>   kernel BUG at lib/list_debug.c:29!
>   RIP: 0010:__list_add_valid_or_report+0xa6/0x100
>    ice_mbx_init_vf_info+0xa7/0x180 [ice]
>    ice_initialize_vf_entry+0x1fa/0x250 [ice]
>    ice_sriov_configure+0x8d7/0x1520 [ice]
>    ? __percpu_ref_switch_mode+0x1b1/0x5d0
>    ? __pfx_ice_sriov_configure+0x10/0x10 [ice]
> 
> Sometimes a KASAN report can be seen instead with a similar stack trace:
>   BUG: KASAN: use-after-free in __list_add_valid_or_report+0xf1/0x100
> 
> VFs are added to this list in ice_mbx_init_vf_info(), but only removed in
> ice_free_vfs(). Move the removing to ice_free_vf_entries(), which is also
> being called in other places where VFs are being removed (including
> ice_free_vfs() itself).
> 
> Fixes: 8cd8a6b17d27 ("ice: move VF overflow message count into struct
> ice_mbx_vf_info")
> Reported-by: Sujai Buvaneswaran <sujai.buvaneswaran@intel.com>
> Closes: https://lore.kernel.org/intel-wired-
> lan/PH0PR11MB50138B635F2E5CEB7075325D961F2@PH0PR11MB5013.nam
> prd11.prod.outlook.com
> Reviewed-by: Martyna Szapar-Mudlaw <martyna.szapar-
> mudlaw@linux.intel.com>
> Signed-off-by: Marcin Szycik <marcin.szycik@linux.intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_sriov.c          | 5 +----
>  drivers/net/ethernet/intel/ice/ice_vf_lib.c         | 8 ++++++++
>  drivers/net/ethernet/intel/ice/ice_vf_lib_private.h | 1 +
>  3 files changed, 10 insertions(+), 4 deletions(-)
> 
Tested-by: Sujai Buvaneswaran <sujai.buvaneswaran@intel.com>