> -----Original Message-----
> From: Intel-wired-lan <[email protected]> On Behalf Of Simon
> Horman
> 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: [Intel-wired-lan] [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;
>
> > + if (tx_ring->desc) {
> > /* If packet counter has not changed the queue is
> > * likely stalled, so force an interrupt for this
> > * queue.
>
> [ ... ]
Tested-by: Rafal Romanowski <[email protected]>
