Re: [PATCH iwl-net v3 1/3] ice: attach and detach VF representors outside of vf->cfg_lock

[Simon Horman <horms@kernel.org>]

On Sat, Oct 10, 2026 at 11:00:23AM +0800, Linkui Xiao wrote:
> 
> 
> On 2026/10/9 21:05, sashiko-bot@kernel.org wrote:
> > 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 <xiaolinkui@kylinos.cn>
> > 
> > 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.
> > 
> Thank you.  Both reports are about code this series does not touch: the
> series only edits ice_sriov.c and ice_vf_lib.c, ice_eswitch.c and
> ice_repr.c are unchanged.
> 
> > [High] ice: UAF in NAPI Rx path due to missing RCU grace period after
> > xa_erase
> 
> The window is real, but it lives in ice_eswitch_detach(), which neither
> this patch nor the other two change:
> 
> 	ice_eswitch_stop_reprs(pf);
> 	repr->ops.rem(repr);                    /* unregister_netdev() */
> 	xa_erase(&pf->eswitch.reprs, repr->id);
> 	...
> 	ice_repr_destroy(repr);                 /* free_netdev() + kfree() */
> 
> That order is deliberate, and it was chosen for this class of problem:
> commit e9942bfe4931 ("ice: Fix use after free during unload with ports in
> bridge") moved unregister_netdev() ahead of xa_erase() so that a lookup
> racing with the teardown still finds the repr while it is being
> unregistered.  Moving that pair, or adding a grace period between the
> erase and the destroy, is a change to ice_eswitch_detach() with its own
> Fixes: history, not something a cfg_lock ordering fix should carry.
> 
> One correction on the mechanism.  ice_eswitch_get_target() is a plain
> xa_load() reached from ice_process_skb_fields() when ICE_RX_FLAGS_MULTIDEV
> is set on the ring, and a NAPI poll is not an RCU read-side section:
> __napi_poll() calls n->poll() directly, and neither net_rx_action() nor
> napi_threaded_poll() takes rcu_read_lock().  xa_load() also takes no
> reference on the object it returns, so an extra grace period after
> xa_erase() would not close this window as the code stands.  The lookup
> itself has to be made safe first, and that is a different patch.  I can
> send it if you want it.
> 
> What this series changes is when ice_eswitch_detach_vf() is called, and
> that does not widen the lookup: the NAPI Rx path never took vf->cfg_lock,
> so it raced with the detach both before and after.
> 
> > [Medium] net/ice: silent failure to apply VF MAC/VLAN to hardware due
> > to TOCTOU race
> 
> The sequence is right, including the outcome: both handlers throw away
> the return value of ice_reset_vf(), so once ICE_VF_DIS is up they still
> report success while the MAC or VLAN only exists in software.  But every
> part of it is baseline.  __ice_set_vf_mac() and ice_set_vf_port_vlan()
> call ice_check_vf_ready_for_cfg() outside cfg_lock, ice_reset_vf()'s
> ice_is_vf_disabled() test sits after the lock is taken, and ice_free_vfs()
> and ice_reset_all_vfs() raise ICE_VF_DIS before entering the loop; the
> patch does not move any of the three.  The same window is reachable today
> with "echo 0 > sriov_numvfs" in place of a PF reset.
> 
> Closing it means changing those two handlers, and probably how the reset
> result is reported, which is unrelated to where the detach and the attach
> are placed.  I would rather send that on its own than fold it into an
> -net series that fixes a lock inversion.
> 
> No code change in this series for either report.

Thanks for the detailed response.
This seems reasonable to me.

Reviewed-by: Simon Horman <horms@kernel.org>