[PATCH net 0/4][pull request] Fix i40e/ice/iavf VF bonding after netdev lock changes

[PATCH net 0/4][pull request] Fix i40e/ice/iavf VF bonding after netdev lock changes

[Tony Nguyen]

Jose Ignacio Tornos Martinez says:

This series fixes VF bonding failures introduced by commit ad7c7b2172c3
("net: hold netdev instance lock during sysfs operations").

When adding VFs to a bond immediately after setting trust mode, MAC
address changes fail with -EAGAIN, preventing bonding setup. This
affects both i40e (700-series) and ice (800-series) Intel NICs.

The core issue is lock contention: iavf_set_mac() is now called with the
netdev lock held and waits for MAC change completion while holding it.
However, both the watchdog task that sends the request and the adminq_task
that processes PF responses also need this lock, creating a deadlock where
neither can run, causing timeouts.

Additionally, setting VF trust triggers an unnecessary ~10 second VF reset
in i40e driver that delays bonding setup, even though filter
synchronization happens naturally during normal VF operation. For ice
driver, the delay is not so big, but in the same way the operation is not
necessary.

This series:
1. Adds safety guard to prevent MAC changes during reset or early
   initialization (before VF is ready)
2. Eliminates unnecessary VF reset when setting trust in i40e (reset only
   if revoking trust and VF has advanced features configured).
3. Fixes lock contention by polling admin queue synchronously
4. Eliminates unnecessary VF reset when setting trust in ice, (reset only
   if revoking trust and VF has advanced features configured).

The key fix (patch 3/4) implements a synchronous MAC change operation
similar to the approach used for ndo_change_mtu deadlock fix:
https://lore.kernel.org/intel-wired-lan/20260211191855.1532226-1-poros@redhat.com/
Instead of scheduling work and waiting, it:

- Sends the virtchnl message directly (not via watchdog)
- Polls the admin queue hardware directly for responses
- Processes all messages inline (including non-MAC messages)
- Returns when complete or times out

This allows the operation to complete synchronously while holding
netdev_lock, without relying on watchdog or adminq_task.

The function can sleep for up to 2.5 seconds polling hardware, but this
is acceptable since netdev_lock is per-device and only serializes
operations on the same interface.

Testing shows VF bonding now works reliably in ~5 seconds vs 15+ seconds
before (i40e), without timeouts or errors (i40e and ice).

Tested on Intel 700-series (i40e) and 800-series (ice) dual-port NICs
with iavf driver.

Thanks to Jan Tluka <jtluka@redhat.com> and Yuying Ma <yuma@redhat.com> for
reporting the issues.
---
These patches are split off from this submission:
https://lore.kernel.org/netdev/20260804222205.1580328-1-anthony.l.nguyen@intel.com/

Which had comments from Sashiko; responses to each patch are linked below.

From patch 1 response:
All the comments below fall into pre-existing issues, concerns already
addressed in previous versions, out-of-scope items, or extreme edge
cases. No code changes are considered necessary for a new version.

Patch 1: https://lore.kernel.org/netdev/20260812065650.10326-1-jtornosm@redhat.com/
Patch 2: https://lore.kernel.org/netdev/20260812065955.10460-1-jtornosm@redhat.com/
Patch 3: https://lore.kernel.org/netdev/20260812070236.10559-1-jtornosm@redhat.com/
Patch 4: https://lore.kernel.org/netdev/20260812070502.10679-1-jtornosm@redhat.com/

IWL: https://lore.kernel.org/intel-wired-lan/20260623101800.991293-1-jtornosm@redhat.com/

The following are changes since commit 746fc0787f616da418ffc04a110296fe95d53491:
  net: usb: cdc_ncm: add Apple MacBook Pro USB product ID 0x1902
and are available in the git repository at:
  git://git.kernel.org/pub/scm/linux/kernel/git/tnguy/net-queue 40GbE

Jose Ignacio Tornos Martinez (4):
  iavf: return EBUSY if reset in progress or not ready during MAC change
  i40e: skip unnecessary VF reset when setting trust
  iavf: send MAC change request synchronously
  ice: skip unnecessary VF reset when setting trust

 .../ethernet/intel/i40e/i40e_virtchnl_pf.c    | 38 +++++--
 drivers/net/ethernet/intel/iavf/iavf.h        | 11 ++-
 drivers/net/ethernet/intel/iavf/iavf_main.c   | 88 +++++++++++++----
 .../net/ethernet/intel/iavf/iavf_virtchnl.c   | 99 +++++++++++++++++--
 drivers/net/ethernet/intel/ice/ice_sriov.c    | 35 ++++++-
 5 files changed, 226 insertions(+), 45 deletions(-)

-- 
2.47.1

[PATCH net 1/4] iavf: return EBUSY if reset in progress or not ready during MAC change

[Tony Nguyen]

From: Jose Ignacio Tornos Martinez <jtornosm@redhat.com>

When a MAC address change is requested while the VF is resetting or still
initializing, return -EBUSY immediately instead of attempting the
operation.

Additionally, during early initialization states (before __IAVF_DOWN),
the PF may be slow to respond to MAC change requests, causing long
delays. Only allow MAC changes once the VF reaches __IAVF_DOWN state or
later, when the watchdog is running and the VF is ready for operations.

