From: Shay Drory 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 Reviewed-by: Akiva Goldberger Signed-off-by: Tariq Toukan --- .../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