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

[PATCH net v2 0/3][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. Eliminates unnecessary VF reset when setting trust in i40e (reset only
   if revoking trust and VF has advanced features configured).
2. Fixes lock contention by polling admin queue synchronously
3. Eliminates unnecessary VF reset when setting trust in ice, (reset only
   if revoking trust and VF has advanced features configured).

The key fix (patch 2/3) 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.
---
v2: 
- Drop patch "iavf: return EBUSY if reset in progress or not ready during MAC change"
- Convert set_bit/clear bit pattern to assign_bit

v1: https://lore.kernel.org/netdev/20260821204537.2189112-1-anthony.l.nguyen@intel.com/

Split off from this submission:
https://lore.kernel.org/netdev/20260804222205.1580328-1-anthony.l.nguyen@intel.com/

The following are changes since commit a7bfaba4823e3c165bb2004c74eff7c096672bc7:
  ipv6: fix prefix route expiry in modify_prefix_route()
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 (3):
  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    | 35 +++++--
 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 +++++++++++++++++--
 drivers/net/ethernet/intel/ice/ice_sriov.c    | 32 +++++-
 5 files changed, 217 insertions(+), 45 deletions(-)

-- 
2.47.1

[PATCH net v2 1/3] 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>
---
Notes:
    All the comments below fall into pre-existing issues, concerns already
    addressed in previous versions and comments, out-of-scope items, or extreme
    edge cases. No code changes are considered necessary for a new version.
    Details for each comment:

    > [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.
    >
    > 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?

    Pre-existing race. The original code writes vf->trusted before calling
    i40e_vc_reset_vf() without synchronization against VFLR-initiated resets
    from the service task. This patch does not change the synchronization
    model and the race window is the same order of magnitude (a few
    instructions). The scenario requires a guest-initiated VFLR at the exact
    moment of the trust change, extremely difficult to reproduce in practice.

    > [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. Nothing revalidates filters that are
    > already installed when the privilege bit is dropped.
    >
    > Can the __I40E_MACVLAN_SYNC_PENDING work repair this instead? Looking at
    > i40e_correct_vf_mac_vlan_filters() it only recomputes the VLAN id of
    > existing entries, so no MAC filter is deleted on trust loss.
    >
    > There is also a functional side effect: mac_add_max drops back to 18 while
    > i40e_count_active_filters(vsi) still reflects the trusted-era filters, so
    > every later VIRTCHNL_OP_ADD_ETH_ADDR from that VF fails with -EPERM,
    > including a re-add of its primary MAC after a guest link down/up.

    Same concern addressed in previous comments. Over-limit filters configured
    while trusted remain after trust revocation, but this is acceptable because
    untrusted VFs can freely delete their own MAC and VLAN filters, there are
    no trust checks in i40e_vc_del_mac_addr_msg() or i40e_vc_remove_vlan_msg().
    The VF simply cannot add more over-limit filters.

    The permission check uses i40e_count_active_filters(vsi) which counts live
    filters, deletions reduce the count immediately. The "primary MAC re-add
    after link down/up" scenario does not apply: existing filters remain in
    place across link events. A guest-initiated VF reset cleans up everything
    via i40e_free_vf_res().

    > [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 service task context without rtnl_lock() and
    > without taking __I40E_VIRTCHNL_OP_PENDING, so it does not exclude this ndo.
    >
    > Does that leave an untrusted VF promiscuous indefinitely? The same ordering
    > appears in i40e_vc_add_cloud_filter() and i40e_vc_add_qch_msg().

    Same concern addressed in previous comments. This race condition exists in
    the original code as well, vf->trusted is set before i40e_vc_reset_vf(),
    creating the same window where the VF can install privileged state while
    the capability bit is still set. This patch does not introduce this race,
    it inherits the same synchronization model. Extremely difficult to
    reproduce in practice.

    Fixing this requires changing the broader synchronization between ndo
    callbacks and virtchnl processing, which is beyond the scope of this patch.

    > [Severity: Medium]
    > This is a pre-existing issue and not introduced by this patch, but
    > i40e_vc_reset_vf() is void and can return having done nothing.
    >
    > Would calling i40e_setup_vf_trust(vf, setting) unconditionally, before the
    > branch, make both artifacts deterministic?

    Pre-existing issue not introduced by this patch, as the reviewer correctly
    identifies. The current structure keeps the logic clear: the else branch
    handles the no-reset case, the if branch delegates everything to the reset
    path.

    > [Severity: Medium]
    > The reset releases the ADQ channel VSIs and i40e_alloc_vf_res() re-creates
    > them with newly assigned seids. i40e_del_all_cloud_filters() then looks
    > the VSI up by the recorded seid. If the seid changed, the teardown takes
    > the error path and the hlist_del(), kfree(cfilter) and
    > vf->num_cloud_filters decrement are all skipped. Does this leak the
    > struct i40e_cloud_filter allocations?

    Pre-existing issue, the same ordering (reset before cloud filter deletion)
    existed in the original code. This patch restructures the control flow
    but preserves the same sequence.

 .../ethernet/intel/i40e/i40e_virtchnl_pf.c    | 35 +++++++++++++------
 1 file changed, 25 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..c6732a24b640 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
@@ -4943,6 +4943,20 @@ 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)
+{
+	assign_bit(I40E_VIRTCHNL_VF_CAP_PRIVILEGE, &vf->vf_caps, setting);
+}
+
 /**
  * i40e_ndo_set_vf_trust
  * @netdev: network interface device structure of the pf
@@ -4987,19 +5001,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 v2 1/3] i40e: skip unnecessary VF reset when setting trust

[netdev-bot+sashiko]

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

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

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] When trust is revoked, filters the VF could only install while
  trusted are no longer removed.
- [Medium] The revoke path has a race (time-of-check/time-of-use) with
  virtchnl handlers and no longer has the reset to clean up after it.
- [Medium] The privilege bit can be lost to a concurrent service-task VF
  reset.

Pre-existing issues:
- [Medium] In the revoke-with-reset branch, CAP_PRIVILEGE is cleared (by
  i40e_alloc_vf_res()) and hardware promisc is dropped (by…
- [Medium] In the revoke reset branch, i40e_del_all_cloud_filters() runs
  after i40e_vc_reset_vf() has released the ADQ channel VSIs…
- [Low] Granting trust does not apply a promiscuous-mode request the PF
  earlier acknowledged but ignored.

--

i40e: skip unnecessary VF reset when setting trust

This commit makes i40e_ndo_set_vf_trust() skip the VF reset in two cases:
when granting trust, and when revoking trust from a VF that has no ADQ,
cloud filters or promiscuous mode configured. In those cases a new
helper, i40e_setup_vf_trust(), only updates I40E_VIRTCHNL_VF_CAP_PRIVILEGE.
The goal is to avoid the ~10 second reset delay during bonding setup.

> diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> index a26c3d47ec156..c6732a24b6400 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
> @@ -4943,6 +4943,20 @@ 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)
> +{
> +	assign_bit(I40E_VIRTCHNL_VF_CAP_PRIVILEGE, &vf->vf_caps, setting);

[Severity: Medium]
Can a VF reset running at the same time in the service task undo this
write?

A reset from a VFLR (i40e_vc_process_vflr_event()), from
VIRTCHNL_OP_RESET_VF or from a request for more queues runs
i40e_alloc_vf_res(). That function reads vf->trusted and then writes the
same bit:

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

Suppose i40e_ndo_set_vf_trust() sets vf->trusted = false and calls
i40e_setup_vf_trust() after that read but before the set_bit(). The bit
then stays set while vf->trusted is false. On the grant path the result
can be the reverse.

Before this patch, the ndo always called i40e_vc_reset_vf(), and
i40e_reset_vf() returns false while the other reset holds the flag:

	if (test_and_set_bit(I40E_VF_STATE_RESETTING, &vf->vf_states))
		return false;

So the ndo retried and ran a new reset, which recomputed the bit from the
final value of vf->trusted.

The notes on this version call this race pre-existing, but that
follow-up reset used to fix it up. Now that the reset is skipped, does
anything bring vf->trusted and vf_caps back in line?

> +}
> +
>  /**
>   * i40e_ndo_set_vf_trust
>   * @netdev: network interface device structure of the pf

[ ... ]

> @@ -4987,19 +5001,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]
Should this condition also cover MAC and VLAN filters that the VF could
only add because it was trusted?

Trust is only checked when a filter is added. In
i40e_check_vf_permission(), a trusted VF skips the admin MAC restriction
and gets a higher limit:

	if (!vf_trusted && !is_multicast_ether_addr(addr) &&
	    vf->pf_set_mac && !ether_addr_equal(addr, vf->default_lan_addr.addr)) {
	...
	if (!vf_trusted)
		mac_add_max = I40E_VC_MAX_MAC_ADDR_PER_VF;
	else
		mac_add_max = I40E_VC_MAX_MACVLAN_PER_TRUSTED_VF(pf->num_alloc_vfs, hw->num_ports);

i40e_vc_add_vlan_msg() only applies the VLAN limit when the bit is
clear:

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

Before this patch, the reset went through
i40e_reset_vf()->i40e_cleanup_reset_vf()->i40e_free_vf_res()->
i40e_vsi_release(). That removed every filter on the VSI and set
vf->num_vlan = 0, so the VF had to add its filters again under the
untrusted limits.

Now a VF with no ADQ, no cloud filters and no promisc bits takes the
else branch, which only clears the bit. These filters stay in hardware:

  - extra unicast MACs that differ from the admin-set MAC
  - MAC filters beyond I40E_VC_MAX_MAC_ADDR_PER_VF (18)
  - VLANs beyond I40E_VC_MAX_VLAN_PER_VF (16)

The VF keeps receiving on them. With spoofchk on, it can also still send
from those MACs.

The __I40E_MACVLAN_SYNC_PENDING work only reaches
i40e_correct_vf_mac_vlan_filters(). That function rewrites the VLAN ids
of existing entries and never deletes a MAC filter.

There is also a functional side effect. When mac_add_max drops back to
18 while i40e_count_active_filters(vsi) is still above 18, this check
fails even when mac_add_cnt is 0:

	if ((i40e_count_active_filters(vsi) + mac_add_cnt) > mac_add_max ||

Does every later VIRTCHNL_OP_ADD_ETH_ADDR from the VF then fail with
-EPERM, for example a new multicast subscription, until the guest
deletes filters itself?

The notes on this version say this is fine because an untrusted VF can
delete its own filters. Doesn't that rely on the guest cooperating, which
can't be assumed when the admin revokes trust?

The bonding case this patch targets is also a typical case for extra MACs
being installed. Are the "clean state" wording in the commit message and
the "no advanced features configured that need cleanup" wording in the
i40e_setup_vf_trust() kernel-doc accurate for that case?

[Severity: Medium]
Can this unlocked check race with a promisc request that is still in
progress?

i40e_vc_config_promiscuous_mode_msg() runs from the service task through
i40e_clean_adminq_subtask()->i40e_vc_process_vf_msg(). It takes neither
rtnl nor __I40E_VIRTCHNL_OP_PENDING. It checks the privilege bit first,
then sends AdminQ commands, and only sets the state bits at the end:

	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 (!test_and_set_bit(I40E_VF_STATE_UC_PROMISC,
				      &vf->vf_states))

A possible interleaving:

service task                          i40e_ndo_set_vf_trust()
privilege test_bit() passes
i40e_config_vf_promiscuous_mode()
  (AdminQ round trip)                 vf->trusted = false
                                      UC/MC_PROMISC still clear,
                                      reset skipped
                                      i40e_setup_vf_trust() clears
                                      the privilege bit
test_and_set_bit(UC_PROMISC)

Would the VF then stay promiscuous while untrusted, with nothing left to
undo it? The window spans a sleeping AdminQ round trip, and the guest can
send promisc requests over and over.

The notes on this version call this race pre-existing. Before this patch,
though, the reset always ran after the vf->trusted write, and
i40e_cleanup_reset_vf() turned promisc off and released the VSI:

	/* disable promisc modes in case they were enabled */
	i40e_config_vf_promiscuous_mode(vf, vf->lan_vsi_id, false, false);

That undid anything set up during the window. The no-reset path has no
such fallback.

> +		i40e_vc_reset_vf(vf, true);

[Severity: Medium]
This is a pre-existing issue and not introduced by this patch, but
i40e_vc_reset_vf() returns void and can return without doing a reset:

  - it returns early if __I40E_VFS_RELEASING is set
  - i40e_reset_vf() returns true without resetting if
    __I40E_VF_RESETS_DISABLED or __I40E_VF_DISABLE is set
  - the loop gives up after 20 tries while I40E_VF_STATE_RESETTING is
    held

In this branch, only i40e_alloc_vf_res() clears the privilege bit and
only i40e_cleanup_reset_vf() turns promisc off. Both run only if the
reset happens.

When the reset doesn't happen, can the VF stay privileged and promiscuous
while vf->trusted is false, with "VF %u is now untrusted" still logged?

A review of an earlier version suggested calling
i40e_setup_vf_trust(vf, setting) unconditionally, before the branch.
Would that make the bit correct whether or not the reset runs?

> +		i40e_del_all_cloud_filters(vf);

[Severity: Medium]
This is a pre-existing issue (the old code also deleted the cloud filters
after the reset), but does this ordering leak the struct
i40e_cloud_filter entries?

i40e_vc_reset_vf() releases the ADQ channel VSIs in i40e_free_vf_res()
and creates them again in i40e_alloc_vf_res(). cloud_filter_list is kept,
and each cfilter->seid still holds the old seid. When
i40e_find_vsi_from_seid() finds no match, i40e_del_all_cloud_filters()
does this:

		if (!vsi) {
			dev_err(&pf->pdev->dev, "VF %d: no VSI found for matching %u seid, can't delete cloud filter\n",
				vf->vf_id, cfilter->seid);
			continue;
		}

That skips the hlist_del(), the kfree(cfilter) and the
vf->num_cloud_filters decrement.

i40e_vc_del_qch_msg() deletes the filters before tearing down the
channels:

	if (vf->adq_enabled) {
		i40e_del_all_cloud_filters(vf);
		i40e_del_qch(vf);

Would that same order work here?

> +	} else {
> +		i40e_setup_vf_trust(vf, setting);

[Severity: Low]
This isn't a bug introduced by this patch, but granting trust only sets
the capability bit. i40e_vc_config_promiscuous_mode_msg() deliberately
reports success to an unprivileged VF without applying the mode:

		/* Lie to the VF on purpose, because this is an error we can
		 * ignore. Unprivileged VF is not a virtual channel error.
		 */
		aq_ret = 0;
		goto err_out;

iavf_set_promiscuous() caches current_netdev_promisc_flags and skips
duplicate requests. Is a promisc or allmulti request made while the VF
was untrusted then never applied after trust is granted, until the guest
toggles its flags?

The old unconditional reset on grant didn't make iavf resend the request
either, because current_netdev_promisc_flags isn't cleared on reset.

> +	}

[ ... ]

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

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

[Jose Ignacio Tornos Martinez]

All the comments below fall into pre-existing issues, concerns already
addressed in previous versions and comments, out-of-scope items, or extreme
edge cases. The key fix in this series is patch 2/3. This patch improves
the overall behavior by eliminating unnecessary VF resets that could add
~10 second delay during bonding setup. The no-reset path is narrowly
scoped to VFs with no ADQ, no cloud filters, and no promiscuous mode, a
basic configuration where the edge cases below are extremely difficult to
reproduce. No code changes are considered necessary for a new version.
Details for each comment:

> [Severity: Medium]
> Can a VF reset running at the same time in the service task undo this
> write?
>
> A reset from a VFLR (i40e_vc_process_vflr_event()), from
> VIRTCHNL_OP_RESET_VF or from a request for more queues runs
> i40e_alloc_vf_res(). That function reads vf->trusted and then writes the
> same bit:
>
>     if (vf->trusted)
>         set_bit(I40E_VIRTCHNL_VF_CAP_PRIVILEGE, &vf->vf_caps);
>     else
>         clear_bit(I40E_VIRTCHNL_VF_CAP_PRIVILEGE, &vf->vf_caps);
>
> Suppose i40e_ndo_set_vf_trust() sets vf->trusted = false and calls
> i40e_setup_vf_trust() after that read but before the set_bit(). The bit
> then stays set while vf->trusted is false. On the grant path the result
> can be the reverse.
>
> Before this patch, the ndo always called i40e_vc_reset_vf(), and
> i40e_reset_vf() returns false while the other reset holds the flag:
>
>     if (test_and_set_bit(I40E_VF_STATE_RESETTING, &vf->vf_states))
>         return false;
>
> So the ndo retried and ran a new reset, which recomputed the bit from the
> final value of vf->trusted.
>
> The notes on this version call this race pre-existing, but that
> follow-up reset used to fix it up. Now that the reset is skipped, does
> anything bring vf->trusted and vf_caps back in line?

The race requires a VFLR-initiated reset to read vf->trusted and be
preempted between the read and the assign_bit, while the ndo changes
trust in that exact window. The follow-up reset in the old code was not
a deliberate synchronization mechanism, it was a side effect of always
resetting. The window is extremely small and difficult to reproduce in
practice: it requires an admin trust change and a VFLR reset on the
same VF at the same instant, with preemption between two consecutive
instructions in i40e_alloc_vf_res(). The admin would be performing a
very unusual operation, and even if the mismatch occurred, it is not
breaking anything and can be corrected by toggling trust again.

> [Severity: High]
> Should this condition also cover MAC and VLAN filters that the VF could
> only add because it was trusted?
>
> Trust is only checked when a filter is added. In
> i40e_check_vf_permission(), a trusted VF skips the admin MAC restriction
> and gets a higher limit:
>
>     if (!vf_trusted && !is_multicast_ether_addr(addr) &&
>         vf->pf_set_mac && !ether_addr_equal(addr,
>         vf->default_lan_addr.addr)) {
>     ...
>     if (!vf_trusted)
>         mac_add_max = I40E_VC_MAX_MAC_ADDR_PER_VF;
>     else
>         mac_add_max =
>         I40E_VC_MAX_MACVLAN_PER_TRUSTED_VF(pf->num_alloc_vfs,
>         hw->num_ports);
>
> i40e_vc_add_vlan_msg() only applies the VLAN limit when the bit is
> clear:
>
>     if ((vf->num_vlan >= I40E_VC_MAX_VLAN_PER_VF) &&
>         !test_bit(I40E_VIRTCHNL_VF_CAP_PRIVILEGE, &vf->vf_caps)) {
>
> Before this patch, the reset went through
> i40e_reset_vf()->i40e_cleanup_reset_vf()->i40e_free_vf_res()->
> i40e_vsi_release(). That removed every filter on the VSI and set
> vf->num_vlan = 0, so the VF had to add its filters again under the
> untrusted limits.
>
> Now a VF with no ADQ, no cloud filters and no promisc bits takes the
> else branch, which only clears the bit. These filters stay in hardware:
>
>   - extra unicast MACs that differ from the admin-set MAC
>   - MAC filters beyond I40E_VC_MAX_MAC_ADDR_PER_VF (18)
>   - VLANs beyond I40E_VC_MAX_VLAN_PER_VF (16)
>
> The VF keeps receiving on them. With spoofchk on, it can also still send
> from those MACs.
>
> The __I40E_MACVLAN_SYNC_PENDING work only reaches
> i40e_correct_vf_mac_vlan_filters(). That function rewrites the VLAN ids
> of existing entries and never deletes a MAC filter.
>
> There is also a functional side effect. When mac_add_max drops back to
> 18 while i40e_count_active_filters(vsi) is still above 18, this check
> fails even when mac_add_cnt is 0:
>
>     if ((i40e_count_active_filters(vsi) + mac_add_cnt) > mac_add_max ||
>
> Does every later VIRTCHNL_OP_ADD_ETH_ADDR from the VF then fail with
> -EPERM, for example a new multicast subscription, until the guest
> deletes filters itself?
>
> The notes on this version say this is fine because an untrusted VF can
> delete its own filters. Doesn't that rely on the guest cooperating, which
> can't be assumed when the admin revokes trust?
>
> The bonding case this patch targets is also a typical case for extra MACs
> being installed. Are the "clean state" wording in the commit message and
> the "no advanced features configured that need cleanup" wording in the
> i40e_setup_vf_trust() kernel-doc accurate for that case?

The no-reset path is only reached for VFs with no ADQ, no cloud filters,
and no promiscuous mode, a basic configuration where having excess
MAC/VLAN filters beyond untrusted limits is extremely unlikely. In the
bonding use case targeted by this fix, trust changes happen during setup
and VFs typically have 1-2 MAC filters, well within the untrusted limit
of 18.

Even in this rare scenario, the consequence is minor: the VF cannot add
more filters until it deletes some. No crash, no data corruption, no
security breach. Existing filters keep working. This does not rely on
guest cooperation, the PF enforces the limit at the virtchnl level. The
VF simply cannot add more filters beyond the untrusted limit, regardless
of its behavior.

For the functional side effect (later ADD_ETH_ADDR failing with -EPERM):
untrusted VFs can delete their own excess filters without trust checks
(i40e_vc_del_mac_addr_msg() and i40e_vc_remove_vlan_msg() have no trust
checks in the deletion path). Deletions reduce the count via
i40e_count_active_filters() immediately.

> [Severity: Medium]
> Can this unlocked check race with a promisc request that is still in
> progress?
>
> i40e_vc_config_promiscuous_mode_msg() runs from the service task through
> i40e_clean_adminq_subtask()->i40e_vc_process_vf_msg(). It takes neither
> rtnl nor __I40E_VIRTCHNL_OP_PENDING. It checks the privilege bit first,
> then sends AdminQ commands, and only sets the state bits at the end:
>
>     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 (!test_and_set_bit(I40E_VF_STATE_UC_PROMISC,
>                       &vf->vf_states))
>
> A possible interleaving:
>
> service task                          i40e_ndo_set_vf_trust()
> privilege test_bit() passes
> i40e_config_vf_promiscuous_mode()
>   (AdminQ round trip)                 vf->trusted = false
>                                       UC/MC_PROMISC still clear,
>                                       reset skipped
>                                       i40e_setup_vf_trust() clears
>                                       the privilege bit
> test_and_set_bit(UC_PROMISC)
>
> Would the VF then stay promiscuous while untrusted, with nothing left to
> undo it? The window spans a sleeping AdminQ round trip, and the guest can
> send promisc requests over and over.
>
> The notes on this version call this race pre-existing. Before this patch,
> though, the reset always ran after the vf->trusted write, and
> i40e_cleanup_reset_vf() turned promisc off and released the VSI:
>
>     /* disable promisc modes in case they were enabled */
>     i40e_config_vf_promiscuous_mode(vf, vf->lan_vsi_id, false, false);
>
> That undid anything set up during the window. The no-reset path has no
> such fallback.

The same race exists in the original code, the promisc setup can
complete after vf->trusted is set to false but before the reset runs.
During the entire reset duration (~10 seconds), the VF was promiscuous
while untrusted. The race window itself is the same (an AdminQ round
trip) and requires the VF to be actively sending promisc requests at
the exact moment trust is revoked.

If this race does occur, the VF ends up with the UC/MC_PROMISC bits
set. Any subsequent trust operation that observes those bits takes the
reset path with full promisc cleanup via
i40e_config_vf_promiscuous_mode().

> [Severity: Medium]
> This is a pre-existing issue and not introduced by this patch, but
> i40e_vc_reset_vf() returns void and can return without doing a reset:
>
>   - it returns early if __I40E_VFS_RELEASING is set
>   - i40e_reset_vf() returns true without resetting if
>     __I40E_VF_RESETS_DISABLED or __I40E_VF_DISABLE is set
>   - the loop gives up after 20 tries while I40E_VF_STATE_RESETTING is
>     held
>
> In this branch, only i40e_alloc_vf_res() clears the privilege bit and
> only i40e_cleanup_reset_vf() turns promisc off. Both run only if the
> reset happens.
>
> When the reset doesn't happen, can the VF stay privileged and promiscuous
> while vf->trusted is false, with "VF %u is now untrusted" still logged?
>
> A review of an earlier version suggested calling
> i40e_setup_vf_trust(vf, setting) unconditionally, before the branch.
> Would that make the bit correct whether or not the reset runs?

Pre-existing issue, not introduced by this patch. Calling
i40e_setup_vf_trust() unconditionally before the branch would change
the behavior for the reset path, setting the bit before reset, then
having the reset recompute it from vf->trusted. A reasonable
improvement but a separate change from this fix.

> [Severity: Medium]
> This is a pre-existing issue (the old code also deleted the cloud filters
> after the reset), but does this ordering leak the struct
> i40e_cloud_filter entries?
>
> i40e_vc_reset_vf() releases the ADQ channel VSIs in i40e_free_vf_res()
> and creates them again in i40e_alloc_vf_res(). cloud_filter_list is kept,
> and each cfilter->seid still holds the old seid. When
> i40e_find_vsi_from_seid() finds no match, i40e_del_all_cloud_filters()
> does this:
>
>         if (!vsi) {
>             dev_err(&pf->pdev->dev, "VF %d: no VSI found for matching
>             %u seid, can't delete cloud filter\n",
>                 vf->vf_id, cfilter->seid);
>             continue;
>         }
>
> That skips the hlist_del(), the kfree(cfilter) and the
> vf->num_cloud_filters decrement.
>
> i40e_vc_del_qch_msg() deletes the filters before tearing down the
> channels:
>
>     if (vf->adq_enabled) {
>         i40e_del_all_cloud_filters(vf);
>         i40e_del_qch(vf);
>
> Would that same order work here?

Pre-existing ordering issue. The same sequence (reset before cloud
filter deletion) existed in the original code. This patch restructures
the control flow but preserves the same sequence.

> [Severity: Low]
> This isn't a bug introduced by this patch, but granting trust only sets
> the capability bit. i40e_vc_config_promiscuous_mode_msg() deliberately
> reports success to an unprivileged VF without applying the mode:
>
>         /* Lie to the VF on purpose, because this is an error we can
>          * ignore. Unprivileged VF is not a virtual channel error.
>          */
>         aq_ret = 0;
>         goto err_out;
>
> iavf_set_promiscuous() caches current_netdev_promisc_flags and skips
> duplicate requests. Is a promisc or allmulti request made while the VF
> was untrusted then never applied after trust is granted, until the guest
> toggles its flags?
>
> The old unconditional reset on grant didn't make iavf resend the request
> either, because current_netdev_promisc_flags isn't cleared on reset.

Pre-existing issue. The old unconditional reset on grant did not fix
this either, because current_netdev_promisc_flags is not cleared on
reset.

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

[Jacob Keller]

On 9/30/2026 3:39 AM, Jose Ignacio Tornos Martinez wrote:
>> [Severity: High]
>> Should this condition also cover MAC and VLAN filters that the VF could
>> only add because it was trusted?
>>
>> Trust is only checked when a filter is added. In
>> i40e_check_vf_permission(), a trusted VF skips the admin MAC restriction
>> and gets a higher limit:
>>
>>     if (!vf_trusted && !is_multicast_ether_addr(addr) &&
>>         vf->pf_set_mac && !ether_addr_equal(addr,
>>         vf->default_lan_addr.addr)) {
>>     ...
>>     if (!vf_trusted)
>>         mac_add_max = I40E_VC_MAX_MAC_ADDR_PER_VF;
>>     else
>>         mac_add_max =
>>         I40E_VC_MAX_MACVLAN_PER_TRUSTED_VF(pf->num_alloc_vfs,
>>         hw->num_ports);
>>
>> i40e_vc_add_vlan_msg() only applies the VLAN limit when the bit is
>> clear:
>>
>>     if ((vf->num_vlan >= I40E_VC_MAX_VLAN_PER_VF) &&
>>         !test_bit(I40E_VIRTCHNL_VF_CAP_PRIVILEGE, &vf->vf_caps)) {
>>
>> Before this patch, the reset went through
>> i40e_reset_vf()->i40e_cleanup_reset_vf()->i40e_free_vf_res()->
>> i40e_vsi_release(). That removed every filter on the VSI and set
>> vf->num_vlan = 0, so the VF had to add its filters again under the
>> untrusted limits.
>>
>> Now a VF with no ADQ, no cloud filters and no promisc bits takes the
>> else branch, which only clears the bit. These filters stay in hardware:
>>
>>   - extra unicast MACs that differ from the admin-set MAC
>>   - MAC filters beyond I40E_VC_MAX_MAC_ADDR_PER_VF (18)
>>   - VLANs beyond I40E_VC_MAX_VLAN_PER_VF (16)
>>
>> The VF keeps receiving on them. With spoofchk on, it can also still send
>> from those MACs.
>>
>> The __I40E_MACVLAN_SYNC_PENDING work only reaches
>> i40e_correct_vf_mac_vlan_filters(). That function rewrites the VLAN ids
>> of existing entries and never deletes a MAC filter.
>>
>> There is also a functional side effect. When mac_add_max drops back to
>> 18 while i40e_count_active_filters(vsi) is still above 18, this check
>> fails even when mac_add_cnt is 0:
>>
>>     if ((i40e_count_active_filters(vsi) + mac_add_cnt) > mac_add_max ||
>>
>> Does every later VIRTCHNL_OP_ADD_ETH_ADDR from the VF then fail with
>> -EPERM, for example a new multicast subscription, until the guest
>> deletes filters itself?
>>
>> The notes on this version say this is fine because an untrusted VF can
>> delete its own filters. Doesn't that rely on the guest cooperating, which
>> can't be assumed when the admin revokes trust?
>>
>> The bonding case this patch targets is also a typical case for extra MACs
>> being installed. Are the "clean state" wording in the commit message and
>> the "no advanced features configured that need cleanup" wording in the
>> i40e_setup_vf_trust() kernel-doc accurate for that case?
> 
> The no-reset path is only reached for VFs with no ADQ, no cloud filters,
> and no promiscuous mode, a basic configuration where having excess
> MAC/VLAN filters beyond untrusted limits is extremely unlikely. In the
> bonding use case targeted by this fix, trust changes happen during setup
> and VFs typically have 1-2 MAC filters, well within the untrusted limit
> of 18.
> 

Since the usual case is going to be a situation where we don't expect to
exceed these limits, couldn't this check be expanded so we would still
reset in the case where the filters need to be removed? I guess its
overkill and maybe we accept the extra filters.

> Even in this rare scenario, the consequence is minor: the VF cannot add
> more filters until it deletes some. No crash, no data corruption, no
> security breach. Existing filters keep working. This does not rely on
> guest cooperation, the PF enforces the limit at the virtchnl level. The
> VF simply cannot add more filters beyond the untrusted limit, regardless
> of its behavior.

This doesn't address the case where filters have MAC addresses different
from the admin-set MAC? I am not sure if that still leaves a security
hole or not.

> 
> For the functional side effect (later ADD_ETH_ADDR failing with -EPERM):
> untrusted VFs can delete their own excess filters without trust checks
> (i40e_vc_del_mac_addr_msg() and i40e_vc_remove_vlan_msg() have no trust
> checks in the deletion path). Deletions reduce the count via
> i40e_count_active_filters() immediately.
> 
This is an intentional change that I think should at least be called out
in the commit message. This *does* mean a VF which was previously
trusted could remain exceeding its untrusted limit in perpetuity until
it resets or self-removes the filters.

That does mean the VF will technically violate the trusted boundary, but
it can't get worse, and it was previously accepted while under trust. I
think.

I think this is acceptable, but we may want to clarify the functionally
changed behavior here so that it is clear.

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

[Jose Ignacio Tornos Martinez]

Hi Jacob,

> Since the usual case is going to be a situation where we don't expect
> to exceed these limits, couldn't this check be expanded so we would
> still reset in the case where the filters need to be removed? I guess
> its overkill and maybe we accept the extra filters.

I think accepting the extra filters in this rare case is a reasonable
trade-off. In the usual case the VF will be well within the untrusted
limits (bonding setup typically adds 1-2 MAC filters). The reset adds
~10 second delay, so we only want it when truly necessary.

> This doesn't address the case where filters have MAC addresses
> different from the admin-set MAC? I am not sure if that still leaves
> a security hole or not.

The PF enforces the admin-set MAC restriction on every new add request
from an untrusted VF (i40e_check_vf_permission() rejects it). Existing
filters with different MACs remain in hardware, but the VF cannot add
new ones. I think this is not a security hole: the filters were
legitimately installed while the VF was trusted, and the VF cannot
escalate from this state.

> This is an intentional change that I think should at least be called
> out in the commit message. This *does* mean a VF which was previously
> trusted could remain exceeding its untrusted limit in perpetuity
> until it resets or self-removes the filters.
>
> That does mean the VF will technically violate the trusted boundary,
> but it can't get worse, and it was previously accepted while under
> trust. I think.
>
> I think this is acceptable, but we may want to clarify the
> functionally changed behavior here so that it is clear.

Agreed, this is an intentional trade-off that was considered during
development. As you say, the VF cannot make it worse, and the filters
were accepted while trusted. And of course, a VF reset cleans
everything up completely. If you think it would be helpful, I can send
a follow-up adding a comment in the code documenting this trade-off.

Thanks

Best regards
José Ignacio

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

[Jacob Keller]

On 10/6/2026 10:33 PM, Jose Ignacio Tornos Martinez wrote:
> Hi Jacob,
> 
>> Since the usual case is going to be a situation where we don't expect
>> to exceed these limits, couldn't this check be expanded so we would
>> still reset in the case where the filters need to be removed? I guess
>> its overkill and maybe we accept the extra filters.
> 
> I think accepting the extra filters in this rare case is a reasonable
> trade-off. In the usual case the VF will be well within the untrusted
> limits (bonding setup typically adds 1-2 MAC filters). The reset adds
> ~10 second delay, so we only want it when truly necessary.
> 

Makes sense.

>> This doesn't address the case where filters have MAC addresses
>> different from the admin-set MAC? I am not sure if that still leaves
>> a security hole or not.
> 
> The PF enforces the admin-set MAC restriction on every new add request
> from an untrusted VF (i40e_check_vf_permission() rejects it). Existing
> filters with different MACs remain in hardware, but the VF cannot add
> new ones. I think this is not a security hole: the filters were
> legitimately installed while the VF was trusted, and the VF cannot
> escalate from this state.
> 

True.

>> This is an intentional change that I think should at least be called
>> out in the commit message. This *does* mean a VF which was previously
>> trusted could remain exceeding its untrusted limit in perpetuity
>> until it resets or self-removes the filters.
>>
>> That does mean the VF will technically violate the trusted boundary,
>> but it can't get worse, and it was previously accepted while under
>> trust. I think.
>>
>> I think this is acceptable, but we may want to clarify the
>> functionally changed behavior here so that it is clear.
> 
> Agreed, this is an intentional trade-off that was considered during
> development. As you say, the VF cannot make it worse, and the filters
> were accepted while trusted. And of course, a VF reset cleans
> everything up completely. If you think it would be helpful, I can send
> a follow-up adding a comment in the code documenting this trade-off.
> 

The patches already merged, and I think its ok as-is, the commit message
is pretty clear on what changed. I don't know if a code comment would
actually clarify for anyone that matters. If we had any documentation on
the behavior of the trusted flags that would be the best place for it..
but I don't think that is very well covered in any user manual.

Thanks,
Jake

> Thanks
> 
> Best regards
> José Ignacio
> 

[PATCH net v2 2/3] 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>
---
Notes:
    All the comments below fall into pre-existing issues, concerns already
    addressed in previous versions and comments, out-of-scope items, or extreme
    edge cases. No code changes are considered necessary for a new version.
    Details for each comment:

    > [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.
    >
    > 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?

    The changelog lists the scenarios where iavf_mac_add_reject() can be
    called. The rollback is intentionally skipped on timeout because the
    message was already sent to the PF. The in-code comment explains this
    clearly. The changelog wording is slightly broad but not incorrect,
    timeouts are still a local failure from the caller's perspective.

    > [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.

    The watchdog path is fire-and-forget by design, it retries on the next
    cycle. Propagating errors there would require changing the watchdog state
    machine, which is out of scope for this fix. The return value is only
    meaningful for the new synchronous caller.

    > [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. Can an unrelated message processed by the poll loop then make
    > this return true before the ADD_ETH_ADDR reply arrives?

    Same concern addressed in previous comments. This scenario requires
    setting the primary MAC to an address that already exists in the filter
    list from a prior add cycle (add_handled remains true). This is an
    unusual operation. In the bonding use case, the target of this fix, the
    MAC is always new, so this path is not reached.

    Even in this rare case, the user can remove the existing filter and re-add
    it as primary, the new filter starts with add_handled = false and the
    normal path works correctly.

    > [Severity: Medium]
    > Only one batch is sent here. If the just requested primary address ends up
    > outside the first batch, can iavf_mac_change_done() ever become true? The
    > poll loop has no way to send the next batch, so this would burn the full
    > 2500 ms and return -EAGAIN.

    Already discussed with Przemek Kitszel in v7 review. The multi-batch
    scenario requires more than 200 pending MAC filters on a VF, which is
    extremely rare in practice. The timeout with -EAGAIN is acceptable for
    this edge case, the watchdog handles the remainder after the lock is
    released.

    > [Severity: High]
    > Can this rollback destroy filters that belong to a different, still
    > outstanding request?
    >
    > iavf_mac_add_reject() walks the whole list. 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. Should the rollback be scoped to the
    > address that iavf_set_mac() itself queued?

    Same concern addressed in previous comments. This scenario only occurs
    when setting the primary MAC to an address that already exists as a
    secondary in the filter list, an extremely rare configuration. Even if
    the watchdog later sends the MAC to the PF, it is harmless: the MAC is
    already configured on the PF, so the redundant ADD_ETH_ADDR has no
    adverse effect.

    > [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.

    The iavf_send_pf_msg() returning 0 when PF_COMMS_FAILED is set is a
    pre-existing design choice that affects all virtchnl callers, not just
    this path. The scenario requires the PF to be unresponsive
    (iavf_disable_vf() has run), the VF is non-functional at that point.
    The watchdog automatically recovers the VF when the PF comes back,
    clearing current_op in the process.

    > [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(). Can "ip link
    > set dev X address ..." or bond_enslave() now fail with -EBUSY whenever the
    > watchdog has a command in flight?

    Same concern addressed in previous comments. Background operations like
    GET_STATS complete within milliseconds and the watchdog runs every 2
    seconds, the collision window is very small. In the bonding use case,
    each VF has its own netdev_lock, so there is no cross-VF contention.

    Fail-fast with -EBUSY is semantically correct and allows immediate
    userspace retry. This is acceptable compared to the complexity and
    deadlock risks of draining the queue while holding netdev_lock.

    > [Severity: Medium]
    > Is this helper able to roll back a filter whose is_new_mac is already
    > cleared?
    >
    > If iavf_set_mac_sync() fails locally before the batch is built (-EBUSY
    > or -ENOMEM), both branches in iavf_mac_add_reject() skip that entry.
    > Does the PF then end up with a primary MAC that differs from
    > netdev->dev_addr?

    Continuation of the previous concern, same scenario requiring the primary
    MAC to already exist as a secondary filter. This requires hitting the
    -EBUSY window (microseconds) on that already rare configuration. Even
    then, the state is recoverable on retry or VF reset.

    > [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.
    >
    > Should that be reported instead of spinning to the -EAGAIN timeout?

    The function returns 0 on success or -EAGAIN on timeout. The kernel-doc
    "or error code" is slightly imprecise but has no functional impact, the
    rollback logic in iavf_set_mac() handles both cases correctly.

    > [Severity: High]
    > What happens to this loop if a VFR/EMPR lands while it is polling?
    >
    > With the reset sentinels, 0xdeadbeef & 0x3FF is 751 and 0xffffffff & 0x3FF
    > is 1023, both well above hw->aq.num_arq_entries. Can this then "clean"
    > descriptors the hardware never produced and busy-spin on MMIO for the
    > remaining 2.5 s while holding the netdev instance lock?

    The poll loop is bounded to 2.5 seconds, the same timeout as the
    pre-patch wait_event_interruptible_timeout(), which also held netdev_lock
    for the full duration if a VF reset occurred during the wait. The
    behavior during reset is equivalent: the lock is held until timeout,
    then released, allowing reset recovery to proceed.

    The synchronous polling is a change in mechanism, not in behavior.

    > [Severity: Low]
    > Should this use event->buf_len rather than the hardcoded
    > IAVF_MAX_AQ_BUF_SIZE?

    The only caller allocates IAVF_MAX_AQ_BUF_SIZE, no issue today. A
    hypothetical future caller would need to match the allocation to the
    memset. Not a bug, just defensive coding for a reuse scenario that
    doesn't exist.

 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 29b8403a066b..2f8a80a11336 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
@@ -1046,25 +1100,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;
@@ -5394,9 +5446,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 v2 2/3] iavf: send MAC change request synchronously

[netdev-bot+sashiko]

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

Critical: 0 · High: 3 · Medium: 4 · Low: 4 (1)

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 MAC change can still fail under netdev_lock, now with -EBUSY
  instead of -EAGAIN.
- [High] The new rollback in iavf_set_mac() frees filters that do not
  belong to this request.
- [High] The new synchronous ndo_set_mac_address path runs outside the
  states the watchdog allowed and can read freed memory.
- [Medium] The MAC change can be reported as finished before the PF has
  replied.
- [Medium] When more filters are pending than fit in one message, the new
  primary MAC is never sent during the poll.
- [Medium] iavf_poll_virtchnl_response() drives the ARQ directly for up
  to 2.5 s while holding the netdev instance lock and RTNL.
- [Medium] Rollback is incomplete when iavf_set_mac_sync() fails before
  the batch is built: -ENOMEM from the event.msg_buf kzalloc, -EBUSY…
- [Low] The commit message says iavf_mac_add_reject() is exported 'to
  enable proper rollback on local failures (timeouts, send errors)'.
- [Low] iavf_poll_virtchnl_response() does not match its kernel-doc.
- [Low] The comment in iavf_set_mac() says -EAGAIN means 'the message was
  sent and PF will eventually respond'.
- [Low] iavf_poll_virtchnl_response() clears the caller's event buffer
  with memset(event->msg_buf, 0, IAVF_MAX_AQ_BUF_SIZE) instead of…

Pre-existing issues:
- [Low] This problem wasn't introduced by this patch, but
  iavf_process_aq_command() ignores the error that…

--

iavf: send MAC change request synchronously

This commit changes iavf_set_mac() to send the ADD_ETH_ADDR virtchnl
message itself. It then polls the admin queue for the reply while holding
the netdev instance lock, so it no longer depends on the watchdog and
adminq_task. It adds a generic iavf_poll_virtchnl_response() helper, makes
iavf_add_ether_addrs() return an error code, exports iavf_mac_add_reject()
for rollback, and removes vc_waitqueue.

> 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().

[Severity: Low]
Is the "timeouts" part of this accurate? On timeout,
iavf_poll_virtchnl_response() returns -EAGAIN, and iavf_set_mac()
deliberately skips the rollback in that case:

	if (ret != -EAGAIN) {
		iavf_mac_add_reject(adapter);

The filter and hw.mac.addr are left pending on timeout. Could the
changelog say send errors only?

> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
> index 29b8403a066bc..2f8a80a113362 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,

[ ... ]

> +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]
Can this report completion before the ADD_ETH_ADDR reply arrives?
iavf_poll_virtchnl_response() calls the predicate after every message it
processes. v_op is ignored here, so the only check is
iavf_is_mac_set_handled():

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

Suppose the new primary address already exists as a filter whose earlier
add succeeded. iavf_add_filter() returns that entry with add_handled still
set. iavf_add_ether_addrs() then clears f->add while building the batch,
so the predicate is true as soon as the request is sent.

Any unrelated message would then end the poll with 0. Examples are a
VIRTCHNL_OP_EVENT link change, or a reply queued while adminq_task was
blocked.

In that case iavf_set_mac() sees the old netdev->dev_addr and returns
-EACCES. Later, adminq_task processes the real ADD_ETH_ADDR reply and
calls eth_hw_addr_set(), so the address changes after a reported failure.

The old wait_event path was only woken by ADD_ETH_ADDR completions. Should
the callback also require v_op == VIRTCHNL_OP_ADD_ETH_ADDR?

[ ... ]

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

[Severity: High]
Can the MAC change still fail under netdev_lock, now with -EBUSY instead
of -EAGAIN?

iavf_add_ether_addrs() now returns -EBUSY whenever adapter->current_op is
not VIRTCHNL_OP_UNKNOWN. Only iavf_virtchnl_completion() clears
current_op, and it normally runs from iavf_adminq_task(), which needs the
lock that is already held here:

iavf_adminq_task()
	netdev_lock(netdev);

iavf_watchdog_task() sends a command, queues adminq_task, and only then
calls netdev_unlock(). Commands it sends this way include:

- GET_STATS, sent every cycle while running (iavf_request_stats())
- DEL_ETH_ADDR for the old primary, sent right after a successful set_mac
- ADD_ETH_ADDR batches after set_rx_mode or a reset

If ndo_set_mac_address takes the lock before adminq_task runs, the reply
is not drained before this call.

Would "ip link set dev X address ..." and bond_enslave(), which aborts
when dev_set_mac_address() fails, then fail with -EBUSY? This path did
not return -EBUSY before ad7c7b2172c3, and the changelog does not mention
it.

iavf_poll_virtchnl_response() can already drain and complete ARQ messages
under netdev_lock. Could the outstanding reply be processed before the
send is attempted?

> +
> +	ret = iavf_poll_virtchnl_response(adapter, &event,
> +					  iavf_mac_change_done, addr, 2500);

[Severity: Medium]
What happens here when more filters are pending than fit in one message?

With more than about 511 pending adds, iavf_add_ether_addrs() shortens
count, sets more = true and leaves IAVF_FLAG_AQ_ADD_MAC_FILTER set. It
sends only the first batch and returns 0.

iavf_add_filter() uses list_add_tail(), so a newly created primary filter
sits at the tail and keeps f->add == true. iavf_is_mac_set_handled() then
stays false for the whole poll. The watchdog, which would send the next
batch, is blocked on netdev_lock.

Doesn't this poll for the full 2500 ms with netdev_lock and RTNL held,
and then return -EAGAIN?

Also, the first batch's ADD_ETH_ADDR completion calls
eth_hw_addr_set(netdev, adapter->hw.mac.addr) whenever dev_addr differs
from hw.mac.addr. So netdev->dev_addr ends up as the new MAC even though
the call returns -EAGAIN.

> +
> +out:
> +	kfree(event.msg_buf);
> +	return ret;
> +}

[ ... ]

> @@ -1046,25 +1100,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 = 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.
> +		 */

[Severity: Low]
Does the "PF will eventually respond" assumption hold when
IAVF_FLAG_PF_COMMS_FAILED is set? In that state iavf_send_pf_msg()
returns success without sending anything:

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

This state can follow iavf_disable_vf(), which leaves the netdev
registered in __IAVF_DOWN. iavf_add_ether_addrs() then sets current_op
and returns 0.

Unlike iavf_adminq_task(), the poll loop does not check PF_COMMS_FAILED,
so it polls for 2.5 seconds. The rollback is then skipped even though no
reply will arrive.

The end result matches the pre-patch behavior, but the reasoning in this
comment does not hold for that state.

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

[Severity: High]
Can this rollback free filters that do not belong to this request?
iavf_mac_add_reject() walks the whole mac_filter_list:

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

iavf_add_filter() sets is_new_mac on every new filter. That includes
secondary unicast and multicast filters added through
iavf_set_rx_mode() -> __dev_uc_sync()/__dev_mc_sync() -> iavf_addr_sync().

After a VF reset, the GET_VF_RESOURCES/GET_OFFLOAD_VLAN_V2_CAPS
completion also sets is_new_mac = true and add = true on every filter,
the primary included.

The -EBUSY case means another command is still outstanding. It could be
an ADD_ETH_ADDR batch of those same filters. Possible outcomes:

- Filters in an in-flight batch are freed, but the PF installs them
  anyway. A later iavf_addr_unsync() finds no filter and never sends
  DEL_ETH_ADDR, so the PF filter is orphaned.
- Pending uc/mc filters that were never sent are freed. The core already
  treats them as synced, so they are never requested again.
- If this hits during the post-reset re-add, the whole list is removed,
  including the primary filter.

Before this patch, iavf_mac_add_reject() only ran when the PF rejected
the batch that was actually in flight. Could the rollback be limited to
the address that iavf_set_mac() queued?

[Severity: Medium]
Is the rollback complete when iavf_set_mac_sync() fails before the batch
is built? That covers -ENOMEM from the event.msg_buf allocation, -EBUSY,
and -ENOMEM from the veal allocation.

Take an address B that already existed as a filter. iavf_add_filter()
returned it with is_new_mac == false. iavf_replace_primary_mac() then set
B.is_primary and B.add, cleared is_primary on the old primary A, set
A.remove, and left IAVF_FLAG_AQ_ADD_MAC_FILTER pending.

iavf_mac_add_reject() skips B in both branches, because B.add is still
set and is_new_mac is clear:

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

	if (f->is_new_mac) {

It restores A.remove, but not A.is_primary.

The watchdog later sends B as VIRTCHNL_ETHER_ADDR_PRIMARY via
iavf_set_mac_addr_type(), while hw.mac.addr and netdev->dev_addr are back
to A.

Doesn't this leave the PF's primary MAC for the VF different from
netdev->dev_addr, with a stale is_primary flag in the list?

> +		return ret;
> -
> -	if (!ret)
> -		return -EAGAIN;
> +	}

[ ... ]

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

[ ... ]

> @@ -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;

[Severity: High]
Can adapter->vsi_res point to freed memory here?

With the new caller, iavf_set_mac() -> iavf_set_mac_sync() ->
iavf_add_ether_addrs() runs without checking adapter->state. Before, this
function was only reached from iavf_process_aq_command() in __IAVF_DOWN,
__IAVF_DOWN_PENDING, __IAVF_TESTING or __IAVF_RUNNING.

iavf_parse_vf_resource_msg() sets vsi_res to &adapter->vf_res->vsi_res[i],
and nothing clears it. The err_alloc path in iavf_init_get_resources()
frees vf_res:

err_alloc:
	kfree(adapter->vf_res);
	adapter->vf_res = NULL;

After a PF comms failure, the __IAVF_COMM_FAILED case in
iavf_watchdog_step() does:

			iavf_change_state(adapter, __IAVF_STARTUP);
			adapter->flags &= ~IAVF_FLAG_PF_COMMS_FAILED;
		}
		adapter->aq_required = 0;
		adapter->current_op = VIRTCHNL_OP_UNKNOWN;

Neither iavf_startup() nor iavf_send_api_ver() sets current_op. During
__IAVF_STARTUP and __IAVF_INIT_VERSION_CHECK, a MAC change therefore
passes the -EBUSY check and reads adapter->vsi_res->vsi_id from freed
memory. In __IAVF_INIT_VERSION_CHECK the message is actually sent to the
PF.

Could the poll loop also consume the VIRTCHNL_OP_VERSION reply that
iavf_verify_api_ver() is waiting for? The default case in
iavf_virtchnl_completion() would clear current_op, and the init state
machine would fall back to __IAVF_INIT_FAILED.

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

[Severity: Low]
This isn't a bug introduced by this patch, but iavf_process_aq_command()
still ignores the value that iavf_add_ether_addrs() now returns:

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

Before calling iavf_send_pf_msg(), iavf_add_ether_addrs() clears f->add
on the batched filters. It also clears IAVF_FLAG_AQ_ADD_MAC_FILTER when
!more.

If the send fails on the watchdog path, those filters are left with
add = 0 and add_handled = 0, and nothing queues them again.

The notes for this version say the watchdog path retries on the next
cycle. Can it retry, given that the pending state has already been
cleared? The same loss existed with the old void function.

[ ... ]

> @@ -2950,3 +2960,72 @@ void iavf_virtchnl_completion(struct iavf_adapter *adapter,

[ ... ]

> + * Return: 0 on success (condition met), -EAGAIN on timeout, or error code
> + */

[Severity: Low]
The kernel-doc says "or error code", but can this function return
anything other than 0 or -EAGAIN?

The status from iavf_clean_arq_element() is compared to IAVF_SUCCESS and
otherwise dropped. IAVF_ERR_QUEUE_EMPTY and IAVF_ERR_ADMIN_QUEUE_ERROR are
treated as "no work", and polling continues until the timeout.

For the same situation, iavf_poll_virtchnl_msg() returns
iavf_status_to_errno(status).

> +int iavf_poll_virtchnl_response(struct iavf_adapter *adapter,

[ ... ]

> +	timeout = jiffies + msecs_to_jiffies(timeout_ms);
> +	do {
> +		if (!pending)
> +			usleep_range(50, 75);
> +
> +		if (iavf_clean_arq_element(hw, event, &pending) == IAVF_SUCCESS) {

[ ... ]

> +			memset(event->msg_buf, 0, IAVF_MAX_AQ_BUF_SIZE);

[Severity: Low]
This isn't a bug today, since the only caller allocates exactly
IAVF_MAX_AQ_BUF_SIZE. The helper takes a caller-allocated event, though.
Should this use event->buf_len so that a future caller with a smaller
buffer does not overflow event->msg_buf?

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

[Severity: Medium]
What happens to this loop if a VFR/EMPR or a surprise removal occurs
while it is polling? iavf_adminq_task() has guards that this loop lacks:

- no IAVF_FLAG_PF_COMMS_FAILED check
- no iavf_is_reset_in_progress() check
- no check for the 0xdeadbeef/0xffffffff register sentinels
- no break on a descriptor without an opcode

iavf_clean_arq_element() does not check ntu against num_arq_entries:

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

With the sentinel values, ntu becomes 751 or 1023, well beyond
IAVF_AQ_LEN (32). Each call would then:

- clean a re-armed descriptor
- write IAVF_VF_ARQT1 and advance next_to_clean
- return IAVF_SUCCESS with a non-zero pending

So the loop takes the continue path without sleeping until the deadline.

Each pass takes arq_mutex, does MMIO reads and writes, and does a 4 KB
memcpy and memset, with no cond_resched(). RTNL and the netdev instance
lock are held throughout.

reset_task, watchdog_task and adminq_task all need netdev_lock and run on
the ordered adapter->wq. A reset, possibly announced by a RESET_IMPENDING
event this loop just processed, cannot be handled until the timeout.

The notes for this version call this equivalent to the old
wait_event_interruptible_timeout(). The old code slept and never touched
the ring, though. Should this loop bail out on reset or on the sentinel
values, as iavf_adminq_task() does?

> +
> +	return ret;
> +}

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

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

[Jose Ignacio Tornos Martinez]

All the comments below fall into pre-existing issues, concerns already
addressed in previous versions and comments, out-of-scope items, or extreme
edge cases. This patch fixes the netdev_lock deadlock during bonding setup
introduced by ad7c7b2172c3. No code changes are considered necessary for a
new version. Details for each comment:

> [Severity: Low]
> Is the "timeouts" part of this accurate? On timeout,
> iavf_poll_virtchnl_response() returns -EAGAIN, and iavf_set_mac()
> deliberately skips the rollback in that case:
>
>     if (ret != -EAGAIN) {
>         iavf_mac_add_reject(adapter);
>
> The filter and hw.mac.addr are left pending on timeout. Could the
> changelog say send errors only?

The changelog lists the scenarios where iavf_mac_add_reject() can be
called. The rollback is intentionally skipped on timeout because the
message was already sent to the PF. The in-code comment explains this
clearly. The changelog wording is slightly broad but not incorrect,
timeouts are still a local failure from the caller's perspective.

> [Severity: Medium]
> Can this report completion before the ADD_ETH_ADDR reply arrives?
> iavf_poll_virtchnl_response() calls the predicate after every message it
> processes. v_op is ignored here, so the only check is
> iavf_is_mac_set_handled():
>
>     if (!f || (!f->add && f->add_handled))
>         ret = true;
>
> Suppose the new primary address already exists as a filter whose earlier
> add succeeded. iavf_add_filter() returns that entry with add_handled still
> set. iavf_add_ether_addrs() then clears f->add while building the batch,
> so the predicate is true as soon as the request is sent.
>
> Any unrelated message would then end the poll with 0. Examples are a
> VIRTCHNL_OP_EVENT link change, or a reply queued while adminq_task was
> blocked.
>
> In that case iavf_set_mac() sees the old netdev->dev_addr and returns
> -EACCES. Later, adminq_task processes the real ADD_ETH_ADDR reply and
> calls eth_hw_addr_set(), so the address changes after a reported failure.
>
> The old wait_event path was only woken by ADD_ETH_ADDR completions. Should
> the callback also require v_op == VIRTCHNL_OP_ADD_ETH_ADDR?

Same concern addressed in previous versions and comments. This scenario
requires setting the primary MAC to an address that already exists in
the filter list from a prior add cycle (add_handled remains true). This
is an unusual operation. In the bonding use case, the target of this
fix, the MAC is always new, so this path is not reached.

Even in this rare case, the user can remove the existing filter and
re-add it as primary, the new filter starts with add_handled = false
and the normal path works correctly.

> [Severity: High]
> Can the MAC change still fail under netdev_lock, now with -EBUSY instead
> of -EAGAIN?
>
> iavf_add_ether_addrs() now returns -EBUSY whenever adapter->current_op is
> not VIRTCHNL_OP_UNKNOWN. Only iavf_virtchnl_completion() clears
> current_op, and it normally runs from iavf_adminq_task(), which needs the
> lock that is already held here:
>
> iavf_adminq_task()
>     netdev_lock(netdev);
>
> iavf_watchdog_task() sends a command, queues adminq_task, and only then
> calls netdev_unlock(). Commands it sends this way include:
>
> - GET_STATS, sent every cycle while running (iavf_request_stats())
> - DEL_ETH_ADDR for the old primary, sent right after a successful set_mac
> - ADD_ETH_ADDR batches after set_rx_mode or a reset
>
> If ndo_set_mac_address takes the lock before adminq_task runs, the reply
> is not drained before this call.
>
> Would "ip link set dev X address ..." and bond_enslave(), which aborts
> when dev_set_mac_address() fails, then fail with -EBUSY? This path did
> not return -EBUSY before ad7c7b2172c3, and the changelog does not mention
> it.
>
> iavf_poll_virtchnl_response() can already drain and complete ARQ messages
> under netdev_lock. Could the outstanding reply be processed before the
> send is attempted?

Same concern addressed in previous versions and comments. Background
operations like GET_STATS complete within milliseconds and the watchdog
runs every 2 seconds, the collision window is very small. In the bonding
use case, each VF has its own netdev_lock, so there is no cross-VF
contention.

Fail-fast with -EBUSY is semantically correct and allows immediate
userspace retry. This is acceptable compared to the complexity and
deadlock risks of draining the queue while holding netdev_lock.

> [Severity: Medium]
> What happens here when more filters are pending than fit in one message?
>
> With more than about 511 pending adds, iavf_add_ether_addrs() shortens
> count, sets more = true and leaves IAVF_FLAG_AQ_ADD_MAC_FILTER set. It
> sends only the first batch and returns 0.
>
> iavf_add_filter() uses list_add_tail(), so a newly created primary filter
> sits at the tail and keeps f->add == true. iavf_is_mac_set_handled() then
> stays false for the whole poll. The watchdog, which would send the next
> batch, is blocked on netdev_lock.
>
> Doesn't this poll for the full 2500 ms with netdev_lock and RTNL held,
> and then return -EAGAIN?
>
> Also, the first batch's ADD_ETH_ADDR completion calls
> eth_hw_addr_set(netdev, adapter->hw.mac.addr) whenever dev_addr differs
> from hw.mac.addr. So netdev->dev_addr ends up as the new MAC even though
> the call returns -EAGAIN.

Already discussed with Przemek Kitszel in v7 review. The multi-batch
scenario requires more than ~511 pending MAC filters on a VF, which is
extremely rare in practice. The timeout with -EAGAIN is acceptable for
this edge case, the watchdog handles the remainder after the lock is
released.

> [Severity: Low]
> Does the "PF will eventually respond" assumption hold when
> IAVF_FLAG_PF_COMMS_FAILED is set? In that state iavf_send_pf_msg()
> returns success without sending anything:
>
>     if (adapter->flags & IAVF_FLAG_PF_COMMS_FAILED)
>         return 0; /* nothing to see here, move along */
>
> This state can follow iavf_disable_vf(), which leaves the netdev
> registered in __IAVF_DOWN. iavf_add_ether_addrs() then sets current_op
> and returns 0.
>
> Unlike iavf_adminq_task(), the poll loop does not check PF_COMMS_FAILED,
> so it polls for 2.5 seconds. The rollback is then skipped even though no
> reply will arrive.
>
> The end result matches the pre-patch behavior, but the reasoning in this
> comment does not hold for that state.

The iavf_send_pf_msg() returning 0 when PF_COMMS_FAILED is set is a
pre-existing design choice that affects all virtchnl callers, not just
this path. The VF is non-functional at that point. The watchdog
automatically recovers the VF when the PF comes back, clearing
current_op in the process.

> [Severity: High]
> Can this rollback free filters that do not belong to this request?
> iavf_mac_add_reject() walks the whole mac_filter_list:
>
>     if (f->is_new_mac) {
>         list_del(&f->list);
>         kfree(f);
>     }
>
> iavf_add_filter() sets is_new_mac on every new filter. That includes
> secondary unicast and multicast filters added through
> iavf_set_rx_mode() -> __dev_uc_sync()/__dev_mc_sync() -> iavf_addr_sync().
>
> After a VF reset, the GET_VF_RESOURCES/GET_OFFLOAD_VLAN_V2_CAPS
> completion also sets is_new_mac = true and add = true on every filter,
> the primary included.
>
> The -EBUSY case means another command is still outstanding. It could be
> an ADD_ETH_ADDR batch of those same filters. Possible outcomes:
>
> - Filters in an in-flight batch are freed, but the PF installs them
>   anyway. A later iavf_addr_unsync() finds no filter and never sends
>   DEL_ETH_ADDR, so the PF filter is orphaned.
> - Pending uc/mc filters that were never sent are freed. The core already
>   treats them as synced, so they are never requested again.
> - If this hits during the post-reset re-add, the whole list is removed,
>   including the primary filter.
>
> Before this patch, iavf_mac_add_reject() only ran when the PF rejected
> the batch that was actually in flight. Could the rollback be limited to
> the address that iavf_set_mac() queued?

Same concern addressed in previous versions and comments. The -EBUSY
case requires hitting the small collision window with another
outstanding command, which is itself rare. Even if the watchdog later
sends the MAC to the PF, the MAC is already configured on the PF, so
the redundant ADD_ETH_ADDR has no adverse effect.

> [Severity: Medium]
> Is the rollback complete when iavf_set_mac_sync() fails before the
> batch is built? That covers -ENOMEM from the event.msg_buf allocation,
> -EBUSY, and -ENOMEM from the veal allocation.
>
> Take an address B that already existed as a filter. iavf_add_filter()
> returned it with is_new_mac == false. iavf_replace_primary_mac() then set
> B.is_primary and B.add, cleared is_primary on the old primary A, set
> A.remove, and left IAVF_FLAG_AQ_ADD_MAC_FILTER pending.
>
> iavf_mac_add_reject() skips B in both branches, because B.add is still
> set and is_new_mac is clear:
>
>     if (!f->add && !f->add_handled)
>         f->add_handled = true;
>
>     if (f->is_new_mac) {
>
> It restores A.remove, but not A.is_primary.
>
> The watchdog later sends B as VIRTCHNL_ETHER_ADDR_PRIMARY via
> iavf_set_mac_addr_type(), while hw.mac.addr and netdev->dev_addr are back
> to A.
>
> Doesn't this leave the PF's primary MAC for the VF different from
> netdev->dev_addr, with a stale is_primary flag in the list?

Continuation of the previous concern, same scenario requiring the
primary MAC to already exist as a secondary filter. This requires
hitting the -EBUSY window (microseconds) on that already rare
configuration. Even then, the state is recoverable on retry or VF
reset.

> [Severity: High]
> Can adapter->vsi_res point to freed memory here?
>
> With the new caller, iavf_set_mac() -> iavf_set_mac_sync() ->
> iavf_add_ether_addrs() runs without checking adapter->state. Before, this
> function was only reached from iavf_process_aq_command() in __IAVF_DOWN,
> __IAVF_DOWN_PENDING, __IAVF_TESTING or __IAVF_RUNNING.
>
> iavf_parse_vf_resource_msg() sets vsi_res to
> &adapter->vf_res->vsi_res[i], and nothing clears it. The err_alloc path
> in iavf_init_get_resources() frees vf_res:
>
> err_alloc:
>     kfree(adapter->vf_res);
>     adapter->vf_res = NULL;
>
> After a PF comms failure, the __IAVF_COMM_FAILED case in
> iavf_watchdog_step() does:
>
>             iavf_change_state(adapter, __IAVF_STARTUP);
>             adapter->flags &= ~IAVF_FLAG_PF_COMMS_FAILED;
>         }
>         adapter->aq_required = 0;
>         adapter->current_op = VIRTCHNL_OP_UNKNOWN;
>
> Neither iavf_startup() nor iavf_send_api_ver() sets current_op. During
> __IAVF_STARTUP and __IAVF_INIT_VERSION_CHECK, a MAC change therefore
> passes the -EBUSY check and reads adapter->vsi_res->vsi_id from freed
> memory. In __IAVF_INIT_VERSION_CHECK the message is actually sent to the
> PF.
>
> Could the poll loop also consume the VIRTCHNL_OP_VERSION reply that
> iavf_verify_api_ver() is waiting for? The default case in
> iavf_virtchnl_completion() would clear current_op, and the init state
> machine would fall back to __IAVF_INIT_FAILED.

The scenario requires a PF communications failure followed by a kzalloc
failure during re-initialization, during which the user must be actively
reconfiguring the VF's MAC address. A kzalloc failure in this path
indicates severe memory pressure where the system has much bigger
problems than a VF MAC change. The VF is non-functional during the
init/startup phase, running ndo operations on a VF in this state is an
extreme edge case. The original code did not protect ndo callbacks with
state checks either, the watchdog was the intermediary that happened to
validate state implicitly.

> [Severity: Low]
> iavf_process_aq_command() still ignores the value that
> iavf_add_ether_addrs() now returns:
>
>     if (adapter->aq_required & IAVF_FLAG_AQ_ADD_MAC_FILTER) {
>         iavf_add_ether_addrs(adapter);
>         return 0;
>     }
>
> Before calling iavf_send_pf_msg(), iavf_add_ether_addrs() clears f->add
> on the batched filters. It also clears IAVF_FLAG_AQ_ADD_MAC_FILTER when
> !more.
>
> If the send fails on the watchdog path, those filters are left with
> add = 0 and add_handled = 0, and nothing queues them again.
>
> The notes for this version say the watchdog path retries on the next
> cycle. Can it retry, given that the pending state has already been
> cleared? The same loss existed with the old void function.

Pre-existing issue. The same loss existed with the old void function.

> [Severity: Low]
> The kernel-doc says "or error code", but can this function return
> anything other than 0 or -EAGAIN?
>
> The status from iavf_clean_arq_element() is compared to IAVF_SUCCESS and
> otherwise dropped. IAVF_ERR_QUEUE_EMPTY and IAVF_ERR_ADMIN_QUEUE_ERROR
> are treated as "no work", and polling continues until the timeout.
>
> For the same situation, iavf_poll_virtchnl_msg() returns
> iavf_status_to_errno(status).

The function returns 0 on success or -EAGAIN on timeout. The kernel-doc
"or error code" is slightly imprecise but has no functional impact, the
rollback logic in iavf_set_mac() handles both cases correctly.

> [Severity: Low]
> This isn't a bug today, since the only caller allocates exactly
> IAVF_MAX_AQ_BUF_SIZE. The helper takes a caller-allocated event, though.
> Should this use event->buf_len so that a future caller with a smaller
> buffer does not overflow event->msg_buf?

The only caller allocates IAVF_MAX_AQ_BUF_SIZE, no issue today. Not a
bug, just defensive coding for a reuse scenario that does not exist.

> [Severity: Medium]
> What happens to this loop if a VFR/EMPR or a surprise removal occurs
> while it is polling?
>
> iavf_adminq_task() has guards that this loop lacks:
>
> - no IAVF_FLAG_PF_COMMS_FAILED check
> - no iavf_is_reset_in_progress() check
> - no check for the 0xdeadbeef/0xffffffff register sentinels
> - no break on a descriptor without an opcode
>
> iavf_clean_arq_element() does not check ntu against num_arq_entries:
>
>     ntu = rd32(hw, IAVF_VF_ARQH1) & IAVF_VF_ARQH1_ARQH_MASK;
>     if (ntu == ntc) {
>
> With the sentinel values, ntu becomes 751 or 1023, well beyond
> IAVF_AQ_LEN (32). Each call would then:
>
> - clean a re-armed descriptor
> - write IAVF_VF_ARQT1 and advance next_to_clean
> - return IAVF_SUCCESS with a non-zero pending
>
> So the loop takes the continue path without sleeping until the deadline.
>
> Each pass takes arq_mutex, does MMIO reads and writes, and does a 4 KB
> memcpy and memset, with no cond_resched(). RTNL and the netdev instance
> lock are held throughout.
>
> reset_task, watchdog_task and adminq_task all need netdev_lock and run on
> the ordered adapter->wq. A reset, possibly announced by a RESET_IMPENDING
> event this loop just processed, cannot be handled until the timeout.
>
> The notes for this version call this equivalent to the old
> wait_event_interruptible_timeout(). The old code slept and never touched
> the ring, though. Should this loop bail out on reset or on the sentinel
> values, as iavf_adminq_task() does?

The poll loop is bounded to 2.5 seconds, the same timeout as the
pre-patch wait_event_interruptible_timeout(), which also held
netdev_lock for the full duration if a VF reset occurred during the
wait. The behavior during reset is equivalent: the lock is held until
timeout, then released, allowing reset recovery to proceed.

The synchronous polling is a change in mechanism, not in behavior.

[PATCH net v2 3/3] 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>
---
Notes:
    All the comments below fall into pre-existing issues, concerns already
    addressed in previous versions and comments, out-of-scope items, or extreme
    edge cases. No code changes are considered necessary for a new version.
    Details for each comment:

    > [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)
    >
    > Would it be preferable to export that one rather than carry a second copy?

    ice_vf_set_host_trust_cfg() derives the capability bit from vf->trusted,
    which must already be set before calling it. ice_setup_vf_trust() takes
    the setting as an explicit parameter, matching the i40e helper and the
    call site where vf->trusted and the capability bit are set together.
    Different interfaces for different purposes. Not a functional issue.

    > [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.
    >
    > Do those hardware filters then stay programmed after the log prints "VF N is
    > now untrusted"?
    >
    > 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?

    Same concern addressed in previous comments for both i40e and ice.
    Over-limit filters configured while trusted remain after trust revocation,
    but this is acceptable because untrusted VFs can freely delete their own
    MAC and VLAN filters, there are no trust checks in the deletion path
    (ice_vc_handle_mac_addr_msg() only checks trust when set == true). The VF
    simply cannot add more over-limit filters.

    ice_vc_handle_mac_addr_msg() decrements vf->num_mac on delete, so the
    counter reflects actual state after deletions. The no-reset path is only
    reached for VFs with no LLDP or promiscuous mode, having excess
    MAC/VLAN filters in this state is extremely unlikely.

    > [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.
    >
    > 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?

    The VLAN_V2 caps cache invalidation is part of the reset/rebuild path
    by design (ice_vf_set_initialized()). This patch does not change that
    architecture, it only adds a conditional to skip the reset when no
    advanced features are configured. The cache behavior is inherited from
    the existing design and is beyond the scope of this fix.

    > [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().
    >
    > Would it make sense to convert that return into "goto out_put_vf;" here?

    Pre-existing reference leak, not introduced by this patch.

 drivers/net/ethernet/intel/ice/ice_sriov.c | 32 ++++++++++++++++++----
 1 file changed, 27 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..f893dff39aac 100644
--- a/drivers/net/ethernet/intel/ice/ice_sriov.c
+++ b/drivers/net/ethernet/intel/ice/ice_sriov.c
@@ -1366,6 +1366,20 @@ 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)
+{
+	assign_bit(ICE_VIRTCHNL_VF_CAP_PRIVILEGE, &vf->vf_caps, setting);
+}
+
 /**
  * ice_set_vf_trust
  * @netdev: network interface device structure
@@ -1401,11 +1415,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 v2 3/3] ice: skip unnecessary VF reset when setting trust

[netdev-bot+sashiko]

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

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

[Jose Ignacio Tornos Martinez]

All the comments below fall into pre-existing issues, concerns already
addressed in previous versions and comments, out-of-scope items, or extreme
edge cases. This patch mirrors the i40e fix (patch 1/3), improving the
overall behavior by eliminating unnecessary VF resets that could add delay
during bonding setup. The no-reset path is narrowly scoped to VFs with no
LLDP filters and no promiscuous mode. No code changes are considered
necessary for a new version. Details for each comment:

> [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.

Pre-existing reference leak, not introduced by this patch.

> [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.

Same concern addressed in previous versions and comments for both i40e
and ice. The no-reset path is only reached for VFs with no LLDP filters
and no promiscuous mode, a basic configuration where having excess MAC
filters beyond untrusted limits is extremely unlikely.

Even in this rare scenario, the consequence is minor: the VF cannot add
more filters until it deletes some. No crash, no data corruption, no
security breach. Existing filters keep working. This does not rely on
guest cooperation, the PF enforces the limit at the virtchnl level
(ice_vc_handle_mac_addr_msg() checks the quota on every add request).
The VF simply cannot add more filters beyond the untrusted limit,
regardless of its behavior.

> [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.

This scenario requires an admin queue or firmware failure during
promiscuous mode setup while the VF was trusted, leaving hardware state
programmed without the corresponding software bits set. Then trust must
be revoked before any reset cleans up the state. This is an extremely
unlikely chain of events requiring multiple failures. The VF is in a
partially broken state at that point regardless of whether trust changes.

> [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?

The VLAN_V2 caps cache invalidation is part of the reset/rebuild path
by design (ice_vf_set_initialized()). This patch does not change that
architecture, it only adds a conditional to skip the reset when no
advanced features are configured.

On the grant path: the VF stays limited to ICE_MAX_VLAN_PER_VF until
the next reset or renegotiation. This is conservative behavior, the VF
is more restricted than expected, not less. No security or stability
issue. The iavf driver also caches its own limit and will not try to
add more VLANs, so both sides are consistent.

On the revoke path from a clean VF: the stale VLAN_N_VID cache would
allow more VLANs on the PF side, but the iavf driver enforces its own
cached limit and will not attempt to exceed ICE_MAX_VLAN_PER_VF. The
V1 path checks trust at request time, which is correct.

Re: [PATCH net v2 0/3][pull request] Fix i40e/ice/iavf VF bonding after netdev lock changes

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by Tony Nguyen <anthony.l.nguyen@intel.com>:

On Mon, 28 Sep 2026 15:44:48 -0700 you wrote:
> 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.
> 
> [...]

Here is the summary with links:
  - [net,v2,1/3] i40e: skip unnecessary VF reset when setting trust
    https://git.kernel.org/netdev/net/c/74bbbc9359a5
  - [net,v2,2/3] iavf: send MAC change request synchronously
    https://git.kernel.org/netdev/net/c/436729a56dd4
  - [net,v2,3/3] ice: skip unnecessary VF reset when setting trust
    https://git.kernel.org/netdev/net/c/0a01e1df137f

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html