After commit ad7c7b2172c3 ("net: hold netdev instance lock
during sysfs operations"), MAC changes are called with the netdev lock
held, so we should not wait with the lock held during reset or
initialization. This allows the caller to retry or handle the busy state
appropriately without blocking other operations.

Signed-off-by: Jose Ignacio Tornos Martinez <jtornosm@redhat.com>
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/iavf/iavf_main.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
index 29b8403a066b..630388e9d28c 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_main.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
@@ -1042,6 +1042,9 @@ static int iavf_set_mac(struct net_device *netdev, void *p)
 	struct sockaddr *addr = p;
 	int ret;
 
+	if (iavf_is_reset_in_progress(adapter) || adapter->state < __IAVF_DOWN)
+		return -EBUSY;
+
 	if (!is_valid_ether_addr(addr->sa_data))
 		return -EADDRNOTAVAIL;
 
-- 
2.47.1

Re: [PATCH net 1/4] iavf: return EBUSY if reset in progress or not ready during MAC change

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
iavf: return EBUSY if reset in progress or not ready during MAC change

This adds an early bail-out to iavf_set_mac() so that a MAC address change
returns -EBUSY when a reset is in progress or when the VF has not yet
reached __IAVF_DOWN.  The stated motivation is to avoid waiting inside the
ndo callback while the netdev instance lock is held.

No Link: tags present in the commit header.

A couple of questions below, one about the commit message and one about the
new check itself.

>     After commit ad7c7b2172c3 ("net: hold netdev instance lock
>     during sysfs operations"), MAC changes are called with the netdev lock
>     held, so we should not wait with the lock held during reset or
>     initialization.

[Severity: Low]
This paragraph describes a user-visible behaviour change caused by
ad7c7b2172c3, but there is no Fixes: tag on this commit.  Was that
intentional?

Worth noting that this commit does not actually remove the wait under the
netdev instance lock: after this patch iavf_set_mac() still calls

	ret = wait_event_interruptible_timeout(adapter->vc_waitqueue,
					       iavf_is_mac_set_handled(netdev, addr->sa_data),
					       msecs_to_jiffies(2500));

so a Fixes: ad7c7b2172c3 here would arguably be misleading.  The later
patch in the same series, "iavf: send MAC change request synchronously",
does carry Fixes: ad7c7b2172c3 and cc: stable, which looks like the right
place for the provenance.  Is that the intended split?

> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
> index 29b8403a066b..630388e9d28c 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> @@ -1042,6 +1042,9 @@ static int iavf_set_mac(struct net_device *netdev, void *p)
>  	struct sockaddr *addr = p;
>  	int ret;
>  
> +	if (iavf_is_reset_in_progress(adapter) || adapter->state < __IAVF_DOWN)
> +		return -EBUSY;
> +

[Severity: Low]
Is this check-then-act sequence racy against the reset scheduling path?

iavf_is_reset_in_progress() does plain reads of adapter->state and
adapter->flags:

	static bool iavf_is_reset_in_progress(struct iavf_adapter *adapter)
	{
		if (adapter->state == __IAVF_RESETTING ||
		    adapter->flags & (IAVF_FLAG_RESET_PENDING |
				      IAVF_FLAG_RESET_NEEDED))
			return true;

		return false;
	}

IAVF_FLAG_RESET_NEEDED can be set from a context that does not hold the
netdev instance lock:

dev_watchdog()
  netif_freeze_queues(dev);
  dev->netdev_ops->ndo_tx_timeout(dev, i);    /* timer context */
    iavf_tx_timeout()
      iavf_schedule_reset(adapter, IAVF_FLAG_RESET_NEEDED)
        adapter->flags |= flags;              /* non-atomic RMW */

net/sched/sch_generic.c only freezes the TX queues around ndo_tx_timeout(),
so there does not appear to be anything serializing that writer against
iavf_set_mac().  If the TX timeout fires just after the new test, does
iavf_set_mac() go on to the 2.5 s wait_event_interruptible_timeout() with
the netdev instance lock held anyway, i.e. exactly the case the check is
meant to reject?

The window is small and the fallback is the pre-patch behaviour (blocked
for up to 2.5 s, then -EAGAIN), so this looks like a best-effort guard
rather than a new failure mode.  Is that the intent, or should the state
and flags reads be done under the same serialization as the reset path
(and with READ_ONCE(), given the concurrent non-atomic |= on
adapter->flags)?

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

[Tony Nguyen]

From: Jose Ignacio Tornos Martinez <jtornosm@redhat.com>

The current implementation triggers a VF reset when changing the trust
setting, causing a ~10 second delay during bonding setup.

In all the cases, the reset causes a ~10 second delay during which:
- VF must reinitialize completely
- Any in-progress operations (like bonding enslave) fail with timeouts
- VF is unavailable

When granting trust, no reset is needed - we can just set the capability
flag to allow privileged operations.

When revoking trust, we only need to reset (conservative approach) if
the VF has actually configured advanced features that require cleanup
(ADQ/cloud filters, promiscuous mode). For VFs in a clean state, we can
safely change the trust setting without the disruptive reset.

When we don't reset, we manually handle capability flag via helper
function, eliminating the delay.

Signed-off-by: Jose Ignacio Tornos Martinez <jtornosm@redhat.com>
Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 .../ethernet/intel/i40e/i40e_virtchnl_pf.c    | 38 ++++++++++++++-----
 1 file changed, 28 insertions(+), 10 deletions(-)

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);
+}
+
 /**
  * i40e_ndo_set_vf_trust
  * @netdev: network interface device structure of the pf
@@ -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);
+	}
+
 	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);
 	return ret;
-- 
2.47.1

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

[Jakub Kicinski]

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 changes i40e_ndo_set_vf_trust() so that the VF reset is only performed
when trust is being revoked and the VF has ADQ, cloud filters or
promiscuous mode configured. In the other cases a new helper,
i40e_setup_vf_trust(), just sets or clears
I40E_VIRTCHNL_VF_CAP_PRIVILEGE. A few questions below about the state that
the skipped reset used to clean up.

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

[Severity: Medium]

This helper writes I40E_VIRTCHNL_VF_CAP_PRIVILEGE from the ndo (rtnl)
context. i40e_alloc_vf_res() derives the same bit from an unsynchronized
read of vf->trusted:

i40e_alloc_vf_res()
	if (vf->trusted)
		set_bit(I40E_VIRTCHNL_VF_CAP_PRIVILEGE, &vf->vf_caps);
	else
		clear_bit(I40E_VIRTCHNL_VF_CAP_PRIVILEGE, &vf->vf_caps);

That runs from i40e_reset_vf()->i40e_cleanup_reset_vf()->i40e_alloc_vf_res()
in service task context, and a VF can request a reset itself through
VIRTCHNL_OP_RESET_VF.

If that path reads vf->trusted and is preempted before the
set_bit()/clear_bit(), can i40e_ndo_set_vf_trust() update vf->trusted and
call i40e_setup_vf_trust() in between, so the stale write lands last? That
would leave vf->trusted true with the capability bit clear, or vf->trusted
false with the bit still set.

Before this patch the ndo always followed the vf->trusted update with
i40e_vc_reset_vf(), so the bit was recomputed from the final value of
vf->trusted. With the reset skipped, is anything left that reconciles the
two?

>  /**
>   * i40e_ndo_set_vf_trust
>   * @netdev: network interface device structure of the pf
> @@ -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 this condition need to consider the MAC and VLAN filters that the VF
was only allowed to install because it was trusted?

Trust is checked at add time only. i40e_check_vf_permission():

	vf_trusted = test_bit(I40E_VIRTCHNL_VF_CAP_PRIVILEGE, &vf->vf_caps);
	...
	if (!vf_trusted && !is_multicast_ether_addr(addr) &&
	    vf->pf_set_mac && ...)
		return -EPERM;
	...
	if (!vf_trusted)
		mac_add_max = I40E_VC_MAX_MAC_ADDR_PER_VF;
	else
		mac_add_max = I40E_VC_MAX_MACVLAN_PER_TRUSTED_VF(...);

and i40e_vc_add_vlan_msg():

	if ((vf->num_vlan >= I40E_VC_MAX_VLAN_PER_VF) &&
	    !test_bit(I40E_VIRTCHNL_VF_CAP_PRIVILEGE, &vf->vf_caps)) {

Nothing revalidates filters that are already installed when the privilege
bit is dropped.

Previously the revoke always went through
i40e_vc_reset_vf()->i40e_reset_vf()->i40e_cleanup_reset_vf(), which calls
i40e_free_vf_res():

i40e_free_vf_res()
	if (vf->lan_vsi_idx) {
		i40e_vsi_release(pf->vsi[vf->lan_vsi_idx]);

so every MAC/VLAN filter was purged, and i40e_cleanup_reset_vf() also set
vf->num_vlan = 0.

A VF that used only the privileged MAC/VLAN allowances has
adq_enabled == false, num_cloud_filters == 0 and both promisc bits clear,
so it takes the else branch here and only the capability bit is cleared.
Can such a VF keep receiving on MAC/VLAN combinations an untrusted VF may
never request, including a MAC that overrides the administratively set
vf->default_lan_addr, while ip link reports trust off?

Is the requested MACVLAN resync enough to cover this? Following
i40e_sync_vsi_filters()->i40e_correct_vf_mac_vlan_filters() in
i40e_main.c:

	new_vlan = i40e_get_vf_new_vlan(vsi, NULL, f, vlan_filters, trusted);
	if (new_vlan != f->vlan) { ... }

that only re-derives I40E_VLAN_ANY versus 0 for existing filters, it does
not delete filters that exceed the untrusted quota.

As a second effect, vf->num_vlan and the active MAC filter count stay above
the untrusted limits, so does every later VLAN/MAC add from that VF get
rejected until an unrelated reset happens?

[Severity: Medium]

Can this sample of VF-controlled state race with the virtchnl handlers that
write it? i40e_vc_process_vf_msg() is called from
i40e_clean_adminq_subtask() in i40e_main.c:

	case i40e_aqc_opc_send_msg_to_pf:
		ret = i40e_vc_process_vf_msg(pf, ...);

which runs in service task context without rtnl_lock and without taking
__I40E_VIRTCHNL_OP_PENDING, so it does not exclude this ndo.

CPU1 in i40e_vc_config_promiscuous_mode_msg() passes the gate and starts
the admin queue work:

	if (!test_bit(I40E_VIRTCHNL_VF_CAP_PRIVILEGE, &vf->vf_caps)) { ... }
	...
	aq_ret = i40e_config_vf_promiscuous_mode(vf, info->vsi_id,
						 allmulti, alluni);
	if (aq_ret)
		goto err_out;
	... test_and_set_bit(I40E_VF_STATE_UC_PROMISC, &vf->vf_states)

CPU0 then runs i40e_ndo_set_vf_trust(), sees both promisc bits still clear
(they are set only after the admin queue sequence), takes the else branch,
clears the privilege bit and skips the reset. CPU1 finishes and sets
I40E_VF_STATE_UC_PROMISC with promiscuous mode programmed in hardware.

Does that leave an untrusted VF promiscuous indefinitely? The same ordering
appears in i40e_vc_add_cloud_filter(), where

	hlist_add_head(&cfilter->cloud_node, &vf->cloud_filter_list);
	vf->num_cloud_filters++;

happens after the filter is programmed, and in i40e_vc_add_qch_msg() which
sets vf->adq_enabled last.

> +		i40e_vc_reset_vf(vf, true);

[Severity: Medium]

This is a pre-existing issue and not introduced by this patch, but in this
branch the capability bit and the hardware promiscuous teardown are only
side effects of the reset, and i40e_vc_reset_vf() is void and can return
having done nothing:

i40e_vc_reset_vf()
	if (test_bit(__I40E_VFS_RELEASING, pf->state))
		return;
	if (i40e_reset_vf(vf, false))
		return;

i40e_reset_vf() also returns true immediately when __I40E_VF_RESETS_DISABLED
or __I40E_VF_DISABLE is set, and after 20 attempts the helper only warns.

In those cases vf->trusted is already false and the driver prints "VF %u is
now untrusted", but the bit consulted by i40e_check_vf_permission(),
i40e_vc_config_promiscuous_mode_msg(), i40e_vc_add_vlan_msg() and
i40e_vc_add_cloud_filter() is still set, since i40e_setup_vf_trust() is
only called from the else branch. i40e_ndo_get_vf_config() reports
ivi->trusted = vf->trusted, so the two can disagree.

Would calling i40e_setup_vf_trust(vf, setting) unconditionally, before the
branch, make both artifacts deterministic?

> +		i40e_del_all_cloud_filters(vf);

[Severity: Medium]

This ordering is pre-existing and not introduced by this patch, the
baseline also called i40e_vc_reset_vf() before i40e_del_all_cloud_filters()
in the "if (vf->adq_enabled && !vf->trusted)" block, but the restructuring
keeps it.

The reset just above releases the ADQ channel VSIs and zeroes their ids in
i40e_free_vf_res():

	if (j)
		i40e_vsi_release(pf->vsi[vf->ch[j].vsi_idx]);
	vf->ch[j].vsi_idx = 0;
	vf->ch[j].vsi_id = 0;

and i40e_alloc_vf_res() re-creates them with newly assigned seids. The
teardown then looks the VSI up by the recorded seid in
i40e_find_vsi_from_seid():

	for (i = 0; i < vf->num_tc ; i++) {
		vsi = i40e_find_vsi_from_id(pf, vf->ch[i].vsi_id);
		if (vsi && vsi->seid == seid)
			return vsi;
	}

If the seid changed, i40e_del_all_cloud_filters() takes the error path:

	vsi = i40e_find_vsi_from_seid(vf, cfilter->seid);
	if (!vsi) {
		dev_err(... "can't delete cloud filter\n" ...);
		continue;
	}

so the hlist_del(), the kfree(cfilter) and the vf->num_cloud_filters
decrement are all skipped. Does this leak the struct i40e_cloud_filter
allocations? Nothing else walks vf->cloud_filter_list, and with
vf->num_cloud_filters stuck non-zero the VF also takes the reset branch
here forever. Would deleting the cloud filters before triggering the reset
be the right order?

> +	} 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);
>  	return ret;

[PATCH net 3/4] iavf: send MAC change request synchronously

[Tony Nguyen]

From: Jose Ignacio Tornos Martinez <jtornosm@redhat.com>

After commit ad7c7b2172c3 ("net: hold netdev instance lock during sysfs
operations"), iavf_set_mac() is called with the netdev instance lock
already held.

The function queues a MAC address change request via
iavf_replace_primary_mac() and then waits for completion. However, in
the current flow, the actual virtchnl message is sent by the watchdog
task, which also needs to acquire the netdev lock to run. Additionally,
the adminq_task which processes virtchnl responses also needs the netdev
lock.

This creates a deadlock scenario:
1. iavf_set_mac() holds netdev lock and waits for MAC change
2. Watchdog needs netdev lock to send the request -> blocked
3. Even if request is sent, adminq_task needs netdev lock to process
   PF response -> blocked
4. MAC change times out after 2.5 seconds
5. iavf_set_mac() returns -EAGAIN

This particularly affects VFs during bonding setup when multiple VFs are
enslaved in quick succession.

Fix by implementing a synchronous MAC change operation similar to the
approach used in commit fdadbf6e84c4 ("iavf: fix incorrect reset handling
in callbacks").

The solution:
1. Send the virtchnl ADD_ETH_ADDR message directly (not via watchdog)
2. Poll the admin queue hardware directly for responses
3. Process all received messages (including non-MAC messages)
4. Return when MAC change completes or times out

A new generic function iavf_poll_virtchnl_response() is introduced that
can be reused for any future synchronous virtchnl operations. It takes a
callback to check completion, allowing flexible condition checking.

This allows the operation to complete synchronously while holding
netdev_lock, without relying on watchdog or adminq_task. The function
can sleep for up to 2.5 seconds polling hardware, but this is acceptable
since netdev_lock is per-device and only serializes operations on the
same interface.

To support this, change iavf_add_ether_addrs() to return an error code
instead of void, allowing callers to detect failures. Additionally,
export iavf_mac_add_reject() to enable proper rollback on local failures
(timeouts, send errors) - PF rejections are already handled automatically
by iavf_virtchnl_completion().

Remove vc_waitqueue entirely because iavf_set_mac was the only waiter on
this waitqueue and after the changes it is not needed.

Fixes: ad7c7b2172c3 ("net: hold netdev instance lock during sysfs operations")
cc: stable@vger.kernel.org
Signed-off-by: Jose Ignacio Tornos Martinez <jtornosm@redhat.com>
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/iavf/iavf.h        | 11 ++-
 drivers/net/ethernet/intel/iavf/iavf_main.c   | 85 ++++++++++++----
 .../net/ethernet/intel/iavf/iavf_virtchnl.c   | 99 +++++++++++++++++--
 3 files changed, 165 insertions(+), 30 deletions(-)

diff --git a/drivers/net/ethernet/intel/iavf/iavf.h b/drivers/net/ethernet/intel/iavf/iavf.h
index dc31202b2a94..8c45536fd502 100644
--- a/drivers/net/ethernet/intel/iavf/iavf.h
+++ b/drivers/net/ethernet/intel/iavf/iavf.h
@@ -259,7 +259,6 @@ struct iavf_adapter {
 	struct work_struct adminq_task;
 	struct work_struct finish_config;
 	wait_queue_head_t down_waitqueue;
-	wait_queue_head_t vc_waitqueue;
 	struct iavf_q_vector *q_vectors;
 	struct list_head vlan_filter_list;
 	int num_vlan_filters;
@@ -588,8 +587,9 @@ void iavf_configure_queues(struct iavf_adapter *adapter);
 void iavf_enable_queues(struct iavf_adapter *adapter);
 void iavf_disable_queues(struct iavf_adapter *adapter);
 void iavf_map_queues(struct iavf_adapter *adapter);
-void iavf_add_ether_addrs(struct iavf_adapter *adapter);
+int iavf_add_ether_addrs(struct iavf_adapter *adapter);
 void iavf_del_ether_addrs(struct iavf_adapter *adapter);
+void iavf_mac_add_reject(struct iavf_adapter *adapter);
 void iavf_add_vlans(struct iavf_adapter *adapter);
 void iavf_del_vlans(struct iavf_adapter *adapter);
 void iavf_set_promiscuous(struct iavf_adapter *adapter);
@@ -606,6 +606,13 @@ void iavf_disable_vlan_stripping(struct iavf_adapter *adapter);
 void iavf_virtchnl_completion(struct iavf_adapter *adapter,
 			      enum virtchnl_ops v_opcode,
 			      enum iavf_status v_retval, u8 *msg, u16 msglen);
+int iavf_poll_virtchnl_response(struct iavf_adapter *adapter,
+				struct iavf_arq_event_info *event,
+				bool (*condition)(struct iavf_adapter *adapter,
+						  const void *data,
+						  enum virtchnl_ops v_op),
+				const void *cond_data,
+				unsigned int timeout_ms);
 int iavf_config_rss(struct iavf_adapter *adapter);
 void iavf_cfg_queues_bw(struct iavf_adapter *adapter);
 void iavf_cfg_queues_quanta_size(struct iavf_adapter *adapter);
diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
index 630388e9d28c..3fa288e3798a 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_main.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
@@ -1029,6 +1029,60 @@ static bool iavf_is_mac_set_handled(struct net_device *netdev,
 	return ret;
 }
 
+/**
+ * iavf_mac_change_done - Check if MAC change completed
+ * @adapter: board private structure
+ * @data: MAC address being checked (as const void *)
+ * @v_op: virtchnl opcode from processed message
+ *
+ * Callback for iavf_poll_virtchnl_response() to check if MAC change completed.
+ *
+ * Return: true if MAC change completed, false otherwise
+ */
+static bool iavf_mac_change_done(struct iavf_adapter *adapter,
+				 const void *data, enum virtchnl_ops v_op)
+{
+	const u8 *addr = data;
+
+	return iavf_is_mac_set_handled(adapter->netdev, addr);
+}
+
+/**
+ * iavf_set_mac_sync - Synchronously change MAC address
+ * @adapter: board private structure
+ * @addr: MAC address to set
+ *
+ * Send MAC change request to PF and poll admin queue for response.
+ * Caller must hold netdev_lock. This can sleep for up to 2.5 seconds.
+ * Event buffer is allocated before sending to avoid state mismatch if
+ * allocation fails after message is sent to PF.
+ *
+ * Return: 0 on success, negative on failure
+ */
+static int iavf_set_mac_sync(struct iavf_adapter *adapter, const u8 *addr)
+{
+	struct iavf_arq_event_info event;
+	int ret;
+
+	netdev_assert_locked(adapter->netdev);
+
+	event.buf_len = IAVF_MAX_AQ_BUF_SIZE;
+	event.msg_buf = kzalloc(event.buf_len, GFP_KERNEL);
+	if (!event.msg_buf)
+		return -ENOMEM;
+
+	ret = iavf_add_ether_addrs(adapter);
+	if (ret)
+		goto out;
+
+	ret = iavf_poll_virtchnl_response(adapter, &event,
+					  iavf_mac_change_done, addr, 2500);
+
+out:
+	kfree(event.msg_buf);
+	return ret;
+}
+
 /**
  * iavf_set_mac - NDO callback to set port MAC address
  * @netdev: network interface device structure
@@ -1049,25 +1103,23 @@ static int iavf_set_mac(struct net_device *netdev, void *p)
 		return -EADDRNOTAVAIL;
 
 	ret = iavf_replace_primary_mac(adapter, addr->sa_data);
-
 	if (ret)
 		return ret;
 
-	ret = wait_event_interruptible_timeout(adapter->vc_waitqueue,
-					       iavf_is_mac_set_handled(netdev, addr->sa_data),
-					       msecs_to_jiffies(2500));
-
-	/* If ret < 0 then it means wait was interrupted.
-	 * If ret == 0 then it means we got a timeout.
-	 * else it means we got response for set MAC from PF,
-	 * check if netdev MAC was updated to requested MAC,
-	 * if yes then set MAC succeeded otherwise it failed return -EACCES
-	 */
-	if (ret < 0)
+	ret = iavf_set_mac_sync(adapter, addr->sa_data);
+	if (ret) {
+		/* Rollback only if send failed (message never reached PF).
+		 * Don't rollback on timeout (-EAGAIN) because the message was
+		 * sent and PF will eventually respond. When the response arrives,
+		 * iavf_virtchnl_completion() will handle rollback (on PF error)
+		 * or acceptance (on PF success) automatically.
+		 */
+		if (ret != -EAGAIN) {
+			iavf_mac_add_reject(adapter);
+			ether_addr_copy(adapter->hw.mac.addr, netdev->dev_addr);
+		}
 		return ret;
-
-	if (!ret)
-		return -EAGAIN;
+	}
 
 	if (!ether_addr_equal(netdev->dev_addr, addr->sa_data))
 		return -EACCES;
