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

All the comments below fall into pre-existing issues, concerns already
addressed in previous versions, out-of-scope items, or extreme edge
cases. No code changes are considered necessary for a new version.
Details for each comment:

[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?

Same as the equivalent comment on i40e_setup_vf_trust(). The kernel-doc
describes the primary design intent. The helper is trivial (set/clear one
bit) and safe to call in any context. Not a functional issue.

[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?

Same concern addressed in previous versions for both i40e and ice.
Over-limit filters configured while trusted remain after trust revocation,
but this is acceptable because untrusted VFs can freely delete their own
MAC and VLAN filters, there are no trust checks in the deletion path
(ice_vc_handle_mac_addr_msg() only checks trust when set == true). The VF
simply cannot add more over-limit filters.

ice_vc_handle_mac_addr_msg() decrements vf->num_mac on delete, so the
counter reflects actual state after deletions. The no-reset path is only
reached for VFs with no LLDP or promiscuous mode, having excess
MAC/VLAN filters in this state is extremely unlikely.

[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?

Pre-existing issue, not introduced by this patch. The pre-patch code had
the same loop with the same call. As the review notes, this is latent
rather than triggerable.

[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?

Pre-existing asymmetry, not introduced by this patch. The early-exit paths
in ice_reset_vf() (ICE_VF_RESETS_DISABLED, VF disabled) all leave
ICE_VF_STATE_ACTIVE cleared, so privileged virtchnl handlers reject
messages regardless. This is a state and reporting inconsistency rather
than retained privilege.

Duplicating the trust setup on both branches was explored during
development and introduced additional issues, it would require a broader
redesign of the reset path, which is beyond the scope of this fix.

[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?

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. The cache behavior is inherited from
the existing design and is beyond the scope of this fix.

[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?

Pre-existing reference leak, not introduced by this patch. This can be
a separate bug fix but not part of this series.