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

[netdev-bot+sashiko@kernel.org]

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