[PATCH net] ice: detach the VF representor when ice_start_vfs() fails

[PATCH net] 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 its reference to
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
    registered too. That port is embedded in struct ice_vf, so it ends
    up pointing into the memory that ice_sriov_free_vf() releases;

  - repr->vf keeps pointing at the freed struct ice_vf, and repr->src_vsi
    at the VF VSI that ice_vf_vsi_release() tore down. The leftover
    netdev is still visible to the user, so even a plain
    "ip -s link show" of it reaches ice_repr_get_stats64(), which calls
    repr->ops.ready() -> ice_check_vf_ready_for_cfg(repr->vf) and then
    reads repr->src_vsi through ice_update_eth_stats();

  - 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, so the uplink VSI is
    left in the switchdev configuration that ice_eswitch_setup_env()
    gave it.

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.

Fixes: fff292b47ac1 ("ice: add VF representors one by one")
Cc: stable@vger.kernel.org
Signed-off-by: Linkui Xiao <xiaolinkui@kylinos.cn>
---
 drivers/net/ethernet/intel/ice/ice_sriov.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.c b/drivers/net/ethernet/intel/ice/ice_sriov.c
index e04de0215596..1b563aba3bbd 100644
--- a/drivers/net/ethernet/intel/ice/ice_sriov.c
+++ b/drivers/net/ethernet/intel/ice/ice_sriov.c
@@ -508,6 +508,7 @@ static int ice_start_vfs(struct ice_pf *pf)
 		if (it_cnt == 0)
 			break;
 
+		ice_eswitch_detach_vf(pf, vf);
 		ice_dis_vf_mappings(vf);
 		ice_vf_vsi_release(vf);
 		it_cnt--;
-- 
2.25.1

RE: [PATCH net] ice: detach the VF representor when ice_start_vfs() fails

[Loktionov, Aleksandr]

> -----Original Message-----
> From: Linkui Xiao <xiaolinkui@126.com>
> Sent: Monday, September 21, 2026 5:16 AM
> 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 net] ice: detach the VF representor when
> ice_start_vfs() fails
> 
> 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 its
> reference to 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
>     registered too. That port is embedded in struct ice_vf, so it ends
>     up pointing into the memory that ice_sriov_free_vf() releases;
> 
>   - repr->vf keeps pointing at the freed struct ice_vf, and repr-
> >src_vsi
>     at the VF VSI that ice_vf_vsi_release() tore down. The leftover
>     netdev is still visible to the user, so even a plain
>     "ip -s link show" of it reaches ice_repr_get_stats64(), which
> calls
>     repr->ops.ready() -> ice_check_vf_ready_for_cfg(repr->vf) and then
>     reads repr->src_vsi through ice_update_eth_stats();
> 
>   - 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, so the uplink VSI is
>     left in the switchdev configuration that ice_eswitch_setup_env()
>     gave it.
> 
> 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.
> 
> Fixes: fff292b47ac1 ("ice: add VF representors one by one")
> Cc: stable@vger.kernel.org
> Signed-off-by: Linkui Xiao <xiaolinkui@kylinos.cn>
> ---
>  drivers/net/ethernet/intel/ice/ice_sriov.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.c
> b/drivers/net/ethernet/intel/ice/ice_sriov.c
> index e04de0215596..1b563aba3bbd 100644
> --- a/drivers/net/ethernet/intel/ice/ice_sriov.c
> +++ b/drivers/net/ethernet/intel/ice/ice_sriov.c
> @@ -508,6 +508,7 @@ static int ice_start_vfs(struct ice_pf *pf)
>  		if (it_cnt == 0)
>  			break;
> 
> +		ice_eswitch_detach_vf(pf, vf);
>  		ice_dis_vf_mappings(vf);
>  		ice_vf_vsi_release(vf);
>  		it_cnt--;
> --
> 2.25.1

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

Re: [PATCH net] ice: detach the VF representor when ice_start_vfs() fails

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 (1) · Medium: 0 (1) · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The newly added ice_eswitch_detach_vf(pf, vf) in the
  ice_start_vfs() teardown loop…

Pre-existing issues:
- [High] AB-BA lock ordering inversion between pf->vfs.table_lock and the
  devlink instance lock.
- [Medium] ice_start_vfs() teardown loop
  (drivers/net/ethernet/intel/ice/ice_sriov.c:507-515) still never…

--

ice: detach the VF representor when ice_start_vfs() fails

The patch adds a call to ice_eswitch_detach_vf() in the ice_start_vfs()
teardown loop so that port representors created by ice_eswitch_attach_vf()
are destroyed when VF startup fails. It is placed ahead of
ice_vf_vsi_release() so that repr->src_vsi is still valid. A few questions
about the new call site below.

> diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.c b/drivers/net/ethernet/intel/ice/ice_sriov.c
> index e04de0215596..1b563aba3bbd 100644
> --- a/drivers/net/ethernet/intel/ice/ice_sriov.c
> +++ b/drivers/net/ethernet/intel/ice/ice_sriov.c
> @@ -508,6 +508,7 @@ static int ice_start_vfs(struct ice_pf *pf)
>  		if (it_cnt == 0)
>  			break;
>  
> +		ice_eswitch_detach_vf(pf, vf);

