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