From: Kiran Patil <[email protected]>
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: [email protected]
Signed-off-by: Kiran Patil <[email protected]>
Signed-off-by: Aleksandr Loktionov <[email protected]>
---
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
