[PATCH net 00/10][pull request] Intel Wired LAN Driver Updates 2026-08-04 (iavf, i40e, ice, igc)

[PATCH net 00/10][pull request] Intel Wired LAN Driver Updates 2026-08-04 (iavf, i40e, ice, igc)

[Tony Nguyen]

Jose Ignacio Tornos Martinez fixes issues with VF bonding that came
about with commit ad7c7b2172c3 ("net: hold netdev instance lock during
sysfs operations").

Further details:
https://lore.kernel.org/netdev/20260623101800.991293-1-jtornosm@redhat.com/

Przemek fixes an issue on ice which increasing VF queue count causes
out-of-bounds access of the statistics arrays.

Doruk Tan Ozturk changes call of metadata_dst_free() to dst_release()
to account for refcount when freeing representors on ice.

Aaron Esau fixes a bad DCB configuration comparison in
i40e_hw_set_dcb_config() by comparing the structures instead of the pointer
variables.

Dawei Feng correct ordering of error path in i40e_vsi_setup() to stop
possible memory leak.

Philipp David changes call of netif_device_attach() to be unconditional
during igc resume, otherwise interfaces that were down prior to the call
will not be restored.

The following are changes since commit a347304b2ca1a5377d5bd2d8a72e4b4f12afe648:
  net/sched: cls_api: Always acquire rtnl_lock when destroying locked classifiers
and are available in the git repository at:
  git://git.kernel.org/pub/scm/linux/kernel/git/tnguy/net-queue 40GbE

Aaron Esau (1):
  i40e: fix memcmp of pointer in i40e_hw_set_dcb_config()

Dawei Feng (1):
  i40e: fix netdev leak in i40e_vsi_setup() error paths

Doruk Tan Ozturk (1):
  ice: eswitch: fix use-after-free of metadata_dst in repr release

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

Philipp David (1):
  igc: fix netdev not re-attached after resume if interface is down

Przemek Kitszel (2):
  ice: move ice_vsi_realloc_stat_arrays() up
  ice: fix stats array overflow via proper realloc

 drivers/net/ethernet/intel/i40e/i40e_main.c   |   8 +-
 .../ethernet/intel/i40e/i40e_virtchnl_pf.c    |  38 +++-
 drivers/net/ethernet/intel/iavf/iavf.h        |  11 +-
 drivers/net/ethernet/intel/iavf/iavf_main.c   |  88 ++++++--
 .../net/ethernet/intel/iavf/iavf_virtchnl.c   |  99 ++++++++-
 drivers/net/ethernet/intel/ice/ice.h          |   2 +
 drivers/net/ethernet/intel/ice/ice_eswitch.c  |   2 +-
 drivers/net/ethernet/intel/ice/ice_lib.c      | 191 ++++++++----------
 drivers/net/ethernet/intel/ice/ice_sriov.c    |  35 +++-
 drivers/net/ethernet/intel/igc/igc_main.c     |   8 +-
 10 files changed, 321 insertions(+), 161 deletions(-)

-- 
2.47.1

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

[Tony Nguyen]

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

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

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

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

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

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

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

[Jakub Kicinski]

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

This adds an early bail-out in iavf_set_mac() so that a MAC address change
requested while the VF is resetting, or before the adapter reaches
__IAVF_DOWN, returns -EBUSY instead of waiting with the netdev instance
lock held.

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

[Severity: Low]
Is the placement of the new gate ahead of is_valid_ether_addr()
intentional?

With this ordering, a request carrying an address that can never be
accepted (all-zero, multicast, other non-unicast MAC) now reports -EBUSY
rather than -EADDRNOTAVAIL whenever iavf_is_reset_in_progress(adapter) is
true or adapter->state < __IAVF_DOWN.

The core does not validate the address itself, so the driver check is the
only one on the rtnetlink/ioctl/sysfs paths:

net/core/dev.c:netif_set_mac_address() {
	if (!ops->ndo_set_mac_address)
		return -EOPNOTSUPP;
	if (ss->ss_family != dev->type)
		return -EINVAL;
	if (!netif_device_present(dev))
		return -ENODEV;
	...
		err = ops->ndo_set_mac_address(dev, ss);
}

Since -EBUSY advertises a transient condition, callers that retry on EBUSY
would loop on input that can never succeed. Would moving the state gate
below the is_valid_ether_addr() check work equally well for the stated
goal?

The ordering is still present at the end of the series, and the commit
message does not mention the change in returned errno for invalid
addresses. Could that be noted, or the checks reordered?

[PATCH net 02/10] i40e: skip unnecessary VF reset when setting trust

[Tony Nguyen]

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

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

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

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

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

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

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

diff --git a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
index a26c3d47ec15..0cc434b26eb8 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_virtchnl_pf.c
@@ -4943,6 +4943,23 @@ int i40e_ndo_set_vf_spoofchk(struct net_device *netdev, int vf_id, bool enable)
 	return ret;
 }
 
+/**
+ * i40e_setup_vf_trust - Enable/disable VF trust mode without reset
+ * @vf: VF to configure
+ * @setting: trust setting
+ *
+ * Update VF flags when changing trust without performing a VF reset.
+ * This is only called when it's safe to skip the reset (VF has no advanced
+ * features configured that need cleanup).
+ */
+static void i40e_setup_vf_trust(struct i40e_vf *vf, bool setting)
+{
+	if (setting)
+		set_bit(I40E_VIRTCHNL_VF_CAP_PRIVILEGE, &vf->vf_caps);
+	else
+		clear_bit(I40E_VIRTCHNL_VF_CAP_PRIVILEGE, &vf->vf_caps);
+}
+
 /**
  * i40e_ndo_set_vf_trust
  * @netdev: network interface device structure of the pf
@@ -4987,19 +5004,20 @@ int i40e_ndo_set_vf_trust(struct net_device *netdev, int vf_id, bool setting)
 	set_bit(__I40E_MACVLAN_SYNC_PENDING, pf->state);
 	pf->vsi[vf->lan_vsi_idx]->flags |= I40E_VSI_FLAG_FILTER_CHANGED;
 
-	i40e_vc_reset_vf(vf, true);
+	/* Reset only if revoking trust and VF has advanced features configured */
+	if (!setting &&
+	    (vf->adq_enabled || vf->num_cloud_filters > 0 ||
+	     test_bit(I40E_VF_STATE_UC_PROMISC, &vf->vf_states) ||
+	     test_bit(I40E_VF_STATE_MC_PROMISC, &vf->vf_states))) {
+		i40e_vc_reset_vf(vf, true);
+		i40e_del_all_cloud_filters(vf);
+	} else {
+		i40e_setup_vf_trust(vf, setting);
+	}
+
 	dev_info(&pf->pdev->dev, "VF %u is now %strusted\n",
 		 vf_id, setting ? "" : "un");
 
-	if (vf->adq_enabled) {
-		if (!vf->trusted) {
-			dev_info(&pf->pdev->dev,
-				 "VF %u no longer Trusted, deleting all cloud filters\n",
-				 vf_id);
-			i40e_del_all_cloud_filters(vf);
-		}
-	}
-
 out:
 	clear_bit(__I40E_VIRTCHNL_OP_PENDING, pf->state);
 	return ret;
-- 
2.47.1

Re: [PATCH net 02/10] i40e: skip unnecessary VF reset when setting trust

[Jakub Kicinski]

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

This changes i40e_ndo_set_vf_trust() so that granting trust never resets
the VF, and revoking trust only resets when the VF has ADQ, cloud filters
or promiscuous mode configured. A new helper i40e_setup_vf_trust() flips
I40E_VIRTCHNL_VF_CAP_PRIVILEGE directly when the reset is skipped.

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

[Severity: Low]
This isn't a bug, but is the stated precondition accurate? The only call
site is the else branch of a condition that begins with !setting, so the
helper runs for every trust grant no matter what the VF has configured.

An untrusted VF can already have "advanced features" configured:
i40e_vc_add_qch_msg() sets vf->adq_enabled = true after checking spoofchk,
negotiated ADQ capability, TC count and queue availability, with no
trust/privilege test, and i40e_validate_cloud_filter() only requires
I40E_VIRTCHNL_VF_CAP_PRIVILEGE for advanced-mode filters.

Would it be clearer to document the real contract, i.e. "called whenever
the reset is skipped: on every grant, and on revokes where no cleanup is
deemed necessary"?

> +static void i40e_setup_vf_trust(struct i40e_vf *vf, bool setting)
> +{
> +	if (setting)
> +		set_bit(I40E_VIRTCHNL_VF_CAP_PRIVILEGE, &vf->vf_caps);
> +	else
> +		clear_bit(I40E_VIRTCHNL_VF_CAP_PRIVILEGE, &vf->vf_caps);
> +}

[Severity: Medium]
Can this write of the privilege bit be clobbered by a concurrent VF reset?

i40e_alloc_vf_res() recomputes the same bit from a plain read of
vf->trusted, with no lock and no READ_ONCE():

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

Its only caller is i40e_cleanup_reset_vf(), reached from i40e_reset_vf()
in the service task for guest-initiated resets (VIRTCHNL_OP_RESET_VF and
VFLR). i40e_vc_process_vf_msg() is dispatched from
i40e_clean_adminq_subtask() without rtnl_lock() and without
__I40E_VIRTCHNL_OP_PENDING, so it runs in parallel with the ndo handler:

service task				ndo path (rtnl held)
i40e_alloc_vf_res()
  reads old vf->trusted
					vf->trusted = setting;
					i40e_setup_vf_trust(vf, setting);
  set_bit()/clear_bit() using
  the stale value

vf->trusted and I40E_VIRTCHNL_VF_CAP_PRIVILEGE then stay inconsistent
until some later reset. Before this patch i40e_ndo_set_vf_trust() always
called i40e_vc_reset_vf(), which retries i40e_reset_vf() (serialized by
test_and_set_bit(I40E_VF_STATE_RESETTING)) until a reset happened in the
caller's own thread, so the bit was always recomputed last from the
up-to-date vf->trusted. Does the new path need a barrier or to hold off
against I40E_VF_STATE_RESETTING?

>  /**
>   * i40e_ndo_set_vf_trust
>   * @netdev: network interface device structure of the pf
> @@ -4987,19 +5004,20 @@ int i40e_ndo_set_vf_trust(struct net_device *netdev, int vf_id, bool setting)
>  	set_bit(__I40E_MACVLAN_SYNC_PENDING, pf->state);
>  	pf->vsi[vf->lan_vsi_idx]->flags |= I40E_VSI_FLAG_FILTER_CHANGED;
>  
> -	i40e_vc_reset_vf(vf, true);
> +	/* Reset only if revoking trust and VF has advanced features configured */
> +	if (!setting &&
> +	    (vf->adq_enabled || vf->num_cloud_filters > 0 ||
> +	     test_bit(I40E_VF_STATE_UC_PROMISC, &vf->vf_states) ||
> +	     test_bit(I40E_VF_STATE_MC_PROMISC, &vf->vf_states))) {
> +		i40e_vc_reset_vf(vf, true);
> +		i40e_del_all_cloud_filters(vf);
> +	} else {
> +		i40e_setup_vf_trust(vf, setting);
> +	}

[Severity: High]
Does revoking trust still remove the MAC and VLAN filters that only a
trusted VF was allowed to install?

The new condition does not consider filter state, so a VF with no ADQ, no
cloud filters and no promiscuous bits takes the else branch, which only
clears the privilege bit.

