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

[sashiko-bot@kernel.org]

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