[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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] ice: UAF in NAPI Rx path due to missing RCU grace period after xa_erase
- [Medium] net/ice: silent failure to apply VF MAC/VLAN to hardware due to TOCTOU race
--

commit 32649319eb3be01d6b10e730f49f29871e5cb572
Author: Linkui Xiao <xiaolinkui@kylinos.cn>

ice: attach and detach VF representors outside of vf->cfg_lock

This patch moves the detach and attach of VF representors outside of
the vf->cfg_lock to resolve a lock inversion between cfg_lock, the
devlink instance lock, and RTNL.

> diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib.c b/drivers/net/ethernet/intel/ice/ice_vf_lib.c
> --- 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);

[Severity: High]
Does this code leave a window for a use-after-free in the NAPI Rx path?

During teardown, ice_eswitch_detach() calls unregister_netdev() which
waits for existing RCU readers. However, the representor pointer remains
in the eswitch.reprs XArray during and after unregister_netdev().

Because the pointer is still in the XArray, a concurrent NAPI Rx thread
calling ice_eswitch_get_target() can successfully fetch it via xa_load()
even after unregister_netdev() completes.

The teardown path then removes the pointer via xa_erase() and immediately
calls ice_repr_destroy(), executing free_netdev() and kfree() with no
additional RCU grace period. If a NAPI thread fetched the pointer just
before xa_erase(), it will dereference freed memory.

> +
> +		mutex_lock(&vf->cfg_lock);
> +
>  		vf->driver_caps = 0;
>  		ice_vc_set_default_allowlist(vf);
>  
>  		ice_vf_fdir_exit(vf);
>  		ice_vf_fdir_init(vf);
>  		/* clean VF control VSI when resetting VFs since it should be
>  		 * setup only when VF creates its first FDIR rule.
>  		 */
>  		if (vf->ctrl_vsi_idx != ICE_NO_VSI)
>  			ice_vf_ctrl_vsi_release(vf);
>  
>  		ice_vf_pre_vsi_rebuild(vf);
>  		if (ice_vf_rebuild_vsi(vf)) {
>  			dev_err(dev, "VF %u VSI rebuild failed, leaving VF disabled\n",
>  				vf->vf_id);
>  			mutex_unlock(&vf->cfg_lock);
>  			continue;
>  		}
>  		ice_vf_post_vsi_rebuild(vf);
>  
> +		mutex_unlock(&vf->cfg_lock);

[Severity: Medium]
Does this sequence leave a window where a concurrent VF MAC or VLAN update
can silently fail to apply to the hardware?

If a user configures the VF MAC, __ice_set_vf_mac() calls
ice_check_vf_ready_for_cfg() before acquiring vf->cfg_lock.

If a PF reset occurs simultaneously, ice_reset_all_vfs() sets ICE_VF_DIS
globally. If this happens right after __ice_set_vf_mac() passes its check,
__ice_set_vf_mac() will wait to acquire vf->cfg_lock.

As seen in the lines above, ice_reset_all_vfs() acquires vf->cfg_lock and
rebuilds the VF VSI using the old MAC address. After ice_reset_all_vfs()
releases the lock, __ice_set_vf_mac() acquires it, updates the software
MAC, and calls ice_reset_vf().

Because ICE_VF_DIS is still set by the PF reset, ice_reset_vf() aborts
the hardware reprogram. __ice_set_vf_mac() then returns success based on
its earlier check, leaving the hardware enforcing the old MAC while
software tools report the new one.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008125754.3520773-1-xiaolinkui@126.com?part=1

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

[Linkui Xiao]

