[PATCH iwl-net v2 0/5] iavf: five correctness fixes

[PATCH iwl-net v2 0/5] iavf: five correctness fixes

[Aleksandr Loktionov]

Small batch of iavf bug fixes. Patches address a NULL-pointer dereference
crash in the hung-tx detector, a spurious free_irq() call in the misc-IRQ
error path, a VSI-state-corruption race when ethtool changes ring
parameters during an active reset, an inverted TC-boundary comparison
that silently steered frames to non-existing traffic classes, and an
-EINVAL that confused upper layers when a TC flower filter was looked up
after its qdisc had already been torn down.

All five are genuine correctness fixes with no functional changes for the
common path. All five are marked for stable given the crash/corruption/
kernel-warning/misdirection/error-reporting impact on shipping kernels.

This series was originally posted in April without a version tag and
stalled without being picked up. Re-posting as v2 with the fixes Simon
Horman requested in review, carrying forward the Tested-by tags collected
on the unchanged patches.

Changes since v1:
- Patch 1: Fixed the Fixes tag, which pointed at an unrelated i40e-only
  commit (9c6c12595b73); the function was actually introduced into the
  iavf lineage by 07d44190a389. Simplified the misleading NULL check on
  tx_ring (an array-element address, never NULL) to a check on
  tx_ring->q_vector instead, and read it with READ_ONCE() so the watchdog
  can't observe a torn/re-read value while a concurrent reset swaps it.
- Patch 4: Reworked the boundary comparison. `tc > adapter->num_tc` still
  let every in-range tc skip the destination-port requirement and let an
  out-of-range tc with a destination port fall through and return 0.
  Now explicitly rejects tc >= adapter->num_tc before checking for a
  destination port.
- Patches 2, 3 and 5 are unchanged from v1.
- Added Cc: stable@vger.kernel.org to all five patches.

Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>

Kiran Patil (2):
  iavf: fix null pointer dereference in iavf_detect_recover_hung
  iavf: return 0 when TC flower filter not found after qdisc teardown

Piotr Gardocki (1):
  iavf: fix error path in iavf_request_misc_irq

Sylwester Dziedziuch (1):
  iavf: prevent VSI corruption when ring params changed during reset

Avinash Dayanand (1):
  iavf: fix TC boundary check in iavf_handle_tclass

 drivers/net/ethernet/intel/iavf/iavf_ethtool.c |  5 +++
 drivers/net/ethernet/intel/iavf/iavf_main.c    | 19 ++++----
 drivers/net/ethernet/intel/iavf/iavf_txrx.c    | 50 +++++++++++++------------
 3 files changed, 45 insertions(+), 29 deletions(-)

-- 
2.52.0

[PATCH iwl-net v2 1/5] iavf: fix null pointer dereference in iavf_detect_recover_hung

[Aleksandr Loktionov]

From: Kiran Patil <kiran.patil@intel.com>

iavf_watchdog_task() and iavf_reset_task() both run as work items on the
same ordered adapter->wq, so they can't race with each other. However,
iavf_set_ringparam() (and other ethtool ops) call iavf_reset_step()
directly from process context under the netdev instance lock, without
going through that workqueue at all. iavf_reset_step() can free and
reallocate adapter->tx_rings and the q_vectors array via
iavf_reinit_interrupt_scheme() while adapter->state still reads
__IAVF_RUNNING, so the watchdog task can concurrently call
iavf_detect_recover_hung() and dereference a NULL q_vector inside
iavf_force_wb(), or index into a NULL tx_rings array, causing a crash.

Guard against this by:
- returning early if vsi->back->tx_rings itself is NULL, since
  num_active_queues can still be nonzero while the array is being
  reallocated;
- skipping rings whose q_vector is NULL;
- reading tx_ring->q_vector once with READ_ONCE() into a local variable
  and reusing that same value for both the NULL check and the
  iavf_force_wb() call, instead of re-reading the field right before
  use, which would leave a window for the concurrent reset to swap it
  from underneath us in between.

Also move the tx_ring declaration into the loop body and drop the
redundant outer NULL initialisation, which the compiler can never
observe since an array-element address is always non-NULL.

Fixes: 07d44190a389 ("i40e/i40evf: Detect and recover hung queue scenario")
Cc: stable@vger.kernel.org
Signed-off-by: Kiran Patil <kiran.patil@intel.com>
Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
---
 drivers/net/ethernet/intel/iavf/iavf_txrx.c | 50 ++++++++++++---------
 1 file changed, 29 insertions(+), 21 deletions(-)

