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 <[email protected]> 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/[email protected]?part=2
