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

[sashiko-bot@kernel.org]

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