[PATCH net] net/mlx5: Fix lock dependency cycle on uplink unbind in switchdev mode

[PATCH net] net/mlx5: Fix lock dependency cycle on uplink unbind in switchdev mode

[Tariq Toukan]

From: Shay Drory <shayd@nvidia.com>

mlx5_eswitch_safe_aux_devs_remove() holds esw->mode_lock for read across
mlx5_core_reps_aux_devs_remove(), which unregisters the REPs auxiliary
devices and therefore takes esw->offloads.reps_lock. This nests
mode_lock outside reps_lock, the exact nesting
mlx5_devlink_eswitch_mode_set() goes out of its way to avoid: there the
eswitch_operation_in_progress flag is raised, mode_lock is dropped, and
only then is reps_lock taken.

With that nesting in place lockdep reports a cycle [1]. IPsec packet
offload takes mode_lock under ipsec->tx_esw->ft.mutex, through
mlx5_eswitch_block_encap() in tx_get(); the mode change path takes
rtnl_mutex under reps_lock, through the netns immutable set; and
unregistering a REP netdev takes ft.mutex, through the xfrm policy
flush, under the netdev instance lock which is itself nested under
rtnl_mutex:

  &ipsec->tx_esw->ft.mutex -> &esw->mode_lock -> &esw->offloads.reps_lock
    -> rtnl_mutex -> &dev_instance_lock_key -> &ipsec->tx_esw->ft.mutex

Reproduced by unbinding the uplink ETH driver with an IPsec packet
offload policy configured in switchdev mode:

  devlink dev eswitch set pci/0000:08:00.0 mode switchdev
  echo 2 > /sys/class/net/eth2/device/sriov_numvfs
  ip addr add 193.237.171.21/16 dev eth2
  ip link set eth2 up
  ip xfrm policy add src 193.237.171.21 dst 193.237.172.22 \
     offload packet dev eth2 dir out \
     tmpl src 193.237.171.21/16 dst 193.237.172.22/16 \
     proto esp reqid 0xdcd95f7b mode transport priority 21
  echo mlx5_core.eth.0 > /sys/bus/auxiliary/drivers/mlx5_core.eth/unbind

Keep mode_lock and reps_lock unnested here as well: take mode_lock for
write, raise eswitch_operation_in_progress, drop mode_lock, remove the
REPs auxiliary devices, then retake mode_lock and clear the flag. The
flag keeps excluding concurrent mode transitions and mode users while
mode_lock is not held.

Acquiring mode_lock for write can't go through mlx5_esw_try_lock(),
which also refuses the lock while esw->user_count is non zero: that
count is held for the lifetime of every offloaded TC flow, and those
flows are only flushed by the REP netdev unregister this function is
about to perform.
Use a local trylock helper that attempts to acquire mode_lock and only
bails if the operation flag is set. Bail out early when the device has
no eswitch, which mlx5_esw_hold() used to cover.