diff --git a/drivers/net/ethernet/intel/iavf/iavf_txrx.c b/drivers/net/ethernet/intel/iavf/iavf_txrx.c
index c30abf1..f1c26a9 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_txrx.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_txrx.c
@@ -176,7 +176,6 @@ static void iavf_force_wb(struct iavf_vsi *vsi, struct iavf_q_vector *q_vector)
  **/
 void iavf_detect_recover_hung(struct iavf_vsi *vsi)
 {
-	struct iavf_ring *tx_ring = NULL;
 	struct net_device *netdev;
 	unsigned int i;
 	int packets;
@@ -194,29 +193,38 @@ void iavf_detect_recover_hung(struct iavf_vsi *vsi)
 	if (!netif_carrier_ok(netdev))
 		return;
 
+	/* tx_rings can be freed/reallocated by a concurrent reset */
+	if (!vsi->back->tx_rings)
+		return;
+
 	for (i = 0; i < vsi->back->num_active_queues; i++) {
-		tx_ring = &vsi->back->tx_rings[i];
-		if (tx_ring && tx_ring->desc) {
-			/* If packet counter has not changed the queue is
-			 * likely stalled, so force an interrupt for this
-			 * queue.
-			 *
-			 * prev_pkt_ctr would be negative if there was no
-			 * pending work.
-			 */
-			packets = tx_ring->stats.packets & INT_MAX;
-			if (tx_ring->prev_pkt_ctr == packets) {
-				iavf_force_wb(vsi, tx_ring->q_vector);
-				continue;
-			}
+		struct iavf_ring *tx_ring = &vsi->back->tx_rings[i];
+		struct iavf_q_vector *q_vector;
 
-			/* Memory barrier between read of packet count and call
-			 * to iavf_get_tx_pending()
-			 */
-			smp_rmb();
-			tx_ring->prev_pkt_ctr =
-			  iavf_get_tx_pending(tx_ring, true) ? packets : -1;
+		/* read once, q_vector can be reassigned by a concurrent reset */
+		q_vector = READ_ONCE(tx_ring->q_vector);
+		if (!q_vector || !tx_ring->desc)
+			continue;
+
+		/* If packet counter has not changed the queue is
+		 * likely stalled, so force an interrupt for this
+		 * queue.
+		 *
+		 * prev_pkt_ctr would be negative if there was no
+		 * pending work.
+		 */
+		packets = tx_ring->stats.packets & INT_MAX;
+		if (tx_ring->prev_pkt_ctr == packets) {
+			iavf_force_wb(vsi, q_vector);
+			continue;
 		}
+
+		/* Memory barrier between read of packet count and call
+		 * to iavf_get_tx_pending()
+		 */
+		smp_rmb();
+		tx_ring->prev_pkt_ctr =
+		  iavf_get_tx_pending(tx_ring, true) ? packets : -1;
 	}
 }
 
-- 
2.52.0

[PATCH iwl-net v2 2/5] iavf: fix error path in iavf_request_misc_irq

[Aleksandr Loktionov]

From: Piotr Gardocki <piotrx.gardocki@intel.com>

When request_irq() fails the interrupt vector was not registered for
the driver. Calling free_irq() on a vector that was never successfully
requested triggers a kernel warning. Drop the erroneous free_irq()
call from the error path.

Fixes: 5eae00c57f5e ("i40evf: main driver core")
Cc: stable@vger.kernel.org
Signed-off-by: Piotr Gardocki <piotrx.gardocki@intel.com>
Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
---
 drivers/net/ethernet/intel/iavf/iavf_main.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
index dad001a..ab5f5adc 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_main.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
@@ -587,7 +587,6 @@ static int iavf_request_misc_irq(struct iavf_adapter *adapter)
 		dev_err(&adapter->pdev->dev,
 			"request_irq for %s failed: %d\n",
 			adapter->misc_vector_name, err);
-		free_irq(adapter->msix_entries[0].vector, netdev);
 	}
 	return err;
 }
-- 
2.52.0

[PATCH iwl-net v2 3/5] iavf: prevent VSI corruption when ring params changed during reset

[Aleksandr Loktionov]

From: Sylwester Dziedziuch <sylwesterx.dziedziuch@intel.com>