On 2026/10/9 21:05, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] ice: UAF in NAPI Rx path due to missing RCU grace period after xa_erase
> - [Medium] net/ice: silent failure to apply VF MAC/VLAN to hardware due to TOCTOU race
> --
> 
> commit 32649319eb3be01d6b10e730f49f29871e5cb572
> Author: Linkui Xiao <xiaolinkui@kylinos.cn>
> 
> ice: attach and detach VF representors outside of vf->cfg_lock
> 
> This patch moves the detach and attach of VF representors outside of
> the vf->cfg_lock to resolve a lock inversion between cfg_lock, the
> devlink instance lock, and RTNL.
> 
>> diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib.c b/drivers/net/ethernet/intel/ice/ice_vf_lib.c
>> --- 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);
> 
> [Severity: High]
> Does this code leave a window for a use-after-free in the NAPI Rx path?
> 
> During teardown, ice_eswitch_detach() calls unregister_netdev() which
> waits for existing RCU readers. However, the representor pointer remains
> in the eswitch.reprs XArray during and after unregister_netdev().
> 
> Because the pointer is still in the XArray, a concurrent NAPI Rx thread
> calling ice_eswitch_get_target() can successfully fetch it via xa_load()
> even after unregister_netdev() completes.
> 
> The teardown path then removes the pointer via xa_erase() and immediately
> calls ice_repr_destroy(), executing free_netdev() and kfree() with no
> additional RCU grace period. If a NAPI thread fetched the pointer just
> before xa_erase(), it will dereference freed memory.
> 
>> +
>> +		mutex_lock(&vf->cfg_lock);
>> +
>>   		vf->driver_caps = 0;
>>   		ice_vc_set_default_allowlist(vf);
>>   
>>   		ice_vf_fdir_exit(vf);
>>   		ice_vf_fdir_init(vf);
>>   		/* clean VF control VSI when resetting VFs since it should be
>>   		 * setup only when VF creates its first FDIR rule.
>>   		 */
>>   		if (vf->ctrl_vsi_idx != ICE_NO_VSI)
>>   			ice_vf_ctrl_vsi_release(vf);
>>   
>>   		ice_vf_pre_vsi_rebuild(vf);
>>   		if (ice_vf_rebuild_vsi(vf)) {
>>   			dev_err(dev, "VF %u VSI rebuild failed, leaving VF disabled\n",
>>   				vf->vf_id);
>>   			mutex_unlock(&vf->cfg_lock);
>>   			continue;
>>   		}
>>   		ice_vf_post_vsi_rebuild(vf);
>>   
>> +		mutex_unlock(&vf->cfg_lock);
> 
> [Severity: Medium]
> Does this sequence leave a window where a concurrent VF MAC or VLAN update
> can silently fail to apply to the hardware?
> 
> If a user configures the VF MAC, __ice_set_vf_mac() calls
> ice_check_vf_ready_for_cfg() before acquiring vf->cfg_lock.
> 
> If a PF reset occurs simultaneously, ice_reset_all_vfs() sets ICE_VF_DIS
> globally. If this happens right after __ice_set_vf_mac() passes its check,
> __ice_set_vf_mac() will wait to acquire vf->cfg_lock.
> 
> As seen in the lines above, ice_reset_all_vfs() acquires vf->cfg_lock and
> rebuilds the VF VSI using the old MAC address. After ice_reset_all_vfs()
> releases the lock, __ice_set_vf_mac() acquires it, updates the software
> MAC, and calls ice_reset_vf().
> 
> Because ICE_VF_DIS is still set by the PF reset, ice_reset_vf() aborts
> the hardware reprogram. __ice_set_vf_mac() then returns success based on
> its earlier check, leaving the hardware enforcing the old MAC while
> software tools report the new one.
> 
Thank you.  Both reports are about code this series does not touch: the
series only edits ice_sriov.c and ice_vf_lib.c, ice_eswitch.c and
ice_repr.c are unchanged.

 > [High] ice: UAF in NAPI Rx path due to missing RCU grace period after
 > xa_erase

The window is real, but it lives in ice_eswitch_detach(), which neither
this patch nor the other two change:

	ice_eswitch_stop_reprs(pf);
	repr->ops.rem(repr);                    /* unregister_netdev() */
	xa_erase(&pf->eswitch.reprs, repr->id);
	...
	ice_repr_destroy(repr);                 /* free_netdev() + kfree() */

