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

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

The patch makes ice_set_vf_trust() skip the VF reset in most cases: granting
trust only sets the ICE_VIRTCHNL_VF_CAP_PRIVILEGE bit, and revoking trust
only resets when vf->num_mac_lldp is non-zero or one of the promiscuous
state bits is set.  A new helper, ice_setup_vf_trust(), toggles the
capability bit for the no-reset path.

A few questions about the new revocation logic below.

> diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.c b/drivers/net/ethernet/intel/ice/ice_sriov.c
> index e04de0215596..558937899818 100644
> --- a/drivers/net/ethernet/intel/ice/ice_sriov.c
> +++ b/drivers/net/ethernet/intel/ice/ice_sriov.c
> @@ -1366,6 +1366,23 @@ int ice_set_vf_mac(struct net_device *netdev, int vf_id, u8 *mac)
>  	return __ice_set_vf_mac(ice_netdev_to_pf(netdev), vf_id, mac);
>  }
>  
> +/**
> + * ice_setup_vf_trust - Enable/disable VF trust mode without reset
> + * @vf: VF to configure
> + * @setting: trust setting
> + *
> + * Update VF flags when changing trust without performing a VF reset.
> + * This is only called when it's safe to skip the reset (VF has no advanced
> + * features configured that need cleanup).
> + */
> +static void ice_setup_vf_trust(struct ice_vf *vf, bool setting)
> +{
> +	if (setting)
> +		set_bit(ICE_VIRTCHNL_VF_CAP_PRIVILEGE, &vf->vf_caps);
> +	else
> +		clear_bit(ICE_VIRTCHNL_VF_CAP_PRIVILEGE, &vf->vf_caps);
> +}

[Severity: Low]
This isn't a bug, but doesn't ice_vf_lib.c already have a helper with
exactly these semantics?

drivers/net/ethernet/intel/ice/ice_vf_lib.c:
static void ice_vf_set_host_trust_cfg(struct ice_vf *vf)
{
	assign_bit(ICE_VIRTCHNL_VF_CAP_PRIVILEGE, &vf->vf_caps, vf->trusted);
}

It is called from ice_vf_rebuild_host_cfg() on the reset path.  Would it be
preferable to export that one (ice_vf_lib_private.h already declares
ice_is_vf_trusted()) rather than carry a second copy of the rule that
derives ICE_VIRTCHNL_VF_CAP_PRIVILEGE from vf->trusted in a different file?

