Re: [PATCH iwl-net v2] 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 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