> -----Original Message-----
> From: Simon Horman <[email protected]>
> Sent: Friday, September 18, 2026 5:11 PM
> To: Loktionov, Aleksandr <[email protected]>
> Cc: [email protected]; Nguyen, Anthony L
> <[email protected]>; [email protected]; Kiran Patil
> <[email protected]>
> Subject: Re: [PATCH iwl-net v2 1/5] iavf: fix null pointer dereference
> in iavf_detect_recover_hung
>
> On Tue, Sep 15, 2026 at 02:55:47PM +0200, Aleksandr Loktionov wrote:
> > From: Kiran Patil <[email protected]>
> >
> > iavf_watchdog_task() and iavf_reset_task() both run as work items on
> > the same ordered adapter->wq, so they can't race with each other.
> > However,
> > iavf_set_ringparam() (and other ethtool ops) call iavf_reset_step()
> > directly from process context under the netdev instance lock, without
> > going through that workqueue at all. iavf_reset_step() can free and
> > reallocate adapter->tx_rings and the q_vectors array via
> > iavf_reinit_interrupt_scheme() while adapter->state still reads
> > __IAVF_RUNNING, so the watchdog task can concurrently call
> > iavf_detect_recover_hung() and dereference a NULL q_vector inside
> > iavf_force_wb(), or index into a NULL tx_rings array, causing a crash.
>
> I am concerned that iavf_reset_step() is also called from
> iavf_set_channels(). And in that case the netdev instance lock is not
> held.
>
> >
> > Guard against this by:
> > - returning early if vsi->back->tx_rings itself is NULL, since
> > num_active_queues can still be nonzero while the array is being
> > reallocated;
> > - skipping rings whose q_vector is NULL;
> > - reading tx_ring->q_vector once with READ_ONCE() into a local
> variable
> > and reusing that same value for both the NULL check and the
> > iavf_force_wb() call, instead of re-reading the field right before
> > use, which would leave a window for the concurrent reset to swap it
> > from underneath us in between.
> >
> > Also move the tx_ring declaration into the loop body and drop the
> > redundant outer NULL initialisation, which the compiler can never
> > observe since an array-element address is always non-NULL.
> >
> > Fixes: 07d44190a389 ("i40e/i40evf: Detect and recover hung queue
> > scenario")
> > Cc: [email protected]
> > Signed-off-by: Kiran Patil <[email protected]>
> > Signed-off-by: Aleksandr Loktionov <[email protected]>
>
> ...
Confirmed - same gap as 3/5. The READ_ONCE()/NULL-check guards here only
narrow the window where iavf_watchdog_task() can observe a half-torn-
down tx_rings/q_vector array; they don't hold a reference or lock that
keeps those allocations alive, so iavf_free_q_vectors() and
iavf_free_queues() can still free out from under the watchdog between
the check and the use, from set_channels or any other path that calls
iavf_reset_step() without the instance lock.
Dropping this patch rather than resend guards that don't actually close
the race. Once there's a real fix for the locking gap across iavf's
ethtool ops, I'll revisit whether any of these NULL checks are still
worth keeping on top of it.
Thanks,
Alex
