[PATCH iwl-net v3 0/3] ice: fix VF representor lock ordering and teardown error paths

[PATCH iwl-net v3 0/3] ice: fix VF representor lock ordering and teardown error paths

[Linkui Xiao]

From: Linkui Xiao <xiaolinkui@kylinos.cn>

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/20260928065306.1514795-1-xiaolinkui@126.com/

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

[PATCH iwl-net v3 1/3] ice: attach and detach VF representors outside of vf->cfg_lock

[Linkui Xiao]

From: Linkui Xiao <xiaolinkui@kylinos.cn>

ice_free_vfs() and ice_reset_all_vfs() call ice_eswitch_detach_vf()
and ice_eswitch_attach_vf() with vf->cfg_lock held. Both take the
devlink instance lock and then register or unregister the representor
netdev, which takes RTNL. That gives

	cfg_lock -> devlink instance lock -> RTNL

while three other paths take cfg_lock with RTNL already held:

  - __ice_set_vf_mac(), from ndo_set_vf_mac()
  - ice_set_vf_port_vlan(), from ndo_set_vf_vlan()
  - ice_repr_ethtool_reset(), via ice_reset_vf() with
    ICE_VF_RESET_LOCK

which is the opposite order, RTNL -> cfg_lock. An "ip link set ... vf
N mac" on one CPU against "echo 0 > sriov_numvfs" or a PF reset on
another can therefore deadlock.

Move the detach and the attach outside of cfg_lock. Representors are
created and removed under pf->vfs.table_lock only, which both callers
already hold, and ice_start_vfs() attaches without cfg_lock too.
Nothing that the detach reads is torn down by moving it: repr->src_vsi
still points at the VF VSI, which is only released later, under
cfg_lock.

Both callers raise ICE_VF_DIS in pf->state before entering the loop,
so a reset that starts after that leaves through ice_is_vf_disabled()
before it reaches ice_eswitch_update_repr(), and
ice_check_vf_ready_for_cfg() keeps the ndo handlers and the ethtool
reset away as well. ICE_VF_DIS does not cover a reset that is already
inside cfg_lock when the loop gets there, so take cfg_lock once before
the detach to wait that one out while its representor is still
attached. Without the wait it could xa_load() the representor that
ice_repr_destroy() is freeing, and that is free_netdev() + kfree(),
with no RCU grace period in between.

In ice_reset_all_vfs() the attach moves after mutex_unlock(), so the
"VSI rebuild failed" path still leaves the VF detached, exactly as it
did before.

Found by code inspection of the VF setup and teardown paths; the same
inversion has also been hit out of tree. It was not triggered here
and no stack trace was captured. Compile-tested only, not run on
hardware.

Fixes: fff292b47ac1 ("ice: add VF representors one by one")
Fixes: c9663f79cd82 ("ice: adjust switchdev rebuild path")
Cc: stable@vger.kernel.org
Suggested-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Signed-off-by: Linkui Xiao <xiaolinkui@kylinos.cn>
---
Changes in v3:
- New patch. Move 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, which is where
  the inversion is: both take the devlink instance lock and then RTNL, while
  ndo_set_vf_mac(), ndo_set_vf_vlan() and the representor ethtool reset take
  vf->cfg_lock with RTNL already held.
  (Przemek Kitszel)
- Take cfg_lock once before the detach as well, so that a reset that is already
  inside the lock is waited out instead of being left with a window between the
  detach and the lock acquisition that follows it.
- Include how the issue was found, that it has not been triggered, and that the
  change is compile tested only, as netdev-bot asked for.
 drivers/net/ethernet/intel/ice/ice_sriov.c  | 12 ++++++++++++
 drivers/net/ethernet/intel/ice/ice_vf_lib.c | 16 ++++++++++++++--
 2 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.c b/drivers/net/ethernet/intel/ice/ice_sriov.c
