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 changes i40e_ndo_set_vf_trust() to skip the VF reset when
granting trust, and when revoking trust unless one of a few advanced
features (ADQ/cloud filters, UC/MC promisc) is currently set on the VF.
On the no-reset path the new helper i40e_setup_vf_trust() flips the
I40E_VIRTCHNL_VF_CAP_PRIVILEGE bit directly.

> diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> index a26c3d47ec15..0cc434b26eb8 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> @@ -4943,6 +4943,23 @@ int i40e_ndo_set_vf_spoofchk(struct net_device *netdev, int vf_id, bool enable)
>  	return ret;
>  }
>  
> +/**
> + * i40e_setup_vf_trust - Enable/disable VF trust mode without reset
> + * @vf: VF to configure
> + * @setting: trust setting
> + *
> + * Update VF flags when changing trust without performing a VF reset.
> + * This is only called when it's safe to skip the reset (VF has no advanced
> + * features configured that need cleanup).
> + */
> +static void i40e_setup_vf_trust(struct i40e_vf *vf, bool setting)
> +{
> +	if (setting)
> +		set_bit(I40E_VIRTCHNL_VF_CAP_PRIVILEGE, &vf->vf_caps);
> +	else
> +		clear_bit(I40E_VIRTCHNL_VF_CAP_PRIVILEGE, &vf->vf_caps);
> +}
> +

[ ... ]

> @@ -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))) {
> +		i40e_vc_reset_vf(vf, true);
> +		i40e_del_all_cloud_filters(vf);
> +	} else {
> +		i40e_setup_vf_trust(vf, setting);
> +	}

[High]
Are the four conditions used here a complete description of "VF in a
clean state" for the purposes of trust revocation?

A trusted VF can install several pieces of privileged state that this
predicate does not consult:

  - more than I40E_VC_MAX_VLAN_PER_VF (16) VLAN filters, accepted by
    i40e_vc_add_vlan_msg() only because the VF is trusted

  - more than I40E_VC_MAX_MAC_ADDR_PER_VF (18) unicast MAC filters,
    permitted by i40e_check_vf_permission() only because the VF is
    trusted

  - a VF-side override of an administratively-set MAC (vf->pf_set_mac),
    where the comment in i40e_check_vf_permission() reads:

        /* ... Unless the VF is privileged and then it can do whatever */

Before this patch, the unconditional i40e_vc_reset_vf() drove
i40e_cleanup_reset_vf() -> i40e_free_vf_res(), which wiped the VF
filter table and re-derived the privilege bit from vf->trusted in
i40e_alloc_vf_resources().

On the new no-reset branch, only the I40E_VIRTCHNL_VF_CAP_PRIVILEGE bit
is cleared, so over-limit VLAN filters, over-limit MAC filters, and the
admin-MAC override remain installed in hardware while the driver
prints "VF %u is now untrusted".

Should this predicate also cover those cases (for example by checking
the VLAN/MAC filter counts on the VF VSI and vf->pf_set_mac), or by
performing the resource cleanup explicitly on the no-reset path?

[High]
Can the no-reset branch race with concurrent virtchnl message handling?

i40e_ndo_set_vf_trust() serializes against other ndo callbacks via
__I40E_VIRTCHNL_OP_PENDING, but i40e_vc_process_vf_msg() (driven from
i40e_clean_adminq_subtask() in the service workqueue) does not test
that bit. It rejects messages only when I40E_VF_STATE_DISABLED is set,
which the no-reset path never sets.

A revoke-trust sequence could then look like:

  CPU0 (ndo_set_vf_trust, setting=false)
     reads vf->num_cloud_filters == 0, !vf->adq_enabled,
           !UC_PROMISC, !MC_PROMISC
     selects the else branch

  CPU1 (i40e_vc_process_vf_msg, e.g. ADD_CLOUD_FILTER /
        ADD_VLAN / ADD_ETH_ADDR / CONFIG_PROMISCUOUS_MODE)
     tests I40E_VIRTCHNL_VF_CAP_PRIVILEGE -- still set
     installs privileged state (++num_cloud_filters,
        sets UC/MC_PROMISC, adds an over-limit VLAN/MAC, ...)

  CPU0
     i40e_setup_vf_trust(vf, false)
        clear_bit(I40E_VIRTCHNL_VF_CAP_PRIVILEGE, &vf->vf_caps);

End state: trust is "revoked" and the privilege bit is cleared, but
privileged state was installed during the window and is never cleaned
up.

The previous unconditional i40e_vc_reset_vf() path masked this because
i40e_trigger_vf_reset() clears I40E_VF_STATE_ACTIVE (so the
i40e_sync_vf_state(I40E_VF_STATE_ACTIVE) check at the top of most
virtchnl handlers fails) and i40e_cleanup_reset_vf() then wipes any
state that did get installed.

Should the no-reset path take a stronger form of synchronisation
against virtchnl processing, or sample the gating conditions and clear
the privilege bit under that synchronisation?

>  
>  	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);
> -		}
> -	}
> -

[Low]
The dev_info() "VF %u no longer Trusted, deleting all cloud filters"
is removed here. The new code still calls i40e_del_all_cloud_filters()
in the equivalent scenario (inside the conditional reset branch), but
the message is no longer emitted and the commit message does not
mention this change.

Was the removal intentional, and if so should it be noted in the
changelog?

>  out:
>  	clear_bit(__I40E_VIRTCHNL_OP_PENDING, pf->state);
>  	return ret;
>  }
-- 
pw-bot: cr