Re: [PATCH net 6/8] ice: skip unnecessary VF reset when setting trust

[Jakub Kicinski <kuba@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.
---
ice: skip unnecessary VF reset when setting trust

Similar to the i40e fix, ice_set_vf_trust() unconditionally calls
ice_reset_vf() when the trust setting changes. While the delay is smaller
than i40e this reset is still unnecessary in most cases.

> diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.c b/drivers/net/ethernet/intel/ice/ice_sriov.c
> --- a/drivers/net/ethernet/intel/ice/ice_sriov.c
> +++ b/drivers/net/ethernet/intel/ice/ice_sriov.c

[ ... ]

> @@ -1399,11 +1416,19 @@ 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 and VF has advanced features configured */
> +	if (!trusted &&
> +	    (vf->num_mac_lldp > 0 ||
> +	     test_bit(ICE_VF_STATE_UC_PROMISC, vf->vf_states) ||
> +	     test_bit(ICE_VF_STATE_MC_PROMISC, vf->vf_states))) {
> +		ice_reset_vf(vf, ICE_VF_RESET_NOTIFY);

[Severity: High]
When revoking trust, ice_reset_vf() is called before updating vf->trusted
to false.

During ice_reset_vf(), the VSI is rebuilt and synchronizes the
ICE_VIRTCHNL_VF_CAP_PRIVILEGE bit in vf->vf_caps with the current value of
vf->trusted, which is still true. The else block below is bypassed, and
although vf->trusted is later set to false, vf_caps will retain the
privileged capability bit.

Because ice_is_vf_trusted() strictly checks vf_caps, won't the VF
incorrectly retain privileged capabilities indefinitely after this operation?

> +		while (vf->num_mac_lldp)
> +			ice_vf_update_mac_lldp_num(vf, ice_get_vf_vsi(vf), false);

[Severity: High]
Is the MAC LLDP filter cleanup loop dead code?

ice_reset_vf() internally calls ice_vf_pre_vsi_rebuild(), which
unconditionally clears vf->num_mac_lldp to 0 via ice_vf_clear_counters().
As a result, the while condition evaluates to false immediately upon
returning from the reset, completely bypassing the intended cleanup logic.

Furthermore, because vf->num_mac_lldp is abruptly zeroed without going
through ice_vf_update_mac_lldp_num(), does the driver skip calling
ice_vsi_cfg_sw_lldp(), silently leaking the software LLDP switch filters
from its bookkeeping?

> +	} else {
> +		ice_setup_vf_trust(vf, trusted);
> +	}

[Severity: High]
If we skip the VF reset when granting trust, what happens to expected VM
configuration workflows for promiscuous mode?

The PF driver intentionally lies to untrusted VFs that attempt to enable
promiscuous mode: it returns success to the VF without actually programming
the hardware. Historically, granting trust forced a full VF reset. This reset
prompted the VF driver to re-initialize and replay its configuration (including
promiscuous mode) to the PF, which would then correctly program the hardware
because the VF was now trusted.

By skipping the reset when granting trust, it appears a VF that previously
requested promiscuous mode will remain permanently desynchronized. The guest
OS believes it is in promiscuous mode, but the hardware is not programmed to
reflect this, which could result in dropped traffic.

[Severity: High]
When skipping the VF reset on trust revocation (because the VF lacks MAC
LLDP or promiscuous mode), are we allowing the VF to persistently retain
resources allocated while it was trusted?

The 'trusted' capability grants a VF the ability to bypass administrative
limits, such as ICE_MAX_MACADDR_PER_VF (which restricts standard VFs to 18
MAC addresses) and the maximum VLAN limit checked in ice_vf_has_max_vlans().

Prior to this patch, revoking trust unconditionally triggered a VF reset,
which rebuilt the VSI and completely wiped all dynamically allocated hardware
filters, effectively enforcing the demoted limits.

Without the reset, might a VF that allocated an excessive number of MAC
addresses or VLANs while trusted retain them in hardware indefinitely after
trust is revoked, creating a persistent resource limit bypass?

>  	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");