Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] ice: UAF in NAPI Rx path due to missing RCU grace period after xa_erase - [Medium] net/ice: silent failure to apply VF MAC/VLAN to hardware due to TOCTOU race --
commit 32649319eb3be01d6b10e730f49f29871e5cb572 Author: Linkui Xiao <[email protected]> ice: attach and detach VF representors outside of vf->cfg_lock This patch moves the detach and attach of VF representors outside of the vf->cfg_lock to resolve a lock inversion between cfg_lock, the devlink instance lock, and RTNL. > diff --git a/drivers/net/ethernet/intel/ice/ice_vf_lib.c > b/drivers/net/ethernet/intel/ice/ice_vf_lib.c > --- a/drivers/net/ethernet/intel/ice/ice_vf_lib.c > +++ b/drivers/net/ethernet/intel/ice/ice_vf_lib.c > @@ -789,9 +789,21 @@ void ice_reset_all_vfs(struct ice_pf *pf) > > /* free VF resources to begin resetting the VSI state */ > ice_for_each_vf(pf, bkt, vf) { > + /* Detach the representor before cfg_lock and attach it after > + * releasing it again: both take the devlink instance lock and > + * then RTNL, while __ice_set_vf_mac(), ice_set_vf_port_vlan() > + * and the representor ethtool reset take cfg_lock under RTNL. > + * Take cfg_lock once ahead of the detach to wait out a reset > + * that is already inside the lock; ICE_VF_DIS, raised above, > + * keeps later ones out. > + */ > mutex_lock(&vf->cfg_lock); > + mutex_unlock(&vf->cfg_lock); > > ice_eswitch_detach_vf(pf, vf); [Severity: High] Does this code leave a window for a use-after-free in the NAPI Rx path? During teardown, ice_eswitch_detach() calls unregister_netdev() which waits for existing RCU readers. However, the representor pointer remains in the eswitch.reprs XArray during and after unregister_netdev(). Because the pointer is still in the XArray, a concurrent NAPI Rx thread calling ice_eswitch_get_target() can successfully fetch it via xa_load() even after unregister_netdev() completes. The teardown path then removes the pointer via xa_erase() and immediately calls ice_repr_destroy(), executing free_netdev() and kfree() with no additional RCU grace period. If a NAPI thread fetched the pointer just before xa_erase(), it will dereference freed memory. > + > + mutex_lock(&vf->cfg_lock); > + > vf->driver_caps = 0; > ice_vc_set_default_allowlist(vf); > > ice_vf_fdir_exit(vf); > ice_vf_fdir_init(vf); > /* clean VF control VSI when resetting VFs since it should be > * setup only when VF creates its first FDIR rule. > */ > if (vf->ctrl_vsi_idx != ICE_NO_VSI) > ice_vf_ctrl_vsi_release(vf); > > ice_vf_pre_vsi_rebuild(vf); > if (ice_vf_rebuild_vsi(vf)) { > dev_err(dev, "VF %u VSI rebuild failed, leaving VF > disabled\n", > vf->vf_id); > mutex_unlock(&vf->cfg_lock); > continue; > } > ice_vf_post_vsi_rebuild(vf); > > + mutex_unlock(&vf->cfg_lock); [Severity: Medium] Does this sequence leave a window where a concurrent VF MAC or VLAN update can silently fail to apply to the hardware? If a user configures the VF MAC, __ice_set_vf_mac() calls ice_check_vf_ready_for_cfg() before acquiring vf->cfg_lock. If a PF reset occurs simultaneously, ice_reset_all_vfs() sets ICE_VF_DIS globally. If this happens right after __ice_set_vf_mac() passes its check, __ice_set_vf_mac() will wait to acquire vf->cfg_lock. As seen in the lines above, ice_reset_all_vfs() acquires vf->cfg_lock and rebuilds the VF VSI using the old MAC address. After ice_reset_all_vfs() releases the lock, __ice_set_vf_mac() acquires it, updates the software MAC, and calls ice_reset_vf(). Because ICE_VF_DIS is still set by the PF reset, ice_reset_vf() aborts the hardware reprogram. __ice_set_vf_mac() then returns success based on its earlier check, leaving the hardware enforcing the old MAC while software tools report the new one. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
