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

[netdev-bot+sashiko@kernel.org]

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

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

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] The new no-reset branch of ice_set_vf_trust()
  (drivers/net/ethernet/intel/ice/ice_sriov.c:1427-1430) updates…
- [Medium] The reset predicate in ice_set_vf_trust()
  (ice_sriov.c:1419-1422) ignores unicast MAC filters that were only…
- [Medium] The new no-reset path treats the software bits
  ICE_VF_STATE_UC_PROMISC/ICE_VF_STATE_MC_PROMISC as the full record of…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but
  ice_set_vf_trust() takes a VF kref through ice_get_vf_by_id()…

--

ice: skip unnecessary VF reset when setting trust

This patch makes ice_set_vf_trust() skip ice_reset_vf() in two cases. The
first is granting trust. The second is revoking trust from a VF that has
no LLDP MAC filters and no unicast or multicast promiscuous mode. In
those cases only vf->trusted and the ICE_VIRTCHNL_VF_CAP_PRIVILEGE bit
are updated.

> diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.c b/drivers/net/ethernet/intel/ice/ice_sriov.c
> index e04de02155966..f893dff39aac6 100644
> --- a/drivers/net/ethernet/intel/ice/ice_sriov.c
> +++ b/drivers/net/ethernet/intel/ice/ice_sriov.c

[ ... ]

> @@ -1401,11 +1415,19 @@ int ice_set_vf_trust(struct net_device *netdev, int vf_id, bool trusted)
>  
>  	mutex_lock(&vf->cfg_lock);
>  

[Severity: Medium]
This isn't a bug introduced by this patch, but the switchdev check
earlier in ice_set_vf_trust() returns without dropping the reference
that ice_get_vf_by_id() took:

	vf = ice_get_vf_by_id(pf, vf_id);
	if (!vf)
		return -EINVAL;

	if (ice_is_eswitch_mode_switchdev(pf)) {
		dev_info(ice_pf_to_dev(pf), "Trusted VF is forbidden in switchdev mode\n");
		return -EOPNOTSUPP;
	}

Does each ndo_set_vf_trust call in switchdev mode leak one VF kref? If
so, ice_release_vf() never runs, and the struct ice_vf and its pci_dev
reference are never released.

The notes on this version already call this a pre-existing leak. Setting
ret = -EOPNOTSUPP and jumping to out_put_vf would fix it.

