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

[PATCH iwl-net v1] 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, but
iavf_remove() resets the VF itself before it frees the rings.

Fixes: e284fc88dfb6 ("i40evf: delay releasing rings")
Assisted-by: LLM
Signed-off-by: Yuho Choi <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_virtchnl.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
index e6b7e8f82c7c..178bba609677 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
@@ -2718,6 +2718,15 @@ void iavf_virtchnl_completion(struct iavf_adapter *adapter,
 		adapter->flags &= ~IAVF_FLAG_QUEUES_DISABLED;
 		break;
 	case VIRTCHNL_OP_DISABLE_QUEUES:
+		/* 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.
+		 */
+		if (v_retval) {
+			iavf_schedule_reset(adapter, IAVF_FLAG_RESET_NEEDED);
+			break;
+		}
 		iavf_free_all_tx_resources(adapter);
 		iavf_free_all_rx_resources(adapter);
 		if (adapter->state == __IAVF_DOWN_PENDING) {

base-commit: c03c1a14507dfaf8b856c2608f9ac216021fc222
-- 
2.43.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] iavf: infinite loop in iavf_remove() when queue disable fails
- [High] iavf: ring resource leak and dangling NAPI in iavf_disable_vf()
- [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 362caa9d16f7d9e3e52d1978b838d37d34c2a1f6
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 patch keeps
the Tx and Rx ring resources allocated and schedules a VF reset instead of
freeing the rings immediately in iavf_virtchnl_completion().

> diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> index e6b7e8f82c7c1..178bba609677f 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> @@ -2718,6 +2718,15 @@ void iavf_virtchnl_completion(struct iavf_adapter *adapter,
>  		adapter->flags &= ~IAVF_FLAG_QUEUES_DISABLED;
>  		break;
>  	case VIRTCHNL_OP_DISABLE_QUEUES:
> +		/* 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.
> +		 */
> +		if (v_retval) {
> +			iavf_schedule_reset(adapter, IAVF_FLAG_RESET_NEEDED);
> +			break;
> +		}

[Severity: High]
Can this cause an infinite loop in iavf_remove() when disabling queues fails?

In drivers/net/ethernet/intel/iavf/iavf_main.c, iavf_close() calls
iavf_down(), sets adapter->state to __IAVF_DOWN_PENDING, releases
netdev_lock(netdev), and waits up to 500ms on adapter->down_waitqueue for
adapter->state to become __IAVF_DOWN.

If iavf_remove() is invoked while adapter->state is __IAVF_DOWN_PENDING
(either while iavf_close() is sleeping in wait_event_timeout() with the
netdev lock released, or after the 500ms wait times out), iavf_remove() sets
__IAVF_IN_REMOVE_TASK and loops until adapter->state leaves
__IAVF_DOWN_PENDING:

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

	/* Wait until port initialization is complete.
	 * There are flows where register/unregister netdev may race.
	 */
	while (1) {
		netdev_lock(netdev);
		if (adapter->state == __IAVF_RUNNING ||
		    adapter->state == __IAVF_DOWN ||
		    adapter->state == __IAVF_INIT_FAILED) {
			netdev_unlock(netdev);
			break;
		}
		/* Simply return if we already went through iavf_shutdown */
		if (adapter->state == __IAVF_REMOVE) {
			netdev_unlock(netdev);
			return;
		}

		netdev_unlock(netdev);
		usleep_range(500, 1000);
	}
...
}

When VIRTCHNL_OP_DISABLE_QUEUES fails with a non-zero v_retval,
iavf_virtchnl_completion() now breaks out without changing adapter->state
from __IAVF_DOWN_PENDING to __IAVF_DOWN, and calls iavf_schedule_reset().
However, iavf_schedule_reset() does nothing when __IAVF_IN_REMOVE_TASK is
already set:

drivers/net/ethernet/intel/iavf/iavf_main.c:iavf_schedule_reset() {
	if (!test_bit(__IAVF_IN_REMOVE_TASK, &adapter->crit_section) &&
	    !(adapter->flags &
	    (IAVF_FLAG_RESET_PENDING | IAVF_FLAG_RESET_NEEDED))) {
		adapter->flags |= flags;
		queue_work(adapter->wq, &adapter->reset_task);
	}
}

Since reset_task is not queued and adapter->state remains
__IAVF_DOWN_PENDING, won't iavf_remove() spin forever in the while (1) loop
before ever reaching unregister_netdev() or iavf_request_reset()?

[Severity: High]
What happens if the reset scheduled here times out in iavf_reset_step() while
adapter->state is __IAVF_DOWN_PENDING?

When VIRTCHNL_OP_DISABLE_QUEUES fails during iavf_close(), adapter->state is
__IAVF_DOWN_PENDING and the per-ring Tx and Rx resources remain allocated. If
the subsequent VF reset never completes in iavf_reset_step(), it calls
iavf_disable_vf() and returns before reaching continue_reset:

drivers/net/ethernet/intel/iavf/iavf_main.c:iavf_reset_step() {
...
	if (i == IAVF_RESET_WAIT_COMPLETE_COUNT) {
		dev_err(&adapter->pdev->dev, "Reset never finished (%x)\n",
			reg_val);
		iavf_disable_vf(adapter);
		return; /* Do not attempt to reinit. It's dead, Jim. */
	}
...
}

In iavf_disable_vf(), iavf_free_all_tx_resources() and
iavf_free_all_rx_resources() are only called when adapter->state is
__IAVF_RUNNING:

drivers/net/ethernet/intel/iavf/iavf_main.c:iavf_disable_vf() {
...
	if (adapter->state == __IAVF_RUNNING) {
		set_bit(__IAVF_VSI_DOWN, adapter->vsi.state);
		netif_carrier_off(adapter->netdev);
		netif_tx_disable(adapter->netdev);
		adapter->link_up = false;
		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);
	}
...
	iavf_free_misc_irq(adapter);
	iavf_free_interrupt_scheme(adapter);
...
}

iavf_free_interrupt_scheme() then calls iavf_free_q_vectors() and
iavf_free_queues():

drivers/net/ethernet/intel/iavf/iavf_main.c:iavf_free_queues() {
	if (!adapter->vsi_res)
		return;
	adapter->num_active_queues = 0;
	kfree(adapter->tx_rings);
	adapter->tx_rings = NULL;
	kfree(adapter->rx_rings);
	adapter->rx_rings = NULL;
}

Because adapter->state is __IAVF_DOWN_PENDING rather than __IAVF_RUNNING,
doesn't this free adapter->q_vectors (and each embedded q_vector->napi) while
the per-ring rx_ring->pp page pools still hold pointers to &q_vector->napi,
and leak all per-ring descriptor rings (tx_ring->desc, rx_ring->desc), buffer
metadata arrays (tx_ring->tx_bi, rx_ring->rx_fqes), mapped skbs and Rx pages,
and rx_ring->pp page pools once adapter->tx_rings and adapter->rx_rings are
freed and adapter->num_active_queues is set to 0?

[Severity: Medium]
Does scheduling a reset here lose pending filter deletions that were queued
by iavf_down()?

When iavf_close() calls iavf_down(), the driver marks active filters for
deletion (f->remove = true, cf->del = true, and
rss->state = IAVF_ADV_RSS_DEL_REQUEST) and sets the corresponding
IAVF_FLAG_AQ_DEL_* flags in adapter->aq_required 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 (!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 sent before any
of the IAVF_FLAG_AQ_DEL_* flags. If VIRTCHNL_OP_DISABLE_QUEUES fails and
iavf_virtchnl_completion() triggers a reset, iavf_reset_step() clears
adapter->aq_required and marks the remaining entries in
adapter->mac_filter_list (and adapter->cloud_filter_list when ADQ is active)
to be re-added:

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 leave advanced RSS configurations stuck in
IAVF_ADV_RSS_DEL_REQUEST on adapter->adv_rss_list_head (causing future
iavf_set_rxfh_fields() calls for the same packet headers to fail with
-EBUSY), leave cloud filters with cf->del = true on
adapter->cloud_filter_list without freeing them (causing future
iavf_configure_clsflower() calls with the same cookie to fail with -EEXIST,
or re-adding them with cf->add = true when ADQ is active), and re-add
secondary or multicast MAC addresses on adapter->mac_filter_list that
iavf_clear_mac_filters() had marked with f->remove = true?

[Severity: High]
Can scheduling a reset here deadlock in iavf_reset_step() if
VIRTCHNL_OP_DISABLE_QUEUES was triggered by iavf_suspend()?

When suspending while the interface is running, iavf_suspend() calls
iavf_down() without changing adapter->state from __IAVF_RUNNING:

drivers/net/ethernet/intel/iavf/iavf_main.c:iavf_suspend() {
...
	if (running)
		iavf_down(adapter);

	iavf_free_misc_irq(adapter);
	iavf_reset_interrupt_capability(adapter);
...
}

iavf_down() calls iavf_napi_disable_all(adapter), which disables each
q_vector->napi (setting NAPIF_STATE_SCHED), and schedules
IAVF_FLAG_AQ_DISABLE_QUEUES.

If VIRTCHNL_OP_DISABLE_QUEUES fails before the admin queue is shut down and
iavf_virtchnl_completion() schedules adapter->reset_task, iavf_reset_step()
runs under netdev_lock(netdev), sees adapter->state == __IAVF_RUNNING, and
calls iavf_napi_disable_all(adapter) a second time (since
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);
	}
...
}

Since the NAPI instances were already disabled by iavf_down(), won't calling
iavf_napi_disable_all() again cause napi_disable_locked() to loop forever
waiting for NAPIF_STATE_SCHED to clear while holding netdev_lock(netdev)?

>  		iavf_free_all_tx_resources(adapter);
>  		iavf_free_all_rx_resources(adapter);
>  		if (adapter->state == __IAVF_DOWN_PENDING) {

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