Re: [PATCH net 02/10] i40e: 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 is the stated precondition accurate? The only call
> site is the else branch of a condition that begins with !setting, so the
> helper runs for every trust grant no matter what the VF has configured.
>
> An untrusted VF can already have "advanced features" configured:
> i40e_vc_add_qch_msg() sets vf->adq_enabled = true after checking spoofchk,
> negotiated ADQ capability, TC count and queue availability, with no
> trust/privilege test, and i40e_validate_cloud_filter() only requires
> I40E_VIRTCHNL_VF_CAP_PRIVILEGE for advanced-mode filters.
>
> Would it be clearer to document the real contract, i.e. "called whenever
> the reset is skipped: on every grant, and on revokes where no cleanup is
> deemed necessary"?

The kernel-doc describes the primary design intent. The helper is trivial
(set/clear one bit) and safe to call in any context, the comment guides
the reader to understand why it exists, not to enumerate every call site.
Not a functional issue.

> [Severity: Medium]
> Can this write of the privilege bit be clobbered by a concurrent VF reset?
>
> i40e_alloc_vf_res() recomputes the same bit from a plain read of
> vf->trusted, with no lock and no READ_ONCE():
>
>     if (vf->trusted)
>         set_bit(I40E_VIRTCHNL_VF_CAP_PRIVILEGE, &vf->vf_caps);
>     else
>         clear_bit(I40E_VIRTCHNL_VF_CAP_PRIVILEGE, &vf->vf_caps);
>
> Its only caller is i40e_cleanup_reset_vf(), reached from i40e_reset_vf()
> in the service task for guest-initiated resets (VIRTCHNL_OP_RESET_VF and
> VFLR). i40e_vc_process_vf_msg() is dispatched from
> i40e_clean_adminq_subtask() without rtnl_lock() and without
> __I40E_VIRTCHNL_OP_PENDING, so it runs in parallel with the ndo handler:
>
> service task                ndo path (rtnl held)
> i40e_alloc_vf_res()
>   reads old vf->trusted
>                     vf->trusted = setting;
>                     i40e_setup_vf_trust(vf, setting);
>   set_bit()/clear_bit() using
>   the stale value
>
> vf->trusted and I40E_VIRTCHNL_VF_CAP_PRIVILEGE then stay inconsistent
> until some later reset. Before this patch i40e_ndo_set_vf_trust() always
> called i40e_vc_reset_vf(), which retries i40e_reset_vf() (serialized by
> test_and_set_bit(I40E_VF_STATE_RESETTING)) until a reset happened in the
> caller's own thread, so the bit was always recomputed last from the
> up-to-date vf->trusted. Does the new path need a barrier or to hold off
> against I40E_VF_STATE_RESETTING?

Pre-existing race. The original code writes vf->trusted before calling
i40e_vc_reset_vf() without synchronization against VFLR-initiated resets
from the service task. This patch does not change the synchronization
model and the race window is the same order of magnitude (a few
instructions). The scenario requires a guest-initiated VFLR at the exact
moment of the trust change, extremely difficult to reproduce in practice.

> [Severity: High]
> Does revoking trust still remove the MAC and VLAN filters that only a
> trusted VF was allowed to install?
>
> The new condition does not consider filter state, so a VF with no ADQ, no
> cloud filters and no promiscuous bits takes the else branch, which only
> clears the privilege bit.
>
> While trusted, i40e_check_vf_permission() lets the VF install MACs that
> override the administratively set vf->pf_set_mac address and raises the
> limit from I40E_VC_MAX_MAC_ADDR_PER_VF to
> I40E_VC_MAX_MACVLAN_PER_TRUSTED_VF():
>
>     if (!vf_trusted && !is_multicast_ether_addr(addr) &&
>         vf->pf_set_mac && !ether_addr_equal(addr, vf->default_lan_addr.addr)) {
>         ... return -EPERM;
>     }
>     ...
>     if (!vf_trusted)
>         mac_add_max = I40E_VC_MAX_MAC_ADDR_PER_VF;
>     else
>         mac_add_max = I40E_VC_MAX_MACVLAN_PER_TRUSTED_VF(...);
>
> i40e_vc_add_vlan_msg() likewise only enforces I40E_VC_MAX_VLAN_PER_VF on
> add when the privilege bit is clear.
>
> Previously the unconditional i40e_vc_reset_vf() reached
> i40e_cleanup_reset_vf() -> i40e_free_vf_res(), which does
> i40e_vsi_release(pf->vsi[vf->lan_vsi_idx]), destroying the VSI and all its
> filters; i40e_alloc_vsi_res() then re-added only vf->default_lan_addr and
> broadcast, and i40e_cleanup_reset_vf() set vf->num_vlan = 0.
>
> Can the __I40E_MACVLAN_SYNC_PENDING work repair this instead? Looking at
> i40e_correct_vf_mac_vlan_filters() it only recomputes the VLAN id of
> existing entries:
>
>     new_vlan = i40e_get_vf_new_vlan(vsi, NULL, f, vlan_filters, trusted);
>     if (new_vlan != f->vlan) { ... }
>
> so no MAC filter is deleted on trust loss.
>
> There is also a functional side effect: mac_add_max drops back to 18 while
> i40e_count_active_filters(vsi) still reflects the trusted-era filters, so
> every later VIRTCHNL_OP_ADD_ETH_ADDR from that VF fails with -EPERM,
> including a re-add of its primary MAC after a guest link down/up.
>
> The commit message says "For VFs in a clean state, we can safely change
> the trust setting without the disruptive reset" and lists only "ADQ/cloud
> filters, promiscuous mode" as state requiring cleanup. Should the MAC and
> VLAN filter state be part of that list, or at least mentioned as a
> behaviour change?