> -	while (!trusted && vf->num_mac_lldp)
> -		ice_vf_update_mac_lldp_num(vf, ice_get_vf_vsi(vf), false);
> -
> -	vf->trusted = trusted;
> -	ice_reset_vf(vf, ICE_VF_RESET_NOTIFY);
> +	/* Reset only if revoking trust and VF has advanced features configured */
> +	if (!trusted &&
> +	    (vf->num_mac_lldp > 0 ||
> +	     test_bit(ICE_VF_STATE_UC_PROMISC, vf->vf_states) ||
> +	     test_bit(ICE_VF_STATE_MC_PROMISC, vf->vf_states))) {

[Severity: Medium]
Is this predicate enough to decide that a VF is in a clean state? It
does not cover unicast MAC filters that the VF could only add because it
was trusted.

While a VF is trusted, ice_can_vf_change_mac() lets it add unicast
filters even when its MAC was set by the admin:

	if (vf->pf_set_mac && !ice_is_vf_trusted(vf))
		return false;

ice_vc_handle_mac_addr_msg() also skips the per-VF quota for trusted
VFs:

	if (set && !ice_is_vf_trusted(vf) &&
	    (vf->num_mac + al->num_elements) > ICE_MAX_MACADDR_PER_VF) {

Before this patch, revoking trust always ran
ice_reset_vf()->ice_vf_reconfig_vsi()->ice_fltr_remove_all(). The
rebuild then re-added only the broadcast and hw_lan_addr filters.

Now the else branch is taken for a VF with num_mac_lldp == 0 and no
promisc bits. That VF keeps all its extra unicast filters in hardware,
and vf->num_mac can stay above ICE_MAX_MACADDR_PER_VF.

Would a pf_set_mac VF keep receiving traffic for MACs that an untrusted
VF may not add, even though the log says "VF N is now untrusted"?

The notes on this version point out that untrusted VFs can delete their
own filters. A guest that does not delete them keeps them until some
unrelated reset.

[Severity: Medium]
Can ICE_VF_STATE_UC_PROMISC and ICE_VF_STATE_MC_PROMISC both be clear
while promiscuous-related hardware state is still programmed?

ice_vc_cfg_promiscuous_mode_msg() disables VLAN pruning first:

	else
		ret = vlan_ops->dis_rx_filtering(vsi);

In the path without true promisc, the multicast promisc rule can be
installed, and then an ice_set_dflt_vsi() failure exits before either
bit is set:

		if (allmulti)
			mcast_err = ice_vf_set_vsi_promisc(vf, vsi, mcast_m);
		...
		if (ret) {
			...
			goto error_param;
		}

In the true-promisc path, if ucast_err and mcast_err are both non-zero,
neither bit is set. VLAN pruning is already disabled at that point, and
some per-VLAN rules may be installed.

Before this patch, the unconditional ice_reset_vf() rebuilt the VSI
through ice_vsi_decfg() and ice_fltr_remove_all(), whatever the bits
said. Now the else branch is taken. The VF cannot undo this state
itself, because ice_vc_cfg_promiscuous_mode_msg() rejects untrusted VFs
before it reaches ena_rx_filtering().

Would the untrusted VF keep VLAN pruning disabled, and possibly an
allmulti or VLAN promisc rule? Getting into this state needs an admin
queue or firmware failure while the VF was trusted.

> +		while (vf->num_mac_lldp)
> +			ice_vf_update_mac_lldp_num(vf, ice_get_vf_vsi(vf), false);
> +		vf->trusted = trusted;
> +		ice_reset_vf(vf, ICE_VF_RESET_NOTIFY);
> +	} else {
> +		vf->trusted = trusted;
> +		ice_setup_vf_trust(vf, trusted);
> +	}

[Severity: High]
What happens to the negotiated VLAN_V2 filter limit when this branch
runs? The limit comes from vf->trusted, but it is only computed when the
VF negotiates VIRTCHNL_OP_GET_OFFLOAD_VLAN_V2_CAPS. In
ice_vc_get_max_vlan_fltrs():

	if (vf->trusted)
		return VLAN_N_VID;
	else
		return ICE_MAX_VLAN_PER_VF;

The result is then cached in ice_vc_get_offload_vlan_v2_caps():

	/* store negotiated caps to prevent invalid VF messages */
	memcpy(&vf->vlan_v2_caps, caps, sizeof(*caps));

Only ice_vf_set_initialized() clears this cache, and it runs on the
reset path that this branch now skips. ice_vc_add_vlan_v2_msg() checks
new filters against the cached value in
ice_vc_validate_add_vlan_filter_list():

	if (num_requested_filters > vfc->max_filters)
		return false;

Granting trust always takes this branch. Does a VLAN_V2 VF then stay
limited to ICE_MAX_VLAN_PER_VF on the PF side?

iavf enforces its own cached copy through iavf_get_max_vlans_allowed(),
so the VF will not try to add more VLANs until it renegotiates. That
seems to contradict this line in the commit message: "we can just set
the capability flag to allow privileged operations".

Revoking trust from a clean VF also takes this branch. Does the
untrusted VF keep the VLAN_N_VID quota? The legacy path in
ice_vf_has_max_vlans() checks ice_is_vf_trusted() at the time of each
request, so the V1 and V2 paths would disagree for the same VF.

The notes on this version describe the cache invalidation as part of
the existing reset/rebuild design. Before this patch, though, every
trust change went through ice_reset_vf(), so the caps were always
renegotiated. The new branch is what lets the cache go stale.

iavf caches max_filters too. Would a VLAN_V2 VF need to renegotiate its
caps (through a reset or something equivalent) whenever its trust
setting changes?

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