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

[Jose Ignacio Tornos Martinez <jtornosm@redhat.com>]

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.