[PATCH net v2] net: fix self-deadlock when an upper disables a feature of its lower

[PATCH net v2] net: fix self-deadlock when an upper disables a feature of its lower

[Eva Kurchatova]

When an upper device ends up without a NETIF_F_UPPER_DISABLES feature
(LRO) that its lower device has, netdev_sync_lower_features() disables
it on the lower device, taking the lower's instance lock since commit
af5f54b0ef9e ("net: Lock lower level devices when updating features").

Upper devices recompute their features in response to NETDEV_FEAT_CHANGE
of the lower device, and for ops locked devices that notification is
sent with the lower's instance lock held, e.g. from ethtool. The task
then blocks on the lock it already holds:

  dev_ethtool()                        takes lower's instance lock
    netdev_features_change(lower)
      <upper>_device_event()
        netdev_update_features(upper)
          netdev_sync_lower_features()
            netdev_lock_ops(lower)     deadlock

Triggered by enabling mlx5 LRO on a host with VM macvtaps on top of it:

  INFO: task ethtool:5963 blocked for more than 122 seconds.
  Call Trace:
   __mutex_lock.constprop.0
   __netdev_update_features
   netdev_update_features
   macvlan_device_event
   notifier_call_chain
   netdev_features_change
   ethnl_set_features
  INFO: task ethtool:5963 is blocked on a mutex likely owned by task ethtool:5963.

Reproduced with these upper device configurations, and netdevsim as the
ops-locked lower device.

 - macsec with LRO in hw_features, but masked by macsec_fix_features().
 - macvtap without LRO enabled via TUNSETOFFLOAD.
 - macvlan, bond or team enslaved to a VRF or batman-adv device.

There is no way to tell whether the current task owns the lower instance
lock. Try to take it, and otherwise leave the job to netdev_work, which
disables the features the uppers lack under the lower's instance lock.
In the deadlock case above, the lock holder is the context that sent
NETDEV_FEAT_CHANGE, so steal the pending work at the end of
netdev_features_change() and run it inline, like netif_rx_mode_sync()
does for rx_mode. The lower device's features are then consistent with
its uppers when the operation returns, as before. The work only runs
asynchronously if an unrelated task held the lock.

Tested in QEMU with lockdep enabled. The stacks above hang without this
patch, and with it the features end up disabled on the lower device
without lockdep reports.

Stacks with bridge, bond, team, ipvlan and VLAN on top of netdevsim
behave as before: these call dev_disable_lro() on their ports.

Fixes: af5f54b0ef9e ("net: Lock lower level devices when updating features")
Reported-by: syzbot+c6c86a1c56b5131f2d97@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/all/6a6be82b.77639fcc.3d4fd0.0014.GAE@google.com/
Signed-off-by: Eva Kurchatova <eva.kurchatova@virtuozzo.com>
---

Changes in v2:
- Send no NETDEV_FEAT_CHANGE when disabling the feature fails, the upper
  deferred the job again and the recursion overflowed the stack.
- Run the deferred job on a detached device too, instead of dropping it:
  before the patch the caller waited for the lock and then disabled the
  feature regardless of the presence.
- Correct the kernel-doc of netdev_features_change() that it may disable
  features, and explain its locking.

Link to v1: https://lore.kernel.org/netdev/20261023000951.237641-1-eva.kurchatova@virtuozzo.com/

 net/core/dev.c         | 47 ++++++++++++++++++++++++++++++++++++++++--
 net/core/dev.h         |  3 +++
 net/core/netdev_work.c |  4 ++++
 3 files changed, 52 insertions(+), 2 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index e76762e29360..853e34d5765e 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1605,11 +1605,20 @@ int dev_get_alias(const struct net_device *dev, char *name, size_t len)
  *	netdev_features_change - device changes features
  *	@dev: device to cause notification
  *
- *	Called to indicate a device has changed features.
+ *	Called to indicate a device has changed features. Also disables the
+ *	features which upper devices of @dev have off and could not disable
+ *	on @dev while its instance lock was held, so the caller must hold
+ *	RTNL and, for an ops locked @dev, its instance lock.
  */
 void netdev_features_change(struct net_device *dev)
 {
 	call_netdevice_notifiers(NETDEV_FEAT_CHANGE, dev);
+
+	/* Uppers reacting to the notification couldn't take our instance
+	 * lock to disable features they lack, do it now.
+	 */
+	if (__netdev_work_core_cancel(dev, NETDEV_WORK_SYNC_UPPERS))
+		netif_sync_upper_disables(dev);
 }
 EXPORT_SYMBOL(netdev_features_change);
 
@@ -10968,6 +10977,31 @@ static netdev_features_t netdev_sync_upper_features(struct net_device *lower,
 	return features;
 }
 
