From: Kiran Patil iavf_watchdog_task() and iavf_reset_task() both run as work items on the same ordered adapter->wq, so they can't race with each other. However, iavf_set_ringparam() (and other ethtool ops) call iavf_reset_step() directly from process context under the netdev instance lock, without going through that workqueue at all. iavf_reset_step() can free and reallocate adapter->tx_rings and the q_vectors array via iavf_reinit_interrupt_scheme() while adapter->state still reads __IAVF_RUNNING, so the watchdog task can concurrently call iavf_detect_recover_hung() and dereference a NULL q_vector inside iavf_force_wb(), or index into a NULL tx_rings array, causing a crash. Guard against this by: - returning early if vsi->back->tx_rings itself is NULL, since num_active_queues can still be nonzero while the array is being reallocated; - skipping rings whose q_vector is NULL; - reading tx_ring->q_vector once with READ_ONCE() into a local variable and reusing that same value for both the NULL check and the iavf_force_wb() call, instead of re-reading the field right before use, which would leave a window for the concurrent reset to swap it from underneath us in between. Also move the tx_ring declaration into the loop body and drop the redundant outer NULL initialisation, which the compiler can never observe since an array-element address is always non-NULL. Fixes: 07d44190a389 ("i40e/i40evf: Detect and recover hung queue scenario") Cc: stable@vger.kernel.org Signed-off-by: Kiran Patil Signed-off-by: Aleksandr Loktionov --- drivers/net/ethernet/intel/iavf/iavf_txrx.c | 50 ++++++++++++--------- 1 file changed, 29 insertions(+), 21 deletions(-) diff --git a/drivers/net/ethernet/intel/iavf/iavf_txrx.c b/drivers/net/ethernet/intel/iavf/iavf_txrx.c index c30abf1..f1c26a9 100644 --- a/drivers/net/ethernet/intel/iavf/iavf_txrx.c +++ b/drivers/net/ethernet/intel/iavf/iavf_txrx.c @@ -176,7 +176,6 @@ static void iavf_force_wb(struct iavf_vsi *vsi, struct iavf_q_vector *q_vector) **/ void iavf_detect_recover_hung(struct iavf_vsi *vsi) { - struct iavf_ring *tx_ring = NULL; struct net_device *netdev; unsigned int i; int packets; @@ -194,29 +193,38 @@ void iavf_detect_recover_hung(struct iavf_vsi *vsi) if (!netif_carrier_ok(netdev)) return; + /* tx_rings can be freed/reallocated by a concurrent reset */ + if (!vsi->back->tx_rings) + return; + for (i = 0; i < vsi->back->num_active_queues; i++) { - tx_ring = &vsi->back->tx_rings[i]; - if (tx_ring && tx_ring->desc) { - /* If packet counter has not changed the queue is - * likely stalled, so force an interrupt for this - * queue. - * - * prev_pkt_ctr would be negative if there was no - * pending work. - */ - packets = tx_ring->stats.packets & INT_MAX; - if (tx_ring->prev_pkt_ctr == packets) { - iavf_force_wb(vsi, tx_ring->q_vector); - continue; - } + struct iavf_ring *tx_ring = &vsi->back->tx_rings[i]; + struct iavf_q_vector *q_vector; - /* Memory barrier between read of packet count and call - * to iavf_get_tx_pending() - */ - smp_rmb(); - tx_ring->prev_pkt_ctr = - iavf_get_tx_pending(tx_ring, true) ? packets : -1; + /* read once, q_vector can be reassigned by a concurrent reset */ + q_vector = READ_ONCE(tx_ring->q_vector); + if (!q_vector || !tx_ring->desc) + continue; + + /* If packet counter has not changed the queue is + * likely stalled, so force an interrupt for this + * queue. + * + * prev_pkt_ctr would be negative if there was no + * pending work. + */ + packets = tx_ring->stats.packets & INT_MAX; + if (tx_ring->prev_pkt_ctr == packets) { + iavf_force_wb(vsi, q_vector); + continue; } + + /* Memory barrier between read of packet count and call + * to iavf_get_tx_pending() + */ + smp_rmb(); + tx_ring->prev_pkt_ctr = + iavf_get_tx_pending(tx_ring, true) ? packets : -1; } } -- 2.52.0