Message-ID: <27f5f292-dcdb-456e-9622-33efedd831b0@intel.com>
Date: Mon, 9 Feb 2026 15:59:40 -0800
From: Jacob Keller <jacob.e.keller@...el.com>
To: Petr Oros <poros@...hat.com>, <netdev@...r.kernel.org>, Anthony Nguyen
	<anthony.l.nguyen@...el.com>
CC: <ivecera@...hat.com>, <aleksandr.loktionov@...el.com>,
	<shaojijie@...wei.com>, Tony Nguyen <anthony.l.nguyen@...el.com>, "Przemek
 Kitszel" <przemyslaw.kitszel@...el.com>, Andrew Lunn <andrew+netdev@...n.ch>,
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, "Stanislav
 Fomichev" <sdf@...ichev.me>, <intel-wired-lan@...ts.osuosl.org>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net v2] iavf: fix deadlock in reset handling



On 2/7/2026 2:22 AM, Petr Oros wrote:
> Three driver callbacks schedule a reset and wait for its completion:
> ndo_change_mtu(), ethtool set_ringparam(), and ethtool set_channels().
> 
> Waiting for reset in ndo_change_mtu() and set_ringparam() was added by
> commit c2ed2403f12c ("iavf: Wait for reset in callbacks which trigger
> it") to fix a race condition where adding an interface to bonding
> immediately after MTU or ring parameter change failed because the
> interface was still in __RESETTING state. The same commit also added
> waiting in iavf_set_priv_flags(), which was later removed by commit
> 53844673d555 ("iavf: kill "legacy-rx" for good").
> 
> Waiting in set_channels() was introduced earlier by commit 4e5e6b5d9d13
> ("iavf: Fix return of set the new channel count") to ensure the PF has
> enough time to complete the VF reset when changing channel count, and to
> return correct error codes to userspace.
> 
> Commit ef490bbb2267 ("iavf: Add net_shaper_ops support") added
> net_shaper_ops to iavf, which required reset_task to use _locked NAPI
> variants (napi_enable_locked, napi_disable_locked) that need the netdev
> instance lock.
> 
> Later, commit 7e4d784f5810 ("net: hold netdev instance lock during
> rtnetlink operations") and commit 2bcf4772e45a ("net: ethtool: try to
> protect all callback with netdev instance lock") started holding the
> netdev instance lock during ndo and ethtool callbacks for drivers with
> net_shaper_ops.
> 
> Finally, commit 120f28a6f314 ("iavf: get rid of the crit lock")
> replaced the driver's crit_lock with netdev_lock in reset_task, making
> the deadlock manifest: the callback holds netdev_lock and waits for
> reset_task, but reset_task needs the same lock:
> 
>    Thread 1 (callback)               Thread 2 (reset_task)
>    -------------------               ---------------------
>    netdev_lock()                     [blocked on workqueue]
>    ndo_change_mtu() or ethtool op
>      iavf_schedule_reset()
>      iavf_wait_for_reset()           iavf_reset_task()
>        waiting...                      netdev_lock() <- DEADLOCK
> 

Only one note: this does not strictly "deadlock" because the 
wait_event() will eventually timeout, exit with an error, release the 
lock, and then the reset will complete. This is obviously incorrect 
behavior and likely to lead to errors as the configuration change may 
actually silently apply even though the user sees an error.

Still worth fixing, but it does not lead to a complete system failure 
deadlock due to the timeout.

> Fix this by extracting the reset logic from iavf_reset_task() into a new
> iavf_reset_step() function that expects netdev_lock to be already held.
> The three callbacks now call iavf_reset_step() directly instead of
> scheduling the work and waiting, performing the reset synchronously in
> the caller's context which already holds netdev_lock. This eliminates
> both the deadlock and the need for iavf_wait_for_reset(), which is
> removed.
> 
> The workqueue-based iavf_reset_task() becomes a thin wrapper that
> acquires netdev_lock and calls iavf_reset_step(), preserving its use
> for PF-initiated resets.
> 
> The callbacks may block for several seconds while iavf_reset_step()
> polls hardware registers, but this is acceptable since netdev_lock is a
> per-device mutex and only serializes operations on the same interface.
> 
> Fixes: 120f28a6f314 ("iavf: get rid of the crit lock")
> Reviewed-by: Jacob Keller <jacob.e.keller@...el.com>
> Signed-off-by: Petr Oros <poros@...hat.com>
> ---

Great work on the analysis and history of the issue, as well as coming 
up with a minimal fix!

I'm happy to see this go straight to net since its an important fix and 
comes from external to Intel, unless Tony has any objections.

I case it helps expedite things: I manually loaded this on my system and 
confirmed it resolves the issues for MTU changes, so:

Tested-by: Jacob Keller <jacob.e.keller@...el.com>

Powered by blists - more mailing lists