While trusted, i40e_check_vf_permission() lets the VF install MACs that
override the administratively set vf->pf_set_mac address and raises the
limit from I40E_VC_MAX_MAC_ADDR_PER_VF to
I40E_VC_MAX_MACVLAN_PER_TRUSTED_VF():

	if (!vf_trusted && !is_multicast_ether_addr(addr) &&
	    vf->pf_set_mac && !ether_addr_equal(addr, vf->default_lan_addr.addr)) {
		... return -EPERM;
	}
	...
	if (!vf_trusted)
		mac_add_max = I40E_VC_MAX_MAC_ADDR_PER_VF;
	else
		mac_add_max = I40E_VC_MAX_MACVLAN_PER_TRUSTED_VF(...);

i40e_vc_add_vlan_msg() likewise only enforces I40E_VC_MAX_VLAN_PER_VF on
add when the privilege bit is clear.

Previously the unconditional i40e_vc_reset_vf() reached
i40e_cleanup_reset_vf() -> i40e_free_vf_res(), which does
i40e_vsi_release(pf->vsi[vf->lan_vsi_idx]), destroying the VSI and all its
filters; i40e_alloc_vsi_res() then re-added only vf->default_lan_addr and
broadcast, and i40e_cleanup_reset_vf() set vf->num_vlan = 0.

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:

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

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.

The commit message says "For VFs in a clean state, we can safely change
the trust setting without the disruptive reset" and lists only "ADQ/cloud
filters, promiscuous mode" as state requiring cleanup. Should the MAC and
VLAN filter state be part of that list, or at least mentioned as a
behaviour change?

[Severity: High]
Is there a check-then-act window here against the virtchnl handlers?

vf->adq_enabled, vf->num_cloud_filters and the promisc state bits are
written from the service task via i40e_clean_adminq_subtask() ->
i40e_vc_process_vf_msg(), which takes neither rtnl_lock() nor
__I40E_VIRTCHNL_OP_PENDING (all test_and_set_bit() sites for that bit are
ndo/sriov_configure entry points). The privilege bit is only cleared after
the sampling:

CPU0 i40e_ndo_set_vf_trust()
	vf->trusted = false;
	test_bit(I40E_VF_STATE_UC_PROMISC) == 0
	test_bit(I40E_VF_STATE_MC_PROMISC) == 0

CPU1 i40e_vc_config_promiscuous_mode_msg()
	/* privilege bit still set */
	i40e_config_vf_promiscuous_mode(vf, info->vsi_id, allmulti, alluni);
	... i40e_aq_set_vsi_unicast_promiscuous()/multicast in HW
	test_and_set_bit(I40E_VF_STATE_UC_PROMISC, &vf->vf_states)

CPU0
	i40e_setup_vf_trust(vf, false);	/* clears privilege, no reset */

Does anything clear the hardware promiscuous flags afterwards? A later VF
request to disable promiscuous mode is rejected once the privilege bit is
clear, and the PF reports success to the VF:

	if (!test_bit(I40E_VIRTCHNL_VF_CAP_PRIVILEGE, &vf->vf_caps)) {
		... aq_ret = 0; goto err_out;
	}

and i40e_sync_vsi_filters() refuses to touch promiscuous for an untrusted
SRIOV VSI. The same window applies to the plain writes vf->adq_enabled =
true in i40e_vc_add_qch_msg() and vf->num_cloud_filters++ in
i40e_vc_add_cloud_filter(), which are read here without READ_ONCE() or a
lock. Since the guest chooses when to send those messages, it can toggle
promisc off and on to make the sampled bits read zero.

[Severity: Medium]
Can the software promisc bits under-report the hardware state?

In i40e_vc_config_promiscuous_mode_msg() the state bits are only set after
a fully successful call:

	aq_ret = i40e_config_vf_promiscuous_mode(vf, info->vsi_id, allmulti, alluni);
	if (aq_ret)
		goto err_out;

but i40e_set_vsi_promisc() issues several admin queue commands and can
succeed on some and fail on others:

	aq_ret = i40e_aq_set_vsi_multicast_promiscuous(hw, seid, multi_enable, NULL);
	if (aq_ret) { ... return aq_ret; }
	aq_ret = i40e_aq_set_vsi_unicast_promiscuous(hw, seid, unicast_enable, NULL, true);
	if (aq_ret) { ... }
	return aq_ret;

If multicast promiscuous is enabled in firmware and the unicast command
fails (or only some VLANs are programmed in the per-VLAN loop), no state
bit is recorded while the VSI is promiscuous in hardware. Trust revocation
then takes the no-reset branch and nothing clears it:
i40e_sync_vsi_filters() does

	if (vsi->type == I40E_VSI_SRIOV && pf->vf &&
	    !pf->vf[vsi->vf_id].trusted) {
		clear_bit(__I40E_VSI_OVERFLOW_PROMISC, vsi->state);
		goto out;
	}

without issuing any promiscuous AQ command. Before this patch
i40e_cleanup_reset_vf() unconditionally ran

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

regardless of the software bits. Would it be safer to call that
unconditionally on revoke rather than infer the state from the bits? Note
this one depends on an admin queue command failing part way through, which
I could not show a guest or admin can force.

> +
>  	dev_info(&pf->pdev->dev, "VF %u is now %strusted\n",
>  		 vf_id, setting ? "" : "un");
>  
> -	if (vf->adq_enabled) {
> -		if (!vf->trusted) {
> -			dev_info(&pf->pdev->dev,
> -				 "VF %u no longer Trusted, deleting all cloud filters\n",
> -				 vf_id);
> -			i40e_del_all_cloud_filters(vf);
> -		}
> -	}
> -
>  out:
>  	clear_bit(__I40E_VIRTCHNL_OP_PENDING, pf->state);
>  	return ret;

[PATCH net 03/10] iavf: send MAC change request synchronously

[Tony Nguyen]

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

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

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

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

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

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

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

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

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

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

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

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

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

Re: [PATCH net 03/10] iavf: send MAC change request synchronously

[Jakub Kicinski]

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

This converts iavf_set_mac() from waiting on vc_waitqueue for the watchdog
to send the ADD_ETH_ADDR message into sending it inline and polling the
admin queue with a new generic helper, iavf_poll_virtchnl_response().
iavf_add_ether_addrs() now returns an error code and iavf_mac_add_reject()
is exported so local failures can be rolled back.

A few questions about the new synchronous path below.

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

[Severity: Medium]
Can this callback report completion before the ADD_ETH_ADDR reply is seen?

v_op is discarded, and iavf_poll_virtchnl_response() runs the callback after
processing any received element, so an unrelated message satisfies it too.

For a MAC that is already on mac_filter_list from an earlier successful add,
iavf_mac_add_ok() left add_handled == true, and iavf_replace_primary_mac()
only sets add = true.  iavf_add_ether_addrs() then clears f->add while
packing the message, so iavf_is_mac_set_handled() is already true:

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

The first unrelated ARQ element (for example a VIRTCHNL_OP_EVENT link change
that piled up while iavf_adminq_task() was blocked on netdev_lock) then makes
the poll break with ret = 0.  netdev->dev_addr is only updated in the
VIRTCHNL_OP_ADD_ETH_ADDR case of iavf_virtchnl_completion(), which has not
run yet, so iavf_set_mac() falls into:

	if (!ether_addr_equal(netdev->dev_addr, addr->sa_data))
		return -EACCES;

and userspace gets -EACCES for a request that is still in flight and will be
accepted.  Would matching received_op against VIRTCHNL_OP_ADD_ETH_ADDR in the
callback avoid this?

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

[Severity: Low]
What happens here when the pending add-filter list does not fit into one
message?  iavf_add_ether_addrs() truncates the batch and returns 0:

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

iavf_add_filter() appends with list_add_tail() and the packing loop walks
from the head, so the newly requested primary MAC is the entry most likely
to be left out.  Its f->add stays true, so iavf_mac_change_done() can never
become true, and the leftovers can only go out from the watchdog, which
cannot run while iavf_set_mac() holds the netdev instance lock.

That leaves the poll sleeping the full 2500 ms and returning -EAGAIN, which
is the timeout plus -EAGAIN behaviour the commit message describes as fixed,
with no way for the caller to tell that its filter was never sent.

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

[Severity: Medium]
Can this rollback drop unrelated unicast and multicast filters?

iavf_mac_add_reject() is not a per-request rollback; it walks the whole
mac_filter_list and unconditionally frees every entry with is_new_mac set:

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

iavf_add_filter() sets is_new_mac = true for every newly allocated filter:

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

which includes addresses queued from iavf_set_rx_mode() -> iavf_addr_sync().
Those returned 0 to __hw_addr_sync_dev(), so the core already marked them
synced and will not ask the driver for them again:

	if (iavf_add_filter(adapter, addr))
		return 0;

So a MAC change that fails locally (-EBUSY or -ENOMEM, where nothing was
transmitted) appears to silently drop those addresses until a VF reset
re-syncs everything.  The same sweep also forces add_handled on all filters
with add == false, including ones belonging to a batch that is still in
flight, and never restores is_primary on the previous primary filter that
iavf_replace_primary_mac() cleared.

The kernel-doc of iavf_mac_add_reject() still says "Remove filters from list
based on PF response", which no longer matches these new callers where there
is no PF response at all.

