Re: [PATCH net 4/8] i40e: skip unnecessary VF reset when setting trust

[Jakub Kicinski <kuba@kernel.org>]

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.

> 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
[ ... ]
> @@ -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;
>  }