On Sat, Oct 10, 2026 at 10:42:49AM +0800, Linkui Xiao wrote: > > On 2026/10/9 21:05, [email protected] 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 <[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--; > > > } > > > > > > 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 <[email protected]>
