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

All the comments below fall into pre-existing issues, concerns already
addressed in previous versions and comments, out-of-scope items, or extreme
edge cases. This patch mirrors the i40e fix (patch 1/3), improving the
overall behavior by eliminating unnecessary VF resets that could add delay
during bonding setup. The no-reset path is narrowly scoped to VFs with no
LLDP filters and no promiscuous mode. No code changes are considered
necessary for a new version. Details for each comment:

[Severity: Medium]
This isn't a bug introduced by this patch, but the switchdev check
earlier in ice_set_vf_trust() returns without dropping the reference
that ice_get_vf_by_id() took:

    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;
    }

Does each ndo_set_vf_trust call in switchdev mode leak one VF kref? If
so, ice_release_vf() never runs, and the struct ice_vf and its pci_dev
reference are never released.

The notes on this version already call this a pre-existing leak. Setting
ret = -EOPNOTSUPP and jumping to out_put_vf would fix it.

Pre-existing reference leak, not introduced by this patch.

[Severity: Medium]
Is this predicate enough to decide that a VF is in a clean state? It
does not cover unicast MAC filters that the VF could only add because it
was trusted.

While a VF is trusted, ice_can_vf_change_mac() lets it add unicast
filters even when its MAC was set by the admin:

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

ice_vc_handle_mac_addr_msg() also skips the per-VF quota for trusted
VFs:

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

Before this patch, revoking trust always ran
ice_reset_vf()->ice_vf_reconfig_vsi()->ice_fltr_remove_all(). The
rebuild then re-added only the broadcast and hw_lan_addr filters.

Now the else branch is taken for a VF with num_mac_lldp == 0 and no
promisc bits. That VF keeps all its extra unicast filters in hardware,
and vf->num_mac can stay above ICE_MAX_MACADDR_PER_VF.

Would a pf_set_mac VF keep receiving traffic for MACs that an untrusted
VF may not add, even though the log says "VF N is now untrusted"?

The notes on this version point out that untrusted VFs can delete their
own filters. A guest that does not delete them keeps them until some
unrelated reset.

Same concern addressed in previous versions and comments for both i40e
and ice. The no-reset path is only reached for VFs with no LLDP filters
and no promiscuous mode, a basic configuration where having excess MAC
filters beyond untrusted limits is extremely unlikely.

Even in this rare scenario, the consequence is minor: the VF cannot add
more filters until it deletes some. No crash, no data corruption, no
security breach. Existing filters keep working. This does not rely on
guest cooperation, the PF enforces the limit at the virtchnl level
(ice_vc_handle_mac_addr_msg() checks the quota on every add request).
The VF simply cannot add more filters beyond the untrusted limit,
regardless of its behavior.

[Severity: Medium]
Can ICE_VF_STATE_UC_PROMISC and ICE_VF_STATE_MC_PROMISC both be clear
while promiscuous-related hardware state is still programmed?

ice_vc_cfg_promiscuous_mode_msg() disables VLAN pruning first:

    else
        ret = vlan_ops->dis_rx_filtering(vsi);

In the path without true promisc, the multicast promisc rule can be
installed, and then an ice_set_dflt_vsi() failure exits before either
bit is set:

        if (allmulti)
            mcast_err = ice_vf_set_vsi_promisc(vf, vsi, mcast_m);
        ...
        if (ret) {
            ...
            goto error_param;
        }

In the true-promisc path, if ucast_err and mcast_err are both non-zero,
neither bit is set. VLAN pruning is already disabled at that point, and
some per-VLAN rules may be installed.

Before this patch, the unconditional ice_reset_vf() rebuilt the VSI
through ice_vsi_decfg() and ice_fltr_remove_all(), whatever the bits
said. Now the else branch is taken. The VF cannot undo this state
itself, because ice_vc_cfg_promiscuous_mode_msg() rejects untrusted VFs
before it reaches ena_rx_filtering().

Would the untrusted VF keep VLAN pruning disabled, and possibly an
allmulti or VLAN promisc rule? Getting into this state needs an admin
queue or firmware failure while the VF was trusted.

This scenario requires an admin queue or firmware failure during
promiscuous mode setup while the VF was trusted, leaving hardware state
programmed without the corresponding software bits set. Then trust must
be revoked before any reset cleans up the state. This is an extremely
unlikely chain of events requiring multiple failures. The VF is in a
partially broken state at that point regardless of whether trust changes.

[Severity: High]
What happens to the negotiated VLAN_V2 filter limit when this branch
runs? The limit comes from vf->trusted, but it is only computed when the
VF negotiates VIRTCHNL_OP_GET_OFFLOAD_VLAN_V2_CAPS. In
ice_vc_get_max_vlan_fltrs():

    if (vf->trusted)
        return VLAN_N_VID;
    else
        return ICE_MAX_VLAN_PER_VF;

The result is then cached in ice_vc_get_offload_vlan_v2_caps():

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

Only ice_vf_set_initialized() clears this cache, and it runs on the
reset path that this branch now skips. ice_vc_add_vlan_v2_msg() checks
new filters against the cached value in
ice_vc_validate_add_vlan_filter_list():

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

Granting trust always takes this branch. Does a VLAN_V2 VF then stay
limited to ICE_MAX_VLAN_PER_VF on the PF side?

iavf enforces its own cached copy through iavf_get_max_vlans_allowed(),
so the VF will not try to add more VLANs until it renegotiates. That
seems to contradict this line in the commit message: "we can just set
the capability flag to allow privileged operations".

Revoking trust from a clean VF also takes this branch. Does the
untrusted VF keep the VLAN_N_VID quota? The legacy path in
ice_vf_has_max_vlans() checks ice_is_vf_trusted() at the time of each
request, so the V1 and V2 paths would disagree for the same VF.

The notes on this version describe the cache invalidation as part of
the existing reset/rebuild design. Before this patch, though, every
trust change went through ice_reset_vf(), so the caps were always
renegotiated. The new branch is what lets the cache go stale.

iavf caches max_filters too. Would a VLAN_V2 VF need to renegotiate its
caps (through a reset or something equivalent) whenever its trust
setting changes?

The VLAN_V2 caps cache invalidation is part of the reset/rebuild path
by design (ice_vf_set_initialized()). This patch does not change that
architecture, it only adds a conditional to skip the reset when no
advanced features are configured.

On the grant path: the VF stays limited to ICE_MAX_VLAN_PER_VF until
the next reset or renegotiation. This is conservative behavior, the VF
is more restricted than expected, not less. No security or stability
issue. The iavf driver also caches its own limit and will not try to
add more VLANs, so both sides are consistent.

On the revoke path from a clean VF: the stale VLAN_N_VID cache would
allow more VLANs on the PF side, but the iavf driver enforces its own
cached limit and will not attempt to exceed ICE_MAX_VLAN_PER_VF. The
V1 path checks trust at request time, which is correct.