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

[Jacob Keller <jacob.e.keller@intel.com>]

On 9/30/2026 3:39 AM, Jose Ignacio Tornos Martinez wrote:
>> [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.
> 

Since the usual case is going to be a situation where we don't expect to
exceed these limits, couldn't this check be expanded so we would still
reset in the case where the filters need to be removed? I guess its
overkill and maybe we accept the extra filters.

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

This doesn't address the case where filters have MAC addresses different
from the admin-set MAC? I am not sure if that still leaves a security
hole or not.

> 
> 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.
> 
This is an intentional change that I think should at least be called out
in the commit message. This *does* mean a VF which was previously
trusted could remain exceeding its untrusted limit in perpetuity until
it resets or self-removes the filters.

That does mean the VF will technically violate the trusted boundary, but
it can't get worse, and it was previously accepted while under trust. I
think.

I think this is acceptable, but we may want to clarify the functionally
changed behavior here so that it is clear.