[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

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 1 · 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] mlx5_eswitch_safe_aux_devs_remove() now sets
  esw->eswitch_operation_in_progress = true, drops mode_lock, removes…
- [Medium] The patch replaces the read trylock (mlx5_esw_hold(),
  down_read_trylock) with a write trylock in esw_try_lock_ignore_users()…

--

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

This patch stops mlx5_eswitch_safe_aux_devs_remove() from holding
esw->mode_lock for read while it removes the REP aux devices. It now takes
mode_lock for write through a new esw_try_lock_ignore_users() helper, sets
eswitch_operation_in_progress, drops mode_lock, removes the REPs, and then
retakes mode_lock to clear the flag.

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

[Severity: Medium]
Can concurrent TC flow insertion now make this loop time out?

The old mlx5_esw_hold() used down_read_trylock(), and other readers never
block that. esw_try_lock_ignore_users() uses down_write_trylock(), which
fails whenever any reader holds mode_lock.

mlx5e_configure_flower() holds mode_lock for read during the whole flow
insert, FW commands included:

drivers/net/ethernet/mellanox/mlx5/core/en_tc.c:mlx5e_configure_flower() {
    ...
	if (!mlx5_esw_hold(priv->mdev))
		return -EBUSY;
    ...
}

TC offload on the REPs doesn't take rtnl, so several of these can run at
the same time. If flow inserts keep overlapping (for example under heavy
OVS offload), every 500 ms poll could find a reader. After
MLX5_ESW_HOLD_TIMEOUT_MS the loop sets hold_esw = false.

In that case the IB and ETH REP aux devices are never removed, and nothing
is logged. _mlx5e_remove() then goes on to tear down the uplink:

_mlx5e_remove()
    mlx5_eswitch_safe_aux_devs_remove(mdev);   <- REPs left bound
    mlx5_core_uplink_netdev_set(mdev, NULL);
    ...
    unregister_netdev() / mlx5e_destroy_netdev()

The uplink REP and the IB REP still reference that netdev. Could this bring
back the unregister_netdevice hang, or a later use of the freed netdev, that
commit 2ae8c7edea87 fixed?

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

[Severity: High]
Does this flag really keep other writers out while mode_lock is dropped?

The commit message says "The flag keeps excluding concurrent mode
transitions and mode users while mode_lock is not held". However, several
devlink paths take mode_lock with a plain down_write(). They set and later
clear the flag without checking it first:

mlx5_devlink_eswitch_encap_mode_set() {
	down_write(&esw->mode_lock);
	...
	esw->eswitch_operation_in_progress = true;
	up_write(&esw->mode_lock);

	esw_destroy_offloads_fdb_tables(esw);
	...
	err = esw_create_offloads_fdb_tables(esw);
	...
	down_write(&esw->mode_lock);
	esw->eswitch_operation_in_progress = false;
    ...
}

mlx5_devlink_eswitch_inline_mode_set() does the same around
mlx5_esw_vports_inline_set(). mlx5_devlink_switchdev_active_mode_change()
does it around mlx5_esw_fdb_inactive() and mlx5_esw_fdb_active().
mlx5_devlink_eswitch_mode_set() calls that function before it calls
mlx5_esw_try_lock().

Before this patch, the read lock held across REP removal blocked all of
these in down_write(). The other flag setters run under devl_lock. The
unbind path (unbind_store->mlx5e_remove->_mlx5e_remove) doesn't take
devl_lock, so this setter can overlap them.

One possible interleaving:

CPU0 (unbind)                        CPU1 (devlink encap-mode set)
flag = true, up_write()
mlx5_core_reps_aux_devs_remove()
                                     down_write() succeeds
                                     flag = true, up_write()
                                     esw_destroy_offloads_fdb_tables()
down_write()
flag = false, up_write()
                                     esw_create_offloads_fdb_tables()

Once CPU0 clears the flag, mlx5e_configure_flower()->mlx5_esw_hold() can
succeed while CPU1 is still rebuilding the FDB tables.

The reverse order is possible too. If CPU1 finishes first, it clears the
flag while CPU0 is still removing REPs. A later
mlx5_devlink_eswitch_mode_set() can then pass mlx5_esw_try_lock() (when
user_count is 0) and start a mode transition in the middle of REP removal.

CPU0 checked mlx5_eswitch_mode() without holding mode_lock. When it
finishes, it also clears the flag that mode_set just raised.

This looks like a flag being used as a lock: set on entry and cleared on
exit. Should the other writers check the flag before setting it, or should
the bool be replaced with a real lock or a counter?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261015093419.1150837-1-tariqt%40nvidia.com