Changing ring parameters via ethtool triggers a VF reset and queue
reconfiguration. If ethtool is called again before the first reset
completes, the second reset races with uninitialised queue state and
can corrupt the VSI resource tree on the PF side.

Return -EAGAIN from iavf_set_ringparam() when the adapter is already
resetting or its queues are disabled.

Fixes: fbb7ddfef253 ("i40evf: core ethtool functionality")
Cc: stable@vger.kernel.org
Signed-off-by: Sylwester Dziedziuch <sylwesterx.dziedziuch@intel.com>
Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
---
 drivers/net/ethernet/intel/iavf/iavf_ethtool.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
index 1cd1f3f..3909131 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
@@ -495,6 +495,11 @@ static int iavf_set_ringparam(struct net_device *netdev,
 	if ((ring->rx_mini_pending) || (ring->rx_jumbo_pending))
 		return -EINVAL;
 
+	if (adapter->state == __IAVF_RESETTING ||
+	    (adapter->state == __IAVF_RUNNING &&
+	     adapter->flags & IAVF_FLAG_QUEUES_DISABLED))
+		return -EAGAIN;
+
 	if (ring->tx_pending > IAVF_MAX_TXD ||
 	    ring->tx_pending < IAVF_MIN_TXD ||
 	    ring->rx_pending > IAVF_MAX_RXD ||
-- 
2.52.0

[PATCH iwl-net v2 4/5] iavf: fix TC boundary check in iavf_handle_tclass

[Aleksandr Loktionov]

From: Avinash Dayanand <avinash.dayanand@intel.com>

The condition 'tc < adapter->num_tc' only gates the destination-port
check for in-range TCs; when tc is out of range (tc >= num_tc) the if
body is skipped entirely and the function falls through to
VIRTCHNL_ACTION_TC_REDIRECT and returns 0, silently steering traffic to
a non-existent traffic class instead of rejecting the request.

Simply flipping the comparison to 'tc > adapter->num_tc' does not fix
this: it would let every in-range, non-zero tc (tc <= num_tc) skip the
destination-port requirement entirely, while an out-of-range tc with a
destination port set would still fall through and return 0.

Fix this by explicitly rejecting tc >= adapter->num_tc with -EINVAL,
and only then requiring a destination port for the remaining in-range,
non-zero TCs.

Fixes: 0075fa0fadd0 ("i40evf: Add support to apply cloud filters")
Cc: stable@vger.kernel.org
Signed-off-by: Avinash Dayanand <avinash.dayanand@intel.com>
Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
---
 drivers/net/ethernet/intel/iavf/iavf_main.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
index 29b8403..deb03f2 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_main.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
@@ -4047,12 +4047,15 @@ static int iavf_handle_tclass(struct iavf_adapter *adapter, u32 tc,
 {
 	if (tc == 0)
 		return 0;
-	if (tc < adapter->num_tc) {
-		if (!filter->f.data.tcp_spec.dst_port) {
-			dev_err(&adapter->pdev->dev,
-				"Specify destination port to redirect to traffic class other than TC0\n");
-			return -EINVAL;
-		}
+	if (tc >= adapter->num_tc) {
+		dev_err(&adapter->pdev->dev,
+			"Unable to add filter because of invalid destination traffic class\n");
+		return -EINVAL;
+	}
+	if (!filter->f.data.tcp_spec.dst_port) {
+		dev_err(&adapter->pdev->dev,
+			"Specify destination port to redirect to traffic class other than TC0\n");
+		return -EINVAL;
 	}
 	/* redirect to a traffic class on the same device */
 	filter->f.action = VIRTCHNL_ACTION_TC_REDIRECT;
-- 
2.52.0

[PATCH iwl-net v2 5/5] iavf: return 0 when TC flower filter not found after qdisc teardown

[Aleksandr Loktionov]

From: Kiran Patil <kiran.patil@intel.com>

When an egress qdisc is destroyed, the driver proactively deletes all
associated cloud filters to prevent stale hardware state, decrementing
num_cloud_filters to zero in the process.

The kernel netdev layer is unaware of this implicit cleanup and may
still try to delete the same filters individually. If the filter is
not found in the driver's list and num_cloud_filters is already zero,
return 0 instead of -EINVAL to avoid confusing upper layers that
believe the filter is still offloaded in hardware.

Fixes: 0075fa0fadd0 ("i40evf: Add support to apply cloud filters")
Cc: stable@vger.kernel.org
Signed-off-by: Kiran Patil <kiran.patil@intel.com>
Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
---
 drivers/net/ethernet/intel/iavf/iavf_main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
index e5bf15b..de80947 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_main.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
@@ -4162,7 +4162,8 @@ static int iavf_delete_clsflower(struct iavf_adapter *adapter,
 	if (filter) {
 		filter->del = true;
 		adapter->aq_required |= IAVF_FLAG_AQ_DEL_CLOUD_FILTER;
-	} else {
+	} else if (adapter->num_cloud_filters) {
+		/* other filters still tracked, so this one is a genuine miss */
 		err = -EINVAL;
 	}
 	spin_unlock_bh(&adapter->cloud_filter_list_lock);
-- 
2.52.0

Re: [PATCH iwl-net v2 1/5] iavf: fix null pointer dereference in iavf_detect_recover_hung

[Simon Horman]

On Tue, Sep 15, 2026 at 02:55:47PM +0200, Aleksandr Loktionov wrote:
> From: Kiran Patil <kiran.patil@intel.com>
> 
> iavf_watchdog_task() and iavf_reset_task() both run as work items on the
> same ordered adapter->wq, so they can't race with each other. However,
> iavf_set_ringparam() (and other ethtool ops) call iavf_reset_step()
> directly from process context under the netdev instance lock, without
> going through that workqueue at all. iavf_reset_step() can free and
> reallocate adapter->tx_rings and the q_vectors array via
> iavf_reinit_interrupt_scheme() while adapter->state still reads
> __IAVF_RUNNING, so the watchdog task can concurrently call
> iavf_detect_recover_hung() and dereference a NULL q_vector inside
> iavf_force_wb(), or index into a NULL tx_rings array, causing a crash.

I am concerned that iavf_reset_step() is also called from
iavf_set_channels(). And in that case the netdev instance lock is not held.

> 
> Guard against this by:
> - returning early if vsi->back->tx_rings itself is NULL, since
>   num_active_queues can still be nonzero while the array is being
>   reallocated;
> - skipping rings whose q_vector is NULL;
> - reading tx_ring->q_vector once with READ_ONCE() into a local variable
>   and reusing that same value for both the NULL check and the
>   iavf_force_wb() call, instead of re-reading the field right before
>   use, which would leave a window for the concurrent reset to swap it
>   from underneath us in between.
> 
> Also move the tx_ring declaration into the loop body and drop the
> redundant outer NULL initialisation, which the compiler can never
> observe since an array-element address is always non-NULL.
> 
> Fixes: 07d44190a389 ("i40e/i40evf: Detect and recover hung queue scenario")
> Cc: stable@vger.kernel.org
> Signed-off-by: Kiran Patil <kiran.patil@intel.com>
> Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>

...

Re: [PATCH iwl-net v2 2/5] iavf: fix error path in iavf_request_misc_irq

[Simon Horman]

On Tue, Sep 15, 2026 at 02:55:48PM +0200, Aleksandr Loktionov wrote:
> From: Piotr Gardocki <piotrx.gardocki@intel.com>
> 
> When request_irq() fails the interrupt vector was not registered for
> the driver. Calling free_irq() on a vector that was never successfully
> requested triggers a kernel warning. Drop the erroneous free_irq()
> call from the error path.
> 
> Fixes: 5eae00c57f5e ("i40evf: main driver core")
> Cc: stable@vger.kernel.org
> Signed-off-by: Piotr Gardocki <piotrx.gardocki@intel.com>
> Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>

This feels more like a clean-up than a bug fix.

But regardless, the change itself looks good to me:

Reviewed-by: Simon Horman <horms@kernel.org>

...

Re: [PATCH iwl-net v2 3/5] iavf: prevent VSI corruption when ring params changed during reset

[Simon Horman]

On Tue, Sep 15, 2026 at 02:55:49PM +0200, Aleksandr Loktionov wrote:
> From: Sylwester Dziedziuch <sylwesterx.dziedziuch@intel.com>
> 
> Changing ring parameters via ethtool triggers a VF reset and queue
> reconfiguration. If ethtool is called again before the first reset
> completes, the second reset races with uninitialised queue state and
> can corrupt the VSI resource tree on the PF side.
> 
> Return -EAGAIN from iavf_set_ringparam() when the adapter is already
> resetting or its queues are disabled.
> 
> Fixes: fbb7ddfef253 ("i40evf: core ethtool functionality")
> Cc: stable@vger.kernel.org
> Signed-off-by: Sylwester Dziedziuch <sylwesterx.dziedziuch@intel.com>
> Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> ---
>  drivers/net/ethernet/intel/iavf/iavf_ethtool.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
> index 1cd1f3f..3909131 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
> @@ -495,6 +495,11 @@ static int iavf_set_ringparam(struct net_device *netdev,
>  	if ((ring->rx_mini_pending) || (ring->rx_jumbo_pending))
>  		return -EINVAL;
>  
> +	if (adapter->state == __IAVF_RESETTING ||
> +	    (adapter->state == __IAVF_RUNNING &&
> +	     adapter->flags & IAVF_FLAG_QUEUES_DISABLED))
> +		return -EAGAIN;
> +
>  	if (ring->tx_pending > IAVF_MAX_TXD ||
>  	    ring->tx_pending < IAVF_MIN_TXD ||
>  	    ring->rx_pending > IAVF_MAX_RXD ||

I believe I reported this for v1 as well, but AI-generated review reports
a TOCTOU issue here. And I believe a similar, pre-existing, problem also
exists in iavf_set_channels.

Text from
https://sashiko.dev/#/patchset/20260915125551.3976068-1-aleksandr.loktionov%40intel.com
follows:

Could this state check introduce a time-of-check to time-of-use (TOCTOU) race
against the background reset task?

The background iavf_reset_task() relies on netdev_lock() to serialize, but
ethtool callbacks for drivers like iavf (which do not set request_ops_lock =
true) are executed under rtnl_lock() without holding netdev_lock().

Because they do not share a common lock, the background task can acquire
netdev_lock(), change the state to __IAVF_RESETTING, and tear down queues
immediately after iavf_set_ringparam() evaluates this condition to false.

Will this lock mismatch also cause a lockdep splat later in the same function?

iavf_set_ringparam() proceeds to call iavf_reset_step() directly:
drivers/net/ethernet/intel/iavf/iavf_ethtool.c:iavf_set_ringparam() {
...
	if (netif_running(netdev)) {
		adapter->flags |= IAVF_FLAG_RESET_NEEDED;
		iavf_reset_step(adapter);
	}
...
}

Looking at iavf_reset_step(), it asserts that netdev_lock() is held:

drivers/net/ethernet/intel/iavf/iavf_main.c:iavf_reset_step() {
...
	netdev_assert_locked(netdev);
...
}

Executing iavf_reset_step() from the ethtool callback without holding
netdev_lock() would trigger this lockdep assertion, and allow it to
concurrently step on any executing reset task.

Re: [PATCH iwl-net v2 4/5] iavf: fix TC boundary check in iavf_handle_tclass

[Simon Horman]

On Tue, Sep 15, 2026 at 02:55:50PM +0200, Aleksandr Loktionov wrote:
> From: Avinash Dayanand <avinash.dayanand@intel.com>
> 
> The condition 'tc < adapter->num_tc' only gates the destination-port
> check for in-range TCs; when tc is out of range (tc >= num_tc) the if
> body is skipped entirely and the function falls through to
> VIRTCHNL_ACTION_TC_REDIRECT and returns 0, silently steering traffic to
> a non-existent traffic class instead of rejecting the request.
> 
> Simply flipping the comparison to 'tc > adapter->num_tc' does not fix
> this: it would let every in-range, non-zero tc (tc <= num_tc) skip the
> destination-port requirement entirely, while an out-of-range tc with a
> destination port set would still fall through and return 0.
> 
> Fix this by explicitly rejecting tc >= adapter->num_tc with -EINVAL,
> and only then requiring a destination port for the remaining in-range,
> non-zero TCs.
> 
> Fixes: 0075fa0fadd0 ("i40evf: Add support to apply cloud filters")
> Cc: stable@vger.kernel.org
> Signed-off-by: Avinash Dayanand <avinash.dayanand@intel.com>
> Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>

Reviewed-by: Simon Horman <horms@kernel.org>

...

Re: [PATCH iwl-net v2 5/5] iavf: return 0 when TC flower filter not found after qdisc teardown

[Simon Horman]

On Tue, Sep 15, 2026 at 02:55:51PM +0200, Aleksandr Loktionov wrote:
> From: Kiran Patil <kiran.patil@intel.com>
> 
> When an egress qdisc is destroyed, the driver proactively deletes all
> associated cloud filters to prevent stale hardware state, decrementing
> num_cloud_filters to zero in the process.
> 
> The kernel netdev layer is unaware of this implicit cleanup and may
> still try to delete the same filters individually. If the filter is
> not found in the driver's list and num_cloud_filters is already zero,
> return 0 instead of -EINVAL to avoid confusing upper layers that
> believe the filter is still offloaded in hardware.
> 
> Fixes: 0075fa0fadd0 ("i40evf: Add support to apply cloud filters")
> Cc: stable@vger.kernel.org
> Signed-off-by: Kiran Patil <kiran.patil@intel.com>
> Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>

Reviewed-by: Simon Horman <horms@kernel.org>

...

RE: [PATCH iwl-net v2 3/5] iavf: prevent VSI corruption when ring params changed during reset

[Loktionov, Aleksandr]

> -----Original Message-----
> From: Simon Horman <horms@kernel.org>
> Sent: Friday, September 18, 2026 5:13 PM
> To: Loktionov, Aleksandr <aleksandr.loktionov@intel.com>
> Cc: intel-wired-lan@lists.osuosl.org; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>; netdev@vger.kernel.org; Sylwester
> Dziedziuch <sylwesterx.dziedziuch@intel.com>
> Subject: Re: [PATCH iwl-net v2 3/5] iavf: prevent VSI corruption when
> ring params changed during reset
> 
> On Tue, Sep 15, 2026 at 02:55:49PM +0200, Aleksandr Loktionov wrote:
> > From: Sylwester Dziedziuch <sylwesterx.dziedziuch@intel.com>
> >
> > Changing ring parameters via ethtool triggers a VF reset and queue
> > reconfiguration. If ethtool is called again before the first reset
> > completes, the second reset races with uninitialised queue state and
> > can corrupt the VSI resource tree on the PF side.
> >
> > Return -EAGAIN from iavf_set_ringparam() when the adapter is already
> > resetting or its queues are disabled.
> >
> > Fixes: fbb7ddfef253 ("i40evf: core ethtool functionality")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Sylwester Dziedziuch
> <sylwesterx.dziedziuch@intel.com>
> > Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> > ---
> >  drivers/net/ethernet/intel/iavf/iavf_ethtool.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
> > b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
> > index 1cd1f3f..3909131 100644
> > --- a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
> > +++ b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
> > @@ -495,6 +495,11 @@ static int iavf_set_ringparam(struct net_device
> *netdev,
> >  	if ((ring->rx_mini_pending) || (ring->rx_jumbo_pending))
> >  		return -EINVAL;
> >
> > +	if (adapter->state == __IAVF_RESETTING ||
> > +	    (adapter->state == __IAVF_RUNNING &&
> > +	     adapter->flags & IAVF_FLAG_QUEUES_DISABLED))
> > +		return -EAGAIN;
> > +
> >  	if (ring->tx_pending > IAVF_MAX_TXD ||
> >  	    ring->tx_pending < IAVF_MIN_TXD ||
> >  	    ring->rx_pending > IAVF_MAX_RXD ||
> 
> I believe I reported this for v1 as well, but AI-generated review
> reports a TOCTOU issue here. And I believe a similar, pre-existing,
> problem also exists in iavf_set_channels.
> 
> Text from
> https://sashiko.dev/#/patchset/20260915125551.3976068-1-
> aleksandr.loktionov%40intel.com
> follows:
> 
> Could this state check introduce a time-of-check to time-of-use
> (TOCTOU) race against the background reset task?
> 
> The background iavf_reset_task() relies on netdev_lock() to serialize,
> but ethtool callbacks for drivers like iavf (which do not set
> request_ops_lock =
> true) are executed under rtnl_lock() without holding netdev_lock().
> 
> Because they do not share a common lock, the background task can
> acquire netdev_lock(), change the state to __IAVF_RESETTING, and tear
> down queues immediately after iavf_set_ringparam() evaluates this
> condition to false.
> 
> Will this lock mismatch also cause a lockdep splat later in the same
> function?
> 
> iavf_set_ringparam() proceeds to call iavf_reset_step() directly:
> drivers/net/ethernet/intel/iavf/iavf_ethtool.c:iavf_set_ringparam() {
> ...
> 	if (netif_running(netdev)) {
> 		adapter->flags |= IAVF_FLAG_RESET_NEEDED;
> 		iavf_reset_step(adapter);
> 	}
> ...
> }
> 
> Looking at iavf_reset_step(), it asserts that netdev_lock() is held:
> 
> drivers/net/ethernet/intel/iavf/iavf_main.c:iavf_reset_step() { ...
> 	netdev_assert_locked(netdev);
> ...
> }
> 
> Executing iavf_reset_step() from the ethtool callback without holding
> netdev_lock() would trigger this lockdep assertion, and allow it to
> concurrently step on any executing reset task.

Good day, Simon

You're right, and it's still unresolved here. iavf's ethtool_ops
doesn't set request_ops_lock, so iavf_set_ringparam() runs under
rtnl_lock() only, with no netdev_lock() held - while iavf_reset_task()
and iavf_watchdog_task() take netdev_lock() from adapter->wq. The state
check and the iavf_reset_step() call right after it aren't serialized
against the background task at all, so the window this patch is meant
to close is still open.

Fixing this properly means closing that lock gap for every ethtool op
that can call iavf_reset_step() - at least set_ringparam and
set_channels, maybe more - not adding a state check to one call site.
That's bigger than this patch, so 

I'm dropping it rather than resend the same check. I'll come back to it as a proper locking fix.

Sorry and, thanks for catching this twice now.

Alex

RE: [PATCH iwl-net v2 1/5] iavf: fix null pointer dereference in iavf_detect_recover_hung

[Loktionov, Aleksandr]

> -----Original Message-----
> From: Simon Horman <horms@kernel.org>
> Sent: Friday, September 18, 2026 5:11 PM
> To: Loktionov, Aleksandr <aleksandr.loktionov@intel.com>
> Cc: intel-wired-lan@lists.osuosl.org; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>; netdev@vger.kernel.org; Kiran Patil
> <kiran.patil@intel.com>
> Subject: Re: [PATCH iwl-net v2 1/5] iavf: fix null pointer dereference
> in iavf_detect_recover_hung
> 
> On Tue, Sep 15, 2026 at 02:55:47PM +0200, Aleksandr Loktionov wrote:
> > From: Kiran Patil <kiran.patil@intel.com>
> >
> > iavf_watchdog_task() and iavf_reset_task() both run as work items on
> > the same ordered adapter->wq, so they can't race with each other.
> > However,
> > iavf_set_ringparam() (and other ethtool ops) call iavf_reset_step()
> > directly from process context under the netdev instance lock, without
> > going through that workqueue at all. iavf_reset_step() can free and
> > reallocate adapter->tx_rings and the q_vectors array via
> > iavf_reinit_interrupt_scheme() while adapter->state still reads
> > __IAVF_RUNNING, so the watchdog task can concurrently call
> > iavf_detect_recover_hung() and dereference a NULL q_vector inside
> > iavf_force_wb(), or index into a NULL tx_rings array, causing a crash.
> 
> I am concerned that iavf_reset_step() is also called from
> iavf_set_channels(). And in that case the netdev instance lock is not
> held.
> 
> >
> > Guard against this by:
> > - returning early if vsi->back->tx_rings itself is NULL, since
> >   num_active_queues can still be nonzero while the array is being
> >   reallocated;
> > - skipping rings whose q_vector is NULL;
> > - reading tx_ring->q_vector once with READ_ONCE() into a local
> variable
> >   and reusing that same value for both the NULL check and the
> >   iavf_force_wb() call, instead of re-reading the field right before
> >   use, which would leave a window for the concurrent reset to swap it
> >   from underneath us in between.
> >
> > Also move the tx_ring declaration into the loop body and drop the
> > redundant outer NULL initialisation, which the compiler can never
> > observe since an array-element address is always non-NULL.
> >
> > Fixes: 07d44190a389 ("i40e/i40evf: Detect and recover hung queue
> > scenario")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Kiran Patil <kiran.patil@intel.com>
> > Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> 
> ...

Confirmed - same gap as 3/5. The READ_ONCE()/NULL-check guards here only
narrow the window where iavf_watchdog_task() can observe a half-torn-
down tx_rings/q_vector array; they don't hold a reference or lock that
keeps those allocations alive, so iavf_free_q_vectors() and
iavf_free_queues() can still free out from under the watchdog between
the check and the use, from set_channels or any other path that calls
iavf_reset_step() without the instance lock.

Dropping this patch rather than resend guards that don't actually close
the race. Once there's a real fix for the locking gap across iavf's
ethtool ops, I'll revisit whether any of these NULL checks are still
worth keeping on top of it.

Thanks,
Alex