Re: [PATCH iwl-net 1/5] iavf: fix null pointer dereference in iavf_detect_recover_hung

[Simon Horman <horms@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
iavf: fix null pointer dereference in iavf_detect_recover_hung

During a concurrent reset, q_vectors are freed and re-allocated while
the watchdog task may still be iterating rings in
iavf_detect_recover_hung(). Dereferencing a NULL q_vector inside
iavf_force_wb() results in a crash. Guard against this by skipping
rings whose q_vector is NULL.

> Fixes: 9c6c12595b73 ("i40e: Detection and recovery of TX queue hung logic moved to service_task from tx_timeout")

The Fixes tag points to commit 9c6c12595b73, but that commit only modified
the i40e driver and did not introduce the iavf_detect_recover_hung function.

The bug was actually introduced by commit 07d44190a389 ("i40e/i40evf: Detect
and recover hung queue scenario"), which added iavf_detect_recover_hung with
the code pattern 'if (tx_ring && tx_ring->desc)' that calls
iavf_force_wb(vsi, tx_ring->q_vector) without checking if q_vector is NULL.

Should the Fixes tag be:

    Fixes: 07d44190a389 ("i40e/i40evf: Detect and recover hung queue scenario")

> diff --git a/drivers/net/ethernet/intel/iavf/iavf_txrx.c b/drivers/net/ethernet/intel/iavf/iavf_txrx.c
> index 363c42bf3dcf..e7e7fc9a5c68 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_txrx.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_txrx.c

[ ... ]

> @@ -195,8 +194,11 @@ void iavf_detect_recover_hung(struct iavf_vsi *vsi)
>  		return;
>
>  	for (i = 0; i < vsi->back->num_active_queues; i++) {
> -		tx_ring = &vsi->back->tx_rings[i];
> -		if (tx_ring && tx_ring->desc) {
> +		struct iavf_ring *tx_ring = &vsi->back->tx_rings[i];
> +
> +		if (!tx_ring || !tx_ring->q_vector)
> +			continue;

The check `if (!tx_ring || ...)` tests whether tx_ring is NULL, but tx_ring
is assigned as `&vsi->back->tx_rings[i]`, which is the address of an array
element. This address is always non-NULL by C language rules, as noted in
the commit message itself: "the compiler can never observe since an
array-element address is always non-NULL."

This creates misleading logic. Other similar code in the driver correctly
omits such checks (for example, in iavf_virtchnl.c and elsewhere in
iavf_txrx.c).

Should this be simplified to just check the q_vector:

    if (!tx_ring->q_vector)
        continue;

> +		if (tx_ring->desc) {
>  			/* If packet counter has not changed the queue is
>  			 * likely stalled, so force an interrupt for this
>  			 * queue.

[ ... ]