Re: [PATCH net 4/8] i40e: skip unnecessary VF reset when setting trust
From: Jakub Kicinski <kuba@kernel.org>
Date: 2026-05-23 00:16:20
This is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible. --- i40e: skip unnecessary VF reset when setting trust This commit attempts to skip the ~10 second VF reset delay when changing the trust setting. It bypasses the reset and only updates the privilege capability bit when revoking trust if no advanced features are active.
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c index a26c3d47ec156..0cc434b26eb88 100644 --- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c +++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -4987,19 +5004,20 @@ int i40e_ndo_set_vf_trust(struct net_device *netdev, int vf_id, bool setting) set_bit(__I40E_MACVLAN_SYNC_PENDING, pf->state); pf->vsi[vf->lan_vsi_idx]->flags |= I40E_VSI_FLAG_FILTER_CHANGED; - i40e_vc_reset_vf(vf, true); + /* Reset only if revoking trust and VF has advanced features configured */ + if (!setting && + (vf->adq_enabled || vf->num_cloud_filters > 0 || + test_bit(I40E_VF_STATE_UC_PROMISC, &vf->vf_states) || + test_bit(I40E_VF_STATE_MC_PROMISC, &vf->vf_states))) {
[Severity: High] Does skipping the VF reset when revoking trust allow the Virtual Function to permanently retain privileged resources? A trusted VF can bypass untrusted limits, allocating a large number of MAC and VLAN filters and overriding administratively set MAC addresses. By skipping the reset upon trust revocation if ADq and promiscuous mode are inactive, the VSI is not destroyed. Since i40e_setup_vf_trust() only updates the capability bit without removing excess filters, could an untrusted VF retain these configurations and bypass isolation limits? [Severity: High] Can this introduce a race condition with concurrent virtchnl messages? Virtchnl messages are processed in the ARQ workqueue without holding rtnl_lock or __I40E_VIRTCHNL_OP_PENDING. A concurrent virtchnl request could verify the privilege capability bit before it is cleared, and successfully configure an advanced feature like promiscuous mode after these checks have bypassed the reset. Will the VF end up untrusted but with advanced features active?
+ i40e_vc_reset_vf(vf, true); + i40e_del_all_cloud_filters(vf);
[Severity: Critical] This is a pre-existing issue, but is vf->cloud_filter_list accessed concurrently without synchronization? i40e_del_all_cloud_filters() iterates over and deletes elements from vf->cloud_filter_list. Concurrently, the ARQ processing thread processes virtchnl messages and adds elements to the same list. Since there is no lock protecting this list across the netlink and ARQ thread contexts, could concurrent modifications corrupt the list pointers and lead to a use-after-free?
+ } else {
+ i40e_setup_vf_trust(vf, setting);
+ }
+
dev_info(&pf->pdev->dev, "VF %u is now %strusted\n",
vf_id, setting ? "" : "un");
- if (vf->adq_enabled) {
- if (!vf->trusted) {
- dev_info(&pf->pdev->dev,
- "VF %u no longer Trusted, deleting all cloud filters\n",
- vf_id);
- i40e_del_all_cloud_filters(vf);
- }
- }
-
out:
clear_bit(__I40E_VIRTCHNL_OP_PENDING, pf->state);[Severity: High] This isn't a bug introduced by this patch, but does using the __I40E_VIRTCHNL_OP_PENDING bit flag as an ad-hoc lock violate networking driver concurrency guidelines? The guidelines explicitly forbid using bit flags to guard code regions. This open-coded locking scheme provides no deadlock detection, lockdep visibility, or strict memory ordering guarantees. Could this be replaced with a proper mutex or lock?
return ret; }