That order is deliberate, and it was chosen for this class of problem:
commit e9942bfe4931 ("ice: Fix use after free during unload with ports in
bridge") moved unregister_netdev() ahead of xa_erase() so that a lookup
racing with the teardown still finds the repr while it is being
unregistered.  Moving that pair, or adding a grace period between the
erase and the destroy, is a change to ice_eswitch_detach() with its own
Fixes: history, not something a cfg_lock ordering fix should carry.

One correction on the mechanism.  ice_eswitch_get_target() is a plain
xa_load() reached from ice_process_skb_fields() when ICE_RX_FLAGS_MULTIDEV
is set on the ring, and a NAPI poll is not an RCU read-side section:
__napi_poll() calls n->poll() directly, and neither net_rx_action() nor
napi_threaded_poll() takes rcu_read_lock().  xa_load() also takes no
reference on the object it returns, so an extra grace period after
xa_erase() would not close this window as the code stands.  The lookup
itself has to be made safe first, and that is a different patch.  I can
send it if you want it.

What this series changes is when ice_eswitch_detach_vf() is called, and
that does not widen the lookup: the NAPI Rx path never took vf->cfg_lock,
so it raced with the detach both before and after.

 > [Medium] net/ice: silent failure to apply VF MAC/VLAN to hardware due
 > to TOCTOU race

The sequence is right, including the outcome: both handlers throw away
the return value of ice_reset_vf(), so once ICE_VF_DIS is up they still
report success while the MAC or VLAN only exists in software.  But every
part of it is baseline.  __ice_set_vf_mac() and ice_set_vf_port_vlan()
call ice_check_vf_ready_for_cfg() outside cfg_lock, ice_reset_vf()'s
ice_is_vf_disabled() test sits after the lock is taken, and ice_free_vfs()
and ice_reset_all_vfs() raise ICE_VF_DIS before entering the loop; the
patch does not move any of the three.  The same window is reachable today
with "echo 0 > sriov_numvfs" in place of a PF reset.

Closing it means changing those two handlers, and probably how the reset
result is reported, which is unrelated to where the detach and the attach
are placed.  I would rather send that on its own than fold it into an
-net series that fixes a lock inversion.

No code change in this series for either report.

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

[Simon Horman]

On Sat, Oct 10, 2026 at 11:00:23AM +0800, Linkui Xiao wrote:
> 
> 
> On 2026/10/9 21:05, sashiko-bot@kernel.org wrote:
> > Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> > - [High] ice: UAF in NAPI Rx path due to missing RCU grace period after xa_erase
> > - [Medium] net/ice: silent failure to apply VF MAC/VLAN to hardware due to TOCTOU race
> > --
> > 
> > commit 32649319eb3be01d6b10e730f49f29871e5cb572
> > Author: Linkui Xiao <xiaolinkui@kylinos.cn>
> > 
> > ice: attach and detach VF representors outside of vf->cfg_lock
> > 
> > This patch moves the detach and attach of VF representors outside of
> > the vf->cfg_lock to resolve a lock inversion between cfg_lock, the
> > devlink instance lock, and RTNL.
> > 
> > > diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib.c b/drivers/net/ethernet/intel/ice/ice_vf_lib.c
> > > --- 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);
> > 
> > [Severity: High]
> > Does this code leave a window for a use-after-free in the NAPI Rx path?
> > 
> > During teardown, ice_eswitch_detach() calls unregister_netdev() which
> > waits for existing RCU readers. However, the representor pointer remains
> > in the eswitch.reprs XArray during and after unregister_netdev().
> > 
> > Because the pointer is still in the XArray, a concurrent NAPI Rx thread
> > calling ice_eswitch_get_target() can successfully fetch it via xa_load()
> > even after unregister_netdev() completes.
> > 
> > The teardown path then removes the pointer via xa_erase() and immediately
> > calls ice_repr_destroy(), executing free_netdev() and kfree() with no
> > additional RCU grace period. If a NAPI thread fetched the pointer just
> > before xa_erase(), it will dereference freed memory.
> > 
> > > +
> > > +		mutex_lock(&vf->cfg_lock);
> > > +
> > >   		vf->driver_caps = 0;
> > >   		ice_vc_set_default_allowlist(vf);
> > >   		ice_vf_fdir_exit(vf);
> > >   		ice_vf_fdir_init(vf);
> > >   		/* clean VF control VSI when resetting VFs since it should be
> > >   		 * setup only when VF creates its first FDIR rule.
> > >   		 */
> > >   		if (vf->ctrl_vsi_idx != ICE_NO_VSI)
> > >   			ice_vf_ctrl_vsi_release(vf);
> > >   		ice_vf_pre_vsi_rebuild(vf);
> > >   		if (ice_vf_rebuild_vsi(vf)) {
> > >   			dev_err(dev, "VF %u VSI rebuild failed, leaving VF disabled\n",
> > >   				vf->vf_id);
> > >   			mutex_unlock(&vf->cfg_lock);
> > >   			continue;
> > >   		}
> > >   		ice_vf_post_vsi_rebuild(vf);
> > > +		mutex_unlock(&vf->cfg_lock);
> > 
> > [Severity: Medium]
> > Does this sequence leave a window where a concurrent VF MAC or VLAN update
> > can silently fail to apply to the hardware?
> > 
> > If a user configures the VF MAC, __ice_set_vf_mac() calls
> > ice_check_vf_ready_for_cfg() before acquiring vf->cfg_lock.
> > 
> > If a PF reset occurs simultaneously, ice_reset_all_vfs() sets ICE_VF_DIS
> > globally. If this happens right after __ice_set_vf_mac() passes its check,
> > __ice_set_vf_mac() will wait to acquire vf->cfg_lock.
> > 
> > As seen in the lines above, ice_reset_all_vfs() acquires vf->cfg_lock and
> > rebuilds the VF VSI using the old MAC address. After ice_reset_all_vfs()
> > releases the lock, __ice_set_vf_mac() acquires it, updates the software
> > MAC, and calls ice_reset_vf().
> > 
> > Because ICE_VF_DIS is still set by the PF reset, ice_reset_vf() aborts
> > the hardware reprogram. __ice_set_vf_mac() then returns success based on
> > its earlier check, leaving the hardware enforcing the old MAC while
> > software tools report the new one.
> > 
> Thank you.  Both reports are about code this series does not touch: the
> series only edits ice_sriov.c and ice_vf_lib.c, ice_eswitch.c and
> ice_repr.c are unchanged.
> 
> > [High] ice: UAF in NAPI Rx path due to missing RCU grace period after
> > xa_erase
> 
> The window is real, but it lives in ice_eswitch_detach(), which neither
> this patch nor the other two change:
> 
> 	ice_eswitch_stop_reprs(pf);
> 	repr->ops.rem(repr);                    /* unregister_netdev() */
> 	xa_erase(&pf->eswitch.reprs, repr->id);
> 	...
> 	ice_repr_destroy(repr);                 /* free_netdev() + kfree() */
> 
> That order is deliberate, and it was chosen for this class of problem:
> commit e9942bfe4931 ("ice: Fix use after free during unload with ports in
> bridge") moved unregister_netdev() ahead of xa_erase() so that a lookup
> racing with the teardown still finds the repr while it is being
> unregistered.  Moving that pair, or adding a grace period between the
> erase and the destroy, is a change to ice_eswitch_detach() with its own
> Fixes: history, not something a cfg_lock ordering fix should carry.
> 
> One correction on the mechanism.  ice_eswitch_get_target() is a plain
> xa_load() reached from ice_process_skb_fields() when ICE_RX_FLAGS_MULTIDEV
> is set on the ring, and a NAPI poll is not an RCU read-side section:
> __napi_poll() calls n->poll() directly, and neither net_rx_action() nor
> napi_threaded_poll() takes rcu_read_lock().  xa_load() also takes no
> reference on the object it returns, so an extra grace period after
> xa_erase() would not close this window as the code stands.  The lookup
> itself has to be made safe first, and that is a different patch.  I can
> send it if you want it.
> 
> What this series changes is when ice_eswitch_detach_vf() is called, and
> that does not widen the lookup: the NAPI Rx path never took vf->cfg_lock,
> so it raced with the detach both before and after.
> 
> > [Medium] net/ice: silent failure to apply VF MAC/VLAN to hardware due
> > to TOCTOU race
> 
> The sequence is right, including the outcome: both handlers throw away
> the return value of ice_reset_vf(), so once ICE_VF_DIS is up they still
> report success while the MAC or VLAN only exists in software.  But every
> part of it is baseline.  __ice_set_vf_mac() and ice_set_vf_port_vlan()
> call ice_check_vf_ready_for_cfg() outside cfg_lock, ice_reset_vf()'s
> ice_is_vf_disabled() test sits after the lock is taken, and ice_free_vfs()
> and ice_reset_all_vfs() raise ICE_VF_DIS before entering the loop; the
> patch does not move any of the three.  The same window is reachable today
> with "echo 0 > sriov_numvfs" in place of a PF reset.
> 
> Closing it means changing those two handlers, and probably how the reset
> result is reported, which is unrelated to where the detach and the attach
> are placed.  I would rather send that on its own than fold it into an
> -net series that fixes a lock inversion.
> 
> No code change in this series for either report.

Thanks for the detailed response.
This seems reasonable to me.

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

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

[Linkui Xiao]

On 2026/10/9 21:05, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] ice: UAF in NAPI Rx path due to missing RCU grace period after xa_erase
> - [Medium] net/ice: silent failure to apply VF MAC/VLAN to hardware due to TOCTOU race
> --
> 
> commit 32649319eb3be01d6b10e730f49f29871e5cb572
> Author: Linkui Xiao <xiaolinkui@kylinos.cn>
> 
> ice: attach and detach VF representors outside of vf->cfg_lock
> 
> This patch moves the detach and attach of VF representors outside of
> the vf->cfg_lock to resolve a lock inversion between cfg_lock, the
> devlink instance lock, and RTNL.
> 
>> diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib.c b/drivers/net/ethernet/intel/ice/ice_vf_lib.c
>> --- 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);
> 
> [Severity: High]
> Does this code leave a window for a use-after-free in the NAPI Rx path?
> 
> During teardown, ice_eswitch_detach() calls unregister_netdev() which
> waits for existing RCU readers. However, the representor pointer remains
> in the eswitch.reprs XArray during and after unregister_netdev().
> 
> Because the pointer is still in the XArray, a concurrent NAPI Rx thread
> calling ice_eswitch_get_target() can successfully fetch it via xa_load()
> even after unregister_netdev() completes.
> 
> The teardown path then removes the pointer via xa_erase() and immediately
> calls ice_repr_destroy(), executing free_netdev() and kfree() with no
> additional RCU grace period. If a NAPI thread fetched the pointer just
> before xa_erase(), it will dereference freed memory.
> 
>> +
>> +		mutex_lock(&vf->cfg_lock);
>> +
>>   		vf->driver_caps = 0;
>>   		ice_vc_set_default_allowlist(vf);
>>   
>>   		ice_vf_fdir_exit(vf);
>>   		ice_vf_fdir_init(vf);
>>   		/* clean VF control VSI when resetting VFs since it should be
>>   		 * setup only when VF creates its first FDIR rule.
>>   		 */
>>   		if (vf->ctrl_vsi_idx != ICE_NO_VSI)
>>   			ice_vf_ctrl_vsi_release(vf);
>>   
>>   		ice_vf_pre_vsi_rebuild(vf);
>>   		if (ice_vf_rebuild_vsi(vf)) {
>>   			dev_err(dev, "VF %u VSI rebuild failed, leaving VF disabled\n",
>>   				vf->vf_id);
>>   			mutex_unlock(&vf->cfg_lock);
>>   			continue;
>>   		}
>>   		ice_vf_post_vsi_rebuild(vf);
>>   
>> +		mutex_unlock(&vf->cfg_lock);
> 
> [Severity: Medium]
> Does this sequence leave a window where a concurrent VF MAC or VLAN update
> can silently fail to apply to the hardware?
> 
> If a user configures the VF MAC, __ice_set_vf_mac() calls
> ice_check_vf_ready_for_cfg() before acquiring vf->cfg_lock.
> 
> If a PF reset occurs simultaneously, ice_reset_all_vfs() sets ICE_VF_DIS
> globally. If this happens right after __ice_set_vf_mac() passes its check,
> __ice_set_vf_mac() will wait to acquire vf->cfg_lock.
> 
> As seen in the lines above, ice_reset_all_vfs() acquires vf->cfg_lock and
> rebuilds the VF VSI using the old MAC address. After ice_reset_all_vfs()
> releases the lock, __ice_set_vf_mac() acquires it, updates the software
> MAC, and calls ice_reset_vf().
> 
> Because ICE_VF_DIS is still set by the PF reset, ice_reset_vf() aborts
> the hardware reprogram. __ice_set_vf_mac() then returns success based on
> its earlier check, leaving the hardware enforcing the old MAC while
> software tools report the new one.
> 
Hi,

