Re: [PATCH net v2 1/3] i40e: 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. The key fix in this series is patch 2/3. This patch improves
the overall behavior by eliminating unnecessary VF resets that could add
~10 second delay during bonding setup. The no-reset path is narrowly
scoped to VFs with no ADQ, no cloud filters, and no promiscuous mode, a
basic configuration where the edge cases below are extremely difficult to
reproduce. No code changes are considered necessary for a new version.
Details for each comment:

[Severity: Medium]
Can a VF reset running at the same time in the service task undo this
write?

A reset from a VFLR (i40e_vc_process_vflr_event()), from
VIRTCHNL_OP_RESET_VF or from a request for more queues runs
i40e_alloc_vf_res(). That function reads vf->trusted and then writes the
same bit:

    if (vf->trusted)
        set_bit(I40E_VIRTCHNL_VF_CAP_PRIVILEGE, &vf->vf_caps);
    else
        clear_bit(I40E_VIRTCHNL_VF_CAP_PRIVILEGE, &vf->vf_caps);

Suppose i40e_ndo_set_vf_trust() sets vf->trusted = false and calls
i40e_setup_vf_trust() after that read but before the set_bit(). The bit
then stays set while vf->trusted is false. On the grant path the result
can be the reverse.

Before this patch, the ndo always called i40e_vc_reset_vf(), and
i40e_reset_vf() returns false while the other reset holds the flag:

    if (test_and_set_bit(I40E_VF_STATE_RESETTING, &vf->vf_states))
        return false;

So the ndo retried and ran a new reset, which recomputed the bit from the
final value of vf->trusted.

The notes on this version call this race pre-existing, but that
follow-up reset used to fix it up. Now that the reset is skipped, does
anything bring vf->trusted and vf_caps back in line?

The race requires a VFLR-initiated reset to read vf->trusted and be
preempted between the read and the assign_bit, while the ndo changes
trust in that exact window. The follow-up reset in the old code was not
a deliberate synchronization mechanism, it was a side effect of always
resetting. The window is extremely small and difficult to reproduce in
practice: it requires an admin trust change and a VFLR reset on the
same VF at the same instant, with preemption between two consecutive
instructions in i40e_alloc_vf_res(). The admin would be performing a
very unusual operation, and even if the mismatch occurred, it is not
breaking anything and can be corrected by toggling trust again.

[Severity: High]
Should this condition also cover MAC and VLAN filters that the VF could
only add because it was trusted?