[Severity: Medium]
Is the rollback complete when the requested MAC is already on the list with
is_new_mac == false?  iavf_mac_add_ok() clears is_new_mac on all filters
after any successful add cycle:

	list_for_each_entry_safe(f, ftmp, &adapter->mac_filter_list, list) {
		f->is_new_mac = false;

On -EBUSY (iavf_add_ether_addrs() returns before touching the list) or
-ENOMEM (returns before the packing loop), f->add and f->is_primary are
still set from iavf_replace_primary_mac():

	new_f->is_primary = true;
	new_f->add = true;
	ether_addr_copy(hw->mac.addr, new_mac);
	...
	iavf_schedule_aq_request(adapter, IAVF_FLAG_AQ_ADD_MAC_FILTER);

iavf_mac_add_reject() only touches filters with !f->add or is_new_mac, so
this one survives untouched and IAVF_FLAG_AQ_ADD_MAC_FILTER is still armed.

Once iavf_set_mac() returns the error and the lock is dropped, the watchdog
sends VIRTCHNL_OP_ADD_ETH_ADDR with VIRTCHNL_ETHER_ADDR_PRIMARY for a change
userspace was told had failed, while hw.mac.addr has been rolled back.  In
the completion path netdev->dev_addr then equals the rolled-back hw.mac.addr,
so dev_addr is not updated and the PF ends up using the new address as the
VF primary MAC while the driver still reports the old one.

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

[ ... ]

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

[ ... ]

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

[Severity: Medium]
On the new synchronous path, is this -EBUSY transient or permanent?

current_op is set by every virtchnl send (for example the watchdog's
iavf_request_stats() issuing GET_STATS) and is only cleared at the end of
iavf_virtchnl_completion().  For the asynchronous path that runs from
iavf_adminq_task(), which needs the same lock iavf_set_mac() is holding:

iavf_adminq_task()
	netdev_lock(netdev);

So while iavf_set_mac() -> iavf_set_mac_sync() is running, an already
in-flight command can never complete and current_op can never go back to
VIRTCHNL_OP_UNKNOWN.  iavf_set_mac_sync() sends once and does not reach
iavf_poll_virtchnl_response(), so the outstanding reply is never drained.

iavf_set_mac() then takes the ret != -EAGAIN branch, runs the
iavf_mac_add_reject() rollback and returns -EBUSY to userspace.  Doesn't that
make a plain MAC change fail in exactly the workload described in the commit
message, several VFs enslaved in quick succession while the watchdog is
issuing virtchnl commands?  Would polling first, or retrying the send after
draining the queue, be more appropriate here?

>  
>  	spin_lock_bh(&adapter->mac_vlan_list_lock);

[ ... ]

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

[Severity: Medium]
Does a return of 0 here always mean the message reached the admin send
queue?  iavf_send_pf_msg() short-circuits:

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

In that case nothing is posted, current_op stays at VIRTCHNL_OP_ADD_ETH_ADDR
(it is only restored on an error return), and the filters have already had
f->add cleared during packing.  iavf_poll_virtchnl_response() then polls a
shut-down queue where iavf_clean_arq_element() returns IAVF_ERR_QUEUE_EMPTY
every iteration (hw->aq.arq.count == 0) for the full 2500 ms under
netdev_lock, and returns -EAGAIN.

The comment in iavf_set_mac() says "the message was sent and PF will
eventually respond" and skips the rollback, so hw.mac.addr keeps the new
address while netdev->dev_addr keeps the old one, and current_op stays armed
so every later command hits "Cannot add filters, command %d pending" until
the watchdog's comm-failed path clears it.

The window looks reachable: iavf_disable_vf() sets the flag, shuts the admin
queue down, clears the reset flag and ends with __IAVF_DOWN:

	adapter->flags |= IAVF_FLAG_PF_COMMS_FAILED;
	...
	iavf_shutdown_adminq(&adapter->hw);
	adapter->flags &= ~IAVF_FLAG_RESET_PENDING;
	iavf_change_state(adapter, __IAVF_DOWN);

while iavf_set_mac() only rejects state < __IAVF_DOWN and
iavf_is_reset_in_progress().  Should the synchronous path check
IAVF_FLAG_PF_COMMS_FAILED before sending and polling?

[ ... ]

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

[ ... ]

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

[Severity: Low]
Can this function ever return the documented "error code"?  ret is only ever
-EAGAIN or 0, and the status from iavf_clean_arq_element() (for example
IAVF_ERR_ADMIN_QUEUE_ERROR for a descriptor with LIBIE_AQ_FLAG_ERR, or
IAVF_ERR_QUEUE_EMPTY for an uninitialised ARQ) is compared against
IAVF_SUCCESS and then dropped.

The other poller on the same queue in this file does the opposite:

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

Since iavf_set_mac() keys its rollback decision on ret != -EAGAIN, reporting
admin queue errors as a plain timeout also feeds the "message was sent"
assumption.  Could the kernel-doc and the implementation be brought in line,
one way or the other?

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

[Severity: Medium]
What happens to this loop if a VF reset starts while it is polling?  There is
no device-in-reset check, no IAVF_FLAG_PF_COMMS_FAILED re-check and no
sanity check on the ARQ head, unlike iavf_adminq_task():

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

and unlike iavf_poll_virtchnl_msg(), which returns -EIO on
VIRTCHNL_EVENT_RESET_IMPENDING.

In iavf_clean_arq_element() the head is masked without a range check:

	ntu = rd32(hw, IAVF_VF_ARQH1) & IAVF_VF_ARQH1_ARQH_MASK;

The mask is 10 bits while num_arq_entries is IAVF_AQ_LEN (32), so a reset
time read of 0xdeadbeef/0xffffffff yields ntu far outside the ring and

	*pending = (ntc > ntu ? hw->aq.arq.count : 0) + (ntu - ntc);

stays non-zero forever.  In the new loop that means "if (!pending)" never
sleeps and "if (pending) continue;" keeps going, so it degenerates into a
tight rd32 / 4 KiB memcpy / tail write / 4 KiB memset spin with no
cond_resched() until the timeout expires, consuming descriptors firmware
never posted and feeding stale contents into iavf_virtchnl_completion().

All of that runs holding netdev_lock, which iavf_reset_task() and
iavf_adminq_task() both need:

iavf_reset_task()
	netdev_lock(netdev);
	iavf_reset_step(adapter);

so reset recovery is delayed for up to 2.5 s, and since adapter->wq is an
ordered workqueue the other iavf work items queue up behind it.
iavf_set_mac() samples iavf_is_reset_in_progress() only once on entry, and
nothing can update those flags while the poll holds the lock, so there is no
way out of the loop early.  Should the loop bail out on the reset register
pattern and on RESET_IMPENDING events?

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

[Severity: Low]
Should this use event->buf_len rather than the hardcoded
IAVF_MAX_AQ_BUF_SIZE?  struct iavf_arq_event_info expresses the size of
msg_buf via buf_len, and iavf_clean_arq_element() honours it:

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

The only caller today, iavf_set_mac_sync(), allocates exactly
IAVF_MAX_AQ_BUF_SIZE, so nothing overruns right now.  But the helper is
exported in iavf.h and its kernel-doc invites reuse "for any future
synchronous virtchnl operations", and other virtchnl call sites such as
iavf_get_vf_supported_rxdids() and iavf_get_vf_ptp_caps() use much smaller
event buffers.  Any such caller would get a 4 KiB out-of-bounds zeroing
write into the heap.

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

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

[Tony Nguyen]

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

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

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

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

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

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

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

diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.c b/drivers/net/ethernet/intel/ice/ice_sriov.c
index e04de0215596..558937899818 100644
--- a/drivers/net/ethernet/intel/ice/ice_sriov.c
+++ b/drivers/net/ethernet/intel/ice/ice_sriov.c
@@ -1366,6 +1366,23 @@ int ice_set_vf_mac(struct net_device *netdev, int vf_id, u8 *mac)
 	return __ice_set_vf_mac(ice_netdev_to_pf(netdev), vf_id, mac);
 }
 
+/**
+ * ice_setup_vf_trust - Enable/disable VF trust mode without reset
+ * @vf: VF to configure
+ * @setting: trust setting
+ *
+ * Update VF flags when changing trust without performing a VF reset.
+ * This is only called when it's safe to skip the reset (VF has no advanced
+ * features configured that need cleanup).
+ */
+static void ice_setup_vf_trust(struct ice_vf *vf, bool setting)
+{
+	if (setting)
+		set_bit(ICE_VIRTCHNL_VF_CAP_PRIVILEGE, &vf->vf_caps);
+	else
+		clear_bit(ICE_VIRTCHNL_VF_CAP_PRIVILEGE, &vf->vf_caps);
+}
+
 /**
  * ice_set_vf_trust
  * @netdev: network interface device structure
@@ -1401,11 +1418,19 @@ int ice_set_vf_trust(struct net_device *netdev, int vf_id, bool trusted)
 
 	mutex_lock(&vf->cfg_lock);
 
-	while (!trusted && vf->num_mac_lldp)
-		ice_vf_update_mac_lldp_num(vf, ice_get_vf_vsi(vf), false);
-
-	vf->trusted = trusted;
-	ice_reset_vf(vf, ICE_VF_RESET_NOTIFY);
+	/* Reset only if revoking trust and VF has advanced features configured */
+	if (!trusted &&
+	    (vf->num_mac_lldp > 0 ||
+	     test_bit(ICE_VF_STATE_UC_PROMISC, vf->vf_states) ||
+	     test_bit(ICE_VF_STATE_MC_PROMISC, vf->vf_states))) {
+		while (vf->num_mac_lldp)
+			ice_vf_update_mac_lldp_num(vf, ice_get_vf_vsi(vf), false);
+		vf->trusted = trusted;
+		ice_reset_vf(vf, ICE_VF_RESET_NOTIFY);
+	} else {
+		vf->trusted = trusted;
+		ice_setup_vf_trust(vf, trusted);
+	}
 	dev_info(ice_pf_to_dev(pf), "VF %u is now %strusted\n",
 		 vf_id, trusted ? "" : "un");
 
-- 
2.47.1

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

[Jakub Kicinski]

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

This changes ice_set_vf_trust() so that granting trust never resets the VF,
and revoking trust only resets when vf->num_mac_lldp is non-zero or one of
the promiscuous state bits is set.  A new helper, ice_setup_vf_trust(),
flips ICE_VIRTCHNL_VF_CAP_PRIVILEGE directly on the no-reset path.

> diff --git a/drivers/net/ethernet/intel/ice/ice_sriov.c b/drivers/net/ethernet/intel/ice/ice_sriov.c
> index e04de02155966..5589378998181 100644
> --- a/drivers/net/ethernet/intel/ice/ice_sriov.c
> +++ b/drivers/net/ethernet/intel/ice/ice_sriov.c
> @@ -1366,6 +1366,23 @@ int ice_set_vf_mac(struct net_device *netdev, int vf_id, u8 *mac)
>  	return __ice_set_vf_mac(ice_netdev_to_pf(netdev), vf_id, mac);
>  }
>  
> +/**
> + * ice_setup_vf_trust - Enable/disable VF trust mode without reset
> + * @vf: VF to configure
> + * @setting: trust setting
> + *
> + * Update VF flags when changing trust without performing a VF reset.
> + * This is only called when it's safe to skip the reset (VF has no advanced
> + * features configured that need cleanup).
> + */

[Severity: Low]
This isn't a bug, but does the kernel-doc match the only call site?  The
comment says the helper "is only called when it's safe to skip the reset (VF
has no advanced features configured that need cleanup)", yet
ice_set_vf_trust() takes the else branch for every trust grant without
looking at vf->num_mac_lldp, the promiscuous bits, vf->num_mac or the VLAN
count at all, and on revoke it only consults the LLDP counter and the two
promiscuous bits.  Would it be clearer to describe the condition the caller
actually enforces?

