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 <[email protected]> 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/[email protected]?part=1
