Re: [PATCH net 04/10] 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

This changes ice_set_vf_trust() so that granting trust never resets the VF,
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(),
flips ICE_VIRTCHNL_VF_CAP_PRIVILEGE directly on the no-reset path.

> diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.c b/drivers/net/ethernet/intel/ice/ice_sriov.c
> index e04de02155966..5589378998181 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).
> + */

[Severity: Low]
This isn't a bug, but does the kernel-doc match the only call site?  The
comment says the helper "is only called when it's safe to skip the reset (VF
has no advanced features configured that need cleanup)", yet
ice_set_vf_trust() takes the else branch for every trust grant without
looking at vf->num_mac_lldp, the promiscuous bits, vf->num_mac or the VLAN
count at all, and on revoke it only consults the LLDP counter and the two
promiscuous bits.  Would it be clearer to describe the condition the caller
actually enforces?

> +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);
> +}
> +
>  /**
>   * 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]
Should this predicate also cover the MAC and VLAN filter quotas that trust
lifts?  Trust does more than gate LLDP filters and promiscuous mode.

The unicast/multicast MAC budget is only enforced for untrusted VFs, in
ice_vc_handle_mac_addr_msg() (virt/virtchnl.c):

	if (set && !ice_is_vf_trusted(vf) &&
	    (vf->num_mac + al->num_elements) > ICE_MAX_MACADDR_PER_VF) {

The VLAN budget works the same way, in ice_vf_has_max_vlans():

	static bool ice_vf_has_max_vlans(struct ice_vf *vf, struct ice_vsi *vsi)
	{
		if (ice_is_vf_trusted(vf))
			return false;

and a trusted VF may override an administratively set MAC, in
ice_can_vf_change_mac():

	if (vf->pf_set_mac && !ice_is_vf_trusted(vf))
		return false;

All three checks run only at add time, so already-programmed filters are
never re-audited.  Previously the reset was what removed them, via
ice_reset_vf() -> ice_vf_reconfig_vsi() -> ice_fltr_remove_all(), and
ice_vf_pre_vsi_rebuild() -> ice_vf_clear_counters() is what reset the
accounting:

	if (vsi)
		vsi->num_vlan = 0;

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

So for a VF that added over-quota MAC or VLAN filters while trusted, but
never enabled promiscuous mode and never added an LLDP MAC, does "ip link
set <pf> vf N trust off" now report success while the hardware switch
filters stay programmed?  Since the ice anti-spoof behaviour is driven by
that filter list, can the now-untrusted guest keep receiving and
transmitting with addresses and VLANs it was only permitted to program while
trusted?

There is a second effect of skipping ice_vf_clear_counters(): vf->num_mac
and vsi->num_vlan stay at their trusted-era values, which are above the
untrusted limits.  Does that make every subsequent filter add from the
untrusted VF fail against stale accounting until some unrelated VF reset
happens?

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

[Severity: Medium]
This isn't a bug introduced by this patch, since the pre-patch loop had the
same shape, but the unchecked ice_get_vf_vsi(vf) result is handed to a callee
whose NULL check comes after a dereference, in ice_vf_update_mac_lldp_num()
(ice_vf_lib.c):

	bool lldp_by_fw = test_bit(ICE_FLAG_FW_LLDP_AGENT, vsi->back->flags);
	bool was_ena = ice_vf_is_lldp_ena(vf) && !lldp_by_fw;
	bool is_ena;

	if (WARN_ON(!vsi)) {
		vf->num_mac_lldp = 0;
		return;
	}

vsi->back is read in the initializer, before WARN_ON(!vsi), so both the
warning and the vf->num_mac_lldp = 0 fallback that terminates this loop are
unreachable for a NULL VSI.

I could not find a reachable state where vf->num_mac_lldp is non-zero with a
NULL VF VSI (ice_free_vf_res() and the reset paths all zero the counter),
so this looks latent rather than triggerable.  Would it be worth computing
lldp_by_fw after the WARN_ON(!vsi) check so the guard actually guards?

> +		vf->trusted = trusted;
> +		ice_reset_vf(vf, ICE_VF_RESET_NOTIFY);

[Severity: Medium]
This is a pre-existing asymmetry rather than something the patch introduces,
but now that ice_setup_vf_trust() exists, should it be called on this branch
too?

On the reset branch, vf->trusted is written but ICE_VIRTCHNL_VF_CAP_PRIVILEGE
in vf->vf_caps, which is what ice_is_vf_trusted() reads, is only re-derived
as a side effect of the rebuild:

ice_reset_vf() -> ice_vf_post_vsi_rebuild() -> ice_vf_rebuild_host_cfg() ->
ice_vf_set_host_trust_cfg()

	static void ice_vf_set_host_trust_cfg(struct ice_vf *vf)
	{
		assign_bit(ICE_VIRTCHNL_VF_CAP_PRIVILEGE, &vf->vf_caps, vf->trusted);
	}

ice_reset_vf() has several exits that never reach the rebuild, for example:

	if (test_bit(ICE_VF_RESETS_DISABLED, pf->state)) {
		dev_dbg(dev, "Trying to reset VF %d, but all VF resets are disabled\n",
			vf->vf_id);
		return 0;
	}

plus the ice_is_vf_disabled() path, the two WARN_ON(!vsi) exits and
ice_vf_reconfig_vsi() failure.  The return value is discarded here and the
dev_info() below prints "VF %u is now %strusted" regardless.

On those paths, do vf->trusted and the privilege bit diverge while the
operation still reports success?  ICE_VF_RESETS_DISABLED is only set in
ice_remove(), and the other exits leave ICE_VF_STATE_ACTIVE cleared, which
the privileged virtchnl handlers check first, so this looks like a state and
reporting inconsistency rather than retained privilege.  Would calling
ice_setup_vf_trust(vf, trusted) on both branches and propagating
ice_reset_vf()'s error be worthwhile?

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

[Severity: Medium]
What happens to the already negotiated VLAN_V2 capabilities when trust
changes without a reset?  The advertised VLAN filter budget is derived from
vf->trusted once, in ice_vc_get_max_vlan_fltrs() (virt/virtchnl.c):

	static u16 ice_vc_get_max_vlan_fltrs(struct ice_vf *vf)
	{
		if (vf->trusted)
			return VLAN_N_VID;
		else
			return ICE_MAX_VLAN_PER_VF;
	}

and cached by ice_vc_get_offload_vlan_v2_caps():

	/* store negotiated caps to prevent invalid VF messages */
	memcpy(&vf->vlan_v2_caps, caps, sizeof(*caps));

