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

[Eva Kurchatova <eva.kurchatova@virtuozzo.com>]

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>
---
 net/core/dev.c         | 39 ++++++++++++++++++++++++++++++++++++++-
 net/core/dev.h         |  3 +++
 net/core/netdev_work.c |  2 ++
 3 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index e76762e29360..d3945140766f 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1610,6 +1610,12 @@ int dev_get_alias(const struct net_device *dev, char *name, size_t len)
 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 +10974,28 @@ 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);
+
+	if (unlikely(dev->features & disable))
+		netdev_WARN(dev, "failed to disable %pNF!\n", &disable);
+}
+
 static void netdev_sync_lower_features(struct net_device *upper,
 	struct net_device *lower, netdev_features_t features)
 {
@@ -10980,7 +11008,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..5a608ba40389 100644
--- a/net/core/netdev_work.c
+++ b/net/core/netdev_work.c
@@ -123,6 +123,8 @@ static void netdev_work_run(struct net_device *dev, unsigned long events,
 
 	if (core & NETDEV_WORK_RX_MODE)
 		netif_rx_mode_run(dev);
+	if (core & NETDEV_WORK_SYNC_UPPERS)
+		netif_sync_upper_disables(dev);
 	if (events && dev->netdev_ops->ndo_work)
 		dev->netdev_ops->ndo_work(dev, events);
 }
-- 
2.56.0