[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