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

[sashiko-bot@kernel.org]

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.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008125754.3520773-1-xiaolinkui@126.com?part=1