> +static void ice_setup_vf_trust(struct ice_vf *vf, bool setting)
> +{
> +	if (setting)
> +		set_bit(ICE_VIRTCHNL_VF_CAP_PRIVILEGE, &vf->vf_caps);
> +	else
> +		clear_bit(ICE_VIRTCHNL_VF_CAP_PRIVILEGE, &vf->vf_caps);
> +}
> +
>  /**
>   * ice_set_vf_trust
>   * @netdev: network interface device structure
> @@ -1401,11 +1418,19 @@ int ice_set_vf_trust(struct net_device *netdev, int vf_id, bool trusted)
>  
>  	mutex_lock(&vf->cfg_lock);
>  
> -	while (!trusted && vf->num_mac_lldp)
> -		ice_vf_update_mac_lldp_num(vf, ice_get_vf_vsi(vf), false);
> -
> -	vf->trusted = trusted;
> -	ice_reset_vf(vf, ICE_VF_RESET_NOTIFY);
> +	/* Reset only if revoking trust and VF has advanced features configured */
> +	if (!trusted &&
> +	    (vf->num_mac_lldp > 0 ||
> +	     test_bit(ICE_VF_STATE_UC_PROMISC, vf->vf_states) ||
> +	     test_bit(ICE_VF_STATE_MC_PROMISC, vf->vf_states))) {

[Severity: High]
Should this predicate also cover the MAC and VLAN filter quotas that trust
lifts?  Trust does more than gate LLDP filters and promiscuous mode.

The unicast/multicast MAC budget is only enforced for untrusted VFs, in
ice_vc_handle_mac_addr_msg() (virt/virtchnl.c):

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

The VLAN budget works the same way, in ice_vf_has_max_vlans():

	static bool ice_vf_has_max_vlans(struct ice_vf *vf, struct ice_vsi *vsi)
	{
		if (ice_is_vf_trusted(vf))
			return false;

and a trusted VF may override an administratively set MAC, in
ice_can_vf_change_mac():

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

All three checks run only at add time, so already-programmed filters are
never re-audited.  Previously the reset was what removed them, via
ice_reset_vf() -> ice_vf_reconfig_vsi() -> ice_fltr_remove_all(), and
ice_vf_pre_vsi_rebuild() -> ice_vf_clear_counters() is what reset the
accounting:

	if (vsi)
		vsi->num_vlan = 0;

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

So for a VF that added over-quota MAC or VLAN filters while trusted, but
never enabled promiscuous mode and never added an LLDP MAC, does "ip link
set <pf> vf N trust off" now report success while the hardware switch
filters stay programmed?  Since the ice anti-spoof behaviour is driven by
that filter list, can the now-untrusted guest keep receiving and
transmitting with addresses and VLANs it was only permitted to program while
trusted?

There is a second effect of skipping ice_vf_clear_counters(): vf->num_mac
and vsi->num_vlan stay at their trusted-era values, which are above the
untrusted limits.  Does that make every subsequent filter add from the
untrusted VF fail against stale accounting until some unrelated VF reset
happens?

> +		while (vf->num_mac_lldp)
> +			ice_vf_update_mac_lldp_num(vf, ice_get_vf_vsi(vf), false);

[Severity: Medium]
This isn't a bug introduced by this patch, since the pre-patch loop had the
same shape, but the unchecked ice_get_vf_vsi(vf) result is handed to a callee
whose NULL check comes after a dereference, in ice_vf_update_mac_lldp_num()
(ice_vf_lib.c):

	bool lldp_by_fw = test_bit(ICE_FLAG_FW_LLDP_AGENT, vsi->back->flags);
	bool was_ena = ice_vf_is_lldp_ena(vf) && !lldp_by_fw;
	bool is_ena;

	if (WARN_ON(!vsi)) {
		vf->num_mac_lldp = 0;
		return;
	}

vsi->back is read in the initializer, before WARN_ON(!vsi), so both the
warning and the vf->num_mac_lldp = 0 fallback that terminates this loop are
unreachable for a NULL VSI.

I could not find a reachable state where vf->num_mac_lldp is non-zero with a
NULL VF VSI (ice_free_vf_res() and the reset paths all zero the counter),
so this looks latent rather than triggerable.  Would it be worth computing
lldp_by_fw after the WARN_ON(!vsi) check so the guard actually guards?

> +		vf->trusted = trusted;
> +		ice_reset_vf(vf, ICE_VF_RESET_NOTIFY);

[Severity: Medium]
This is a pre-existing asymmetry rather than something the patch introduces,
but now that ice_setup_vf_trust() exists, should it be called on this branch
too?

On the reset branch, vf->trusted is written but ICE_VIRTCHNL_VF_CAP_PRIVILEGE
in vf->vf_caps, which is what ice_is_vf_trusted() reads, is only re-derived
as a side effect of the rebuild:

ice_reset_vf() -> ice_vf_post_vsi_rebuild() -> ice_vf_rebuild_host_cfg() ->
ice_vf_set_host_trust_cfg()

	static void ice_vf_set_host_trust_cfg(struct ice_vf *vf)
	{
		assign_bit(ICE_VIRTCHNL_VF_CAP_PRIVILEGE, &vf->vf_caps, vf->trusted);
	}

ice_reset_vf() has several exits that never reach the rebuild, for example:

	if (test_bit(ICE_VF_RESETS_DISABLED, pf->state)) {
		dev_dbg(dev, "Trying to reset VF %d, but all VF resets are disabled\n",
			vf->vf_id);
		return 0;
	}

plus the ice_is_vf_disabled() path, the two WARN_ON(!vsi) exits and
ice_vf_reconfig_vsi() failure.  The return value is discarded here and the
dev_info() below prints "VF %u is now %strusted" regardless.

On those paths, do vf->trusted and the privilege bit diverge while the
operation still reports success?  ICE_VF_RESETS_DISABLED is only set in
ice_remove(), and the other exits leave ICE_VF_STATE_ACTIVE cleared, which
the privileged virtchnl handlers check first, so this looks like a state and
reporting inconsistency rather than retained privilege.  Would calling
ice_setup_vf_trust(vf, trusted) on both branches and propagating
ice_reset_vf()'s error be worthwhile?

> +	} else {
> +		vf->trusted = trusted;
> +		ice_setup_vf_trust(vf, trusted);
> +	}

[Severity: Medium]
What happens to the already negotiated VLAN_V2 capabilities when trust
changes without a reset?  The advertised VLAN filter budget is derived from
vf->trusted once, in ice_vc_get_max_vlan_fltrs() (virt/virtchnl.c):

	static u16 ice_vc_get_max_vlan_fltrs(struct ice_vf *vf)
	{
		if (vf->trusted)
			return VLAN_N_VID;
		else
			return ICE_MAX_VLAN_PER_VF;
	}

and cached by ice_vc_get_offload_vlan_v2_caps():

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

VIRTCHNL_OP_ADD_VLAN_V2 is then validated only against that cached copy, in
ice_vc_validate_add_vlan_filter_list():

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

with no ice_is_vf_trusted() re-check.  The cache is invalidated only by
ice_vf_set_initialized():

	memset(&vf->vlan_v2_caps, 0, sizeof(vf->vlan_v2_caps));

which is reached from ice_vf_post_vsi_rebuild(), i.e. only on the reset path
this branch skips.  The iavf side caches and enforces the advertised limit
itself as well, in iavf_get_max_vlans_allowed() and iavf_vlan_rx_add_vid().

So for a VLAN_V2 capable VF, is granting trust here a no-op for the VLAN
path, leaving the VF and PF both at ICE_MAX_VLAN_PER_VF?  And in the other
direction, if the VF negotiated caps while already trusted and trust is
revoked through this branch, does it keep the VLAN_N_VID budget while
untrusted?

>  	dev_info(ice_pf_to_dev(pf), "VF %u is now %strusted\n",
>  		 vf_id, trusted ? "" : "un");
>  

[Severity: Medium]
This is a pre-existing issue and not introduced by this patch, but it is in
the function being modified.  Earlier in ice_set_vf_trust(), the switchdev
check returns without releasing the reference taken by ice_get_vf_by_id():

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

ice_get_vf_by_id() documents that "This function takes a reference to the VF,
which must be released by calling ice_put_vf()", and the out_put_vf label at
the end of the function is what normally does that.  Does each "ip link set
<pf> vf N trust on|off" issued while the PF is in switchdev mode leak a VF
reference, so ice_sriov_free_vf() never runs for that struct ice_vf?  Would
"ret = -EOPNOTSUPP; goto out_put_vf;" be the right fix while here?

[PATCH net 05/10] ice: move ice_vsi_realloc_stat_arrays() up

[Tony Nguyen]

From: Przemek Kitszel <przemyslaw.kitszel@intel.com>

Move ice_vsi_realloc_stat_arrays() up, to allow calling it from
ice_vsi_cfg_def() by the next commit.

Fix kdoc for touched code. One line break removed, "int i" scope
minimized to the loop, no changes otherwise.

Reviewed-by: Marcin Szycik <marcin.szycik@linux.intel.com>
Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Reviewed-by: Simon Horman <horms@kernel.org>
Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_lib.c | 119 +++++++++++------------
 1 file changed, 59 insertions(+), 60 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
index 8cdc4fda89e9..e48ee5940f17 100644
--- a/drivers/net/ethernet/intel/ice/ice_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_lib.c
@@ -2303,6 +2303,65 @@ static int ice_vsi_cfg_tc_lan(struct ice_pf *pf, struct ice_vsi *vsi)
 	return 0;
 }
 
