[PATCH iwl-net v2 2/2] i40e: avoid resetting BQL state when freeing temporary Tx rings in set_ringparam

Aleksandr Loktionov <[email protected]>
Newsgroups org.osuosl.intel-wired-lan,org.kernel.vger.netdev
Message-ID <[email protected]>
Temporary Tx rings built by i40e_set_ringparam() while resizing
descriptor counts are shallow struct copies of the live vsi->tx_rings[]
entries, so they retain the same ring->netdev and ring->queue_index as
the live ring they are staged to replace.

If an error path frees one of these clones with
i40e_free_tx_resources() before i40e_down() has run - either the
i40e_setup_tx_descriptors() failure unwind loop, or the free_tx: error
label - i40e_clean_tx_ring() will call netdev_tx_reset_queue() on
txring_txq(tx_ring), which resolves to the *same* netdev_queue as the
live ring because netdev/queue_index alias. That call resets the
queue's BQL/dql state (dql_reset(): num_queued = num_completed = 0)
while the live ring is still actively transmitting and completing
Tx on that same queue.

The next time the live ring reports completions for bytes that were
queued before the reset, dql_completed() can underflow its own
sanity check and hit:

	BUG_ON(count > num_queued - dql->num_completed);

i.e. a guaranteed kernel panic, not just a warning, given a queue with
outstanding (queued but not yet completed) Tx traffic at the moment
the clone is freed.

Reproduced on real hardware by forcing i40e_setup_tx_descriptors() to
fail partway through building tx_rings[] while flooding real Tx
traffic on the affected queues:

    kernel BUG at lib/dynamic_queue_limits.c:99!
    RIP: 0010:dql_completed+0x285/0x2a0
    Call Trace:
     <IRQ>
     i40e_napi_poll+0x74b/0x1700 [i40e]
     __napi_poll+0x10a/0x200
     net_rx_action+0x2f7/0x380
     handle_softirqs+0xcc/0x270

Confirmed the same trigger no longer panics with this fix applied,
including repeated resize/traffic cycles.

Clear the clone ring netdev before freeing it in both error paths, so
i40e_clean_tx_ring()'s existing `if (!tx_ring->netdev) return;` guard
skips the netdev_tx_reset_queue() call for the clone. This mirrors the
same idiom i40e already uses for XDP Tx rings, which also have no
netdev_queue of their own (ring->netdev = NULL in i40e_vsi_setup_tx()).
Only the temporary, about-to-be-freed clone is touched; the live ring
in vsi->tx_rings[] keeps its netdev.

Fixes: 9f65e15b4f98 ("i40e: Move rings from pointer to array to array of pointers")
Cc: [email protected]
Cc: Simon Horman <[email protected]>
Signed-off-by: Aleksandr Loktionov <[email protected]>
---
v2: New patch, split out of the set_ringparam NULL-deref fix (1/2)
    after review pointed out this related but distinct, older,
    pre-existing hazard on the same error path. Trimmed the inline
    comments to one line each.

 drivers/net/ethernet/intel/i40e/i40e_ethtool.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
index 6d2b076..0e5b456 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
@@ -2143,6 +2143,8 @@ static int i40e_set_ringparam(struct net_device *netdev,
 					i--;
 					if (!i40e_active_tx_ring_index(vsi, i))
 						continue;
+					/* not live yet, skip its BQL reset on free */
+					tx_rings[i].netdev = NULL;
 					i40e_free_tx_resources(&tx_rings[i]);
 				}
 				kfree(tx_rings);
@@ -2248,8 +2250,11 @@ static int i40e_set_ringparam(struct net_device *netdev,
 	/* error cleanup if the Rx allocations failed after getting Tx */
 	if (tx_rings) {
 		for (i = 0; i < tx_alloc_queue_pairs; i++) {
-			if (i40e_active_tx_ring_index(vsi, i))
-				i40e_free_tx_resources(&tx_rings[i]);
+			if (i40e_active_tx_ring_index(vsi, i)) {
+				/* not live yet, skip its BQL reset on free */
+				tx_rings[i].netdev = NULL;
+				i40e_free_tx_resources(&tx_rings[i]);
+			}
 		}
 		kfree(tx_rings);
 		tx_rings = NULL;
-- 
2.52.0
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.