> -----Original Message-----
> From: Simon Horman <[email protected]>
> Sent: Friday, September 18, 2026 5:13 PM
> To: Loktionov, Aleksandr <[email protected]>
> Cc: [email protected]; Nguyen, Anthony L
> <[email protected]>; [email protected]; Sylwester
> Dziedziuch <[email protected]>
> Subject: Re: [PATCH iwl-net v2 3/5] iavf: prevent VSI corruption when
> ring params changed during reset
>
> On Tue, Sep 15, 2026 at 02:55:49PM +0200, Aleksandr Loktionov wrote:
> > From: Sylwester Dziedziuch <[email protected]>
> >
> > Changing ring parameters via ethtool triggers a VF reset and queue
> > reconfiguration. If ethtool is called again before the first reset
> > completes, the second reset races with uninitialised queue state and
> > can corrupt the VSI resource tree on the PF side.
> >
> > Return -EAGAIN from iavf_set_ringparam() when the adapter is already
> > resetting or its queues are disabled.
> >
> > Fixes: fbb7ddfef253 ("i40evf: core ethtool functionality")
> > Cc: [email protected]
> > Signed-off-by: Sylwester Dziedziuch
> <[email protected]>
> > Signed-off-by: Aleksandr Loktionov <[email protected]>
> > ---
> > drivers/net/ethernet/intel/iavf/iavf_ethtool.c | 5 +++++
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
> > b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
> > index 1cd1f3f..3909131 100644
> > --- a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
> > +++ b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
> > @@ -495,6 +495,11 @@ static int iavf_set_ringparam(struct net_device
> *netdev,
> > if ((ring->rx_mini_pending) || (ring->rx_jumbo_pending))
> > return -EINVAL;
> >
> > + if (adapter->state == __IAVF_RESETTING ||
> > + (adapter->state == __IAVF_RUNNING &&
> > + adapter->flags & IAVF_FLAG_QUEUES_DISABLED))
> > + return -EAGAIN;
> > +
> > if (ring->tx_pending > IAVF_MAX_TXD ||
> > ring->tx_pending < IAVF_MIN_TXD ||
> > ring->rx_pending > IAVF_MAX_RXD ||
>
> I believe I reported this for v1 as well, but AI-generated review
> reports a TOCTOU issue here. And I believe a similar, pre-existing,
> problem also exists in iavf_set_channels.
>
> Text from
> https://sashiko.dev/#/patchset/20260915125551.3976068-1-
> aleksandr.loktionov%40intel.com
> follows:
>
> Could this state check introduce a time-of-check to time-of-use
> (TOCTOU) race against the background reset task?
>
> The background iavf_reset_task() relies on netdev_lock() to serialize,
> but ethtool callbacks for drivers like iavf (which do not set
> request_ops_lock =
> true) are executed under rtnl_lock() without holding netdev_lock().
>
> Because they do not share a common lock, the background task can
> acquire netdev_lock(), change the state to __IAVF_RESETTING, and tear
> down queues immediately after iavf_set_ringparam() evaluates this
> condition to false.
>
> Will this lock mismatch also cause a lockdep splat later in the same
> function?
>
> iavf_set_ringparam() proceeds to call iavf_reset_step() directly:
> drivers/net/ethernet/intel/iavf/iavf_ethtool.c:iavf_set_ringparam() {
> ...
> if (netif_running(netdev)) {
> adapter->flags |= IAVF_FLAG_RESET_NEEDED;
> iavf_reset_step(adapter);
> }
> ...
> }
>
> Looking at iavf_reset_step(), it asserts that netdev_lock() is held:
>
> drivers/net/ethernet/intel/iavf/iavf_main.c:iavf_reset_step() { ...
> netdev_assert_locked(netdev);
> ...
> }
>
> Executing iavf_reset_step() from the ethtool callback without holding
> netdev_lock() would trigger this lockdep assertion, and allow it to
> concurrently step on any executing reset task.
Good day, Simon
You're right, and it's still unresolved here. iavf's ethtool_ops
doesn't set request_ops_lock, so iavf_set_ringparam() runs under
rtnl_lock() only, with no netdev_lock() held - while iavf_reset_task()
and iavf_watchdog_task() take netdev_lock() from adapter->wq. The state
check and the iavf_reset_step() call right after it aren't serialized
against the background task at all, so the window this patch is meant
to close is still open.
Fixing this properly means closing that lock gap for every ethtool op
that can call iavf_reset_step() - at least set_ringparam and
set_channels, maybe more - not adding a state check to one call site.
That's bigger than this patch, so
I'm dropping it rather than resend the same check. I'll come back to it as a
proper locking fix.
Sorry and, thanks for catching this twice now.
Alex