[1]
 WARNING: possible circular locking dependency detected
 7.2.0-rc2+ #2 Tainted: G        W
 ------------------------------------------------------
 run.sh/7007 is trying to acquire lock:
 (&ipsec->tx_esw->ft.mutex){+.+.}-{4:4}, at: tx_ft_put_policy+0x81/0x130 [mlx5_core]

 but task is already holding lock:
 (&dev_instance_lock_key#9){+.+.}-{4:4}, at: unregister_netdevice_many_notify+0x3eb/0x2140

 which lock already depends on the new lock.
 [...]
 Chain exists of:
   &ipsec->tx_esw->ft.mutex --> rtnl_mutex --> &dev_instance_lock_key#9

  Possible unsafe locking scenario:

        CPU0                    CPU1
        ----                    ----
   lock(&dev_instance_lock_key#9);
                                lock(rtnl_mutex);
                                lock(&dev_instance_lock_key#9);
   lock(&ipsec->tx_esw->ft.mutex);

  *** DEADLOCK ***

 8 locks held by run.sh/7007:
  [...]
  #3: (&esw->mode_lock){++++}-{4:4}, at: mlx5_esw_hold+0xcc/0x150 [mlx5_core]
  #4: (&dev->mutex){....}-{4:4}, at: device_release_driver_internal+0xa5/0x560
  #5: (&esw->offloads.reps_lock){+.+.}-{4:4}, at: mlx5_eswitch_unregister_vport_reps+0x1f/0x40 [mlx5_core]
  #6: (rtnl_mutex){+.+.}-{4:4}, at: rtnl_net_dev_lock+0xde/0x260
  #7: (&dev_instance_lock_key#9){+.+.}-{4:4}, at: unregister_netdevice_many_notify+0x3eb/0x2140

 Call Trace:
  <TASK>
  tx_ft_put_policy+0x81/0x130 [mlx5_core]
  mlx5e_xfrm_del_policy+0x36/0x90 [mlx5_core]
  xfrm_policy_kill+0xc3/0x410
  xfrm_dev_policy_flush+0x258/0x360
  xfrm_dev_event+0x2a3/0x3b0
  notifier_call_chain+0x97/0x2b0
  netif_close_many+0x367/0x710
  netif_close_many_and_unlock+0x1d/0x240
  unregister_netdevice_many_notify+0x567/0x2140
  unregister_netdevice_queue+0x29a/0x350
  unregister_netdev+0x18/0x60
  mlx5e_vport_rep_unload+0x20b/0x2b0 [mlx5_core]
  mlx5_eswitch_unregister_vport_reps_blocked+0xb0/0x230 [mlx5_core]
  mlx5_eswitch_unregister_vport_reps+0x2a/0x40 [mlx5_core]
  device_release_driver_internal+0x3b2/0x560
  bus_remove_device+0x2a9/0x510
  device_del+0x3b9/0x990
  mlx5_core_reps_aux_devs_remove+0x135/0x200 [mlx5_core]
  mlx5_eswitch_safe_aux_devs_remove+0xa5/0xd0 [mlx5_core]
  mlx5e_remove+0x10d/0x370 [mlx5_core]
  device_release_driver_internal+0x3b2/0x560
  unbind_store+0xce/0xf0
  kernfs_fop_write_iter+0x3c6/0x5d0
  vfs_write+0x4d9/0xf70
  ksys_write+0xec/0x1c0
  do_syscall_64+0xaf/0x5c0
  entry_SYSCALL_64_after_hwframe+0x4b/0x53
  </TASK>

Fixes: 2ae8c7edea87 ("net/mlx5: Fix Unbinding uplink-netdev in switchdev mode")
Signed-off-by: Shay Drory <shayd@nvidia.com>
Reviewed-by: Mark Bloch <mbloch@nvidia.com>
Reviewed-by: Akiva Goldberger <agoldberger@nvidia.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
 .../mellanox/mlx5/core/eswitch_offloads.c     | 37 +++++++++++++++++--
 1 file changed, 34 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
index eb74b6260168..f934cb2b26b0 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
@@ -4438,15 +4438,36 @@ static bool mlx5_devlink_switchdev_active_mode_change(struct mlx5_eswitch *esw,
 #define MLX5_ESW_HOLD_TIMEOUT_MS 7000
 #define MLX5_ESW_HOLD_RETRY_DELAY_MS 500
 
+/* Unlike mlx5_esw_try_lock(), don't refuse the lock over eswitch users.
+ * Teardown must run even with TC flows still offloaded, as those are only
+ * flushed once the REPs are removed.
+ */
+static bool esw_try_lock_ignore_users(struct mlx5_eswitch *esw)
+{
+	if (!down_write_trylock(&esw->mode_lock))
+		return false;
+
+	if (esw->eswitch_operation_in_progress) {
+		up_write(&esw->mode_lock);
+		return false;
+	}
+
+	return true;
+}
+
 void mlx5_eswitch_safe_aux_devs_remove(struct mlx5_core_dev *dev)
 {
+	struct mlx5_eswitch *esw = dev->priv.eswitch;
 	unsigned long timeout;
 	bool hold_esw = true;
 
+	if (!mlx5_esw_allowed(esw))
+		return;
+
 	/* Wait for any concurrent eswitch mode transition to complete. */
-	if (!mlx5_esw_hold(dev)) {
+	if (!esw_try_lock_ignore_users(esw)) {
 		timeout = jiffies + msecs_to_jiffies(MLX5_ESW_HOLD_TIMEOUT_MS);
-		while (!mlx5_esw_hold(dev)) {
+		while (!esw_try_lock_ignore_users(esw)) {
 			if (!time_before(jiffies, timeout)) {
 				hold_esw = false;
 				break;
@@ -4455,9 +4476,19 @@ void mlx5_eswitch_safe_aux_devs_remove(struct mlx5_core_dev *dev)
 		}
 	}
 	if (hold_esw) {
+		/* Keep mode_lock and reps_lock unnested. The operation flag
+		 * excludes mode users while mode_lock is dropped before the
+		 * REPs removal takes reps_lock.
+		 */
+		esw->eswitch_operation_in_progress = true;
+		mlx5_esw_unlock(esw);
+
 		if (mlx5_eswitch_mode(dev) == MLX5_ESWITCH_OFFLOADS)
 			mlx5_core_reps_aux_devs_remove(dev);
-		mlx5_esw_release(dev);
+
+		down_write(&esw->mode_lock);
+		esw->eswitch_operation_in_progress = false;
+		mlx5_esw_unlock(esw);
 	}
 }
 

base-commit: 513444cc62e231a11c9b15ea88de0890da9b2b4f
-- 
2.44.0