[Severity: High]
Should this call be made with vf->cfg_lock held?

ice_start_vfs() only holds pf->vfs.table_lock (see the
lockdep_assert_held(&pf->vfs.table_lock) at the top of the function). Both
existing callers of ice_eswitch_detach_vf() hold vf->cfg_lock across it:

drivers/net/ethernet/intel/ice/ice_sriov.c:ice_free_vfs() {
	ice_for_each_vf(pf, bkt, vf) {
		mutex_lock(&vf->cfg_lock);

		ice_eswitch_detach_vf(pf, vf);
	...
}

and ice_reset_all_vfs() in ice_vf_lib.c does the same.

cfg_lock rather than table_lock is what serializes VF reconfiguration:
ice_vc_process_vf_msg() in virt/virtchnl.c takes only
mutex_lock(&vf->cfg_lock) before dispatching through vf->virtchnl_ops, and
the ndo handlers ice_set_vf_mac(), ice_set_vf_trust() and
ice_set_vf_port_vlan() take cfg_lock and then call ice_reset_vf(), which
reaches ice_eswitch_update_repr().

Since pci_enable_sriov() has already run by the time ice_start_vfs() is
called, and ice_sriov_configure() does not take rtnl, can the following
interleave?

CPU0 (sysfs sriov_numvfs, teardown loop, no cfg_lock)
ice_eswitch_detach_vf()
  ice_eswitch_detach()
    xa_erase(&pf->eswitch.reprs, repr->id);
    ice_repr_destroy(repr);   /* free_netdev()/kfree() */

CPU1 (ip link set dev ... vf N trust on, cfg_lock only)
ice_reset_vf()
  ice_eswitch_update_repr() {
	repr = xa_load(&pf->eswitch.reprs, *repr_id);
	if (!repr)
		return;

	repr->src_vsi = vsi;
	repr->dst->u.port_info.port_id = vsi->vsi_num;

	if (repr->br_port)
		repr->br_port->vsi = vsi;
  }

Does this write through a freed struct ice_repr? The VFs this loop walks
still have ICE_VF_STATE_INIT set and ICE_VF_STATE_DIS clear, so
ice_check_vf_ready_for_cfg() succeeds on the concurrent path; unlike
ice_free_vfs(), the new loop never sets ICE_VF_STATE_DIS.

The same unlocked window also has ice_repr_rem_vf() calling
ice_virtchnl_set_dflt_ops(repr->vf) while ice_vc_process_vf_msg() reads
vf->virtchnl_ops under cfg_lock.

[Severity: High]
This isn't a bug introduced by this patch, but it adds one more instance of
it: ice_eswitch_detach_vf() takes the devlink instance lock internally,

drivers/net/ethernet/intel/ice/ice_eswitch.c:ice_eswitch_detach_vf() {
	...
	devl_lock(devlink);
	ice_eswitch_detach(pf, repr);
	devl_unlock(devlink);
}

so this call establishes table_lock -> devl_lock, the same order that
ice_eswitch_attach_vf() in this function and ice_eswitch_detach_vf() in
ice_free_vfs() already use.

The reverse order exists in ice_tear_down_devlink_rate_tree():

drivers/net/ethernet/intel/ice/devlink/devlink.c {
	devl_lock(devlink);
	mutex_lock(&pf->vfs.table_lock);
	ice_for_each_vf(pf, bkt, vf) {
	...
}

which is reached from ice_setup_tc_mqprio_qdisc() in ice_main.c and from
ice_pf_dcb_cfg() in ice_dcb_lib.c, neither of which holds devl_lock
already and neither of which is excluded while switchdev/SR-IOV is active.

Can this deadlock?

CPU0: write to sriov_numvfs -> ice_ena_vfs() -> ice_start_vfs() rollback
      holds pf->vfs.table_lock, blocks on devl_lock

CPU1: tc qdisc ... mqprio hw 1 mode channel, or a DCB reconfiguration
      holds devl_lock, blocks on pf->vfs.table_lock

ice_sriov_configure() runs under the PCI sysfs device_lock and takes no
rtnl, while the TC/DCB paths run under rtnl, so the two sides do not seem
to be mutually exclusive. Would it make sense to converge on one hierarchy
here, for example acquiring devl_lock outside table_lock in ice_ena_vfs()
and using lock-held attach/detach helpers?

>  		ice_dis_vf_mappings(vf);
>  		ice_vf_vsi_release(vf);
>  		it_cnt--;

[Severity: Medium]
This is a pre-existing issue, but since the loop is being made symmetric
with ice_free_vfs(), does this loop also need to return the VF MSI-X
window?

ice_init_vf_vsi_res() reserves it:

	vf->first_vector_idx = ice_virt_get_irqs(pf, vf->num_msix);
	if (vf->first_vector_idx < 0)
		return -ENOMEM;

ice_virt_get_irqs() does bitmap_set() on the PF-wide
pf->virt_irq_tracker.bm, and the only release helper,
ice_virt_free_irqs(), is not called anywhere on this failure path: not in
this teardown loop, not at the release_vsi label of
ice_init_vf_vsi_res(), not in the ice_eswitch_attach_vf() failure branch,
and not in the caller, where err_unroll_vf_entries -> ice_free_vf_entries()
-> ice_put_vf() -> ice_sriov_free_vf() only does mutex_destroy() and
kfree_rcu().

ice_free_vfs() pairs the two calls the new loop now half-mirrors:

		ice_eswitch_detach_vf(pf, vf);
		ice_dis_vf_qs(vf);
		ice_virt_free_irqs(pf, vf->first_vector_idx, vf->num_msix);

The tracker bitmap is allocated once per PF in
ice_init_virt_irq_tracker() and freed only in
ice_deinit_virt_irq_tracker(), and ice_set_per_vf_res() sizes VFs from
pf->virt_irq_tracker.num_entries rather than from the number of free bits.
Does each failed "echo N > sriov_numvfs" therefore leak
num_vfs * num_msix_per_vf entries of the VF MSI-X range until
ice_virt_get_irqs() starts returning an error and SR-IOV can no longer be
enabled without a driver reload?

Note also that ice_dis_vf_mappings() clears VPINT_ALLOC/VPINT_ALLOC_PCI and
re-points GLINT_VECT2FUNC of exactly those vectors back at the PF, so the
hardware mapping and the software bookkeeping end up disagreeing.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921031616.3390259-1-xiaolinkui%40126.com

Re: [PATCH net] ice: detach the VF representor when ice_start_vfs() fails

[Linkui Xiao]

 > [High] The newly added ice_eswitch_detach_vf(pf, vf) in the 
ice_start_vfs()
 > teardown loop -- Should this call be made with vf->cfg_lock held?

Agreed, and thanks for the exact interleaving. Both existing callers,
ice_free_vfs() and ice_reset_all_vfs(), hold vf->cfg_lock across the detach,
and the VFs this loop unwinds are precisely the ones that already reached
set_bit(ICE_VF_STATE_INIT) -- which is what ice_check_vf_ready_for_cfg()
checks -- so nothing keeps a concurrent "ip link set dev <pf> vf N ..." or
a VF mailbox message out of them any more.

v2 holds vf->cfg_lock across the whole teardown of each VF, the way
ice_free_vfs() does, which also covers ice_dis_vf_mappings() and
ice_vf_vsi_release(). Both writers in your interleaving are excluded by
that: ice_reset_vf() (reached from __ice_set_vf_mac(), ice_set_vf_trust(),
ice_set_vf_port_vlan() and the other ndo handlers) and
ice_vc_process_vf_msg() take the same lock. Attach stays lockless on
purpose: on the way up ICE_VF_STATE_INIT is not set yet, so
ice_check_vf_init() keeps the configuration paths out.

 > [High] AB-BA lock ordering inversion between pf->vfs.table_lock and the
 > devlink instance lock.

I do not think this one is added by the patch, and the fix for it is much
larger than this UAF fix.

The pair table_lock -> devl_lock is already taken in this very loop before
the failure: whenever the teardown loop has anything to unwind (it_cnt 
!= 0),
at least one ice_eswitch_attach_vf() has already run in the forward loop
above, under the same table_lock. ice_eswitch_detach_vf() in ice_free_vfs()
and ice_reset_all_vfs() does the same. The call added here goes through the
same helper, inside the region that is already under table_lock, so it does
not establish a new ordering pair.

Converging on one hierarchy, as you suggest, means taking devl_lock outside
table_lock in ice_ena_vfs()/ice_free_vfs()/ice_reset_all_vfs() and adding
devl_lock-held variants of ice_eswitch_attach_vf()/ice_eswitch_detach_vf().
That is a lock-hierarchy change across three callers plus the eswitch API,
and it does not belong in a -net patch that has to be backported to stable.
I would rather propose it as a separate series if you think it is worth
doing.

 > [Medium] ... does this loop also need to return the VF MSI-X window?

Agreed, this one is real. ice_virt_get_irqs() does bitmap_set() on the
PF-wide pf->virt_irq_tracker.bm and nothing on this path calls
ice_virt_free_irqs(): not the release_vsi label of ice_init_vf_vsi_res(),
not the ice_eswitch_attach_vf() failure branch, and not the teardown loop.
The caller does not help either. Since the tracker lives for the lifetime of
the PF and ice_set_per_vf_res() sizes the VFs from
pf->virt_irq_tracker.num_entries rather than from the free area, every
failed "echo N > sriov_numvfs" leaks a little more of the range.

As you say, it is pre-existing, and its root cause is different from the
representor bug, so v2 sends it as patch 2/2 instead of folding it into
patch 1/2, with its own Fixes: tag. It returns the window on every failure
path, in the order ice_free_vfs() uses.

v2 is two patches on the same baseline as v1:

   1/2 ice: detach the VF representor when ice_start_vfs() fails
   2/2 ice: release the VF MSI-X window when ice_start_vfs() fails   (new)

Code of 1/2 changed to add the cfg_lock, so the Reviewed-by from Aleksandr
Loktionov is not carried over.

pw-bot: cr