diff mbox series

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

Message ID 20250211174322.603652-1-marcin.szycik@linux.intel.com (mailing list archive)
State Awaiting Upstream
Delegated to: Netdev Maintainers
Headers show
Series [iwl-net,1/2] ice: Fix deinitializing VF in error path | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers fail 2 blamed authors not CCed: anthony.l.nguyen@intel.com jacob.e.keller@intel.com; 7 maintainers not CCed: anthony.l.nguyen@intel.com andrew+netdev@lunn.ch edumazet@google.com jacob.e.keller@intel.com przemyslaw.kitszel@intel.com pabeni@redhat.com kuba@kernel.org
netdev/build_clang success Errors and warnings before: 5 this patch: 5
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 38 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 74 this patch: 74
netdev/source_inline success Was 0 now: 0

Commit Message

Marcin Szycik Feb. 11, 2025, 5:43 p.m. UTC
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(-)

Comments

Simon Horman Feb. 13, 2025, 10:55 a.m. UTC | #1
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>
Marcin Szycik Feb. 13, 2025, 12:32 p.m. UTC | #2
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>
>
Simon Horman Feb. 15, 2025, 7:31 p.m. UTC | #3
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.

...
Buvaneswaran, Sujai Feb. 21, 2025, 8:15 a.m. UTC | #4
> -----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>
diff mbox series

Patch

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);