@@ -5397,9 +5449,6 @@ static int iavf_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	/* Setup the wait queue for indicating transition to down status */
 	init_waitqueue_head(&adapter->down_waitqueue);
 
-	/* Setup the wait queue for indicating virtchannel events */
-	init_waitqueue_head(&adapter->vc_waitqueue);
-
 	INIT_LIST_HEAD(&adapter->ptp.aq_cmds);
 	init_waitqueue_head(&adapter->ptp.phc_time_waitqueue);
 	mutex_init(&adapter->ptp.aq_cmd_lock);
diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
index ec234cc8bd9d..e6b7e8f82c7c 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
@@ -2,6 +2,7 @@
 /* Copyright(c) 2013 - 2018 Intel Corporation. */
 
 #include <linux/net/intel/libie/rx.h>
+#include <net/netdev_lock.h>
 
 #include "iavf.h"
 #include "iavf_ptp.h"
@@ -555,20 +556,23 @@ iavf_set_mac_addr_type(struct virtchnl_ether_addr *virtchnl_ether_addr,
  * @adapter: adapter structure
  *
  * Request that the PF add one or more addresses to our filters.
- **/
-void iavf_add_ether_addrs(struct iavf_adapter *adapter)
+ *
+ * Return: 0 on success, negative on failure
+ */
+int iavf_add_ether_addrs(struct iavf_adapter *adapter)
 {
 	struct virtchnl_ether_addr_list *veal;
 	struct iavf_mac_filter *f;
 	int i = 0, count = 0;
 	bool more = false;
 	size_t len;
+	int ret;
 
 	if (adapter->current_op != VIRTCHNL_OP_UNKNOWN) {
 		/* bail because we already have a command pending */
 		dev_err(&adapter->pdev->dev, "Cannot add filters, command %d pending\n",
 			adapter->current_op);
-		return;
+		return -EBUSY;
 	}
 
 	spin_lock_bh(&adapter->mac_vlan_list_lock);
@@ -580,7 +584,7 @@ void iavf_add_ether_addrs(struct iavf_adapter *adapter)
 	if (!count) {
 		adapter->aq_required &= ~IAVF_FLAG_AQ_ADD_MAC_FILTER;
 		spin_unlock_bh(&adapter->mac_vlan_list_lock);
-		return;
+		return 0;
 	}
 	adapter->current_op = VIRTCHNL_OP_ADD_ETH_ADDR;
 
@@ -594,8 +598,9 @@ void iavf_add_ether_addrs(struct iavf_adapter *adapter)
 
 	veal = kzalloc(len, GFP_ATOMIC);
 	if (!veal) {
+		adapter->current_op = VIRTCHNL_OP_UNKNOWN;
 		spin_unlock_bh(&adapter->mac_vlan_list_lock);
-		return;
+		return -ENOMEM;
 	}
 
 	veal->vsi_id = adapter->vsi_res->vsi_id;
@@ -615,8 +620,15 @@ void iavf_add_ether_addrs(struct iavf_adapter *adapter)
 
 	spin_unlock_bh(&adapter->mac_vlan_list_lock);
 
-	iavf_send_pf_msg(adapter, VIRTCHNL_OP_ADD_ETH_ADDR, (u8 *)veal, len);
+	ret = iavf_send_pf_msg(adapter, VIRTCHNL_OP_ADD_ETH_ADDR, (u8 *)veal, len);
 	kfree(veal);
+	if (ret) {
+		dev_err(&adapter->pdev->dev,
+			"Unable to send ADD_ETH_ADDR message to PF, error %d\n", ret);
+		adapter->current_op = VIRTCHNL_OP_UNKNOWN;
+	}
+
+	return ret;
 }
 
 /**
@@ -712,8 +724,8 @@ static void iavf_mac_add_ok(struct iavf_adapter *adapter)
  * @adapter: adapter structure
  *
  * Remove filters from list based on PF response.
- **/
-static void iavf_mac_add_reject(struct iavf_adapter *adapter)
+ */
+void iavf_mac_add_reject(struct iavf_adapter *adapter)
 {
 	struct net_device *netdev = adapter->netdev;
 	struct iavf_mac_filter *f, *ftmp;
@@ -2364,7 +2376,6 @@ void iavf_virtchnl_completion(struct iavf_adapter *adapter,
 			iavf_mac_add_reject(adapter);
 			/* restore administratively set MAC address */
 			ether_addr_copy(adapter->hw.mac.addr, netdev->dev_addr);
-			wake_up(&adapter->vc_waitqueue);
 			break;
 		case VIRTCHNL_OP_DEL_ETH_ADDR:
 			dev_err(&adapter->pdev->dev, "Failed to delete MAC filter, error %s\n",
@@ -2555,7 +2566,6 @@ void iavf_virtchnl_completion(struct iavf_adapter *adapter,
 			eth_hw_addr_set(netdev, adapter->hw.mac.addr);
 			netif_addr_unlock_bh(netdev);
 		}
-		wake_up(&adapter->vc_waitqueue);
 		break;
 	case VIRTCHNL_OP_GET_STATS: {
 		struct iavf_eth_stats *stats =
@@ -2950,3 +2960,72 @@ void iavf_virtchnl_completion(struct iavf_adapter *adapter,
 	} /* switch v_opcode */
 	adapter->current_op = VIRTCHNL_OP_UNKNOWN;
 }
+
+/**
+ * iavf_poll_virtchnl_response - Poll admin queue for virtchnl response
+ * @adapter: adapter structure
+ * @event: pre-allocated event buffer to use for polling
+ * @condition: callback to check if desired response received
+ * @cond_data: context data passed to condition callback
+ * @timeout_ms: maximum time to wait in milliseconds
+ *
+ * Polls the admin queue and processes all incoming virtchnl messages.
+ * After processing each valid message, calls the condition callback to check
+ * if the expected response has been received. The callback receives the opcode
+ * of the processed message to identify which response was received. Continues
+ * polling until the callback returns true or timeout expires.
+ *
+ * Caller must allocate event buffer before sending any messages to PF to avoid
+ * state mismatch if allocation fails after message is sent.
+ *
+ * Caller must hold netdev_lock. This can sleep for up to timeout_ms while
+ * polling hardware.
+ *
+ * Return: 0 on success (condition met), -EAGAIN on timeout, or error code
+ */
+int iavf_poll_virtchnl_response(struct iavf_adapter *adapter,
+				struct iavf_arq_event_info *event,
+				bool (*condition)(struct iavf_adapter *adapter,
+						  const void *data,
+						  enum virtchnl_ops v_op),
+				const void *cond_data,
+				unsigned int timeout_ms)
+{
+	struct iavf_hw *hw = &adapter->hw;
+	enum virtchnl_ops received_op;
+	unsigned long timeout;
+	int ret = -EAGAIN;
+	u16 pending = 0;
+	u32 v_retval;
+
+	netdev_assert_locked(adapter->netdev);
+
+	timeout = jiffies + msecs_to_jiffies(timeout_ms);
+	do {
+		if (!pending)
+			usleep_range(50, 75);
+
+		if (iavf_clean_arq_element(hw, event, &pending) == IAVF_SUCCESS) {
+			received_op = (enum virtchnl_ops)le32_to_cpu(event->desc.cookie_high);
+			if (received_op != VIRTCHNL_OP_UNKNOWN) {
+				v_retval = le32_to_cpu(event->desc.cookie_low);
+
+				iavf_virtchnl_completion(adapter, received_op,
+							 (enum iavf_status)v_retval,
+							 event->msg_buf, event->msg_len);
+
+				if (condition(adapter, cond_data, received_op)) {
+					ret = 0;
+					break;
+				}
+			}
+
+			memset(event->msg_buf, 0, IAVF_MAX_AQ_BUF_SIZE);
+
+			if (pending)
+				continue;
+		}
+	} while (time_before(jiffies, timeout));
+
+	return ret;
+}
-- 
2.47.1

Re: [PATCH net 3/4] iavf: send MAC change request synchronously

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
iavf: send MAC change request synchronously

This converts iavf_set_mac() from queueing the ADD_ETH_ADDR request for the
watchdog and waiting on vc_waitqueue into sending the virtchnl message
directly and polling the admin receive queue for the reply. A new generic
helper iavf_poll_virtchnl_response() is added, iavf_add_ether_addrs() now
returns an error code, iavf_mac_add_reject() is exported for rollback, and
vc_waitqueue is removed.

A few questions about the new synchronous path below.

First, about the changelog text:

>     export iavf_mac_add_reject() to enable proper rollback on local failures
>     (timeouts, send errors) - PF rejections are already handled automatically
>     by iavf_virtchnl_completion().

[Severity: Low]

Is the "timeouts" part accurate? iavf_poll_virtchnl_response() returns
-EAGAIN on timeout, and iavf_set_mac() explicitly skips the rollback for
that value:

	if (ret != -EAGAIN) {
		iavf_mac_add_reject(adapter);
		ether_addr_copy(adapter->hw.mac.addr, netdev->dev_addr);
	}

So on a timeout nothing is rolled back, which is the opposite of what the
changelog describes (and matches the in-code comment). Could the changelog
be adjusted to say send errors only?

> diff --git a/drivers/net/ethernet/intel/iavf/iavf.h b/drivers/net/ethernet/intel/iavf/iavf.h
> index dc31202b2a94..8c45536fd502 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf.h
> +++ b/drivers/net/ethernet/intel/iavf/iavf.h

[ ... ]

> @@ -588,8 +587,9 @@ void iavf_configure_queues(struct iavf_adapter *adapter);
>  void iavf_enable_queues(struct iavf_adapter *adapter);
>  void iavf_disable_queues(struct iavf_adapter *adapter);
>  void iavf_map_queues(struct iavf_adapter *adapter);
> -void iavf_add_ether_addrs(struct iavf_adapter *adapter);
> +int iavf_add_ether_addrs(struct iavf_adapter *adapter);
>  void iavf_del_ether_addrs(struct iavf_adapter *adapter);
> +void iavf_mac_add_reject(struct iavf_adapter *adapter);

[Severity: Low]

Now that iavf_add_ether_addrs() reports -EBUSY, -ENOMEM and send failures,
should the watchdog side caller consume it too? iavf_process_aq_command()
still discards it and returns 0 unconditionally:

	if (adapter->aq_required & IAVF_FLAG_AQ_ADD_MAC_FILTER) {
		iavf_add_ether_addrs(adapter);
		return 0;
	}

which also disagrees with that function's own kernel-doc ("Returns error
code if no command was sent or error code if the command failed").

[ ... ]

> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
> index 630388e9d28c..3fa288e3798a 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> @@ -1029,6 +1029,60 @@ static bool iavf_is_mac_set_handled(struct net_device *netdev,
>  	return ret;
>  }
>  
> +/**
> + * iavf_mac_change_done - Check if MAC change completed
> + * @adapter: board private structure
> + * @data: MAC address being checked (as const void *)
> + * @v_op: virtchnl opcode from processed message
> + *
> + * Callback for iavf_poll_virtchnl_response() to check if MAC change completed.
> + *
> + * Return: true if MAC change completed, false otherwise
> + */
> +static bool iavf_mac_change_done(struct iavf_adapter *adapter,
> +				 const void *data, enum virtchnl_ops v_op)
> +{
> +	const u8 *addr = data;
> +
> +	return iavf_is_mac_set_handled(adapter->netdev, addr);
> +}

[Severity: Medium]

iavf_mac_change_done() ignores v_op and looks only at filter state, while
iavf_poll_virtchnl_response() calls the predicate after every message it
processes.

Consider setting an address that already has a filter whose earlier add
succeeded, so add_handled is true and is_new_mac is false.
iavf_replace_primary_mac() sets add = true but does not reset add_handled,
and iavf_add_ether_addrs() clears add again while building the batch:

iavf_is_mac_set_handled():
	f = iavf_find_filter(adapter, macaddr);

	if (!f || (!f->add && f->add_handled))
		ret = true;

Can an unrelated message processed by the poll loop, for example a
VIRTCHNL_OP_EVENT link change, then make this return true before the
ADD_ETH_ADDR reply arrives? netdev->dev_addr is only updated from
iavf_virtchnl_completion(), so iavf_set_mac() would return -EACCES for a
request that is still in flight and later applied.

The old code only re-evaluated the condition on
wake_up(&adapter->vc_waitqueue), which was issued exclusively from the
VIRTCHNL_OP_ADD_ETH_ADDR completion paths.

> +
> +/**
> + * iavf_set_mac_sync - Synchronously change MAC address
> + * @adapter: board private structure
> + * @addr: MAC address to set
> + *
> + * Send MAC change request to PF and poll admin queue for response.
> + * Caller must hold netdev_lock. This can sleep for up to 2.5 seconds.
> + * Event buffer is allocated before sending to avoid state mismatch if
> + * allocation fails after message is sent to PF.
> + *
> + * Return: 0 on success, negative on failure
> + */
> +static int iavf_set_mac_sync(struct iavf_adapter *adapter, const u8 *addr)
> +{
> +	struct iavf_arq_event_info event;
> +	int ret;
> +
> +	netdev_assert_locked(adapter->netdev);
> +
> +	event.buf_len = IAVF_MAX_AQ_BUF_SIZE;
> +	event.msg_buf = kzalloc(event.buf_len, GFP_KERNEL);
> +	if (!event.msg_buf)
> +		return -ENOMEM;
> +
> +	ret = iavf_add_ether_addrs(adapter);
> +	if (ret)
> +		goto out;
> +
> +	ret = iavf_poll_virtchnl_response(adapter, &event,
> +					  iavf_mac_change_done, addr, 2500);

[Severity: Medium]

Only one batch is sent here. iavf_add_ether_addrs() truncates the request
when it does not fit the admin queue buffer:

	len = virtchnl_struct_size(veal, list, count);
	if (len > IAVF_MAX_AQ_BUF_SIZE) {
		dev_warn(&adapter->pdev->dev, "Too many add MAC changes in one request\n");
		while (len > IAVF_MAX_AQ_BUF_SIZE)
			len = virtchnl_struct_size(veal, list, --count);
		more = true;
	}

The remaining filters keep f->add == true and IAVF_FLAG_AQ_ADD_MAC_FILTER
stays set, and iavf_add_filter() appends new filters to the tail of
mac_filter_list. If the just requested primary address ends up outside the
first batch, can iavf_mac_change_done() ever become true, given
iavf_is_mac_set_handled() requires !f->add? The poll loop has no way to send
the next batch, so this would burn the full 2500 ms and return -EAGAIN to
userspace while the watchdog applies the change afterwards.

> +
> +out:
> +	kfree(event.msg_buf);
> +	return ret;
> +}
> +
>  /**
>   * iavf_set_mac - NDO callback to set port MAC address
>   * @netdev: network interface device structure
> @@ -1049,25 +1103,23 @@ static int iavf_set_mac(struct net_device *netdev, void *p)
>  		return -EADDRNOTAVAIL;
>  
>  	ret = iavf_replace_primary_mac(adapter, addr->sa_data);
> -
>  	if (ret)
>  		return ret;
>  
> -	ret = wait_event_interruptible_timeout(adapter->vc_waitqueue,
> -					       iavf_is_mac_set_handled(netdev, addr->sa_data),
> -					       msecs_to_jiffies(2500));
> -
> -	/* If ret < 0 then it means wait was interrupted.
> -	 * If ret == 0 then it means we got a timeout.
> -	 * else it means we got response for set MAC from PF,
> -	 * check if netdev MAC was updated to requested MAC,
> -	 * if yes then set MAC succeeded otherwise it failed return -EACCES
> -	 */
> -	if (ret < 0)
> +	ret = iavf_set_mac_sync(adapter, addr->sa_data);
> +	if (ret) {
> +		/* Rollback only if send failed (message never reached PF).
> +		 * Don't rollback on timeout (-EAGAIN) because the message was
> +		 * sent and PF will eventually respond. When the response arrives,
> +		 * iavf_virtchnl_completion() will handle rollback (on PF error)
> +		 * or acceptance (on PF success) automatically.
> +		 */
> +		if (ret != -EAGAIN) {
> +			iavf_mac_add_reject(adapter);
> +			ether_addr_copy(adapter->hw.mac.addr, netdev->dev_addr);
> +		}

[Severity: High]

Can this rollback destroy filters that belong to a different, still
outstanding request?

iavf_mac_add_reject() is a completion time helper that walks the whole list:

	list_for_each_entry_safe(f, ftmp, &adapter->mac_filter_list, list) {
		if (f->remove && ether_addr_equal(f->macaddr, netdev->dev_addr))
			f->remove = false;

		if (!f->add && !f->add_handled)
			f->add_handled = true;

		if (f->is_new_mac) {
			list_del(&f->list);
			kfree(f);
		}
	}

One way to reach it with ret == -EBUSY is an ADD_ETH_ADDR batch the watchdog
already sent and whose reply has not been processed yet. Those filters are
exactly in state add == false, is_new_mac == true (is_new_mac is only
cleared by iavf_mac_add_ok() on a successful reply), so they are freed here
while the PF still has the adds pending.

Newly queued unicast/multicast filters from iavf_addr_sync() are in the same
boat, since iavf_add_filter() sets:

	f->add = true;
	f->add_handled = false;
	f->is_new_mac = true;

The netdev core already considers those addresses synced, so once they are
dropped here nothing programs or retries them until a reset. Should the
rollback be scoped to the address that iavf_set_mac() itself queued?

[Severity: Medium]

Does the -EAGAIN assumption in the comment above ("the message was sent and
PF will eventually respond") hold when IAVF_FLAG_PF_COMMS_FAILED is set?
iavf_send_pf_msg() returns success without posting anything in that case:

	if (adapter->flags & IAVF_FLAG_PF_COMMS_FAILED)
		return 0; /* nothing to see here, move along */

iavf_add_ether_addrs() then reports 0 while current_op stays
VIRTCHNL_OP_ADD_ETH_ADDR and f->add has already been cleared, so the poll
spins for the full 2.5 s under rtnl_lock plus the netdev instance lock,
returns -EAGAIN, and no rollback runs. adapter->hw.mac.addr and the filter
bookkeeping then diverge from netdev->dev_addr, and current_op stays pending
so later virtchnl requests hit "Cannot add filters, command %d pending".

iavf_disable_vf() sets IAVF_FLAG_PF_COMMS_FAILED, clears
IAVF_FLAG_RESET_PENDING and moves the state to __IAVF_DOWN, so the guard at
the top of iavf_set_mac() still passes in that window:

	if (iavf_is_reset_in_progress(adapter) || adapter->state < __IAVF_DOWN)
		return -EBUSY;

>  		return ret;
> -
> -	if (!ret)
> -		return -EAGAIN;
> +	}
>  
>  	if (!ether_addr_equal(netdev->dev_addr, addr->sa_data))
>  		return -EACCES;

[ ... ]

> diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> index ec234cc8bd9d..e6b7e8f82c7c 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c

[ ... ]

> @@ -555,20 +556,23 @@ iavf_set_mac_addr_type(struct virtchnl_ether_addr *virtchnl_ether_addr,
>   * @adapter: adapter structure
>   *
>   * Request that the PF add one or more addresses to our filters.
> - **/
> -void iavf_add_ether_addrs(struct iavf_adapter *adapter)
> + *
> + * Return: 0 on success, negative on failure
> + */
> +int iavf_add_ether_addrs(struct iavf_adapter *adapter)
>  {
>  	struct virtchnl_ether_addr_list *veal;
>  	struct iavf_mac_filter *f;
>  	int i = 0, count = 0;
>  	bool more = false;
>  	size_t len;
> +	int ret;
>  
>  	if (adapter->current_op != VIRTCHNL_OP_UNKNOWN) {
>  		/* bail because we already have a command pending */
>  		dev_err(&adapter->pdev->dev, "Cannot add filters, command %d pending\n",
>  			adapter->current_op);
> -		return;
> +		return -EBUSY;
>  	}

[Severity: Medium]

Is this -EBUSY reachable as a hard failure from the new caller?

iavf_set_mac() runs with the netdev instance lock held, and the only context
that clears current_op is iavf_virtchnl_completion() as called from
iavf_adminq_task(), which starts with netdev_lock(netdev). The watchdog does
the same. So while iavf_set_mac() holds the lock, an outstanding command's
reply cannot be consumed and current_op cannot go back to
VIRTCHNL_OP_UNKNOWN.

iavf_set_mac_sync() sends once and gives up:

	ret = iavf_add_ether_addrs(adapter);
	if (ret)
		goto out;

Before this change iavf_set_mac() only set IAVF_FLAG_AQ_ADD_MAC_FILTER and
the watchdog retried the send after the previous command completed, so the
operation succeeded. Can "ip link set dev X address ..." or bond_enslave()
now fail with -EBUSY whenever the watchdog has a command in flight, for
instance the VIRTCHNL_OP_GET_STATS request it sends every couple of
watchdog ticks?

[ ... ]

> @@ -615,8 +620,15 @@ void iavf_add_ether_addrs(struct iavf_adapter *adapter)
>  
>  	spin_unlock_bh(&adapter->mac_vlan_list_lock);
>  
> -	iavf_send_pf_msg(adapter, VIRTCHNL_OP_ADD_ETH_ADDR, (u8 *)veal, len);
> +	ret = iavf_send_pf_msg(adapter, VIRTCHNL_OP_ADD_ETH_ADDR, (u8 *)veal, len);
>  	kfree(veal);
> +	if (ret) {
> +		dev_err(&adapter->pdev->dev,
> +			"Unable to send ADD_ETH_ADDR message to PF, error %d\n", ret);
> +		adapter->current_op = VIRTCHNL_OP_UNKNOWN;
> +	}
> +
> +	return ret;
>  }
>  
>  /**
> @@ -712,8 +724,8 @@ static void iavf_mac_add_ok(struct iavf_adapter *adapter)
>   * @adapter: adapter structure
>   *
>   * Remove filters from list based on PF response.
> - **/
> -static void iavf_mac_add_reject(struct iavf_adapter *adapter)
> + */
> +void iavf_mac_add_reject(struct iavf_adapter *adapter)
>  {
>  	struct net_device *netdev = adapter->netdev;
>  	struct iavf_mac_filter *f, *ftmp;

[Severity: Medium]

Is this helper able to roll back a filter whose is_new_mac is already
cleared?

Setting an address that was programmed successfully before leaves
is_new_mac == false, and iavf_replace_primary_mac() then does:

	old_f->is_primary = false;
	old_f->remove = true;
	...
	new_f->is_primary = true;
	new_f->add = true;

If iavf_set_mac_sync() fails locally before the batch is built (-EBUSY from
the current_op check, or -ENOMEM from the veal kzalloc), both branches in
iavf_mac_add_reject() skip that entry: f->add is still true, and
f->is_new_mac is false. So f->add and IAVF_FLAG_AQ_ADD_MAC_FILTER survive,
and the watchdog later sends ADD_ETH_ADDR for the address whose change was
just reported as failed. Since f->is_primary is still set,
iavf_set_mac_addr_type() marks it as primary:

	virtchnl_ether_addr->type = filter->is_primary ?
		VIRTCHNL_ETHER_ADDR_PRIMARY :
		VIRTCHNL_ETHER_ADDR_EXTRA;

Does the PF then end up with a primary MAC that differs from
netdev->dev_addr, with no filter marked primary for the address the
interface is actually using, since old_f->is_primary is never restored?

[ ... ]

> @@ -2950,3 +2960,72 @@ void iavf_virtchnl_completion(struct iavf_adapter *adapter,
>  	} /* switch v_opcode */
>  	adapter->current_op = VIRTCHNL_OP_UNKNOWN;
>  }
> +
> +/**
> + * iavf_poll_virtchnl_response - Poll admin queue for virtchnl response

[ ... ]

> + * Return: 0 on success (condition met), -EAGAIN on timeout, or error code
> + */
> +int iavf_poll_virtchnl_response(struct iavf_adapter *adapter,
> +				struct iavf_arq_event_info *event,
> +				bool (*condition)(struct iavf_adapter *adapter,
> +						  const void *data,
> +						  enum virtchnl_ops v_op),
> +				const void *cond_data,
> +				unsigned int timeout_ms)
> +{
> +	struct iavf_hw *hw = &adapter->hw;
> +	enum virtchnl_ops received_op;
> +	unsigned long timeout;
> +	int ret = -EAGAIN;
> +	u16 pending = 0;
> +	u32 v_retval;
> +
> +	netdev_assert_locked(adapter->netdev);
> +
> +	timeout = jiffies + msecs_to_jiffies(timeout_ms);
> +	do {
> +		if (!pending)
> +			usleep_range(50, 75);
> +
> +		if (iavf_clean_arq_element(hw, event, &pending) == IAVF_SUCCESS) {

[Severity: Medium]

The kernel-doc promises "or error code", but ret is only ever -EAGAIN or 0,
and the iavf_clean_arq_element() status is dropped here. The sibling helper
in this file does convert it:

iavf_poll_virtchnl_msg():
	status = iavf_clean_arq_element(hw, event, NULL);
	if (status != IAVF_SUCCESS)
		return iavf_status_to_errno(status);

For the IAVF_ERR_ADMIN_QUEUE_ERROR case, iavf_clean_arq_element() has
already consumed the descriptor, re-posted it and advanced IAVF_VF_ARQT1:

	flags = le16_to_cpu(desc->flags);
	if (flags & LIBIE_AQ_FLAG_ERR) {
		ret_code = IAVF_ERR_ADMIN_QUEUE_ERROR;

so the awaited reply no longer exists. Should that be reported instead of
spinning to the -EAGAIN timeout, which iavf_set_mac() reads as "still
outstanding, the PF will respond later" and therefore skips the rollback,
leaving current_op pending?

[Severity: High]

What happens to this loop if a VFR/EMPR lands while it is polling? The other
ARQ consumer checks for that explicitly:

iavf_adminq_task():
	if (iavf_is_reset_in_progress(adapter))
		goto freedom;

	/* check for error indications */
	val = rd32(hw, IAVF_VF_ARQLEN1);
	if (val == 0xdeadbeef || val == 0xffffffff) /* device in reset */
		goto freedom;

and it also clears the sticky ARQVFE/ARQOVFL/ARQCRIT bits. None of that
happens here, and iavf_clean_arq_element() takes the producer index straight
from the register without validating it:

	ntu = rd32(hw, IAVF_VF_ARQH1) & IAVF_VF_ARQH1_ARQH_MASK;
	if (ntu == ntc) {

With the reset sentinels, 0xdeadbeef & 0x3FF is 751 and 0xffffffff & 0x3FF
is 1023, both well above hw->aq.num_arq_entries and never equal to
next_to_clean, so IAVF_ERR_ADMIN_QUEUE_NO_WORK is not returned. Can this
then "clean" descriptors the hardware never produced, rewrite them, do
wr32(hw, IAVF_VF_ARQT1, ntc) on a disabled queue and advance
next_to_clean/next_to_use?

*pending would also come back as hundreds, so the throttle is skipped:

	if (!pending)
		usleep_range(50, 75);
	...
		if (pending)
			continue;

Does that busy-spin on MMIO for the remaining 2.5 s while holding the netdev
instance lock and the caller's rtnl_lock, which is what iavf_reset_task()
needs to recover? Note the loop itself can process
VIRTCHNL_EVENT_RESET_IMPENDING, which only schedules the reset and does not
stop the polling.

> +			received_op = (enum virtchnl_ops)le32_to_cpu(event->desc.cookie_high);
> +			if (received_op != VIRTCHNL_OP_UNKNOWN) {
> +				v_retval = le32_to_cpu(event->desc.cookie_low);
> +
> +				iavf_virtchnl_completion(adapter, received_op,
> +							 (enum iavf_status)v_retval,
> +							 event->msg_buf, event->msg_len);
> +
> +				if (condition(adapter, cond_data, received_op)) {
> +					ret = 0;
> +					break;
> +				}
> +			}
> +
> +			memset(event->msg_buf, 0, IAVF_MAX_AQ_BUF_SIZE);

[Severity: Low]

Should this use event->buf_len rather than the hardcoded
IAVF_MAX_AQ_BUF_SIZE? The buffer is caller owned and its size is described
by event->buf_len, which iavf_clean_arq_element() honours:

	e->msg_len = min(datalen, e->buf_len);

Today's only caller allocates exactly IAVF_MAX_AQ_BUF_SIZE so nothing is
overwritten, but the helper is exported in iavf.h as generic, and this file
already has ARQ polling callers with small buffers, e.g.:

	event.msg_buf = (u8 *)&rxdids;
	event.buf_len = sizeof(rxdids);

Such a caller would get a 4096 byte memset of a few bytes of storage.

> +
> +			if (pending)
> +				continue;
> +		}
> +	} while (time_before(jiffies, timeout));
> +
> +	return ret;
> +}

[PATCH net 4/4] ice: skip unnecessary VF reset when setting trust

[Tony Nguyen]

From: Jose Ignacio Tornos Martinez <jtornosm@redhat.com>

Similar to the i40e fix, ice_set_vf_trust() unconditionally calls
ice_reset_vf() when the trust setting changes. While the delay is smaller
than i40e, this reset is still unnecessary in most cases.

When granting trust, no reset is needed - we can just set the capability
flag to allow privileged operations.

When revoking trust, we only need to reset (conservative approach) if
the VF has actually configured advanced features that require cleanup
(MAC LLDP filters, promiscuous mode). For VFs in a clean state, we can
safely change the trust setting without the disruptive reset.

When we do reset, we maintain the original ice pattern that has been
reliable in production: cleanup LLDP filters first, then set vf->trusted,
then reset. This ensures the privilege capability bit is handled correctly
during reset rebuild.

When we don't reset, we manually handle the capability flag via helper
function, eliminating the delay.

Signed-off-by: Jose Ignacio Tornos Martinez <jtornosm@redhat.com>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_sriov.c | 35 ++++++++++++++++++----
 1 file changed, 30 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.c b/drivers/net/ethernet/intel/ice/ice_sriov.c
index e04de0215596..558937899818 100644
--- a/drivers/net/ethernet/intel/ice/ice_sriov.c
+++ b/drivers/net/ethernet/intel/ice/ice_sriov.c
@@ -1366,6 +1366,23 @@ int ice_set_vf_mac(struct net_device *netdev, int vf_id, u8 *mac)
 	return __ice_set_vf_mac(ice_netdev_to_pf(netdev), vf_id, mac);
 }
 
+/**
+ * ice_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 ice_setup_vf_trust(struct ice_vf *vf, bool setting)
+{
+	if (setting)
+		set_bit(ICE_VIRTCHNL_VF_CAP_PRIVILEGE, &vf->vf_caps);
+	else
+		clear_bit(ICE_VIRTCHNL_VF_CAP_PRIVILEGE, &vf->vf_caps);
+}
+
 /**
  * ice_set_vf_trust
  * @netdev: network interface device structure
@@ -1401,11 +1418,19 @@ int ice_set_vf_trust(struct net_device *netdev, int vf_id, bool trusted)
 
 	mutex_lock(&vf->cfg_lock);
 
-	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))) {
+		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);
+	}
 	dev_info(ice_pf_to_dev(pf), "VF %u is now %strusted\n",
 		 vf_id, trusted ? "" : "un");
 
-- 
2.47.1

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

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
    ice: skip unnecessary VF reset when setting trust

The patch makes ice_set_vf_trust() skip the VF reset in most cases: granting
trust only sets the ICE_VIRTCHNL_VF_CAP_PRIVILEGE bit, and revoking trust
only resets when vf->num_mac_lldp is non-zero or one of the promiscuous
state bits is set.  A new helper, ice_setup_vf_trust(), toggles the
capability bit for the no-reset path.

A few questions about the new revocation logic below.

> diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.c b/drivers/net/ethernet/intel/ice/ice_sriov.c
> index e04de0215596..558937899818 100644
> --- a/drivers/net/ethernet/intel/ice/ice_sriov.c
> +++ b/drivers/net/ethernet/intel/ice/ice_sriov.c
> @@ -1366,6 +1366,23 @@ int ice_set_vf_mac(struct net_device *netdev, int vf_id, u8 *mac)
>  	return __ice_set_vf_mac(ice_netdev_to_pf(netdev), vf_id, mac);
>  }
>  
> +/**
> + * ice_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 ice_setup_vf_trust(struct ice_vf *vf, bool setting)
> +{
> +	if (setting)
> +		set_bit(ICE_VIRTCHNL_VF_CAP_PRIVILEGE, &vf->vf_caps);
> +	else
> +		clear_bit(ICE_VIRTCHNL_VF_CAP_PRIVILEGE, &vf->vf_caps);
> +}

[Severity: Low]
This isn't a bug, but doesn't ice_vf_lib.c already have a helper with
exactly these semantics?

drivers/net/ethernet/intel/ice/ice_vf_lib.c:
static void ice_vf_set_host_trust_cfg(struct ice_vf *vf)
{
	assign_bit(ICE_VIRTCHNL_VF_CAP_PRIVILEGE, &vf->vf_caps, vf->trusted);
}

It is called from ice_vf_rebuild_host_cfg() on the reset path.  Would it be
preferable to export that one (ice_vf_lib_private.h already declares
ice_is_vf_trusted()) rather than carry a second copy of the rule that
derives ICE_VIRTCHNL_VF_CAP_PRIVILEGE from vf->trusted in a different file?

>  /**
>   * ice_set_vf_trust
>   * @netdev: network interface device structure
> @@ -1401,11 +1418,19 @@ int ice_set_vf_trust(struct net_device *netdev, int vf_id, bool trusted)
>  
>  	mutex_lock(&vf->cfg_lock);
>  
> -	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: High]
Is this predicate complete with respect to everything trust gates in ice?
Besides LLDP filters and promiscuous mode, vf->trusted also gates the MAC
and VLAN filter quotas and the administratively assigned MAC:

drivers/net/ethernet/intel/ice/virt/virtchnl.c:ice_can_vf_change_mac() {
	if (vf->pf_set_mac && !ice_is_vf_trusted(vf))
		return false;
}

drivers/net/ethernet/intel/ice/virt/virtchnl.c:ice_vc_handle_mac_addr_msg() {
	if (set && !ice_is_vf_trusted(vf) &&
	    (vf->num_mac + al->num_elements) > ICE_MAX_MACADDR_PER_VF) {
}

drivers/net/ethernet/intel/ice/virt/virtchnl.c:ice_vf_has_max_vlans() {
	if (ice_is_vf_trusted(vf))
		return false;
}

So a guest that was trusted can have installed extra unicast/multicast MAC
filters, a MAC that overrides the pf_set_mac address, and more than
ICE_MAX_VLAN_PER_VF VLANs.  None of that shows up in num_mac_lldp or the
promisc bits, so with those clear, "ip link set <pf> vf N trust off" takes
the new else branch.

Do those hardware filters then stay programmed after the log prints "VF N is
now untrusted"?  Previously the unconditional reset reached:

drivers/net/ethernet/intel/ice/ice_vf_lib.c:ice_vf_reconfig_vsi() {
	ice_vsi_decfg(vsi);
	ice_fltr_remove_all(vsi);
}

followed by ice_vf_rebuild_host_cfg(), which re-adds only broadcast plus the
host-sanctioned MAC/VLAN config.

The software counters look affected too.  They are cleared only on the reset
path:

drivers/net/ethernet/intel/ice/ice_vf_lib.c:ice_vf_clear_counters() {
	if (vsi)
		vsi->num_vlan = 0;

	vf->num_mac = 0;
	vf->num_mac_lldp = 0;
}

Can vf->num_mac therefore remain above ICE_MAX_MACADDR_PER_VF after trust is
revoked, so that later legitimate MAC adds from the now-untrusted VF are
rejected until some unrelated reset happens?

Relatedly, the kernel-doc on ice_setup_vf_trust() says it "is only called
when it's safe to skip the reset (VF has no advanced features configured
that need cleanup)", and the commit message says the features needing cleanup
are "(MAC LLDP filters, promiscuous mode)".  Should both mention the MAC and
VLAN filter state as well?

> +		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 capabilities when trust changes
without a reset?  The advertised limit is derived from vf->trusted once, at
negotiation time, and then cached on the PF:

drivers/net/ethernet/intel/ice/virt/virtchnl.c:ice_vc_get_max_vlan_fltrs() {
	if (vf->trusted)
		return VLAN_N_VID;
	else
		return ICE_MAX_VLAN_PER_VF;
}

drivers/net/ethernet/intel/ice/virt/virtchnl.c:ice_vc_get_offload_vlan_v2_caps() {
	/* store negotiated caps to prevent invalid VF messages */
	memcpy(&vf->vlan_v2_caps, caps, sizeof(*caps));
}

VLAN V2 adds are validated against that snapshot only:

drivers/net/ethernet/intel/ice/virt/virtchnl.c:ice_vc_validate_add_vlan_filter_list() {
	if (num_requested_filters > vfc->max_filters)
		return false;
}

and the snapshot is invalidated only on the reset path:

drivers/net/ethernet/intel/ice/ice_vf_lib.c:ice_vf_set_initialized() {
	memset(&vf->vlan_v2_caps, 0, sizeof(vf->vlan_v2_caps));
}

If the VF negotiated VIRTCHNL_VF_OFFLOAD_VLAN_V2 while trusted, does the
else branch leave max_filters at VLAN_N_VID, allowing the now-untrusted VF
to keep programming VLAN filters well beyond ICE_MAX_VLAN_PER_VF?

In the other direction, after granting trust without a reset both the PF
cache and the guest cache still hold 8:

drivers/net/ethernet/intel/iavf/iavf_main.c:iavf_get_max_vlans_allowed() {
	else if (VLAN_V2_ALLOWED(adapter))
		return adapter->vlan_v2_caps.filtering.max_filters;
}

so is the commit message statement "When granting trust, no reset is needed
- we can just set the capability flag to allow privileged operations"
accurate for VLAN filtering, where the privilege only becomes usable after
renegotiation?

[Severity: Medium]
This isn't a bug introduced by this patch, but while ice_set_vf_trust() is
being touched: the switchdev check earlier in this same function returns
without releasing the VF reference taken by ice_get_vf_by_id():

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

Every other exit uses "goto out_put_vf;" so that ice_put_vf() drops the
kref_get_unless_zero() reference.  Since the leaked reference keeps the
refcount above zero, ice_release_vf() never runs, so neither
pci_dev_put(vf->vfdev) nor the kfree_rcu() of the ice_vf ever happens when
SR-IOV is disabled or the driver is unbound.  Would it make sense to convert
that return into "goto out_put_vf;" here?