Trust is only checked when a filter is added. In
i40e_check_vf_permission(), a trusted VF skips the admin MAC restriction
and gets a higher limit:

    if (!vf_trusted && !is_multicast_ether_addr(addr) &&
        vf->pf_set_mac && !ether_addr_equal(addr,
        vf->default_lan_addr.addr)) {
    ...
    if (!vf_trusted)
        mac_add_max = I40E_VC_MAX_MAC_ADDR_PER_VF;
    else
        mac_add_max =
        I40E_VC_MAX_MACVLAN_PER_TRUSTED_VF(pf->num_alloc_vfs,
        hw->num_ports);

i40e_vc_add_vlan_msg() only applies the VLAN limit when the bit is
clear:

    if ((vf->num_vlan >= I40E_VC_MAX_VLAN_PER_VF) &&
        !test_bit(I40E_VIRTCHNL_VF_CAP_PRIVILEGE, &vf->vf_caps)) {

Before this patch, the reset went through
i40e_reset_vf()->i40e_cleanup_reset_vf()->i40e_free_vf_res()->
i40e_vsi_release(). That removed every filter on the VSI and set
vf->num_vlan = 0, so the VF had to add its filters again under the
untrusted limits.

Now a VF with no ADQ, no cloud filters and no promisc bits takes the
else branch, which only clears the bit. These filters stay in hardware:

  - extra unicast MACs that differ from the admin-set MAC
  - MAC filters beyond I40E_VC_MAX_MAC_ADDR_PER_VF (18)
  - VLANs beyond I40E_VC_MAX_VLAN_PER_VF (16)

The VF keeps receiving on them. With spoofchk on, it can also still send
from those MACs.

The __I40E_MACVLAN_SYNC_PENDING work only reaches
i40e_correct_vf_mac_vlan_filters(). That function rewrites the VLAN ids
of existing entries and never deletes a MAC filter.

There is also a functional side effect. When mac_add_max drops back to
18 while i40e_count_active_filters(vsi) is still above 18, this check
fails even when mac_add_cnt is 0:

    if ((i40e_count_active_filters(vsi) + mac_add_cnt) > mac_add_max ||

Does every later VIRTCHNL_OP_ADD_ETH_ADDR from the VF then fail with
-EPERM, for example a new multicast subscription, until the guest
deletes filters itself?

The notes on this version say this is fine because an untrusted VF can
delete its own filters. Doesn't that rely on the guest cooperating, which
can't be assumed when the admin revokes trust?

The bonding case this patch targets is also a typical case for extra MACs
being installed. Are the "clean state" wording in the commit message and
the "no advanced features configured that need cleanup" wording in the
i40e_setup_vf_trust() kernel-doc accurate for that case?

The no-reset path is only reached for VFs with no ADQ, no cloud filters,
and no promiscuous mode, a basic configuration where having excess
MAC/VLAN filters beyond untrusted limits is extremely unlikely. In the
bonding use case targeted by this fix, trust changes happen during setup
and VFs typically have 1-2 MAC filters, well within the untrusted limit
of 18.

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. The
VF simply cannot add more filters beyond the untrusted limit, regardless
of its behavior.

For the functional side effect (later ADD_ETH_ADDR failing with -EPERM):
untrusted VFs can delete their own excess filters without trust checks
(i40e_vc_del_mac_addr_msg() and i40e_vc_remove_vlan_msg() have no trust
checks in the deletion path). Deletions reduce the count via
i40e_count_active_filters() immediately.

[Severity: Medium]
Can this unlocked check race with a promisc request that is still in
progress?

i40e_vc_config_promiscuous_mode_msg() runs from the service task through
i40e_clean_adminq_subtask()->i40e_vc_process_vf_msg(). It takes neither
rtnl nor __I40E_VIRTCHNL_OP_PENDING. It checks the privilege bit first,
then sends AdminQ commands, and only sets the state bits at the end:

    if (!test_bit(I40E_VIRTCHNL_VF_CAP_PRIVILEGE, &vf->vf_caps)) {
    ...
    aq_ret = i40e_config_vf_promiscuous_mode(vf, info->vsi_id, allmulti,
                         alluni);
    ...
        if (!test_and_set_bit(I40E_VF_STATE_UC_PROMISC,
                      &vf->vf_states))

A possible interleaving:

service task                          i40e_ndo_set_vf_trust()
privilege test_bit() passes
i40e_config_vf_promiscuous_mode()
  (AdminQ round trip)                 vf->trusted = false
                                      UC/MC_PROMISC still clear,
                                      reset skipped
                                      i40e_setup_vf_trust() clears
                                      the privilege bit
test_and_set_bit(UC_PROMISC)

Would the VF then stay promiscuous while untrusted, with nothing left to
undo it? The window spans a sleeping AdminQ round trip, and the guest can
send promisc requests over and over.

The notes on this version call this race pre-existing. Before this patch,
though, the reset always ran after the vf->trusted write, and
i40e_cleanup_reset_vf() turned promisc off and released the VSI:

    /* disable promisc modes in case they were enabled */
    i40e_config_vf_promiscuous_mode(vf, vf->lan_vsi_id, false, false);

That undid anything set up during the window. The no-reset path has no
such fallback.

The same race exists in the original code, the promisc setup can
complete after vf->trusted is set to false but before the reset runs.
During the entire reset duration (~10 seconds), the VF was promiscuous
while untrusted. The race window itself is the same (an AdminQ round
trip) and requires the VF to be actively sending promisc requests at
the exact moment trust is revoked.

If this race does occur, the VF ends up with the UC/MC_PROMISC bits
set. Any subsequent trust operation that observes those bits takes the
reset path with full promisc cleanup via
i40e_config_vf_promiscuous_mode().

[Severity: Medium]
This is a pre-existing issue and not introduced by this patch, but
i40e_vc_reset_vf() returns void and can return without doing a reset:

  - it returns early if __I40E_VFS_RELEASING is set
  - i40e_reset_vf() returns true without resetting if
    __I40E_VF_RESETS_DISABLED or __I40E_VF_DISABLE is set
  - the loop gives up after 20 tries while I40E_VF_STATE_RESETTING is
    held

In this branch, only i40e_alloc_vf_res() clears the privilege bit and
only i40e_cleanup_reset_vf() turns promisc off. Both run only if the
reset happens.

When the reset doesn't happen, can the VF stay privileged and promiscuous
while vf->trusted is false, with "VF %u is now untrusted" still logged?

A review of an earlier version suggested calling
i40e_setup_vf_trust(vf, setting) unconditionally, before the branch.
Would that make the bit correct whether or not the reset runs?

Pre-existing issue, not introduced by this patch. Calling
i40e_setup_vf_trust() unconditionally before the branch would change
the behavior for the reset path, setting the bit before reset, then
having the reset recompute it from vf->trusted. A reasonable
improvement but a separate change from this fix.

[Severity: Medium]
This is a pre-existing issue (the old code also deleted the cloud filters
after the reset), but does this ordering leak the struct
i40e_cloud_filter entries?

i40e_vc_reset_vf() releases the ADQ channel VSIs in i40e_free_vf_res()
and creates them again in i40e_alloc_vf_res(). cloud_filter_list is kept,
and each cfilter->seid still holds the old seid. When
i40e_find_vsi_from_seid() finds no match, i40e_del_all_cloud_filters()
does this:

        if (!vsi) {
            dev_err(&pf->pdev->dev, "VF %d: no VSI found for matching
            %u seid, can't delete cloud filter\n",
                vf->vf_id, cfilter->seid);
            continue;
        }

That skips the hlist_del(), the kfree(cfilter) and the
vf->num_cloud_filters decrement.

i40e_vc_del_qch_msg() deletes the filters before tearing down the
channels:

    if (vf->adq_enabled) {
        i40e_del_all_cloud_filters(vf);
        i40e_del_qch(vf);

Would that same order work here?

Pre-existing ordering issue. The same sequence (reset before cloud
filter deletion) existed in the original code. This patch restructures
the control flow but preserves the same sequence.

[Severity: Low]
This isn't a bug introduced by this patch, but granting trust only sets
the capability bit. i40e_vc_config_promiscuous_mode_msg() deliberately
reports success to an unprivileged VF without applying the mode:

        /* Lie to the VF on purpose, because this is an error we can
         * ignore. Unprivileged VF is not a virtual channel error.
         */
        aq_ret = 0;
        goto err_out;

iavf_set_promiscuous() caches current_netdev_promisc_flags and skips
duplicate requests. Is a promisc or allmulti request made while the VF
was untrusted then never applied after trust is granted, until the guest
toggles its flags?

The old unconditional reset on grant didn't make iavf resend the request
either, because current_netdev_promisc_flags isn't cleared on reset.

Pre-existing issue. The old unconditional reset on grant did not fix
this either, because current_netdev_promisc_flags is not cleared on
reset.