+/**
+ * ice_vsi_realloc_stat_arrays - Frees unused stat structures or alloc new ones
+ * @vsi: VSI pointer
+ * Return: 0 on success or -ENOMEM on allocation failure.
+ */
+static int ice_vsi_realloc_stat_arrays(struct ice_vsi *vsi)
+{
+	u16 req_txq = vsi->req_txq ? vsi->req_txq : vsi->alloc_txq;
+	u16 req_rxq = vsi->req_rxq ? vsi->req_rxq : vsi->alloc_rxq;
+	struct ice_ring_stats **tx_ring_stats;
+	struct ice_ring_stats **rx_ring_stats;
+	struct ice_vsi_stats *vsi_stat;
+	struct ice_pf *pf = vsi->back;
+	u16 prev_txq = vsi->alloc_txq;
+	u16 prev_rxq = vsi->alloc_rxq;
+
+	vsi_stat = pf->vsi_stats[vsi->idx];
+
+	if (req_txq < prev_txq) {
+		for (int i = req_txq; i < prev_txq; i++) {
+			if (vsi_stat->tx_ring_stats[i]) {
+				kfree_rcu(vsi_stat->tx_ring_stats[i], rcu);
+				WRITE_ONCE(vsi_stat->tx_ring_stats[i], NULL);
+			}
+		}
+	}
+
+	tx_ring_stats = vsi_stat->tx_ring_stats;
+	vsi_stat->tx_ring_stats =
+		krealloc_array(vsi_stat->tx_ring_stats, req_txq,
+			       sizeof(*vsi_stat->tx_ring_stats),
+			       GFP_KERNEL | __GFP_ZERO);
+	if (!vsi_stat->tx_ring_stats) {
+		vsi_stat->tx_ring_stats = tx_ring_stats;
+		return -ENOMEM;
+	}
+
+	if (req_rxq < prev_rxq) {
+		for (int i = req_rxq; i < prev_rxq; i++) {
+			if (vsi_stat->rx_ring_stats[i]) {
+				kfree_rcu(vsi_stat->rx_ring_stats[i], rcu);
+				WRITE_ONCE(vsi_stat->rx_ring_stats[i], NULL);
+			}
+		}
+	}
+
+	rx_ring_stats = vsi_stat->rx_ring_stats;
+	vsi_stat->rx_ring_stats =
+		krealloc_array(vsi_stat->rx_ring_stats, req_rxq,
+			       sizeof(*vsi_stat->rx_ring_stats),
+			       GFP_KERNEL | __GFP_ZERO);
+	if (!vsi_stat->rx_ring_stats) {
+		vsi_stat->rx_ring_stats = rx_ring_stats;
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
 /**
  * ice_vsi_cfg_def - configure default VSI based on the type
  * @vsi: pointer to VSI
@@ -3011,66 +3070,6 @@ ice_vsi_rebuild_set_coalesce(struct ice_vsi *vsi,
 	}
 }
 
-/**
- * ice_vsi_realloc_stat_arrays - Frees unused stat structures or alloc new ones
- * @vsi: VSI pointer
- */
-static int
-ice_vsi_realloc_stat_arrays(struct ice_vsi *vsi)
-{
-	u16 req_txq = vsi->req_txq ? vsi->req_txq : vsi->alloc_txq;
-	u16 req_rxq = vsi->req_rxq ? vsi->req_rxq : vsi->alloc_rxq;
-	struct ice_ring_stats **tx_ring_stats;
-	struct ice_ring_stats **rx_ring_stats;
-	struct ice_vsi_stats *vsi_stat;
-	struct ice_pf *pf = vsi->back;
-	u16 prev_txq = vsi->alloc_txq;
-	u16 prev_rxq = vsi->alloc_rxq;
-	int i;
-
-	vsi_stat = pf->vsi_stats[vsi->idx];
-
-	if (req_txq < prev_txq) {
-		for (i = req_txq; i < prev_txq; i++) {
-			if (vsi_stat->tx_ring_stats[i]) {
-				kfree_rcu(vsi_stat->tx_ring_stats[i], rcu);
-				WRITE_ONCE(vsi_stat->tx_ring_stats[i], NULL);
-			}
-		}
-	}
-
-	tx_ring_stats = vsi_stat->tx_ring_stats;
-	vsi_stat->tx_ring_stats =
-		krealloc_array(vsi_stat->tx_ring_stats, req_txq,
-			       sizeof(*vsi_stat->tx_ring_stats),
-			       GFP_KERNEL | __GFP_ZERO);
-	if (!vsi_stat->tx_ring_stats) {
-		vsi_stat->tx_ring_stats = tx_ring_stats;
-		return -ENOMEM;
-	}
-
-	if (req_rxq < prev_rxq) {
-		for (i = req_rxq; i < prev_rxq; i++) {
-			if (vsi_stat->rx_ring_stats[i]) {
-				kfree_rcu(vsi_stat->rx_ring_stats[i], rcu);
-				WRITE_ONCE(vsi_stat->rx_ring_stats[i], NULL);
-			}
-		}
-	}
-
-	rx_ring_stats = vsi_stat->rx_ring_stats;
-	vsi_stat->rx_ring_stats =
-		krealloc_array(vsi_stat->rx_ring_stats, req_rxq,
-			       sizeof(*vsi_stat->rx_ring_stats),
-			       GFP_KERNEL | __GFP_ZERO);
-	if (!vsi_stat->rx_ring_stats) {
-		vsi_stat->rx_ring_stats = rx_ring_stats;
-		return -ENOMEM;
-	}
-
-	return 0;
-}
-
 /**
  * ice_vsi_rebuild - Rebuild VSI after reset
  * @vsi: VSI to be rebuild
-- 
2.47.1

[PATCH net 06/10] ice: fix stats array overflow via proper realloc

[Tony Nguyen]

From: Przemek Kitszel <przemyslaw.kitszel@intel.com>

Integrate ice_vsi_alloc_stat_arrays() with realloc variant.

Instead of keeping two functions for stat arrays allocation, change the
ice_vsi_realloc_stat_arrays() to handle initial condition (no vsi_stat
entry) and replace ice_vsi_alloc_stat_arrays() by the more generic
ice_vsi_realloc_stat_arrays().

Note that VSIs of ICE_VSI_CHNL type are ignored in realloc variant as they
were in the replaced ice_vsi_alloc_stat_arrays().

This is a fix for stats array overflow that occurs when VF is given more
queues (an operation that will be more frequent, and by bigger increase,
when we will merge my "XLVF" series).

Splat for increasing number of queues thanks to Michal Schmidt:
KASAN detects the bug:
 ==================================================================
 BUG: KASAN: slab-out-of-bounds in ice_vsi_alloc_ring_stats+0x385/0x4a0 [ice]
 Read of size 8 at addr ffff88810affea60 by task kworker/u131:7/221

 CPU: 24 UID: 0 PID: 221 Comm: kworker/u131:7 Not tainted 7.1.0-rc1+ #1 PREEMPT(lazy)
 ...
 Workqueue: ice ice_service_task [ice]
 Call Trace:
  <TASK>
  ...
  kasan_report+0xd7/0x120
  ice_vsi_alloc_ring_stats+0x385/0x4a0 [ice]
  ice_vsi_cfg_def+0x12e2/0x2060 [ice]
  ice_vsi_cfg+0xb5/0x3c0 [ice]
  ice_reset_vf+0x858/0xf80 [ice]
  ice_vc_request_qs_msg+0x1da/0x290 [ice]
  ice_vc_process_vf_msg+0xb15/0x1430 [ice]
  __ice_clean_ctrlq+0x70d/0x9d0 [ice]
  ice_service_task+0x840/0xf20 [ice]
  process_one_work+0x690/0xff0
  worker_thread+0x4d9/0xd20
  kthread+0x322/0x410
  ret_from_fork+0x332/0x660
  ret_from_fork_asm+0x1a/0x30
  </TASK>

 Allocated by task 2439:
  kasan_save_stack+0x1c/0x40
  kasan_save_track+0x10/0x30
  __kasan_kmalloc+0x96/0xb0
  __kmalloc_noprof+0x1d8/0x580
  ice_vsi_cfg_def+0x115c/0x2060 [ice]
  ice_vsi_cfg+0xb5/0x3c0 [ice]
  ice_vsi_setup+0x180/0x320 [ice]
  ice_start_vfs+0x1f3/0x590 [ice]
  ice_ena_vfs+0x66d/0x798 [ice]
  ice_sriov_configure.cold+0xe4/0x121 [ice]
  sriov_numvfs_store+0x279/0x480
  kernfs_fop_write_iter+0x331/0x4f0
  vfs_write+0x4c4/0xe40
  ksys_write+0x10c/0x240
  do_syscall_64+0xd9/0x650
  entry_SYSCALL_64_after_hwframe+0x76/0x7e

 The buggy address belongs to the object at ffff88810affea40
                which belongs to the cache kmalloc-32 of size 32
 The buggy address is located 0 bytes to the right of
                allocated 32-byte region [ffff88810affea40, ffff88810affea60)

Fixes: 2a2cb4c6c181 ("ice: replace ice_vf_recreate_vsi() with ice_vf_reconfig_vsi()")
Closes: https://redhat.atlassian.net/browse/RHEL-164321
Reviewed-by: Marcin Szycik <marcin.szycik@linux.intel.com>
Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Reviewed-by: Simon Horman <horms@kernel.org>
Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice.h     |  2 +
 drivers/net/ethernet/intel/ice/ice_lib.c | 80 ++++++++----------------
 2 files changed, 29 insertions(+), 53 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
index fc91b6665f90..83da5ba6eeb1 100644
--- a/drivers/net/ethernet/intel/ice/ice.h
+++ b/drivers/net/ethernet/intel/ice/ice.h
@@ -328,6 +328,8 @@ enum ice_vsi_state {
 struct ice_vsi_stats {
 	struct ice_ring_stats **tx_ring_stats;  /* Tx ring stats array */
 	struct ice_ring_stats **rx_ring_stats;  /* Rx ring stats array */
+	u16 tx_ring_stats_len;
+	u16 rx_ring_stats_len;
 };
 
 /* struct that defines a VSI, associated with a dev */
diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
index e48ee5940f17..c71849119d04 100644
--- a/drivers/net/ethernet/intel/ice/ice_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_lib.c
@@ -338,7 +338,6 @@ static void ice_vsi_free_stats(struct ice_vsi *vsi)
 {
 	struct ice_vsi_stats *vsi_stat;
 	struct ice_pf *pf = vsi->back;
-	int i;
 
 	if (vsi->type == ICE_VSI_CHNL)
 		return;
@@ -349,14 +348,14 @@ static void ice_vsi_free_stats(struct ice_vsi *vsi)
 	if (!vsi_stat)
 		return;
 
-	ice_for_each_alloc_txq(vsi, i) {
+	for (int i = 0; i < vsi_stat->tx_ring_stats_len; i++) {
 		if (vsi_stat->tx_ring_stats[i]) {
 			kfree_rcu(vsi_stat->tx_ring_stats[i], rcu);
 			WRITE_ONCE(vsi_stat->tx_ring_stats[i], NULL);
 		}
 	}
 
-	ice_for_each_alloc_rxq(vsi, i) {
+	for (int i = 0; i < vsi_stat->rx_ring_stats_len; i++) {
 		if (vsi_stat->rx_ring_stats[i]) {
 			kfree_rcu(vsi_stat->rx_ring_stats[i], rcu);
 			WRITE_ONCE(vsi_stat->rx_ring_stats[i], NULL);
@@ -513,51 +512,6 @@ static irqreturn_t ice_msix_clean_rings(int __always_unused irq, void *data)
 	return IRQ_HANDLED;
 }
 
-/**
- * ice_vsi_alloc_stat_arrays - Allocate statistics arrays
- * @vsi: VSI pointer
- */
-static int ice_vsi_alloc_stat_arrays(struct ice_vsi *vsi)
-{
-	struct ice_vsi_stats *vsi_stat;
-	struct ice_pf *pf = vsi->back;
-
-	if (vsi->type == ICE_VSI_CHNL)
-		return 0;
-	if (!pf->vsi_stats)
-		return -ENOENT;
-
-	if (pf->vsi_stats[vsi->idx])
-	/* realloc will happen in rebuild path */
-		return 0;
-
-	vsi_stat = kzalloc_obj(*vsi_stat);
-	if (!vsi_stat)
-		return -ENOMEM;
-
-	vsi_stat->tx_ring_stats =
-		kzalloc_objs(*vsi_stat->tx_ring_stats, vsi->alloc_txq);
-	if (!vsi_stat->tx_ring_stats)
-		goto err_alloc_tx;
-
-	vsi_stat->rx_ring_stats =
-		kzalloc_objs(*vsi_stat->rx_ring_stats, vsi->alloc_rxq);
-	if (!vsi_stat->rx_ring_stats)
-		goto err_alloc_rx;
-
-	pf->vsi_stats[vsi->idx] = vsi_stat;
-
-	return 0;
-
-err_alloc_rx:
-	kfree(vsi_stat->rx_ring_stats);
-err_alloc_tx:
-	kfree(vsi_stat->tx_ring_stats);
-	kfree(vsi_stat);
-	pf->vsi_stats[vsi->idx] = NULL;
-	return -ENOMEM;
-}
-
 /**
  * ice_vsi_alloc_def - set default values for already allocated VSI
  * @vsi: ptr to VSI
@@ -2316,11 +2270,19 @@ static int ice_vsi_realloc_stat_arrays(struct ice_vsi *vsi)
 	struct ice_ring_stats **rx_ring_stats;
 	struct ice_vsi_stats *vsi_stat;
 	struct ice_pf *pf = vsi->back;
-	u16 prev_txq = vsi->alloc_txq;
-	u16 prev_rxq = vsi->alloc_rxq;
+	u16 prev_txq, prev_rxq;
+
+	if (vsi->type == ICE_VSI_CHNL)
+		return 0;
 
 	vsi_stat = pf->vsi_stats[vsi->idx];
+	if (!vsi_stat) {
+		vsi_stat = kzalloc_obj(*vsi_stat);
+		if (!vsi_stat)
+			return -ENOMEM;
+	}
 
+	prev_txq = vsi_stat->tx_ring_stats_len;
 	if (req_txq < prev_txq) {
 		for (int i = req_txq; i < prev_txq; i++) {
 			if (vsi_stat->tx_ring_stats[i]) {
@@ -2337,9 +2299,11 @@ static int ice_vsi_realloc_stat_arrays(struct ice_vsi *vsi)
 			       GFP_KERNEL | __GFP_ZERO);
 	if (!vsi_stat->tx_ring_stats) {
 		vsi_stat->tx_ring_stats = tx_ring_stats;
-		return -ENOMEM;
+		goto err_free_partial_vsi_stat;
 	}
+	vsi_stat->tx_ring_stats_len = req_txq;
 
+	prev_rxq = vsi_stat->rx_ring_stats_len;
 	if (req_rxq < prev_rxq) {
 		for (int i = req_rxq; i < prev_rxq; i++) {
 			if (vsi_stat->rx_ring_stats[i]) {
@@ -2356,10 +2320,20 @@ static int ice_vsi_realloc_stat_arrays(struct ice_vsi *vsi)
 			       GFP_KERNEL | __GFP_ZERO);
 	if (!vsi_stat->rx_ring_stats) {
 		vsi_stat->rx_ring_stats = rx_ring_stats;
-		return -ENOMEM;
+		goto err_free_partial_vsi_stat;
 	}
+	vsi_stat->rx_ring_stats_len = req_rxq;
 
+	pf->vsi_stats[vsi->idx] = vsi_stat;
 	return 0;
+
+err_free_partial_vsi_stat:
+	if (!pf->vsi_stats[vsi->idx]) {
+		/* vsi_stat was not visible before current alloc attempt */
+		kfree(vsi_stat->tx_ring_stats);
+		kfree(vsi_stat);
+	}
+	return -ENOMEM;
 }
 
 /**
@@ -2379,7 +2353,7 @@ static int ice_vsi_cfg_def(struct ice_vsi *vsi)
 		return ret;
 
 	/* allocate memory for Tx/Rx ring stat pointers */
-	ret = ice_vsi_alloc_stat_arrays(vsi);
+	ret = ice_vsi_realloc_stat_arrays(vsi);
 	if (ret)
 		goto unroll_vsi_alloc;
 
-- 
2.47.1

Re: [PATCH net 06/10] ice: fix stats array overflow via proper realloc

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
ice: fix stats array overflow via proper realloc

This drops ice_vsi_alloc_stat_arrays() and makes
ice_vsi_realloc_stat_arrays() handle the initial allocation as well, so
ice_vsi_cfg_def() now goes through the realloc path.  The array sizes are
tracked in two new fields, tx_ring_stats_len and rx_ring_stats_len, which
the free path uses instead of vsi->alloc_txq/alloc_rxq.

Closes: https://redhat.atlassian.net/browse/RHEL-164321

> diff --git a/drivers/net/ethernet/intel/ice/ice.h b/drivers/net/ethernet/intel/ice/ice.h
> index fc91b6665f905..83da5ba6eeb17 100644
> --- a/drivers/net/ethernet/intel/ice/ice.h
> +++ b/drivers/net/ethernet/intel/ice/ice.h
> @@ -328,6 +328,8 @@ enum ice_vsi_state {
>  struct ice_vsi_stats {
>  	struct ice_ring_stats **tx_ring_stats;  /* Tx ring stats array */
>  	struct ice_ring_stats **rx_ring_stats;  /* Rx ring stats array */
> +	u16 tx_ring_stats_len;
> +	u16 rx_ring_stats_len;
>  };
>  
>  /* struct that defines a VSI, associated with a dev */

> diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c b/drivers/net/ethernet/intel/ice/ice_lib.c
> index e48ee5940f173..c71849119d04e 100644
> --- a/drivers/net/ethernet/intel/ice/ice_lib.c
> +++ b/drivers/net/ethernet/intel/ice/ice_lib.c

[ ... ]

> @@ -513,51 +512,6 @@ static irqreturn_t ice_msix_clean_rings(int __always_unused irq, void *data)

[ ... ]

> -	vsi_stat->tx_ring_stats =
> -		kzalloc_objs(*vsi_stat->tx_ring_stats, vsi->alloc_txq);
> -	if (!vsi_stat->tx_ring_stats)
> -		goto err_alloc_tx;
> -
> -	vsi_stat->rx_ring_stats =
> -		kzalloc_objs(*vsi_stat->rx_ring_stats, vsi->alloc_rxq);
> -	if (!vsi_stat->rx_ring_stats)
> -		goto err_alloc_rx;

[ ... ]

> @@ -2316,11 +2270,19 @@ static int ice_vsi_realloc_stat_arrays(struct ice_vsi *vsi)
>  	struct ice_ring_stats **rx_ring_stats;
>  	struct ice_vsi_stats *vsi_stat;
>  	struct ice_pf *pf = vsi->back;
> -	u16 prev_txq = vsi->alloc_txq;
> -	u16 prev_rxq = vsi->alloc_rxq;
> +	u16 prev_txq, prev_rxq;
> +
> +	if (vsi->type == ICE_VSI_CHNL)
> +		return 0;
>  
>  	vsi_stat = pf->vsi_stats[vsi->idx];
> +	if (!vsi_stat) {
> +		vsi_stat = kzalloc_obj(*vsi_stat);
> +		if (!vsi_stat)
> +			return -ENOMEM;
> +	}
>  
> +	prev_txq = vsi_stat->tx_ring_stats_len;
>  	if (req_txq < prev_txq) {
>  		for (int i = req_txq; i < prev_txq; i++) {
>  			if (vsi_stat->tx_ring_stats[i]) {

[Severity: High]

This isn't a bug introduced by this patch, but can the array still end up
smaller than what the consumers iterate for a VF VSI?  The replaced
ice_vsi_alloc_stat_arrays() sized the arrays from vsi->alloc_txq /
vsi->alloc_rxq, while ice_vsi_realloc_stat_arrays() sizes them from the
req_* values:

drivers/net/ethernet/intel/ice/ice_lib.c:ice_vsi_realloc_stat_arrays() {
	u16 req_txq = vsi->req_txq ? vsi->req_txq : vsi->alloc_txq;
	u16 req_rxq = vsi->req_rxq ? vsi->req_rxq : vsi->alloc_rxq;
	...
}

For ICE_VSI_VF, ice_vsi_set_num_qs() does not consult vsi->req_txq /
vsi->req_rxq at all:

	case ICE_VSI_VF:
		if (vf->num_req_qs)
			vf->num_vf_qs = vf->num_req_qs;
		vsi->alloc_txq = vf->num_vf_qs;
		vsi->alloc_rxq = vf->num_vf_qs;

So the two counts have independent writers.  vsi->req_txq / vsi->req_rxq
are set on a VF VSI only by ice_sriov_set_msix_vec_count() (the
sriov_vf_msix_count sysfs write) and I could not find anything that clears
them again for a VF VSI -- neither ice_vsi_decfg() nor ice_vsi_alloc_def()
touches them, and the only vsi->req_txq = 0 is on the PF mqprio path in
ice_main.c.

Given that, does this sequence still overflow tx_ring_stats[] /
rx_ring_stats[]?

1) host writes sriov_vf_msix_count, so ice_sriov_set_msix_vec_count() sets
   vsi->req_txq = vsi->req_rxq = 2 and they stay at 2
2) the guest sends VIRTCHNL_OP_REQUEST_QUEUES for 16 queues;
   ice_vc_request_qs_msg() bounds it only by ICE_MAX_RSS_QS_PER_VF and the
   free queue count, sets vf->num_req_qs and calls ice_reset_vf()
3) ice_reset_vf() -> ice_vf_reconfig_vsi() -> ice_vsi_cfg() ->
   ice_vsi_cfg_def(); ice_vsi_alloc_def() -> ice_vsi_set_num_qs() sets
   alloc_txq = alloc_rxq = 16
4) ice_vsi_realloc_stat_arrays() then shrinks to req_txq = 2 and records
   tx_ring_stats_len = 2

ice_vsi_alloc_ring_stats() still walks 0..alloc_txq-1 with no bound check
against the new length fields:

drivers/net/ethernet/intel/ice/ice_lib.c:ice_vsi_alloc_ring_stats() {
	ice_for_each_alloc_txq(vsi, i) {
		...
		ring_stats = tx_ring_stats[i];
		if (!ring_stats) {
			...
			WRITE_ONCE(tx_ring_stats[i], ring_stats);
		}
		ring->ring_stats = ring_stats;
	}
}

That is both an out-of-bounds read and an out-of-bounds pointer store for
i >= 2, which looks like the same slab-out-of-bounds signature quoted in
the commit message.  ice_qp_reset_stats() in ice_base.c indexes
vsi_stat->tx_ring_stats[q_idx] / rx_ring_stats[q_idx] the same way without
consulting the new lengths.

Would using the freshly computed vsi->alloc_txq / vsi->alloc_rxq (or
max(req_*, alloc_*)) for the sizing close this?  The req_* preference looks
only meaningful for the pre-ice_vsi_cfg_def() call in ice_vsi_rebuild(),
where alloc_* is still stale.

>  
> @@ -2379,7 +2353,7 @@ static int ice_vsi_cfg_def(struct ice_vsi *vsi)
>  		return ret;
>  
>  	/* allocate memory for Tx/Rx ring stat pointers */
> -	ret = ice_vsi_alloc_stat_arrays(vsi);
> +	ret = ice_vsi_realloc_stat_arrays(vsi);
>  	if (ret)
>  		goto unroll_vsi_alloc;
>

[PATCH net 07/10] ice: eswitch: fix use-after-free of metadata_dst in repr release

[Tony Nguyen]

From: Doruk Tan Ozturk <doruk@0sec.ai>

ice_eswitch_release_repr() frees the port representor metadata_dst via
metadata_dst_free(), which directly kfree()s the object and ignores the
dst_entry refcount. The eswitch slow-path TX routine
ice_eswitch_port_start_xmit() takes a reference on this dst with
dst_hold() and attaches it to the skb via skb_dst_set(). If such an skb
is still in flight (e.g. queued in a qdisc) when the representor is torn
down, the metadata_dst is freed while the skb still points at it. When
the skb is later freed, dst_release() operates on already-freed memory.

Replace metadata_dst_free() with dst_release() so the metadata_dst is
freed only after the last reference is dropped. The dst subsystem frees
metadata_dst objects from dst_destroy() once the refcount reaches zero
(DST_METADATA is set by metadata_dst_alloc()).

Same class of bug and fix as commit c32b26aaa2f9 ("netfilter:
nft_tunnel: fix use-after-free on object destroy").

Fixes: 1a1c40df2e80 ("ice: set and release switchdev environment")
Cc: stable@vger.kernel.org
Signed-off-by: Doruk Tan Ozturk <doruk@0sec.ai>
Reviewed-by: Simon Horman <horms@kernel.org>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Reviewed-by: Marcin Szycik <marcin.szycik@linux.intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_eswitch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_eswitch.c b/drivers/net/ethernet/intel/ice/ice_eswitch.c
index b069e6c514fb..6e7bba473898 100644
--- a/drivers/net/ethernet/intel/ice/ice_eswitch.c
+++ b/drivers/net/ethernet/intel/ice/ice_eswitch.c
@@ -95,7 +95,7 @@ ice_eswitch_release_repr(struct ice_pf *pf, struct ice_repr *repr)
 		return;
 
 	ice_vsi_update_security(vsi, ice_vsi_ctx_set_antispoof);
-	metadata_dst_free(repr->dst);
+	dst_release(&repr->dst->dst);
 	repr->dst = NULL;
 	ice_fltr_add_mac_and_broadcast(vsi, repr->parent_mac,
 				       ICE_FWD_TO_VSI);
-- 
2.47.1

Re: [PATCH net 07/10] ice: eswitch: fix use-after-free of metadata_dst in repr release

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
ice: eswitch: fix use-after-free of metadata_dst in repr release

This replaces metadata_dst_free() with dst_release() in
ice_eswitch_release_repr() so that the port representor metadata_dst is
freed only after the last dst_entry reference goes away, instead of being
kfree()d while in-flight skbs may still reference it.

(No Link: tags present in the commit header.)

> diff --git a/drivers/net/ethernet/intel/ice/ice_eswitch.c b/drivers/net/ethernet/intel/ice/ice_eswitch.c
> index b069e6c514fb1..6e7bba4738989 100644
> --- a/drivers/net/ethernet/intel/ice/ice_eswitch.c
> +++ b/drivers/net/ethernet/intel/ice/ice_eswitch.c
> @@ -95,7 +95,7 @@ ice_eswitch_release_repr(struct ice_pf *pf, struct ice_repr *repr)
>  		return;
>  
>  	ice_vsi_update_security(vsi, ice_vsi_ctx_set_antispoof);
> -	metadata_dst_free(repr->dst);
> +	dst_release(&repr->dst->dst);
>  	repr->dst = NULL;
>  	ice_fltr_add_mac_and_broadcast(vsi, repr->parent_mac,
>  				       ICE_FWD_TO_VSI);

[Severity: High]

This isn't a bug introduced by this patch, but with the put side now fixed,
is the get side of repr->dst still unsynchronized?

ice_eswitch_port_start_xmit() does three separate plain loads of repr->dst
with no lock, no RCU section and no READ_ONCE():

	skb_dst_drop(skb);
	dst_hold((struct dst_entry *)repr->dst);
	skb_dst_set(skb, (struct dst_entry *)repr->dst);
	skb->dev = repr->dst->u.port_info.lower_dev;

while ice_eswitch_release_repr() drops the last reference and then stores
the NULL:

	dst_release(&repr->dst->dst);
	repr->dst = NULL;

In the normal teardown path the ordering appears to save this, because
ice_eswitch_detach() calls repr->ops.rem() (unregister_netdev(), which
quiesces the representor's ndo_start_xmit) before
ice_eswitch_release_repr().

The attach error path has the opposite order:

	err_xa_alloc:
		ice_eswitch_release_repr(pf, repr);
	err_setup_repr:
		repr->ops.rem(repr);

At err_xa_alloc the representor netdev is already registered, since
repr->ops.add() runs ice_repr_add_vf()/ice_repr_add_sf() ->
ice_repr_reg_netdev() -> register_netdev(), and the attach path holds
devl_lock but not RTNL, so userspace can open the interface and transmit.

Two outcomes seem possible in that window:

  - the xmit CPU loaded a non-NULL repr->dst just before dst_release()
    drove the rcuref to zero, so dst_hold() hits its
    WARN_ON(!rcuref_get(&dst->__rcuref)), takes no reference, and the skb
    carries a metadata_dst that dst_destroy_rcu() frees after the grace
    period, which then gets read by ice_eswitch_set_target_vsi() or by the
    skb free path;

  - the xmit CPU loads repr->dst after the NULL store, giving dst_hold(NULL)
    and a NULL dereference inside rcuref_get().

Would swapping the two error labels so repr->ops.rem() runs before
ice_eswitch_release_repr(), or clearing repr->dst followed by
synchronize_net() before dst_release(), close this window? Also, since
err_setup_repr is reached when metadata_dst_alloc() fails, does that path
leave a registered netdev whose repr->dst is NULL until repr->ops.rem()
runs?

[Severity: High]

This is a pre-existing issue as well, and not something this patch changes,
but does the deferred free help the LAG path at all?

ice_lag_config_eswitch() writes through repr->dst with no NULL check:

	xa_for_each(&lag->pf->eswitch.reprs, id, repr)
		repr->dst->u.port_info.lower_dev = netdev;

xa_for_each() expands to xa_find()/xa_find_after(), which take and drop
rcu_read_lock() internally, so repr is used outside any RCU read-side
section.

The caller ice_lag_process_event() is a work item and holds only
pf->lag_mutex, while the teardown side (ice_free_vfs() ->
ice_eswitch_detach_vf(), and the ice_reset_all_vfs() path) holds
vf->cfg_lock and devl_lock, so the locksets do not intersect.

That seems to allow:

	LAG work				detach thread
	repr = xa_find(...)
						xa_erase(...)
						ice_eswitch_release_repr()
						  repr->dst = NULL;
						ice_repr_destroy()
						  kfree(repr);
	repr->dst->u.port_info.lower_dev = netdev;

which is either a store through NULL or a store into freed memory, since
ice_repr_destroy() does a plain kfree(repr) with no grace period.

The change from metadata_dst_free() to dst_release() only defers freeing the
metadata_dst; the repr->dst = NULL store and the kfree(repr) are still
immediate. Should this path be serialized against the eswitch teardown, or
at least keep the repr lookup inside an RCU read-side section with an
RCU-deferred repr free?

[PATCH net 08/10] i40e: fix memcmp of pointer in i40e_hw_set_dcb_config()

[Tony Nguyen]

From: Aaron Esau <aaron1esau@gmail.com>

In i40e_hw_set_dcb_config(), both new_cfg and old_cfg are pointers to
struct i40e_dcbx_config, so sizeof(new_cfg) evaluates to the size of a
pointer (8 bytes on 64-bit) rather than the size of the struct. Likewise,
&new_cfg and &old_cfg are the addresses of the pointer variables on the
stack, not the addresses of the actual config structs.

As a result, the memcmp never compares the actual configuration data,
meaning the "no change needed" early return never fires. Every call to
this function performs a full DCB reconfiguration (quiescing all VSIs,
reprogramming via "Set LLDP MIB" AQC, and reconfiguring VEB/VSIs) even
when the configuration has not changed.

Fix this by comparing the structs themselves rather than the pointers.

Fixes: 4b208eaa8078 ("i40e: Add init and default config of software based DCB")
Cc: stable@vger.kernel.org
Signed-off-by: Aaron Esau <aaron1esau@gmail.com>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Tested-by: Arpana Arland <arpanax.arland@intel.com> (A Contingent worker at Intel)
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index a04683004a56..0ac1be6289f6 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -6907,7 +6907,7 @@ static int i40e_hw_set_dcb_config(struct i40e_pf *pf,
 	int ret;
 
 	/* Check if need reconfiguration */
-	if (!memcmp(&new_cfg, &old_cfg, sizeof(new_cfg))) {
+	if (!memcmp(new_cfg, old_cfg, sizeof(*new_cfg))) {
 		dev_dbg(&pf->pdev->dev, "No Change in DCB Config required.\n");
 		return 0;
 	}
-- 
2.47.1

Re: [PATCH net 08/10] i40e: fix memcmp of pointer in i40e_hw_set_dcb_config()

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
i40e: fix memcmp of pointer in i40e_hw_set_dcb_config()

The patch changes the "need reconfiguration" check in
i40e_hw_set_dcb_config() so that memcmp() compares the two
struct i40e_dcbx_config objects instead of the two pointer variables on
the stack. With the old code the compare could never succeed, so the
early return was never taken.

> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index a04683004a567..0ac1be6289f6b 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -6907,7 +6907,7 @@ static int i40e_hw_set_dcb_config(struct i40e_pf *pf,
>  	int ret;
>  
>  	/* Check if need reconfiguration */
> -	if (!memcmp(&new_cfg, &old_cfg, sizeof(new_cfg))) {
> +	if (!memcmp(new_cfg, old_cfg, sizeof(*new_cfg))) {
>  		dev_dbg(&pf->pdev->dev, "No Change in DCB Config required.\n");
>  		return 0;
>  	}

[Severity: High]
Does this compare now always report "no change" when the function is
reached from the tail of i40e_hw_dcb_config()?

i40e_hw_set_dcb_config() takes its old side straight from the PF:

	struct i40e_dcbx_config *old_cfg = &pf->hw.local_dcbx_config;

but i40e_hw_dcb_config() has already published the new configuration into
that same object before making the call:

i40e_main.c:i40e_hw_dcb_config() {
	...
	old_cfg = &hw->local_dcbx_config;
	...
	/* Update the port DCBx configuration */
	*old_cfg = *new_cfg;

	/* Changes in configuration update VEB/VSI */
	i40e_dcb_reconfigure(pf);
out:
	...
		/* registers are set, lets apply */
		if (test_bit(I40E_HW_CAP_USE_SET_LLDP_MIB, pf->hw.caps))
			ret = i40e_hw_set_dcb_config(pf, new_cfg);
}

So new_cfg and pf->hw.local_dcbx_config are byte-identical at that point,
memcmp() returns 0, and the callee logs "No Change in DCB Config required."
and returns 0.

Does that make the "registers are set, lets apply" step dead code for this
caller? Everything after the early return is skipped, including the etsrec
normalization and the only code that pushes the configuration to firmware:

	*old_cfg = *new_cfg;
	old_cfg->etsrec = old_cfg->etscfg;
	ret = i40e_set_dcb_config(&pf->hw);

i40e_dcb.c:i40e_set_dcb_config() is the sole path that builds an LLDPDU via
i40e_dcb_config_to_lldp() from hw->local_dcbx_config and issues
i40e_aq_set_lldp_mib() for the local MIB. With the early return taken, the
DCB registers, packet buffer and PFC state plus the cached
local_dcbx_config carry the new values while the firmware's local DCBX MIB
still holds the previous one.

Which callers does this affect? The DCBNL entry points in i40e_dcb_nl.c
(i40e_dcbnl_ieee_setets(), i40e_dcbnl_ieee_setpfc(), i40e_dcbnl_ieee_setapp(),
i40e_dcbnl_ieee_delapp() and the CEE i40e_dcbnl_cee_set_all()) all copy
*old_cfg into pf->tmp_cfg and then call i40e_hw_dcb_config(pf, &pf->tmp_cfg),
so on parts with I40E_HW_CAP_USE_SET_LLDP_MIB the AQC is no longer sent for
those changes.

Is the resulting behaviour also inverted? On the error paths of
i40e_hw_dcb_config() (the goto out after i40e_aq_config_switch_comp_ets() or
i40e_aq_dcb_updated() fails) the "*old_cfg = *new_cfg" assignment is skipped,
the two structs still differ, and the full quiesce / Set LLDP MIB /
reconfigure sequence does run. That would mean the AQC is now issued only
when the earlier register programming failed.

The i40e_dcb_sw_default_config() call site looks unaffected, since its
freshly memset pf->tmp_cfg differs from a previously cached
local_dcbx_config (etsrec in particular), so the compare still fails there.

Would moving the comparison ahead of the caller's "*old_cfg = *new_cfg", or
not pre-publishing the config in the caller, be a safer shape for a fix that
is tagged for stable?

The commit message says:

    As a result, the memcmp never compares the actual configuration data,
    meaning the "no change needed" early return never fires. Every call to
    this function performs a full DCB reconfiguration (quiescing all VSIs,
    reprogramming via "Set LLDP MIB" AQC, and reconfiguring VEB/VSIs) even
    when the configuration has not changed.

Given the i40e_hw_dcb_config() call site above, is it accurate to describe
the only effect as a redundant reconfiguration? For that caller the old
comparison was load-bearing: comparing &pf->tmp_cfg against
&pf->hw.local_dcbx_config could never match, which is what kept the
Set Local LLDP MIB programming running. Could the changelog cover what
happens at that call site after the fix?

[PATCH net 09/10] i40e: fix netdev leak in i40e_vsi_setup() error paths

[Tony Nguyen]

From: Dawei Feng <dawei.feng@seu.edu.cn>

i40e_config_netdev() allocates vsi->netdev for main and VMDQ VSIs. If
i40e_netif_set_realnum_tx_rx_queues(), i40e_devlink_create_port(), or
register_netdev() fails, i40e_vsi_setup() goes to err_netdev without
releasing the netdev. The existing cleanup only frees the netdev after a
successful register_netdev(), so these error paths leak the allocation.

Reorder the error paths at err_netdev to ensure proper cleanup of the
allocated device.

The bug was first flagged by an experimental analysis tool we are
developing for kernel memory-management bugs while analyzing
v6.13-rc1. The tool is still under development and is not yet publicly
available. Manual inspection confirms that the bug is still
present in v7.1-rc5.

An x86_64 allyesconfig build showed no new warnings. As we do not have an
Intel Ethernet Controller XL710 family adapter to test with, no runtime
testing was able to be performed.

Fixes: 41c445ff0f48 ("i40e: main driver core")
Cc: stable@vger.kernel.org
Signed-off-by: Zilin Guan <zilin@seu.edu.cn>
Signed-off-by: Dawei Feng <dawei.feng@seu.edu.cn>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Reviewed-by: Marcin Szycik <marcin.szycik@linux.intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_main.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index 0ac1be6289f6..4f0f7626605b 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -14494,13 +14494,15 @@ struct i40e_vsi *i40e_vsi_setup(struct i40e_pf *pf, u8 type,
 	if (vsi->netdev_registered) {
 		vsi->netdev_registered = false;
 		unregister_netdev(vsi->netdev);
-		free_netdev(vsi->netdev);
-		vsi->netdev = NULL;
 	}
 err_dl_port:
 	if (vsi->type == I40E_VSI_MAIN)
 		i40e_devlink_destroy_port(pf);
 err_netdev:
+	if (vsi->netdev) {
+		free_netdev(vsi->netdev);
+		vsi->netdev = NULL;
+	}
 	i40e_aq_delete_element(&pf->hw, vsi->seid, NULL);
 err_vsi:
 	i40e_vsi_clear(vsi);
-- 
2.47.1

Re: [PATCH net 09/10] i40e: fix netdev leak in i40e_vsi_setup() error paths

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
i40e: fix netdev leak in i40e_vsi_setup() error paths

This moves the free_netdev() call out of the netdev_registered block at
err_msix and into a new unconditional block at err_netdev, so that the
netdev allocated by i40e_config_netdev() is released when
i40e_netif_set_realnum_tx_rx_queues(), i40e_devlink_create_port() or
register_netdev() fail.

> diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> index 0ac1be6289f6b..4f0f7626605ba 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> @@ -14494,13 +14494,15 @@ struct i40e_vsi *i40e_vsi_setup(struct i40e_pf *pf, u8 type,
>  	if (vsi->netdev_registered) {
>  		vsi->netdev_registered = false;
>  		unregister_netdev(vsi->netdev);
> -		free_netdev(vsi->netdev);
> -		vsi->netdev = NULL;
>  	}
>  err_dl_port:
>  	if (vsi->type == I40E_VSI_MAIN)
>  		i40e_devlink_destroy_port(pf);
>  err_netdev:
> +	if (vsi->netdev) {
> +		free_netdev(vsi->netdev);
> +		vsi->netdev = NULL;
> +	}

[Severity: Medium]
On the newly covered paths the netdev was never registered, but the VSI
is already published in pf->vsi[vsi->idx] by i40e_vsi_mem_alloc(), and
i40e_config_netdev() has already run i40e_add_mac_filter() for the
unicast and broadcast addresses, which does:

drivers/net/ethernet/intel/i40e/i40e_main.c:i40e_add_filter() {
	...
	vsi->flags |= I40E_VSI_FLAG_FILTER_CHANGED;
	set_bit(__I40E_MACVLAN_SYNC_PENDING, vsi->back->state);
	...
}

The service work item then walks pf->vsi[] with no lock:

i40e_sync_filters_subtask() {
	i40e_pf_for_each_vsi(pf, v, vsi) {
		if ((vsi->flags & I40E_VSI_FLAG_FILTER_CHANGED) &&
		    !test_bit(__I40E_VSI_RELEASING, vsi->state)) {
			int ret = i40e_sync_vsi_filters(vsi);
	...
}

and i40e_sync_vsi_filters() dereferences the netdev:

i40e_sync_vsi_filters() {
	...
	if (vsi->netdev) {
		changed_flags = vsi->current_netdev_flags ^ vsi->netdev->flags;
		vsi->current_netdev_flags = vsi->netdev->flags;
	}
	...
}

Can the service task load vsi->netdev, pass the NULL test, and then
dereference it after this free_netdev()?  The error path does not set
__I40E_VSI_RELEASING first, which is the ordering i40e_vsi_release()
uses before its own free_netdev(), and the vsi->netdev = NULL store
carries no lock or barrier.

This is reachable at runtime for I40E_VSI_VMDQ2 VSIs created from
debugfs, where i40e_dbg_command_write() calls
i40e_vsi_setup(pf, I40E_VSI_VMDQ2, vsi_seid, 0) while the service work
item is live.  Would setting __I40E_VSI_RELEASING (or clearing
I40E_VSI_FLAG_FILTER_CHANGED / unpublishing pf->vsi[idx] under
pf->switch_mutex) before dropping the netdev close this window, mirroring
i40e_vsi_release()?

The exposure window is short, since the following i40e_vsi_clear() also
kfree()s the still-published struct i40e_vsi on these same paths, which
is a pre-existing unsynchronised teardown.

[Severity: High]
This isn't a bug introduced by this patch, but the other in-tree caller of
i40e_config_netdev() does not follow the ownership rule this hunk
codifies.  In i40e_init_recovery_mode() the netdev is never released:

	err = i40e_config_netdev(vsi);
	if (err)
		goto err_switch_setup;
	err = register_netdev(vsi->netdev);
	if (err)
		goto err_switch_setup;
	vsi->netdev_registered = true;
	i40e_dbg_pf_init(pf);

	err = i40e_setup_misc_vector_for_recovery_mode(pf);
	if (err)
		goto err_switch_setup;

If register_netdev() fails, is the net_device allocated by
alloc_etherdev_mq() inside i40e_config_netdev() leaked?  i40e_probe()
does "return i40e_init_recovery_mode(pf, hw);", so i40e_remove() never
runs for the failed probe and nothing later frees it.

The case where register_netdev() succeeded and
i40e_setup_misc_vector_for_recovery_mode() then fails looks worse, since
err_switch_setup never calls unregister_netdev():

err_switch_setup:
	i40e_reset_interrupt_capability(pf);
	timer_shutdown_sync(&pf->service_timer);
	i40e_shutdown_adminq(hw);
	iounmap(hw->hw_addr);
	pci_release_mem_regions(pf->pdev);
	pci_disable_device(pf->pdev);
	i40e_free_pf(pf);

	return err;

i40e_free_pf() is devlink_free() on the devlink whose priv is the struct
i40e_pf, so the netdev stays registered in the netns while its
netdev_priv()->vsi->back has been freed and hw->hw_addr has been
iounmapped.  Can a later "ip link set <dev> up" on the leftover interface
reach i40e_open() and touch the freed pf and the unmapped register
window?  The debugfs directory created by i40e_dbg_pf_init(pf) is left in
place pointing at the freed pf as well.

Would it make sense to fix that call site in the same series, so both
users of i40e_config_netdev() release the netdev consistently?

[PATCH net 10/10] igc: fix netdev not re-attached after resume if interface is down

[Tony Nguyen]

From: Philipp David <pd-lkml@3b.pm>

__igc_resume() calls netif_device_attach() only inside the
netif_running() branch, so an interface that was down during suspend
is never re-attached on resume. It then stays in the not-present state
that __igc_shutdown() set via netif_device_detach(): ethtool reports
ENODEV and every attempt to bring the interface up fails the
netif_device_present() check in __dev_open() with -ENODEV, silently,
since __igc_resume() returns 0. Only reloading the driver recovers the
device.

This is easy to hit in practice because NetworkManager brings managed
interfaces down before sleep unless Wake-on-LAN is configured, making
the adapter unusable after every suspend/resume cycle with WoL
disabled.

Re-attach the netdev on every successful resume, as igb and e1000e do.

Fixes: 6f31d6b643a3 ("igc: Refactor runtime power management flow")
Cc: stable@vger.kernel.org
Signed-off-by: Philipp David <pd-lkml@3b.pm>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Reviewed-by: Dima Ruinskiy <dima.ruinskiy@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/igc/igc_main.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index 16fcc8cf6191..39043d8ca1bf 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -7585,11 +7585,13 @@ static int __igc_resume(struct device *dev, bool rpm)
 		err = __igc_open(netdev, true);
 		if (!rpm)
 			rtnl_unlock();
-		if (!err)
-			netif_device_attach(netdev);
+		if (err)
+			return err;
 	}
 
-	return err;
+	netif_device_attach(netdev);
+
+	return 0;
 }
 
 static int igc_resume(struct device *dev)
-- 
2.47.1

Re: [PATCH net 00/10][pull request] Intel Wired LAN Driver Updates 2026-08-04 (iavf, i40e, ice, igc)

[Jakub Kicinski]

On Tue,  4 Aug 2026 15:21:53 -0700 Tony Nguyen wrote:
> The following are changes since commit a347304b2ca1a5377d5bd2d8a72e4b4f12afe648:
>   net/sched: cls_api: Always acquire rtnl_lock when destroying locked classifiers
> and are available in the git repository at:
>   git://git.kernel.org/pub/scm/linux/kernel/git/tnguy/net-queue 40GbE

I'll pick up the last patch and send the Sashiko reports for the rest.
LMK if none of the reports are actionable..

Re: [PATCH net 00/10][pull request] Intel Wired LAN Driver Updates 2026-08-04 (iavf, i40e, ice, igc)

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Tue,  4 Aug 2026 15:21:53 -0700 you wrote:
> Jose Ignacio Tornos Martinez fixes issues with VF bonding that came
> about with commit ad7c7b2172c3 ("net: hold netdev instance lock during
> sysfs operations").
> 
> Further details:
> https://lore.kernel.org/netdev/20260623101800.991293-1-jtornosm@redhat.com/
> 
> [...]

Here is the summary with links:
  - [net,01/10] iavf: return EBUSY if reset in progress or not ready during MAC change
    (no matching commit)
  - [net,02/10] i40e: skip unnecessary VF reset when setting trust
    (no matching commit)
  - [net,03/10] iavf: send MAC change request synchronously
    (no matching commit)
  - [net,04/10] ice: skip unnecessary VF reset when setting trust
    (no matching commit)
  - [net,05/10] ice: move ice_vsi_realloc_stat_arrays() up
    (no matching commit)
  - [net,06/10] ice: fix stats array overflow via proper realloc
    (no matching commit)
  - [net,07/10] ice: eswitch: fix use-after-free of metadata_dst in repr release
    (no matching commit)
  - [net,08/10] i40e: fix memcmp of pointer in i40e_hw_set_dcb_config()
    (no matching commit)
  - [net,09/10] i40e: fix netdev leak in i40e_vsi_setup() error paths
    (no matching commit)
  - [net,10/10] igc: fix netdev not re-attached after resume if interface is down
    https://git.kernel.org/netdev/net/c/b0ce5fd9fabe

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