[PATCH net 1/4] net/mlx5: SD, serialize SD LAG init/cleanup against LAG mode changes

[Tariq Toukan <tariqt@nvidia.com>]

From: Shay Drory <shayd@nvidia.com>

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@nvidia.com>
Reviewed-by: Akiva Goldberger <agoldberger@nvidia.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
 .../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