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

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 2 (2) · Low: 0 (1)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] When trust is revoked, filters the VF could only install while
  trusted are no longer removed.
- [Medium] The revoke path has a race (time-of-check/time-of-use) with
  virtchnl handlers and no longer has the reset to clean up after it.
- [Medium] The privilege bit can be lost to a concurrent service-task VF
  reset.

Pre-existing issues:
- [Medium] In the revoke-with-reset branch, CAP_PRIVILEGE is cleared (by
  i40e_alloc_vf_res()) and hardware promisc is dropped (by…
- [Medium] In the revoke reset branch, i40e_del_all_cloud_filters() runs
  after i40e_vc_reset_vf() has released the ADQ channel VSIs…
- [Low] Granting trust does not apply a promiscuous-mode request the PF
  earlier acknowledged but ignored.

--

i40e: skip unnecessary VF reset when setting trust

This commit makes i40e_ndo_set_vf_trust() skip the VF reset in two cases:
when granting trust, and when revoking trust from a VF that has no ADQ,
cloud filters or promiscuous mode configured. In those cases a new
helper, i40e_setup_vf_trust(), only updates I40E_VIRTCHNL_VF_CAP_PRIVILEGE.
The goal is to avoid the ~10 second reset delay during bonding setup.

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

[ ... ]

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

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

+		i40e_vc_reset_vf(vf, true);

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

+		i40e_del_all_cloud_filters(vf);

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

+	} else {
+		i40e_setup_vf_trust(vf, setting);

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

+	}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928224454.483072-1-anthony.l.nguyen%40intel.com