[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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] net/mlx5: race condition in mlx5_eswitch_safe_aux_devs_remove() teardown
--

commit 2e432cf53969d688514fb1e87aa9e496706a3090
Author: Shay Drory <shayd@nvidia.com>

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

This commit addresses a lockdep cycle involving mode_lock, reps_lock, and
rtnl_mutex. It resolves the cycle by setting the eswitch_operation_in_progress
flag, dropping the mode_lock before removing auxiliary devices, and retaking
it afterward to keep mode_lock and reps_lock un-nested.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> index eb74b62601682..f934cb2b26b08 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c

[ ... ]

> @@ -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);

[Severity: High]
Does this safely prevent concurrent devlink commands from executing?

This teardown path drops mode_lock and relies on eswitch_operation_in_progress
to prevent concurrent mode changes.

However, devlink callbacks like mlx5_devlink_switchdev_active_mode_change()
do not seem to check this flag via mlx5_esw_try_lock(). Instead, they
unconditionally acquire the lock:

mlx5_devlink_switchdev_active_mode_change() {
    down_write(&esw->mode_lock);
    esw->offloads_inactive = !esw->offloads_inactive;
    esw->eswitch_operation_in_progress = true;
    up_write(&esw->mode_lock);
    
    ...
    
    down_write(&esw->mode_lock);
    esw->eswitch_operation_in_progress = false;
    up_write(&esw->mode_lock);
}

If a concurrent devlink command runs after mode_lock is dropped here, could
it acquire the lock, unconditionally overwrite the flag, execute, and then
clear the flag to false upon exit?

If this happens, it would prematurely clear the flag while
mlx5_core_reps_aux_devs_remove() is still actively tearing down the auxiliary
devices, potentially exposing the un-nested lock region to other concurrent
mode transitions or TC offloads.

>  		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);
>  	}
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015093419.1150837-1-tariqt@nvidia.com?part=1