VIRTCHNL_OP_ADD_VLAN_V2 is then validated only against that cached copy, in
ice_vc_validate_add_vlan_filter_list():

	if (num_requested_filters > vfc->max_filters)
		return false;

with no ice_is_vf_trusted() re-check.  The cache is invalidated only by
ice_vf_set_initialized():

	memset(&vf->vlan_v2_caps, 0, sizeof(vf->vlan_v2_caps));

which is reached from ice_vf_post_vsi_rebuild(), i.e. only on the reset path
this branch skips.  The iavf side caches and enforces the advertised limit
itself as well, in iavf_get_max_vlans_allowed() and iavf_vlan_rx_add_vid().

So for a VLAN_V2 capable VF, is granting trust here a no-op for the VLAN
path, leaving the VF and PF both at ICE_MAX_VLAN_PER_VF?  And in the other
direction, if the VF negotiated caps while already trusted and trust is
revoked through this branch, does it keep the VLAN_N_VID budget while
untrusted?

>  	dev_info(ice_pf_to_dev(pf), "VF %u is now %strusted\n",
>  		 vf_id, trusted ? "" : "un");
>  

[Severity: Medium]
This is a pre-existing issue and not introduced by this patch, but it is in
the function being modified.  Earlier in ice_set_vf_trust(), the switchdev
check returns without releasing the reference taken by ice_get_vf_by_id():

	vf = ice_get_vf_by_id(pf, vf_id);
	if (!vf)
		return -EINVAL;

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

ice_get_vf_by_id() documents that "This function takes a reference to the VF,
which must be released by calling ice_put_vf()", and the out_put_vf label at
the end of the function is what normally does that.  Does each "ip link set
<pf> vf N trust on|off" issued while the PF is in switchdev mode leak a VF
reference, so ice_sriov_free_vf() never runs for that struct ice_vf?  Would
"ret = -EOPNOTSUPP; goto out_put_vf;" be the right fix while here?