Thanks for the review. I did not modify the original patch, but both issues
reported here are now addressed by separate patches sent to netdev:

[High] UAF in NAPI Rx path:
   https://lore.kernel.org/all/20261012072959.254206-1-xiaolinkui@126.com/

   This patch erases the xarray entry first, then adds synchronize_net()
   before the representor is freed, and holds the RCU read lock around the
   netdev usage in ice_process_skb_fields().

[Medium] silent failure to apply VF MAC/VLAN due to TOCTOU race:
 
https://lore.kernel.org/netdev/20261012082910.283229-1-xiaolinkui@126.com/

   This patch addresses the race between 
__ice_set_vf_mac()/ice_set_vf_port_vlan()
   and the PF reset path.

Both patches are independent of the original series and were not included in
a repost of it.

Thanks,
Linkui

[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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ice: ABBA deadlock between pf->vfs.table_lock, devl_lock, and rtnl_lock
--

commit e31928012cb273c164e36248e1b33c296e805e8d
Author: Linkui Xiao <xiaolinkui@kylinos.cn>

ice: detach the VF representor when ice_start_vfs() fails

This commit modifies the error teardown path of ice_start_vfs() to explicitly
detach the VF representor. This prevents memory leaks, use-after-free
scenarios, and stale configuration states.

> diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.c b/drivers/net/ethernet/intel/ice/ice_sriov.c
> index bf6410c0d85d4..13d97dbd517c8 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)
[ ... ]
> +		/* 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);

[Severity: High]
Does this introduce an ABBA deadlock involving pf->vfs.table_lock, devl_lock,
and rtnl_lock?

ice_start_vfs() runs with pf->vfs.table_lock held. Adding a call to
ice_eswitch_detach_vf() here establishes a lock acquisition order of:

table_lock -> devl_lock (acquired in ice_eswitch_detach_vf) -> rtnl_lock
(acquired via unregister_netdev in ice_repr_rem_vf).

However, there is an opposing lock acquisition order during TC mqprio
configuration:

rtnl_lock (held during ndo_setup_tc) -> devl_lock (acquired in
ice_tear_down_devlink_rate_tree) -> table_lock (acquired right after
devl_lock in ice_tear_down_devlink_rate_tree).

If a user enables SR-IOV, triggering an error in ice_start_vfs(), while
concurrently configuring TC mqprio, could this opposing lock acquisition
order result in a deadlock?

> +
> +		mutex_lock(&vf->cfg_lock);
>  		ice_dis_vf_mappings(vf);
>  		ice_vf_vsi_release(vf);
> +		mutex_unlock(&vf->cfg_lock);
> +
>  		it_cnt--;
>  	}
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008125754.3520773-1-xiaolinkui@126.com?part=2

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

[Linkui Xiao]

On 2026/10/9 21:05, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] ice: ABBA deadlock between pf->vfs.table_lock, devl_lock, and rtnl_lock
> --
> 
> commit e31928012cb273c164e36248e1b33c296e805e8d
> Author: Linkui Xiao <xiaolinkui@kylinos.cn>
> 
> ice: detach the VF representor when ice_start_vfs() fails
> 
> This commit modifies the error teardown path of ice_start_vfs() to explicitly
> detach the VF representor. This prevents memory leaks, use-after-free
> scenarios, and stale configuration states.
> 
>> diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.c b/drivers/net/ethernet/intel/ice/ice_sriov.c
>> index bf6410c0d85d4..13d97dbd517c8 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)
> [ ... ]
>> +		/* 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);
> 
> [Severity: High]
> Does this introduce an ABBA deadlock involving pf->vfs.table_lock, devl_lock,
> and rtnl_lock?
> 
> ice_start_vfs() runs with pf->vfs.table_lock held. Adding a call to
> ice_eswitch_detach_vf() here establishes a lock acquisition order of:
> 
> table_lock -> devl_lock (acquired in ice_eswitch_detach_vf) -> rtnl_lock
> (acquired via unregister_netdev in ice_repr_rem_vf).
> 
> However, there is an opposing lock acquisition order during TC mqprio
> configuration:
> 
> rtnl_lock (held during ndo_setup_tc) -> devl_lock (acquired in
> ice_tear_down_devlink_rate_tree) -> table_lock (acquired right after
> devl_lock in ice_tear_down_devlink_rate_tree).
> 
> If a user enables SR-IOV, triggering an error in ice_start_vfs(), while
> concurrently configuring TC mqprio, could this opposing lock acquisition
> order result in a deadlock?
> 
>> +
>> +		mutex_lock(&vf->cfg_lock);
>>   		ice_dis_vf_mappings(vf);
>>   		ice_vf_vsi_release(vf);
>> +		mutex_unlock(&vf->cfg_lock);
>> +
>>   		it_cnt--;
>>   	}
>>
> 
Thanks for the review.

I agree that the three lock orders do not converge, but I do not think
this patch introduces the inversion, and the cycle is reachable without
it.

ice_start_vfs() already runs under pf->vfs.table_lock and already takes
devl_lock with that lock held, before any failure can happen. Its
forward loop does

	if (ice_is_eswitch_mode_switchdev(pf)) {
		retval = ice_eswitch_attach_vf(pf, vf);
		...
	}

and ice_eswitch_attach_vf() takes devl_lock() around
ice_eswitch_attach(). The teardown loop unwinds only the VFs whose
attach succeeded -- it walks the VFs in the same order and stops once
it_cnt reaches 0 -- so whenever the ice_eswitch_detach_vf() added here
can reach devl_lock(), an ice_eswitch_attach_vf() has already taken it
earlier in the same call, under the same table_lock. In legacy mode
there are no representors to begin with: ice_eswitch_detach_vf()
returns at its xa_load() before reaching devl_lock(), and nothing
attached either. Either way, table_lock -> devl_lock is not a new pair
here.

The full table_lock -> devl_lock -> rtnl_lock chain is pre-existing
too. The other two callers of the same helper hold table_lock and reach
RTNL through it:

	ice_free_vfs()        table_lock -> ice_eswitch_detach_vf()
	ice_reset_all_vfs()                  -> devl_lock()
	                                     -> repr->ops.rem()
	                                     -> ice_repr_rem_vf()
	                                     -> unregister_netdev()

That is in the baseline, not added by the series, and the opposite
order, rtnl_lock -> devl_lock -> table_lock in
ice_tear_down_devlink_rate_tree() reached from ndo_setup_tc, is equally
untouched. So "echo 0 > sriov_numvfs" racing a TC mqprio configuration
can reach both halves today. This is the same pair that was raised in
the v1 review, and the conclusion has not changed.

That said, the two orders do not converge, and converging them is worth
doing. It means taking devl_lock outside pf->vfs.table_lock in
ice_ena_vfs(), ice_free_vfs() and ice_reset_all_vfs(), plus variants of
ice_eswitch_attach_vf() and ice_eswitch_detach_vf() that expect
devl_lock() to be held, since devl_lock() is a plain mutex and the
helpers take it internally. That is a lock hierarchy change across
three callers and the eswitch API, and it is unrelated to the
cfg_lock -> RTNL inversion this series is about, so I would rather send
it as a separate series than carry it in an iwl-net fix that has to be
backported to stable.

No code change in this series for this report.

Thanks,
Linkui

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

[Simon Horman]

On Sat, Oct 10, 2026 at 10:42:49AM +0800, Linkui Xiao wrote:
> 
> On 2026/10/9 21:05, sashiko-bot@kernel.org wrote:
> > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> > - [High] ice: ABBA deadlock between pf->vfs.table_lock, devl_lock, and rtnl_lock
> > --
> > 
> > commit e31928012cb273c164e36248e1b33c296e805e8d
> > Author: Linkui Xiao <xiaolinkui@kylinos.cn>
> > 
> > ice: detach the VF representor when ice_start_vfs() fails
> > 
> > This commit modifies the error teardown path of ice_start_vfs() to explicitly
> > detach the VF representor. This prevents memory leaks, use-after-free
> > scenarios, and stale configuration states.
> > 
> > > diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.c b/drivers/net/ethernet/intel/ice/ice_sriov.c
> > > index bf6410c0d85d4..13d97dbd517c8 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)
> > [ ... ]
> > > +		/* 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);
> > 
> > [Severity: High]
> > Does this introduce an ABBA deadlock involving pf->vfs.table_lock, devl_lock,
> > and rtnl_lock?
> > 
> > ice_start_vfs() runs with pf->vfs.table_lock held. Adding a call to
> > ice_eswitch_detach_vf() here establishes a lock acquisition order of:
> > 
> > table_lock -> devl_lock (acquired in ice_eswitch_detach_vf) -> rtnl_lock
> > (acquired via unregister_netdev in ice_repr_rem_vf).
> > 
> > However, there is an opposing lock acquisition order during TC mqprio
> > configuration:
> > 
> > rtnl_lock (held during ndo_setup_tc) -> devl_lock (acquired in
> > ice_tear_down_devlink_rate_tree) -> table_lock (acquired right after
> > devl_lock in ice_tear_down_devlink_rate_tree).
> > 
> > If a user enables SR-IOV, triggering an error in ice_start_vfs(), while
> > concurrently configuring TC mqprio, could this opposing lock acquisition
> > order result in a deadlock?
> > 
> > > +
> > > +		mutex_lock(&vf->cfg_lock);
> > >   		ice_dis_vf_mappings(vf);
> > >   		ice_vf_vsi_release(vf);
> > > +		mutex_unlock(&vf->cfg_lock);
> > > +
> > >   		it_cnt--;
> > >   	}
> > > 
> > 
> Thanks for the review.
> 
> I agree that the three lock orders do not converge, but I do not think
> this patch introduces the inversion, and the cycle is reachable without
> it.
> 
> ice_start_vfs() already runs under pf->vfs.table_lock and already takes
> devl_lock with that lock held, before any failure can happen. Its
> forward loop does
> 
> 	if (ice_is_eswitch_mode_switchdev(pf)) {
> 		retval = ice_eswitch_attach_vf(pf, vf);
> 		...
> 	}
> 
> and ice_eswitch_attach_vf() takes devl_lock() around
> ice_eswitch_attach(). The teardown loop unwinds only the VFs whose
> attach succeeded -- it walks the VFs in the same order and stops once
> it_cnt reaches 0 -- so whenever the ice_eswitch_detach_vf() added here
> can reach devl_lock(), an ice_eswitch_attach_vf() has already taken it
> earlier in the same call, under the same table_lock. In legacy mode
> there are no representors to begin with: ice_eswitch_detach_vf()
> returns at its xa_load() before reaching devl_lock(), and nothing
> attached either. Either way, table_lock -> devl_lock is not a new pair
> here.
> 
> The full table_lock -> devl_lock -> rtnl_lock chain is pre-existing
> too. The other two callers of the same helper hold table_lock and reach
> RTNL through it:
> 
> 	ice_free_vfs()        table_lock -> ice_eswitch_detach_vf()
> 	ice_reset_all_vfs()                  -> devl_lock()
> 	                                     -> repr->ops.rem()
> 	                                     -> ice_repr_rem_vf()
> 	                                     -> unregister_netdev()
> 
> That is in the baseline, not added by the series, and the opposite
> order, rtnl_lock -> devl_lock -> table_lock in
> ice_tear_down_devlink_rate_tree() reached from ndo_setup_tc, is equally
> untouched. So "echo 0 > sriov_numvfs" racing a TC mqprio configuration
> can reach both halves today. This is the same pair that was raised in
> the v1 review, and the conclusion has not changed.
> 
> That said, the two orders do not converge, and converging them is worth
> doing. It means taking devl_lock outside pf->vfs.table_lock in
> ice_ena_vfs(), ice_free_vfs() and ice_reset_all_vfs(), plus variants of
> ice_eswitch_attach_vf() and ice_eswitch_detach_vf() that expect
> devl_lock() to be held, since devl_lock() is a plain mutex and the
> helpers take it internally. That is a lock hierarchy change across
> three callers and the eswitch API, and it is unrelated to the
> cfg_lock -> RTNL inversion this series is about, so I would rather send
> it as a separate series than carry it in an iwl-net fix that has to be
> backported to stable.
> 
> No code change in this series for this report.

Thanks for the detailed response.
This seems reasonable to me.

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

[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

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

[Simon Horman]

On Thu, Oct 08, 2026 at 08:57:54PM +0800, Linkui Xiao wrote:
> 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>

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

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

[Loktionov, Aleksandr]

> -----Original Message-----
> From: Linkui Xiao <xiaolinkui@126.com>
> Sent: Thursday, October 8, 2026 2:58 PM
> To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Kitszel, Przemyslaw
> <przemyslaw.kitszel@intel.com>; andrew+netdev@lunn.ch;
> davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com
> Cc: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; Linkui Xiao <xiaolinkui@kylinos.cn>;
> stable@vger.kernel.org
> Subject: [PATCH iwl-net v3 3/3] ice: free the VF MSI-X vectors when VF
> start fails
> 
> 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

Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>