>  /**
>   * ice_set_vf_trust
>   * @netdev: network interface device structure
> @@ -1401,11 +1418,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);
> -
> -	vf->trusted = trusted;
> -	ice_reset_vf(vf, ICE_VF_RESET_NOTIFY);
> +	/* 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))) {

[Severity: High]
Is this predicate complete with respect to everything trust gates in ice?
Besides LLDP filters and promiscuous mode, vf->trusted also gates the MAC
and VLAN filter quotas and the administratively assigned MAC:

drivers/net/ethernet/intel/ice/virt/virtchnl.c:ice_can_vf_change_mac() {
	if (vf->pf_set_mac && !ice_is_vf_trusted(vf))
		return false;
}

drivers/net/ethernet/intel/ice/virt/virtchnl.c:ice_vc_handle_mac_addr_msg() {
	if (set && !ice_is_vf_trusted(vf) &&
	    (vf->num_mac + al->num_elements) > ICE_MAX_MACADDR_PER_VF) {
}

drivers/net/ethernet/intel/ice/virt/virtchnl.c:ice_vf_has_max_vlans() {
	if (ice_is_vf_trusted(vf))
		return false;
}

So a guest that was trusted can have installed extra unicast/multicast MAC
filters, a MAC that overrides the pf_set_mac address, and more than
ICE_MAX_VLAN_PER_VF VLANs.  None of that shows up in num_mac_lldp or the
promisc bits, so with those clear, "ip link set <pf> vf N trust off" takes
the new else branch.

Do those hardware filters then stay programmed after the log prints "VF N is
now untrusted"?  Previously the unconditional reset reached:

drivers/net/ethernet/intel/ice/ice_vf_lib.c:ice_vf_reconfig_vsi() {
	ice_vsi_decfg(vsi);
	ice_fltr_remove_all(vsi);
}

followed by ice_vf_rebuild_host_cfg(), which re-adds only broadcast plus the
host-sanctioned MAC/VLAN config.

The software counters look affected too.  They are cleared only on the reset
path:

drivers/net/ethernet/intel/ice/ice_vf_lib.c:ice_vf_clear_counters() {
	if (vsi)
		vsi->num_vlan = 0;

	vf->num_mac = 0;
	vf->num_mac_lldp = 0;
}

Can vf->num_mac therefore remain above ICE_MAX_MACADDR_PER_VF after trust is
revoked, so that later legitimate MAC adds from the now-untrusted VF are
rejected until some unrelated reset happens?

Relatedly, the kernel-doc on ice_setup_vf_trust() says it "is only called
when it's safe to skip the reset (VF has no advanced features configured
that need cleanup)", and the commit message says the features needing cleanup
are "(MAC LLDP filters, promiscuous mode)".  Should both mention the MAC and
VLAN filter state as well?

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

[Severity: High]
What happens to the negotiated VLAN V2 capabilities when trust changes
without a reset?  The advertised limit is derived from vf->trusted once, at
negotiation time, and then cached on the PF:

drivers/net/ethernet/intel/ice/virt/virtchnl.c:ice_vc_get_max_vlan_fltrs() {
	if (vf->trusted)
		return VLAN_N_VID;
	else
		return ICE_MAX_VLAN_PER_VF;
}

drivers/net/ethernet/intel/ice/virt/virtchnl.c:ice_vc_get_offload_vlan_v2_caps() {
	/* store negotiated caps to prevent invalid VF messages */
	memcpy(&vf->vlan_v2_caps, caps, sizeof(*caps));
}

VLAN V2 adds are validated against that snapshot only:

drivers/net/ethernet/intel/ice/virt/virtchnl.c:ice_vc_validate_add_vlan_filter_list() {
	if (num_requested_filters > vfc->max_filters)
		return false;
}

and the snapshot is invalidated only on the reset path:

drivers/net/ethernet/intel/ice/ice_vf_lib.c:ice_vf_set_initialized() {
	memset(&vf->vlan_v2_caps, 0, sizeof(vf->vlan_v2_caps));
}

If the VF negotiated VIRTCHNL_VF_OFFLOAD_VLAN_V2 while trusted, does the
else branch leave max_filters at VLAN_N_VID, allowing the now-untrusted VF
to keep programming VLAN filters well beyond ICE_MAX_VLAN_PER_VF?

In the other direction, after granting trust without a reset both the PF
cache and the guest cache still hold 8:

drivers/net/ethernet/intel/iavf/iavf_main.c:iavf_get_max_vlans_allowed() {
	else if (VLAN_V2_ALLOWED(adapter))
		return adapter->vlan_v2_caps.filtering.max_filters;
}

so is the commit message statement "When granting trust, no reset is needed
- we can just set the capability flag to allow privileged operations"
accurate for VLAN filtering, where the privilege only becomes usable after
renegotiation?

[Severity: Medium]
This isn't a bug introduced by this patch, but while ice_set_vf_trust() is
being touched: the switchdev check earlier in this same function returns
without releasing the VF reference taken by ice_get_vf_by_id():

	if (ice_is_eswitch_mode_switchdev(pf)) {
		dev_info(ice_pf_to_dev(pf), "Trusted VF is forbidden in switchdev mode\n");
		return -EOPNOTSUPP;
	}

Every other exit uses "goto out_put_vf;" so that ice_put_vf() drops the
kref_get_unless_zero() reference.  Since the leaked reference keeps the
refcount above zero, ice_release_vf() never runs, so neither
pci_dev_put(vf->vfdev) nor the kfree_rcu() of the ice_vf ever happens when
SR-IOV is disabled or the driver is unbound.  Would it make sense to convert
that return into "goto out_put_vf;" here?