[PATCH net 1/4] net/mlx5: SD, serialize SD LAG init/cleanup against LAG mode changes
From: Tariq Toukan
Date: Wed Sep 02 2026 - 12:49:00 EST
From: Shay Drory <shayd@xxxxxxxxxx>
A secondary SD is spliced into the primary's shared LAG device (ldev)
by sd_lag_init() and removed by sd_lag_cleanup(). The LAG mode-change
paths drop ldev->lock mid-operation while iterating ldev->pfs, and rely
on ldev->mode_changes_in_progress to keep the member set stable across
that window.
sd_lag_cleanup() did not honor ldev->mode_changes_in_progress. Which
means a concurrent SD teardown could xa_erase() and kfree() a pf out of
ldev->pfs during a mode change's dropped-lock window, leading to a
NULL/use-after-free dereference of the secondary pf.
Make sd_lag_init() and sd_lag_cleanup() wait until
mode_changes_in_progress drops to zero before touching ldev, mirroring
mlx5_lag_remove_mdev().
In addition, fold the SD shared-FDB teardown in
mlx5_lag_disable_change() into the main locked section. Otherwise the
two acquire sd_devcom and mode_changes_in_progress in opposite orders -
sd_lag_init/cleanup takes sd_devcom then waits on
mode_changes_in_progress, while disable_change increments
mode_changes_in_progress then takes sd_devcom - an ABBA deadlock.
The same ABBA deadlock is present in mlx5_eswitch_disable(), which takes
sd_devcom between mlx5_lag_disable_change() and
mlx5_lag_enable_change(). Move that call after mlx5_lag_enable_change(),
matching the ordering already used by mlx5_devlink_eswitch_mode_set().
Fixes: 3c103110835d ("net/mlx5: SD, introduce Socket Direct LAG")
Signed-off-by: Shay Drory <shayd@xxxxxxxxxx>
Reviewed-by: Akiva Goldberger <agoldberger@xxxxxxxxxx>
Signed-off-by: Tariq Toukan <tariqt@xxxxxxxxxx>
---
.../net/ethernet/mellanox/mlx5/core/eswitch.c | 2 +-
.../net/ethernet/mellanox/mlx5/core/lag/lag.c | 30 +++++++++----------
.../net/ethernet/mellanox/mlx5/core/lib/sd.c | 13 ++++++++
3 files changed, 28 insertions(+), 17 deletions(-)
Internal sashiko comment:
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c
b/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c
> index ee2fdefa1945..2b2b7f3916f7 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c
> @@ -342,7 +342,14 @@ static void sd_lag_init(struct mlx5_core_dev *dev)
> return;
> }
>
> +recheck:
> mutex_lock(&ldev->lock);
> + if (ldev->mode_changes_in_progress) {
> + mutex_unlock(&ldev->lock);
> + msleep(100);
> + goto recheck;
> +
}
> +
Does this open-coded retry loop reimplement a wait mechanism without
immediate
wakeups or fairness?
It looks like we are polling the mode_changes_in_progress flag using a
hard
coded msleep(100). Could this unnecessarily delay the initialization
path if
the condition clears much sooner than 100ms? Would it be better to use a
proper
synchronization primitive like a waitqueue here instead of an ad-hoc
flag loop?
[SD] This is the same check as in mlx5_lag_remove_mdev().
I agree we need to change it, but this is net-next material
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
index b6e2c153b4f7..50e158b6a684 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
@@ -2062,8 +2062,8 @@ void mlx5_eswitch_disable(struct mlx5_eswitch *esw)
mlx5_esw_reps_unblock(esw);
esw->mode = MLX5_ESWITCH_LEGACY;
- mlx5_sd_eswitch_mode_set(esw->dev, MLX5_ESWITCH_LEGACY);
mlx5_lag_enable_change(esw->dev);
+ mlx5_sd_eswitch_mode_set(esw->dev, MLX5_ESWITCH_LEGACY);
}
static int mlx5_esw_sf_max_pf_functions(struct mlx5_core_dev *dev,
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
index 2285c889c215..aee5ce471eba 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
@@ -2589,6 +2589,8 @@ void mlx5_lag_disable_change(struct mlx5_core_dev *dev)
mpesw = ldev->mode == MLX5_LAG_MODE_MPESW;
if (mpesw)
mlx5_mpesw_sd_devcoms_lock(ldev);
+ else if (sd_devcom)
+ mlx5_devcom_comp_lock(sd_devcom);
mutex_lock(&ldev->lock);
ldev->mode_changes_in_progress++;
@@ -2599,26 +2601,22 @@ void mlx5_lag_disable_change(struct mlx5_core_dev *dev)
mlx5_disable_lag(ldev);
}
+ if (sd_devcom) {
+ mlx5_lag_for_each(i, 0, ldev, MLX5_LAG_FILTER_ALL) {
+ pf = mlx5_lag_pf(ldev, i);
+ if (pf->dev == dev && pf->sd_fdb_active) {
+ mlx5_lag_shared_fdb_destroy(ldev, pf->group_id);
+ break;
+ }
+ }
+ }
+
mutex_unlock(&ldev->lock);
if (mpesw)
mlx5_mpesw_sd_devcoms_unlock(ldev);
+ else if (sd_devcom)
+ mlx5_devcom_comp_unlock(sd_devcom);
mlx5_devcom_comp_unlock(primary->priv.hca_devcom_comp);
-
- if (!sd_devcom)
- return;
-
- /* Teardown SD shared FDB for this device's group if active */
- mlx5_devcom_comp_lock(sd_devcom);
- mutex_lock(&ldev->lock);
- mlx5_lag_for_each(i, 0, ldev, MLX5_LAG_FILTER_ALL) {
- pf = mlx5_lag_pf(ldev, i);
- if (pf->dev == dev && pf->sd_fdb_active) {
- mlx5_lag_shared_fdb_destroy(ldev, pf->group_id);
- break;
- }
- }
- mutex_unlock(&ldev->lock);
- mlx5_devcom_comp_unlock(sd_devcom);
}
void mlx5_lag_enable_change(struct mlx5_core_dev *dev)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c b/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c
index 4cdc50cd6f03..99cf455a61e1 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c
@@ -345,7 +345,14 @@ static void sd_lag_init(struct mlx5_core_dev *dev)
return;
}
+recheck:
mutex_lock(&ldev->lock);
+ if (ldev->mode_changes_in_progress) {
+ mutex_unlock(&ldev->lock);
+ msleep(100);
+ goto recheck;
+ }
+
pf = mlx5_lag_pf_by_dev(ldev, primary);
if (!pf) {
sd_warn(primary, "%s: primary not registered in ldev, skipping\n",
@@ -388,7 +395,13 @@ static void sd_lag_cleanup(struct mlx5_core_dev *dev)
if (!ldev)
return;
+recheck:
mutex_lock(&ldev->lock);
+ if (ldev->mode_changes_in_progress) {
+ mutex_unlock(&ldev->lock);
+ msleep(100);
+ goto recheck;
+ }
mlx5_sd_for_each_secondary(i, primary, pos)
mlx5_ldev_remove_mdev(ldev, pos);
--
2.44.0