From: Linkui Xiao <[email protected]>
Patch 1 moves the ice_eswitch_detach_vf() and ice_eswitch_attach_vf()
calls in ice_free_vfs() and ice_reset_all_vfs() out of vf->cfg_lock.
Both take the devlink instance lock and RTNL through the representor
netdev, while ndo_set_vf_mac(), ndo_set_vf_vlan() and the representor's
ethtool reset take vf->cfg_lock with RTNL already held, so cfg_lock ->
RTNL against RTNL -> cfg_lock is an inversion; that is the AB-BA
Sashiko reported. The detach is preceded by a single cfg_lock
acquisition, which waits out a reset that is already inside the lock:
ICE_VF_DIS, raised before both loops, keeps later ones out but not one
that got in ahead of it, and that one can reach
ice_eswitch_update_repr() on a representor that is being freed with
free_netdev() + kfree(), with no RCU grace period in between.
Patch 2 does the same for the teardown loop in ice_start_vfs(), which
in addition never detaches the representors it attached before the
failure. That path does not set ICE_VF_DIS and the VFs it unwinds
already reached ICE_VF_STATE_INIT, so vf->cfg_lock is still needed
around ice_dis_vf_mappings() and ice_vf_vsi_release(): the
ice_check_vf_ready_for_cfg() test runs before the lock is taken and
cannot keep out "ip link set dev <pf> vf N ...". The patch marks the
VF disabled under cfg_lock before the detach, which also waits out an
ice_reset_vf() that is already inside the lock.
Patch 3 gives back the MSI-X window that ice_init_vf_vsi_res()
reserves and that none of its error paths releases.
Representors are created and destroyed only under pf->vfs.table_lock,
so the detach does not need vf->cfg_lock to stay safe against other PF
operations, and all three functions are called with that lock held.
The three issues were found by manual code inspection; none of them
was triggered at runtime, and the series is compile tested only, with
no hardware test.
Changes in v3:
- New patch 1: move the detach/attach calls in ice_free_vfs() and
ice_reset_all_vfs() out of vf->cfg_lock, and take cfg_lock once before
the detach so that a reset which is already inside it is waited out.
(Przemek Kitszel)
- Patch 2: detach the representor outside vf->cfg_lock, and mark the VF
disabled under cfg_lock before the detach.
(Przemek Kitszel, Sashiko AI review)
- Patch 3: correct the Fixes: tag to 4d38cb44bd32, which replaced the
computed first vector index with a reservation from the bitmap and left
the error paths below it unchanged; commit a203163274a4 ("ice: simplify
VF MSI-X managing") only renamed that helper.
- Add the information netdev-bot asked for to the commit messages.
- Drop the Reviewed-by tags from Tomasz Lichwala and Aleksandr Loktionov:
patch 2 changed code again, and the hunk in patch 3's teardown loop
moved inside vf->cfg_lock.
---
Link:
https://lore.kernel.org/netdev/[email protected]/
Linkui Xiao (3):
ice: attach and detach VF representors outside of vf->cfg_lock
ice: detach the VF representor when ice_start_vfs() fails
ice: free the VF MSI-X vectors when VF start fails
drivers/net/ethernet/intel/ice/ice_sriov.c | 42 ++++++++++++++++++++-
drivers/net/ethernet/intel/ice/ice_vf_lib.c | 16 +++++++-
2 files changed, 54 insertions(+), 4 deletions(-)
--
2.25.1