index e04de0215596..471c1e29a865 100644
--- a/drivers/net/ethernet/intel/ice/ice_sriov.c
+++ b/drivers/net/ethernet/intel/ice/ice_sriov.c
@@ -154,9 +154,21 @@ void ice_free_vfs(struct ice_pf *pf)
 	mutex_lock(&vfs->table_lock);
 
 	ice_for_each_vf(pf, bkt, vf) {
+		/* Detach the representor before cfg_lock: it takes the
+		 * devlink instance lock and then RTNL, while
+		 * __ice_set_vf_mac(), ice_set_vf_port_vlan() and the
+		 * representor ethtool reset take cfg_lock under RTNL.
+		 * Take cfg_lock once ahead of it to wait out a reset that
+		 * is already inside the lock; ICE_VF_DIS, raised above,
+		 * keeps later ones out.
+		 */
 		mutex_lock(&vf->cfg_lock);
+		mutex_unlock(&vf->cfg_lock);
 
 		ice_eswitch_detach_vf(pf, vf);
+
+		mutex_lock(&vf->cfg_lock);
+
 		ice_dis_vf_qs(vf);
 		ice_virt_free_irqs(pf, vf->first_vector_idx, vf->num_msix);
 
diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib.c b/drivers/net/ethernet/intel/ice/ice_vf_lib.c
index a54cb2b8d3c7..b91eec0adf51 100644
--- a/drivers/net/ethernet/intel/ice/ice_vf_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_vf_lib.c
@@ -789,9 +789,21 @@ void ice_reset_all_vfs(struct ice_pf *pf)
 
 	/* free VF resources to begin resetting the VSI state */
 	ice_for_each_vf(pf, bkt, vf) {
+		/* Detach the representor before cfg_lock and attach it after
+		 * releasing it again: both take the devlink instance lock and
+		 * then RTNL, while __ice_set_vf_mac(), ice_set_vf_port_vlan()
+		 * and the representor ethtool reset take cfg_lock under RTNL.
+		 * Take cfg_lock once ahead of the detach to wait out a reset
+		 * that is already inside the lock; ICE_VF_DIS, raised above,
+		 * keeps later ones out.
+		 */
 		mutex_lock(&vf->cfg_lock);
+		mutex_unlock(&vf->cfg_lock);
 
 		ice_eswitch_detach_vf(pf, vf);
+
+		mutex_lock(&vf->cfg_lock);
+
 		vf->driver_caps = 0;
 		ice_vc_set_default_allowlist(vf);
 
@@ -812,10 +824,10 @@ void ice_reset_all_vfs(struct ice_pf *pf)
 		}
 		ice_vf_post_vsi_rebuild(vf);
 
+		mutex_unlock(&vf->cfg_lock);
+
 		if (ice_is_eswitch_mode_switchdev(pf))
 			ice_eswitch_attach_vf(pf, vf);
-
-		mutex_unlock(&vf->cfg_lock);
 	}
 
 	ice_flush(hw);
-- 
2.25.1

[PATCH iwl-net v3 2/3] ice: detach the VF representor when ice_start_vfs() fails

[Linkui Xiao]

From: Linkui Xiao <xiaolinkui@kylinos.cn>

ice_start_vfs() attaches every VF it brings up to the eswitch with
ice_eswitch_attach_vf(), but the teardown path only undoes the queue
mappings and the VF VSI. Nothing calls ice_eswitch_detach_vf() for the
VFs that were attached before the failure, and the caller,
ice_ena_vfs(), goes straight to ice_free_vf_entries(), which drops the
last reference on every VF.

The port representors created for those VFs therefore outlive the
failed VF creation:

  - the representor netdev stays registered and its devlink port stays
    allocated, so both leak;

  - repr->vf keeps pointing at the struct ice_vf that ice_put_vf() has
    just freed through ice_sriov_free_vf(), and repr->src_vsi keeps
    pointing at the VF VSI that ice_vf_vsi_release() tore down, so any
    later use of a leftover netdev, for example
    ice_eswitch_stop_all_tx_queues() walking pf->eswitch.reprs during a
    PF reset, dereferences freed memory;

  - the virtchnl ops of that VF, which ice_repr_add_vf() replaced with
    ice_virtchnl_set_repr_ops(), are never handed back to
    ice_virtchnl_set_dflt_ops();

  - pf->eswitch.reprs never becomes empty, so ice_eswitch_detach()
    never calls ice_eswitch_disable_switchdev().
    pf->eswitch.is_running stays true with the bridge offloads and the
    devlink rate topology still up, and ice_eswitch_release_env() is
    skipped, leaving the uplink VSI in the switchdev configuration that
    ice_eswitch_setup_env() gave it: local loopback enabled, Rx
    filtering disabled and the default VSI steering removed.

