Re: [PATCH net v4 4/4] ice: skip unnecessary VF reset when setting trust

[Simon Horman <horms@kernel.org>]

From: 'Simon Horman' <horms@kernel.org>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
ice: skip unnecessary VF reset when setting trust

This patch attempts to optimize VF trust setting changes by skipping the VF
reset when granting trust, and only resetting when revoking trust if MAC LLDP
filters are present. It also fixes a race condition by doing the reset before
clearing the LLDP filters.

> diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.c b/drivers/net/ethernet/intel/ice/ice_sriov.c
> index 7e00e091756ddc..8686c382404f3f 100644
> --- a/drivers/net/ethernet/intel/ice/ice_sriov.c
> +++ b/drivers/net/ethernet/intel/ice/ice_sriov.c
[ ... ]
> @@ -1399,11 +1427,16 @@ int ice_set_vf_trust(struct net_device *netdev, int vf_id, bool trusted)
>  
>  	mutex_lock(&vf->cfg_lock);
>  
> -	while (!trusted && vf->num_mac_lldp)
> -		ice_vf_update_mac_lldp_num(vf, ice_get_vf_vsi(vf), false);
> -
> +	/* Reset only if revoking trust with MAC LLDP filters */
> +	if (!trusted && vf->num_mac_lldp) {
> +		ice_reset_vf(vf, ICE_VF_RESET_NOTIFY);

Does ice_reset_vf() clear vf->num_mac_lldp to 0 when it calls
ice_vf_pre_vsi_rebuild() internally?

If so, does this make the following while loop dead code, bypassing the
ice_vf_update_mac_lldp_num() calls that cleanly decrement the count and
trigger ice_vsi_cfg_sw_lldp() to clean up the LLDP software switch rules?

> +		while (vf->num_mac_lldp)
> +			ice_vf_update_mac_lldp_num(vf, ice_get_vf_vsi(vf), false);
> +	} else {
> +		ice_setup_vf_trust(vf, trusted);
> +	}

If we revoke trust (!trusted) and vf->num_mac_lldp > 0, we enter the if block
and skip the else block containing ice_setup_vf_trust().

When ice_reset_vf() rebuilds the VSI, vf->trusted is still true. Will the
rebuild process use this true value to set the ICE_VIRTCHNL_VF_CAP_PRIVILEGE
bit in vf->vf_caps?

Since ice_setup_vf_trust() is skipped, does this fail to explicitly clear the
privilege capability bit? Could a malicious VM add a MAC LLDP filter before
trust is revoked to ensure num_mac_lldp > 0, thereby retaining its privileges?

Furthermore, when revoking trust and vf->num_mac_lldp == 0, we take the else
branch and skip ice_reset_vf().

Trusted VFs are permitted to allocate more than ICE_MAX_MACADDR_PER_VF or
ICE_MAX_VLAN_PER_VF limits. Previously, the unconditional ice_reset_vf()
would tear down the VSI and purge these extra non-default filters.

Without the reset, is there any logic in ice_setup_vf_trust() to prune the
extra MAC and VLAN filters that were added while the VF was trusted? Will
skipping the reset allow an untrusted VF to retain excessive hardware filters
indefinitely and potentially exhaust PF resources?

>  	vf->trusted = trusted;
> -	ice_reset_vf(vf, ICE_VF_RESET_NOTIFY);
> +
>  	dev_info(ice_pf_to_dev(pf), "VF %u is now %strusted\n",
>  		 vf_id, trusted ? "" : "un");
>