[PATCH iwl-net v2] iavf: reset the VF instead of freeing rings when queue disable fails

[PATCH iwl-net v2] iavf: reset the VF instead of freeing rings when queue disable fails

[Yuho Choi]

iavf_close() leaves the Tx/Rx rings in place because the hardware can
still DMA into them, and iavf_virtchnl_completion() frees them once the
PF has answered VIRTCHNL_OP_DISABLE_QUEUES. It does so whatever the PF
answered: for a non-zero v_retval the error is only logged and the
DISABLE_QUEUES case runs anyway.

The PF can fail the request while queues are still running. ice and
i40e give up before the Rx queues when a Tx queue cannot be stopped,
and ice also fails when an Rx queue does not stop in time. The rings
and Rx buffers are then freed while the queues still point at them, and
received packets and descriptor write-backs land in freed memory.

Keep the rings when the PF reports an error and request a VF reset
instead. The reset makes the PF stop all queues of the VF, and
iavf_reset_step() frees the rings once it has completed. Until then
iavf_close() may warn that the resources are not yet released, and
iavf_open() returns -EBUSY.

During removal no reset is scheduled, and iavf_remove() would wait for
the state to leave __IAVF_DOWN_PENDING forever. As it resets the VF
itself before it frees the rings, complete the transition to
__IAVF_DOWN in that case. If the reset never completes,
iavf_disable_vf() now frees the kept rings as it does for a running VF,
instead of leaving them with page pools that point to the q_vectors it
frees.

Fixes: e284fc88dfb6 ("i40evf: delay releasing rings")
Signed-off-by: Yuho Choi <oss.patchbox@gmail.com>
---
v2:
- Let iavf_remove() go on when the failure arrives during removal, where
  no reset can be scheduled (sashiko).
- Free the kept rings in iavf_disable_vf() when the reset never
  completes (sashiko).
- Rebased on v7.4-rc1.

v1: https://lore.kernel.org/r/20261022192753.69765-2-oss.patchbox@gmail.com

This was found by code inspection as part of ongoing, not yet
published research. It has not been reproduced, as that needs a PF
that fails VIRTCHNL_OP_DISABLE_QUEUES.
Compile-tested only (x86_64 defconfig with IAVF, I40E and ICE, W=1).

 drivers/net/ethernet/intel/iavf/iavf_main.c    |  7 +++++--
 .../net/ethernet/intel/iavf/iavf_virtchnl.c    | 18 ++++++++++++++++--
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
index 2f8a80a11336..70ee5ae14a64 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_main.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
@@ -3064,9 +3064,12 @@ static void iavf_disable_vf(struct iavf_adapter *adapter)
 		iavf_napi_disable_all(adapter);
 		iavf_irq_disable(adapter);
 		iavf_free_traffic_irqs(adapter);
-		iavf_free_all_tx_resources(adapter);
-		iavf_free_all_rx_resources(adapter);
 	}
+	/* Also release rings that iavf_close() left in place because the
+	 * PF failed to disable the queues.
+	 */
+	iavf_free_all_tx_resources(adapter);
+	iavf_free_all_rx_resources(adapter);
 
 	spin_lock_bh(&adapter->mac_vlan_list_lock);
 
diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
index e6b7e8f82c7c..28818319a6df 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
@@ -2718,8 +2718,22 @@ void iavf_virtchnl_completion(struct iavf_adapter *adapter,
 		adapter->flags &= ~IAVF_FLAG_QUEUES_DISABLED;
 		break;
 	case VIRTCHNL_OP_DISABLE_QUEUES:
-		iavf_free_all_tx_resources(adapter);
-		iavf_free_all_rx_resources(adapter);
+		/* If the PF could not stop the queues, the hardware may still
+		 * access the rings and Rx buffers. Keep them and request a VF
+		 * reset; the reset path releases them after the PF has reset
+		 * the VF. During removal no reset is scheduled, but
+		 * iavf_remove() resets the VF itself before it releases the
+		 * rings, so let it stop waiting for the queues to go down.
+		 */
+		if (v_retval) {
+			iavf_schedule_reset(adapter, IAVF_FLAG_RESET_NEEDED);
+			if (!test_bit(__IAVF_IN_REMOVE_TASK,
+				      &adapter->crit_section))
+				break;
+		} else {
+			iavf_free_all_tx_resources(adapter);
+			iavf_free_all_rx_resources(adapter);
+		}
 		if (adapter->state == __IAVF_DOWN_PENDING) {
 			iavf_change_state(adapter, __IAVF_DOWN);
 			wake_up(&adapter->down_waitqueue);
-- 
2.43.0

Re: [PATCH iwl-net v2] iavf: reset the VF instead of freeing rings when queue disable fails

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] iavf: ring overwrite and leak if iavf_open() races with removal after disable failure
- [High] iavf: deadlock in iavf_reset_step() when disable queues fails on suspend
- [Medium] iavf: pending filter deletions lost when reset is scheduled on queue disable failure
--

commit aa95b33cf39978724a2d6af912102047e7464b0f
Author: Yuho Choi <oss.patchbox@gmail.com>

iavf: reset the VF instead of freeing rings when queue disable fails

When the PF returns an error for VIRTCHNL_OP_DISABLE_QUEUES, this commit
keeps the Tx and Rx rings allocated and schedules a VF reset instead of
freeing the rings while queues may still be active. It also updates removal
and VF disable paths to handle the kept rings and state transitions.

> diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> index 66c8b0261fd62..ac74b235aa871 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> @@ -2726,8 +2726,22 @@ void iavf_virtchnl_completion(struct iavf_adapter *adapter,
>  		adapter->flags &= ~IAVF_FLAG_QUEUES_DISABLED;
>  		break;
>  	case VIRTCHNL_OP_DISABLE_QUEUES:
> -		iavf_free_all_tx_resources(adapter);
> -		iavf_free_all_rx_resources(adapter);
> +		/* If the PF could not stop the queues, the hardware may still
> +		 * access the rings and Rx buffers. Keep them and request a VF
> +		 * reset; the reset path releases them after the PF has reset
> +		 * the VF. During removal no reset is scheduled, but
> +		 * iavf_remove() resets the VF itself before it releases the
> +		 * rings, so let it stop waiting for the queues to go down.
> +		 */
> +		if (v_retval) {
> +			iavf_schedule_reset(adapter, IAVF_FLAG_RESET_NEEDED);

[Severity: Medium]
As reported by Sashiko on v1 [1], when iavf_close() calls iavf_down() in
drivers/net/ethernet/intel/iavf/iavf_main.c, active filters are marked for
deletion and their adminq flags are set alongside
IAVF_FLAG_AQ_DISABLE_QUEUES:

drivers/net/ethernet/intel/iavf/iavf_main.c:iavf_down() {
	iavf_clear_mac_filters(adapter);
	iavf_clear_cloud_filters(adapter);
	iavf_clear_fdir_filters(adapter);
	iavf_clear_adv_rss_conf(adapter);
	...
	if (!test_bit(__IAVF_IN_REMOVE_TASK, &adapter->crit_section)) {
		adapter->current_op = VIRTCHNL_OP_UNKNOWN;
		if (!list_empty(&adapter->mac_filter_list))
			adapter->aq_required |= IAVF_FLAG_AQ_DEL_MAC_FILTER;
		if (!list_empty(&adapter->cloud_filter_list))
			adapter->aq_required |= IAVF_FLAG_AQ_DEL_CLOUD_FILTER;
		if (!list_empty(&adapter->fdir_list_head))
			adapter->aq_required |= IAVF_FLAG_AQ_DEL_FDIR_FILTER;
		if (!list_empty(&adapter->adv_rss_list_head))
			adapter->aq_required |= IAVF_FLAG_AQ_DEL_ADV_RSS_CFG;
	}

	iavf_schedule_aq_request(adapter, IAVF_FLAG_AQ_DISABLE_QUEUES);
}

In iavf_process_aq_command(), IAVF_FLAG_AQ_DISABLE_QUEUES is processed
before the IAVF_FLAG_AQ_DEL_* flags. When VIRTCHNL_OP_DISABLE_QUEUES fails
and schedules a reset here, iavf_reset_step() zeroes adapter->aq_required:

drivers/net/ethernet/intel/iavf/iavf_main.c:iavf_reset_step() {
	adapter->aq_required = 0;
	...
	/* re-add all MAC filters */
	list_for_each_entry(f, &adapter->mac_filter_list, list) {
		f->add = true;
	}
}

Won't this lose the pending filter deletions and leak or corrupt the filter
lists? Specifically:
- Advanced RSS filters in adapter->adv_rss_list_head remain stuck in
  IAVF_ADV_RSS_DEL_REQUEST and are never freed, causing subsequent
  iavf_set_rxfh_fields() calls for the same headers to return -EBUSY.
- Cloud filters in adapter->cloud_filter_list remain with cf->del = true
  and are never freed, causing subsequent iavf_configure_clsflower() calls
  with the same cookie to fail with -EEXIST (or, when ADQ is active, get
  cf->add = true set in iavf_reset_step() and get re-added while cf->del is
  still true).
- Secondary and multicast MAC filters that were unsynced and marked with
  f->remove = true in iavf_clear_mac_filters() get f->add = true set in
  iavf_reset_step() and f->remove = false in iavf_virtchnl_completion(),
  re-adding them on the PF.

[Severity: High]
As reported by Sashiko on v1 [2], what happens if VIRTCHNL_OP_DISABLE_QUEUES
fails during system suspend?

In drivers/net/ethernet/intel/iavf/iavf_main.c, iavf_suspend() calls
iavf_down() while leaving adapter->state set to __IAVF_RUNNING:

drivers/net/ethernet/intel/iavf/iavf_main.c:iavf_suspend() {
	running = (adapter->state == __IAVF_RUNNING);
	if (running)
		iavf_down(adapter);

	iavf_free_misc_irq(adapter);
	iavf_reset_interrupt_capability(adapter);
}

iavf_down() calls iavf_napi_disable_all(adapter) and schedules
IAVF_FLAG_AQ_DISABLE_QUEUES. If VIRTCHNL_OP_DISABLE_QUEUES fails and
iavf_virtchnl_completion() schedules reset_task, iavf_reset_step() (or
iavf_disable_vf() if the reset times out) sees adapter->state ==
__IAVF_RUNNING while adapter->num_msix_vectors is still non-zero:

drivers/net/ethernet/intel/iavf/iavf_main.c:iavf_reset_step() {
	running = adapter->state == __IAVF_RUNNING;

	if (running) {
		netif_carrier_off(netdev);
		netif_tx_stop_all_queues(netdev);
		adapter->link_up = false;
		iavf_napi_disable_all(adapter);
	}
}

Wouldn't calling iavf_napi_disable_all() a second time hang forever in
napi_disable_locked() while holding netdev_lock(netdev)?

> +			if (!test_bit(__IAVF_IN_REMOVE_TASK,
> +				      &adapter->crit_section))
> +				break;
> +		} else {
> +			iavf_free_all_tx_resources(adapter);
> +			iavf_free_all_rx_resources(adapter);
> +		}
> +		if (adapter->state == __IAVF_DOWN_PENDING) {
> +			iavf_change_state(adapter, __IAVF_DOWN);
> +			wake_up(&adapter->down_waitqueue);
> +		}

[Severity: High]
Can this transition to __IAVF_DOWN while rings are still allocated race
with a concurrent iavf_open() when a non-removal dev_close() overlaps with
iavf_remove()?

Consider a thread in dev_close() -> iavf_close() waiting on
adapter->down_waitqueue while holding rtnl_lock. Concurrently, iavf_remove()
starts, sets __IAVF_IN_REMOVE_TASK, and waits for adapter->state to leave
__IAVF_DOWN_PENDING before calling unregister_netdev():

drivers/net/ethernet/intel/iavf/iavf_main.c:iavf_remove() {
	if (test_and_set_bit(__IAVF_IN_REMOVE_TASK, &adapter->crit_section))
		return;

	while (1) {
		netdev_lock(netdev);
		if (adapter->state == __IAVF_RUNNING ||
		    adapter->state == __IAVF_DOWN ||
		    adapter->state == __IAVF_INIT_FAILED) {
			netdev_unlock(netdev);
			break;
		}
		netdev_unlock(netdev);
		usleep_range(500, 1000);
	}
	cancel_delayed_work_sync(&adapter->watchdog_task);
	cancel_work_sync(&adapter->finish_config);

	if (adapter->netdev_registered) {
		unregister_netdev(netdev);
		...
	}
}

If VIRTCHNL_OP_DISABLE_QUEUES fails while __IAVF_IN_REMOVE_TASK is set,
iavf_virtchnl_completion() skips freeing the Tx/Rx rings, sets
adapter->state to __IAVF_DOWN, and wakes adapter->down_waitqueue. Once
iavf_close() finishes and drops rtnl_lock, before iavf_remove() reaches
unregister_netdev(netdev) and acquires rtnl_lock, another thread can call
dev_open() -> iavf_open():

drivers/net/ethernet/intel/iavf/iavf_main.c:iavf_open() {
	if (adapter->state != __IAVF_DOWN)
		return -EBUSY;
	...
	err = iavf_setup_all_tx_resources(adapter);
	if (err)
		goto err_setup_tx;
	err = iavf_setup_all_rx_resources(adapter);
}

Because iavf_open() does not check __IAVF_IN_REMOVE_TASK and sees
adapter->state == __IAVF_DOWN, won't it call iavf_setup_tx_descriptors()
and iavf_setup_rx_descriptors() on the kept rings, triggering
WARN_ON(tx_ring->tx_bi) in drivers/net/ethernet/intel/iavf/iavf_txrx.c and
overwriting and leaking tx_ring->tx_bi, tx_ring->desc, rx_ring->rx_fqes,
rx_ring->desc, and rx_ring->pp (leaving the leaked page_pool with a
dangling pool->p.napi pointer to adapter->q_vectors after removal frees
them)?

Also, because iavf_remove() sets __IAVF_IN_REMOVE_TASK without holding
netdev_lock, testing __IAVF_IN_REMOVE_TASK once inside
iavf_schedule_reset() and a second time here in iavf_virtchnl_completion()
can see different values if __IAVF_IN_REMOVE_TASK is set in between,
both queuing adapter->reset_task and transitioning adapter->state to
__IAVF_DOWN.

[1] https://lore.kernel.org/r/20261022192753.69765-2-oss.patchbox@gmail.com
[2] https://lore.kernel.org/r/20261022192753.69765-2-oss.patchbox@gmail.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102032744.304907-2-oss.patchbox@gmail.com?part=1