> -----Original Message-----
> From: Simon Horman <[email protected]>
> Sent: Wednesday, April 15, 2026 2:49 PM
> To: Loktionov, Aleksandr <[email protected]>
> Cc: Simon Horman <[email protected]>; [email protected];
> [email protected]; [email protected];
> [email protected]; Nguyen, Anthony L
> <[email protected]>; [email protected]
> Subject: Re: [PATCH iwl-net 1/5] iavf: fix null pointer dereference in
> iavf_detect_recover_hung
>
> 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;
>
Agree, thank you.
> > + if (tx_ring->desc) {
> > /* If packet counter has not changed the queue is
> > * likely stalled, so force an interrupt for this
> > * queue.
>
> [ ... ]
