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.
---
Based on my verification, all three findings are confirmed. Let me now generate the report.

commit 848ee33529de0af7967f298f70c37bfb75ea5498
Author: Jose Ignacio Tornos Martinez <jtornosm@redhat.com>

ice: skip unnecessary VF reset when setting trust

This patch makes ice_set_vf_trust() skip the VF reset for both the
grant-trust path and for revocation when the VF has not configured
LLDP filters or promiscuous mode. When a reset is still needed, it
reorders the work so the reset runs before the LLDP filter teardown.

> diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.c b/drivers/net/ethernet/intel/ice/ice_sriov.c
> index 7e00e091756d..3c64ed1b41a8 100644
> --- 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);

[Critical]
Can the privilege capability bit get left set after revocation here?

The actual trust check used at runtime is ice_is_vf_trusted(), which
reads the bit, not vf->trusted:

  bool ice_is_vf_trusted(struct ice_vf *vf)
  {
          return test_bit(ICE_VIRTCHNL_VF_CAP_PRIVILEGE, &vf->vf_caps);
  }

The new code calls ice_reset_vf() while vf->trusted is still true (the
assignment is deferred to after the reset so ice_vf_is_lldp_ena() works
during cleanup). Inside the reset, the rebuild path runs:

  ice_reset_vf()
    -> ice_vf_post_vsi_rebuild()
       -> ice_vf_rebuild_host_cfg()
          -> ice_vf_set_host_trust_cfg()
             assign_bit(ICE_VIRTCHNL_VF_CAP_PRIVILEGE,
                        &vf->vf_caps, vf->trusted);

Because vf->trusted is still true at that moment, the bit gets
re-asserted to 1. After the reset returns, vf->trusted is set to false,
but ice_setup_vf_trust(vf, false) is only called in the else branch, so
the privilege bit is never cleared on this path.

After this returns, ice_is_vf_trusted() still returns true, which gates
ice_can_vf_change_mac(), ice_vc_handle_mac_addr_msg(),
ice_vc_cfg_promiscuous_mode_msg() and ice_vf_has_max_vlans(). The kernel
log says "VF X is now untrusted" but the trust-gated capabilities remain
available to the guest.

Would calling ice_setup_vf_trust(vf, false) (or assigning vf->trusted
before the reset, as the previous code did) be necessary to actually
clear the bit on this path?

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

[High]
Is this loop reachable after the reset above?

ice_reset_vf() goes through ice_vf_pre_vsi_rebuild() which calls
ice_vf_clear_counters():

  static void ice_vf_clear_counters(struct ice_vf *vf)
  {
          struct ice_vsi *vsi = ice_get_vf_vsi(vf);

          if (vsi)
                  vsi->num_vlan = 0;

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

By the time control returns to ice_set_vf_trust(), vf->num_mac_lldp is
already zero, so the while loop never iterates and
ice_vf_update_mac_lldp_num() -> ice_vsi_cfg_sw_lldp(vsi, false, false)
never runs.

ice_vf_reconfig_vsi() inside the reset does invoke ice_fltr_remove_all()
for per-VSI rules, but ice_vsi_cfg_sw_lldp() also drives the AQ-based
ice_lldp_fltr_add_remove() removal, gated by the PF-wide
ICE_FLAG_LLDP_AQ_FLTR flag, which is not torn down by ice_fltr_remove_all().
On firmware that uses the AQ fallback (set when the per-VSI rule add
fails), the AQ LLDP filter rule installed when trust was granted stays
in place, and the VF can keep receiving LLDP frames after revocation.

The commit message says:

    When we do reset (MAC LLDP case), we fix the race condition by
    resetting first to clear VF state (which blocks new MAC LLDP filter
    additions), then delete existing filters safely.

Does the deletion step actually run, given that the loop counter is
zeroed by the reset before the loop is reached? Would saving
num_mac_lldp before ice_reset_vf() and using that saved value (or
unconditionally calling ice_vsi_cfg_sw_lldp(vsi, false, false) when an
LLDP rule was present) be needed for the intended teardown to happen?

> +	} else {
> +		ice_setup_vf_trust(vf, trusted);
> +	}
>  	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");

[Medium]
Is the predicate that selects the reset path complete?

The condition only covers num_mac_lldp and the UC/MC promisc state bits.
While trusted, however, a VF can also:

  - override the host-set MAC and add unicast MACs beyond
    ICE_MAX_MACADDR_PER_VF (ice_can_vf_change_mac() and
    ice_vc_handle_mac_addr_msg() consult ice_is_vf_trusted())
  - add VLAN filters beyond ICE_MAX_VLAN_PER_VF
    (ice_vf_has_max_vlans() / ice_vc_get_max_vlan_fltrs() grant a higher
    cap when ice_is_vf_trusted() is true)

A VF that used these higher quotas but is not currently in promisc mode
and has no LLDP filter takes the no-reset path, and the over-quota MAC
and VLAN filters carry over past trust revocation. The previous
unconditional ice_reset_vf() rebuilt the VSI through
ice_vf_rebuild_host_mac_cfg(), which restored only the host-set MAC and
dropped VF-added extras.

The commit message says the reset is taken "if the VF has actually
configured advanced features that require cleanup". Should the
condition also account for VF-installed MAC and VLAN filters that
exceed the untrusted limits?