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

[sashiko-bot@kernel.org]

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