[PATCH iwl-net v1] i40e: fix set_ringparam error path freeing live Tx rings

[PATCH iwl-net v1] i40e: fix set_ringparam error path freeing live Tx rings

[Aleksandr Loktionov]

The free_tx: error label in i40e_set_ringparam() is supposed to release
the temporary tx_rings[] array built before i40e_down() is called.
Instead it passes vsi->tx_rings[i] to i40e_free_tx_resources() - a
pointer to the live, NAPI-active ring - rather than &tx_rings[i].

i40e_free_tx_resources() unconditionally NULLs out ring->desc and
ring->tx_bi.  Because i40e_down() has not run at this point, NAPI is
still scheduled and the next i40e_clean_tx_irq() call hits a NULL
descriptor pointer:

    BUG: unable to handle page fault for address: 0000000000002000
    RIP: i40e_napi_poll (i40e_txrx.c:942 i40e_txrx.c:2769)
    RAX: 0000000000000000  RBX: ffff8d0a53ea9800

The trigger is ethtool -G with a TX descriptor count change (so
tx_rings[] is allocated) followed by i40e_alloc_rx_buffers() returning
failure, e.g. under memory pressure.

Pass &tx_rings[i] instead so the temporary rings' DMA descriptors and
software buffer arrays are freed, leaving the live VSI rings intact.

Fixes: 74608d17fe29 ("i40e: add support for XDP_TX action")
Cc: stable@vger.kernel.org
Signed-off-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_ethtool.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
index 3da9ec4..6d2b076 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
@@ -2249,7 +2249,7 @@ static int i40e_set_ringparam(struct net_device *netdev,
 	if (tx_rings) {
 		for (i = 0; i < tx_alloc_queue_pairs; i++) {
 			if (i40e_active_tx_ring_index(vsi, i))
-				i40e_free_tx_resources(vsi->tx_rings[i]);
+				i40e_free_tx_resources(&tx_rings[i]);
 		}
 		kfree(tx_rings);
 		tx_rings = NULL;
-- 
2.52.0

Re: [PATCH iwl-net v1] i40e: fix set_ringparam error path freeing live Tx rings

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
--- Patch [1]: [PATCH iwl-net v1] i40e: fix set_ringparam error path freeing live Tx rings ---
commit 7042e7b9f58a75bdb26437aed842a7b6780df035
Author: Aleksandr Loktionov <aleksandr.loktionov@intel.com>

i40e: fix set_ringparam error path freeing live Tx rings

This commit fixes an issue in the i40e_set_ringparam() error path where it
mistakenly frees live, NAPI-active TX rings instead of the temporary rings
allocated for a configuration change. It does this by passing the address of
the temporary rings instead of the live ring pointer.

> diff --git a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> index 3da9ec49cc745..6d2b076049f7e 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_ethtool.c
> @@ -2249,7 +2249,7 @@ static int i40e_set_ringparam(struct net_device *netdev,
>  	if (tx_rings) {
>  		for (i = 0; i < tx_alloc_queue_pairs; i++) {
>  			if (i40e_active_tx_ring_index(vsi, i))
> -				i40e_free_tx_resources(vsi->tx_rings[i]);
> +				i40e_free_tx_resources(&tx_rings[i]);

[Severity: High]
This is a pre-existing issue, but does calling i40e_free_tx_resources() on
the cloned &tx_rings[i] inadvertently reset the Byte Queue Limits (BQL)
state on the live TX queues?

Because the temporary rings are shallow copies of the live rings, they
inherit the netdev and queue_index fields. When i40e_free_tx_resources()
cleans up the temporary rings, it calls i40e_clean_tx_ring():

drivers/net/ethernet/intel/i40e/i40e_txrx.c:i40e_clean_tx_ring() {
    ...
    netdev_tx_reset_queue(txring_txq(tx_ring));
    ...
}

Since i40e_down() has not yet been called on this error path, the live
queue is still actively transmitting. Locklessly resetting BQL here
zeroes num_queued. When the hardware subsequently completes the
already-queued packets, won't dql_completed() underflow and trigger
a BUG_ON()?

The same issue also seems to be present in the earlier error path if
i40e_setup_tx_descriptors() fails.

>  		}
>  		kfree(tx_rings);
>  		tx_rings = NULL;