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 <[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 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/[email protected] [2] https://lore.kernel.org/r/[email protected] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