+/* Disable the features of @dev which any of its upper devices has disabled,
+ * deferred from netdev_sync_lower_features().
+ */
+void netif_sync_upper_disables(struct net_device *dev)
+{
+	netdev_features_t disable = 0;
+	struct net_device *upper;
+	struct list_head *iter;
+
+	netdev_for_each_upper_dev_rcu(dev, upper, iter)
+		disable |= ~upper->features;
+	disable &= NETIF_F_UPPER_DISABLES & dev->features;
+	if (!disable)
+		return;
+
+	dev->wanted_features &= ~disable;
+	__netdev_update_features(dev);
+
+	/* Notifying a failure would make the uppers defer this again */
+	if (unlikely(dev->features & disable))
+		netdev_WARN(dev, "failed to disable %pNF!\n", &disable);
+	else
+		netdev_features_change(dev);
+}
+
 static void netdev_sync_lower_features(struct net_device *upper,
 	struct net_device *lower, netdev_features_t features)
 {
@@ -10980,7 +11014,16 @@ static void netdev_sync_lower_features(struct net_device *upper,
 		if (!(features & feature) && (lower->features & feature)) {
 			netdev_dbg(upper, "Disabling feature %pNF on lower dev %s.\n",
 				   &feature, lower->name);
-			netdev_lock_ops(lower);
+			/* The caller may hold the lower's instance lock already,
+			 * e.g. when the upper reacts to NETDEV_FEAT_CHANGE of
+			 * the lower. Don't wait for it, let netdev_work disable
+			 * the feature instead.
+			 */
+			if (netdev_need_ops_lock(lower) && !netdev_trylock(lower)) {
+				__netdev_work_core_sched(lower,
+							 NETDEV_WORK_SYNC_UPPERS);
+				continue;
+			}
 			lower->wanted_features &= ~feature;
 			__netdev_update_features(lower);
 
diff --git a/net/core/dev.h b/net/core/dev.h
index 04fb0e9a571e..c3d9d66bfb5b 100644
--- a/net/core/dev.h
+++ b/net/core/dev.h
@@ -174,8 +174,11 @@ void netif_rx_mode_cancel_retry(struct net_device *dev);
 /* Events for the async netdev work, tracked in netdev->work_core_pending. */
 enum netdev_work_core {
 	NETDEV_WORK_RX_MODE	= BIT(0),	/* run the rx_mode update */
+	NETDEV_WORK_SYNC_UPPERS	= BIT(1),	/* disable features uppers lack */
 };
 
+void netif_sync_upper_disables(struct net_device *dev);
+
 void __netdev_work_core_sched(struct net_device *dev, unsigned long event);
 unsigned long
 __netdev_work_core_cancel(struct net_device *dev, unsigned long mask);
diff --git a/net/core/netdev_work.c b/net/core/netdev_work.c
index e721a06d58df..097c9535845b 100644
--- a/net/core/netdev_work.c
+++ b/net/core/netdev_work.c
@@ -118,6 +118,10 @@ __netdev_work_core_cancel(struct net_device *dev, unsigned long mask)
 static void netdev_work_run(struct net_device *dev, unsigned long events,
 			    unsigned long core)
 {
+	/* Not gated on presence: the caller used to wait only for the lock */
+	if (core & NETDEV_WORK_SYNC_UPPERS)
+		netif_sync_upper_disables(dev);
+
 	if (!netif_device_present(dev))
 		return;
 
-- 
2.56.0

Re: [PATCH net v2] net: fix self-deadlock when an upper disables a feature of its lower

[Wang Zhan]

Hi,

On Tue, 27 Oct 2026 16:00:49 +0200, Eva Kurchatova wrote:
> There is no way to tell whether the current task owns the lower instance
> lock. Try to take it, and otherwise leave the job to netdev_work, which
> disables the features the uppers lack under the lower's instance lock.

We may not need to tell whether the current task owns the lower instance
lock. Instead, we can check whether the device is already in the current
notification chain. netdev_features_change() is always called with RTNL
held, so we can track it with a global feature-change stack, like this:

  /* RTNL serialized */
  static stack netdev_feature_change_stack;

  netdev_features_change(dev):
          push(netdev_feature_change_stack, dev)
          call_netdevice_notifiers(NETDEV_FEAT_CHANGE, dev)
          pop(netdev_feature_change_stack)

  netdev_sync_lower_features(upper, lower, ...):
          take_ops_lock = !in_stack(netdev_feature_change_stack, lower)

          if (take_ops_lock)
                  netdev_lock_ops(lower)

          ...

          if (take_ops_lock)
                  netdev_unlock_ops(lower)

> In the deadlock case above, the lock holder is the context that sent
> NETDEV_FEAT_CHANGE, so steal the pending work at the end of
> netdev_features_change() and run it inline, like netif_rx_mode_sync()
> does for rx_mode.

The "otherwise" path takes only the instance lock for ops-locked devices:

  netdev_work_proc()
    netdev_lock_ops_compat(dev)

but then reaches:

  netif_sync_upper_disables()
    __netdev_update_features()
      ASSERT_RTNL()

ndo_*_features callbacks also require RTNL, so the deferred path cannot
run feature synchronization.

Re: [PATCH net v2] net: fix self-deadlock when an upper disables a feature of its lower

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] When the trylock fails, the patch moves the 'disable features
  the uppers lack' job into netdev_work, but that worker never holds…

--

net: fix self-deadlock when an upper disables a feature of its lower

This patch fixes a self-deadlock in netdev_sync_lower_features() when the
caller already holds the lower device's instance lock. It now tries to take
the lock with a trylock, and on failure defers the work to netdev_work as
NETDEV_WORK_SYNC_UPPERS. netdev_features_change() steals that work and runs
it inline, and in other cases the worker runs it asynchronously.

Closes: https://lore.kernel.org/all/6a6be82b.77639fcc.3d4fd0.0014.GAE@google.com/

> diff --git a/net/core/netdev_work.c b/net/core/netdev_work.c
> index 1b11bcbeb196e..3859ca9087584 100644
> --- a/net/core/netdev_work.c
> +++ b/net/core/netdev_work.c
> @@ -117,6 +117,10 @@ __netdev_work_core_cancel(struct net_device *dev, unsigned long mask)
>  static void netdev_work_run(struct net_device *dev, unsigned long events,
>  			    unsigned long core)
>  {
> +	/* Not gated on presence: the caller used to wait only for the lock */
> +	if (core & NETDEV_WORK_SYNC_UPPERS)
> +		netif_sync_upper_disables(dev);
> +

[Severity: High]
Does the worker run netif_sync_upper_disables() without RTNL?

netdev_sync_lower_features() only queues NETDEV_WORK_SYNC_UPPERS when
netdev_need_ops_lock(lower) is true. For those devices, netdev_work_proc()
takes only the instance lock:

netdev_work_proc()
    netdev_lock_ops_compat(dev);
    ...
    netdev_work_run(dev, events, core);

include/net/netdev_lock.h:netdev_lock_ops_compat() {
	if (netdev_need_ops_lock(dev))
		netdev_lock(dev);
	else
		rtnl_lock();
}

With only dev->lock held, netif_sync_upper_disables() then does three
things.

First, it walks dev->adj_list.upper with netdev_for_each_upper_dev_rcu()
while holding neither rcu_read_lock() nor RTNL. That should trigger the
lockdep WARN_ON_ONCE() in netdev_upper_get_next_dev_rcu(). A concurrent
RTNL-only __netdev_upper_dev_unlink(), for example from macvlan_dellink(),
can also list_del_rcu() and kfree_rcu() the netdev_adjacent entry while this
preemptible worker is walking it. Could that lead to a use-after-free of
the adjacency entry or of the upper device?

Second, it calls __netdev_update_features(dev), which starts with
ASSERT_RTNL().

Third, it calls netdev_features_change(dev), which reaches
call_netdevice_notifiers_info(). That function has ASSERT_RTNL() and walks
the raw notifier chains, which RTNL protects. Handlers such as
macvlan_device_event() would then call netdev_update_features() on uppers
that are not ops-locked, without RTNL. This also contradicts the new
kernel-doc for netdev_features_change(), which says the caller must hold
RTNL.

The commit message says the work "only runs asynchronously if an unrelated
task held the lock". Several contexts hold the lower's instance lock
without RTNL. One is netdev_work_proc() itself, running rx_mode or
ndo_work for the lower. Another is the netdev-genl ops that take only the
instance lock.

For example, the worker could be running NETDEV_WORK_RX_MODE for an
ops-locked lower while an RTNL holder runs TUNSETOFFLOAD on a macvtap on
top of it. The trylock in netdev_sync_lower_features() then fails and
SYNC_UPPERS is queued on the lower:

netdev_sync_lower_features()
    if (netdev_need_ops_lock(lower) && !netdev_trylock(lower)) {
        __netdev_work_core_sched(lower, NETDEV_WORK_SYNC_UPPERS);
        continue;
    }

The later netdev_features_change(upper) only cancels work pending on the
upper, so nothing steals the lower's job. The worker then runs it while
holding only lower->lock. Before this patch, this caller blocked on the
lock and kept RTNL for the whole operation.

Taking rtnl_lock() inside the worker after the instance lock would invert
the rtnl -> instance lock order. Would this event need to be handled
differently? One option is to take rtnl_lock() before the instance lock
for SYNC_UPPERS. Another is to reschedule the work when rtnl_trylock()
fails.

>  	if (!netif_device_present(dev))
>  		return;
>

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261027140121.1192299-1-eva.kurchatova%40virtuozzo.com