Detach the representor in the teardown loop the way ice_free_vfs()
does, ahead of ice_vf_vsi_release(), because ice_repr_rem_vf() and
ice_eswitch_release_repr() both need repr->src_vsi to still be valid.
Every VF the teardown loop walks completed ice_eswitch_attach_vf()
successfully, and ice_eswitch_detach_vf() already returns early for a
VF without a representor, so no extra condition is needed.

The detach runs outside of vf->cfg_lock, the way the previous patch
leaves it in ice_free_vfs() and ice_reset_all_vfs(): taking the
devlink instance lock and then RTNL under cfg_lock is the wrong way
round against the ndo_set_vf_mac(), ndo_set_vf_vlan() and representor
ethtool reset paths. The rest of the loop body still runs under
cfg_lock, as in ice_free_vfs().

The VF is marked disabled first, because nothing else keeps a reset
away here. ICE_VF_DIS in pf->state is only set once ice_ena_vfs()
succeeds, and ice_sriov_configure() runs under the PCI device lock
rather than RTNL, so ICE_VF_STATE_DIS is what makes
ice_check_vf_ready_for_cfg() reject __ice_set_vf_mac() and
ice_set_vf_port_vlan(). Setting it under cfg_lock also waits out an
ice_reset_vf() that is already running, which would otherwise reach
ice_eswitch_update_repr() on a destroyed representor.
ice_vc_process_vf_msg() tests the same bit before it reads
vf->virtchnl_ops, which ice_repr_rem_vf() restores.

Found by code inspection of the VF setup and teardown error paths. It
was not triggered and no stack trace or error message was observed.
Compile-tested only, not run on hardware.

Fixes: fff292b47ac1 ("ice: add VF representors one by one")
Cc: stable@vger.kernel.org
Signed-off-by: Linkui Xiao <xiaolinkui@kylinos.cn>
---
Changes in v3:
- Detach the representor outside vf->cfg_lock instead of under it, as Przemek
  suggested, and mark the VF disabled under cfg_lock before the detach so that
  an ice_reset_vf() that is already past its own readiness check cannot reach
  ice_eswitch_update_repr() while the representor goes away.
  (Przemek Kitszel, Sashiko AI review)
- Include how the issue was found, that it has not been triggered, and that the
  change is compile tested only, as netdev-bot asked for.
- Not carrying over the Reviewed-by tags from Tomasz Lichwala and
  Aleksandr Loktionov, as the code changed after their reviews.
 drivers/net/ethernet/intel/ice/ice_sriov.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.c b/drivers/net/ethernet/intel/ice/ice_sriov.c
index 471c1e29a865..470aec8849b6 100644
--- a/drivers/net/ethernet/intel/ice/ice_sriov.c
+++ b/drivers/net/ethernet/intel/ice/ice_sriov.c
@@ -520,8 +520,27 @@ static int ice_start_vfs(struct ice_pf *pf)
 		if (it_cnt == 0)
 			break;
 
+		/* Mark the VF disabled before its representor and its VSI go
+		 * away, the way ice_free_vfs() does, and take cfg_lock
+		 * around it to wait out an ice_reset_vf() already in
+		 * progress. pf->state has no ICE_VF_DIS on this path and the
+		 * loop leaves ICE_VF_STATE_INIT set, so without the bit a
+		 * concurrent ice_reset_vf() would pass ice_is_vf_disabled()
+		 * and reach ice_eswitch_update_repr() on a destroyed
+		 * representor, or trip WARN_ON(!vsi) in ice_dis_vf_mappings().
+		 */
+		mutex_lock(&vf->cfg_lock);
+		set_bit(ICE_VF_STATE_DIS, vf->vf_states);
+		mutex_unlock(&vf->cfg_lock);
+
+		/* detach outside of cfg_lock, see ice_free_vfs() */
+		ice_eswitch_detach_vf(pf, vf);
+
+		mutex_lock(&vf->cfg_lock);
 		ice_dis_vf_mappings(vf);
 		ice_vf_vsi_release(vf);
+		mutex_unlock(&vf->cfg_lock);
+
 		it_cnt--;
 	}
 
-- 
2.25.1

[PATCH iwl-net v3 3/3] ice: free the VF MSI-X vectors when VF start fails

[Linkui Xiao]

From: Linkui Xiao <xiaolinkui@kylinos.cn>