Same concern addressed in previous versions. 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 i40e_vc_del_mac_addr_msg() or i40e_vc_remove_vlan_msg().
The VF simply cannot add more over-limit filters.

The permission check uses i40e_count_active_filters(vsi) which counts live
filters, deletions reduce the count immediately. The "primary MAC re-add
after link down/up" scenario does not apply: existing filters remain in
place across link events. A guest-initiated VF reset cleans up everything
via i40e_free_vf_res().

> [Severity: High]
> Is there a check-then-act window here against the virtchnl handlers?
>
> vf->adq_enabled, vf->num_cloud_filters and the promisc state bits are
> written from the service task via i40e_clean_adminq_subtask() ->
> i40e_vc_process_vf_msg(), which takes neither rtnl_lock() nor
> __I40E_VIRTCHNL_OP_PENDING (all test_and_set_bit() sites for that bit are
> ndo/sriov_configure entry points). The privilege bit is only cleared after
> the sampling:
>
> CPU0 i40e_ndo_set_vf_trust()
>     vf->trusted = false;
>     test_bit(I40E_VF_STATE_UC_PROMISC) == 0
>     test_bit(I40E_VF_STATE_MC_PROMISC) == 0
>
> CPU1 i40e_vc_config_promiscuous_mode_msg()
>     /* privilege bit still set */
>     i40e_config_vf_promiscuous_mode(vf, info->vsi_id, allmulti, alluni);
>     ... i40e_aq_set_vsi_unicast_promiscuous()/multicast in HW
>     test_and_set_bit(I40E_VF_STATE_UC_PROMISC, &vf->vf_states)
>
> CPU0
>     i40e_setup_vf_trust(vf, false);    /* clears privilege, no reset */
>
> Does anything clear the hardware promiscuous flags afterwards? A later VF
> request to disable promiscuous mode is rejected once the privilege bit is
> clear, and the PF reports success to the VF:
>
>     if (!test_bit(I40E_VIRTCHNL_VF_CAP_PRIVILEGE, &vf->vf_caps)) {
>         ... aq_ret = 0; goto err_out;
>     }
>
> and i40e_sync_vsi_filters() refuses to touch promiscuous for an untrusted
> SRIOV VSI. The same window applies to the plain writes vf->adq_enabled =
> true in i40e_vc_add_qch_msg() and vf->num_cloud_filters++ in
> i40e_vc_add_cloud_filter(), which are read here without READ_ONCE() or a
> lock. Since the guest chooses when to send those messages, it can toggle
> promisc off and on to make the sampled bits read zero.

Same concern addressed in previous versions. This race condition exists in
the original code as well, vf->trusted is set before i40e_vc_reset_vf(),
creating the same window where the VF can install privileged state while
the capability bit is still set. This patch does not introduce this race,
it inherits the same synchronization model. Extremely difficult to
reproduce in practice.

Fixing this requires changing the broader synchronization between ndo
callbacks and virtchnl processing, which is beyond the scope of this patch.

> [Severity: Medium]
> Can the software promisc bits under-report the hardware state?
>
> In i40e_vc_config_promiscuous_mode_msg() the state bits are only set after
> a fully successful call:
>
>     aq_ret = i40e_config_vf_promiscuous_mode(vf, info->vsi_id, allmulti, alluni);
>     if (aq_ret)
>         goto err_out;
>
> but i40e_set_vsi_promisc() issues several admin queue commands and can
> succeed on some and fail on others:
>
>     aq_ret = i40e_aq_set_vsi_multicast_promiscuous(hw, seid, multi_enable, NULL);
>     if (aq_ret) { ... return aq_ret; }
>     aq_ret = i40e_aq_set_vsi_unicast_promiscuous(hw, seid, unicast_enable, NULL, true);
>     if (aq_ret) { ... }
>     return aq_ret;
>
> If multicast promiscuous is enabled in firmware and the unicast command
> fails (or only some VLANs are programmed in the per-VLAN loop), no state
> bit is recorded while the VSI is promiscuous in hardware. Trust revocation
> then takes the no-reset branch and nothing clears it:
> i40e_sync_vsi_filters() does
>
>     if (vsi->type == I40E_VSI_SRIOV && pf->vf &&
>         !pf->vf[vsi->vf_id].trusted) {
>         clear_bit(__I40E_VSI_OVERFLOW_PROMISC, vsi->state);
>         goto out;
>     }
>
> without issuing any promiscuous AQ command. Before this patch
> i40e_cleanup_reset_vf() unconditionally ran
>
>     /* disable promisc modes in case they were enabled */
>     i40e_config_vf_promiscuous_mode(vf, vf->lan_vsi_id, false, false);
>
> regardless of the software bits. Would it be safer to call that
> unconditionally on revoke rather than infer the state from the bits? Note
> this one depends on an admin queue command failing part way through, which
> I could not show a guest or admin can force.

Pre-existing issue. As noted in the review itself, this scenario "depends
on an admin queue command failing part way through, which [could not be
shown] a guest or admin can force." This exists regardless of this patch.