ice_init_vf_vsi_res() reserves vf->num_msix vectors out of
pf->virt_irq_tracker with ice_virt_get_irqs() as its very first step,
but neither of the two error paths below it gives them back. A NULL
from ice_vf_vsi_setup() returns -ENOMEM straight away, and the
release_vsi label only releases the VSI.

ice_start_vfs() leaks the same vectors. Its teardown loop undoes the
queue mappings and the VF VSI of the VFs it already started, and the
eswitch attach failure path releases the VSI of the VF it is working
on, but neither calls ice_virt_free_irqs(). The caller then runs
ice_free_vf_entries(), which drops the last reference on every VF, so
nothing further down the error path can release the reservation
either.

The tracker bitmap is only freed in ice_deinit_virt_irq_tracker(), so
the leaked vectors stay reserved for the whole lifetime of the driver
instance. Every failed "echo N > sriov_numvfs" permanently shrinks the
pool that ice_set_per_vf_res() divides up, and after enough retries
ice_virt_get_irqs() fails with -ENOENT for good even though the
hardware vectors are idle. ice_dis_vf_mappings() meanwhile re-points
GLINT_VECT2FUNC of exactly those vectors back at the PF while the
bitmap still books them to the VF.

Release the vectors on all three paths, the way ice_free_vfs() does
for a VF that is torn down normally.

Found by code inspection of the VF setup and teardown error paths. It
was not triggered and no stack trace or error message was observed.
Compile-tested only, not run on hardware.

Fixes: 4d38cb44bd32 ("ice: manage VFs MSI-X using resource tracking")
Cc: stable@vger.kernel.org
Signed-off-by: Linkui Xiao <xiaolinkui@kylinos.cn>
---
Changes in v3:
- Correct the Fixes: tag. The leak starts at 4d38cb44bd32, which replaced the
  computed first vector index with a reservation from the bitmap and left the
  error paths below it unchanged; a203163274a4 only renamed that helper and
  moved the tracker, so it is not the commit that introduced the leak.
- Include how the issue was found, that it has not been triggered, and that the
  change is compile tested only, as netdev-bot asked for.
- Free the IRQs before the VSI is released on the eswitch attach failure
  path, as Tomasz asked for, and keep the teardown loop in that same order.
  The teardown loop hunk now sits inside vf->cfg_lock instead of next to
  the detach, so the Reviewed-by tags from Tomasz Lichwala and Aleksandr
  Loktionov are not carried over.
 drivers/net/ethernet/intel/ice/ice_sriov.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.c b/drivers/net/ethernet/intel/ice/ice_sriov.c
index 470aec8849b6..444d05a9bcb0 100644
--- a/drivers/net/ethernet/intel/ice/ice_sriov.c
+++ b/drivers/net/ethernet/intel/ice/ice_sriov.c
@@ -458,8 +458,10 @@ static int ice_init_vf_vsi_res(struct ice_vf *vf)
 		return -ENOMEM;
 
 	vsi = ice_vf_vsi_setup(vf);
-	if (!vsi)
-		return -ENOMEM;
+	if (!vsi) {
+		err = -ENOMEM;
+		goto free_irqs;
+	}
 
 	err = ice_vf_init_host_cfg(vf, vsi);
 	if (err)
@@ -469,6 +471,8 @@ static int ice_init_vf_vsi_res(struct ice_vf *vf)
 
 release_vsi:
 	ice_vf_vsi_release(vf);
+free_irqs:
+	ice_virt_free_irqs(pf, vf->first_vector_idx, vf->num_msix);
 	return err;
 }
 
@@ -501,6 +505,8 @@ static int ice_start_vfs(struct ice_pf *pf)
 			if (retval) {
 				dev_err(ice_pf_to_dev(pf), "Failed to attach VF %d to eswitch, error %d",
 					vf->vf_id, retval);
+				ice_virt_free_irqs(pf, vf->first_vector_idx,
+						   vf->num_msix);
 				ice_vf_vsi_release(vf);
 				goto teardown;
 			}
@@ -537,6 +543,7 @@ static int ice_start_vfs(struct ice_pf *pf)
 		ice_eswitch_detach_vf(pf, vf);
 
 		mutex_lock(&vf->cfg_lock);
+		ice_virt_free_irqs(pf, vf->first_vector_idx, vf->num_msix);
 		ice_dis_vf_mappings(vf);
 		ice_vf_vsi_release(vf);
 		mutex_unlock(&vf->cfg_lock);
-- 
2.25.1