[PATCH net V2 0/4] net/mlx5: SD LAG and devcom stability fixes

[PATCH net V2 0/4] net/mlx5: SD LAG and devcom stability fixes

[Tariq Toukan]

Hi,

This series by Shay fixes four bugs in the Socket Direct LAG and devcom
subsystems, all related to initialization/teardown ordering and
concurrent access to the LAG device.

Regards,
Tariq

Internal sashiko comment:
> @@ -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

V2:
- Fix git am apply error, and move pre-replies to cover letter.

V1:
https://lore.kernel.org/all/20260902163716.3656279-1-tariqt@nvidia.com/

Shay Drory (4):
  net/mlx5: SD, serialize SD LAG init/cleanup against LAG mode changes
  net/mlx5: devcom, Base component size on linked devices
  net/mlx5: SD, unload reps on shared FDB create error path
  net/mlx5: LAG, reload IB reps of LAG master before the rest

 .../net/ethernet/mellanox/mlx5/core/eswitch.c |  2 +-
 .../net/ethernet/mellanox/mlx5/core/lag/lag.c | 74 ++++++++++++-------
 .../mellanox/mlx5/core/lag/shared_fdb.c       |  1 +
 .../ethernet/mellanox/mlx5/core/lib/devcom.c  |  5 +-
 .../net/ethernet/mellanox/mlx5/core/lib/sd.c  | 13 ++++
 5 files changed, 65 insertions(+), 30 deletions(-)


base-commit: 2b4707a149a55e8fa75c9ef32b359d60f470a566
-- 
2.44.0

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

[Tariq Toukan]

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(-)

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

[PATCH net V2 2/4] net/mlx5: devcom, Base component size on linked devices

[Tariq Toukan]

From: Shay Drory <shayd@nvidia.com>

mlx5_devcom_comp_get_size() returns the component's kref count. That
kref is bumped in mlx5_devcom_register_component() under comp_list_lock,
before the comp_dev is linked onto comp_dev_list_head under comp->sem.
The event broadcast (mlx5_devcom_locked_send_event()) walks that list.

Hence, a caller can read the expected size, but send_event won't be sent
to all peers. In the SD group registration path, this lets a member
broadcast its role-election event over an incomplete list, electing a
primary that never completes the group, is never marked ready, and
leaves the group with a stale primary.

Track the number of linked comp_devs in a dedicated counter, maintained
under comp->sem together with the list add/remove, and return it from
mlx5_devcom_comp_get_size().

Fixes: 9bb1ac80738a ("net/mlx5: devcom, Add component size getter")
Signed-off-by: Shay Drory <shayd@nvidia.com>
Reviewed-by: Akiva Goldberger <agoldberger@nvidia.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/lib/devcom.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/devcom.c b/drivers/net/ethernet/mellanox/mlx5/core/lib/devcom.c
index 64f92427602d..75855481522b 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/lib/devcom.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/devcom.c
@@ -37,6 +37,7 @@ struct mlx5_devcom_comp {
 	struct mlx5_devcom_key key;
 	mlx5_devcom_event_handler_t handler;
 	struct kref ref;
+	int nr_devs;
 	bool ready;
 	struct rw_semaphore sem;
 	struct lock_class_key lock_key;
@@ -170,6 +171,7 @@ devcom_alloc_comp_dev(struct mlx5_devcom_dev *devc,
 
 	down_write(&comp->sem);
 	list_add_tail(&devcom->list, &comp->comp_dev_list_head);
+	WRITE_ONCE(comp->nr_devs, comp->nr_devs + 1);
 	up_write(&comp->sem);
 
 	return devcom;
@@ -182,6 +184,7 @@ devcom_free_comp_dev(struct mlx5_devcom_comp_dev *devcom)
 
 	down_write(&comp->sem);
 	list_del(&devcom->list);
+	WRITE_ONCE(comp->nr_devs, comp->nr_devs - 1);
 	up_write(&comp->sem);
 
 	kref_put(&devcom->devc->ref, mlx5_devcom_dev_release);
@@ -284,7 +287,7 @@ int mlx5_devcom_comp_get_size(struct mlx5_devcom_comp_dev *devcom)
 {
 	struct mlx5_devcom_comp *comp = devcom->comp;
 
-	return kref_read(&comp->ref);
+	return READ_ONCE(comp->nr_devs);
 }
 
 int mlx5_devcom_locked_send_event(struct mlx5_devcom_comp_dev *devcom,
-- 
2.44.0

[PATCH net V2 3/4] net/mlx5: SD, unload reps on shared FDB create error path

[Tariq Toukan]

From: Shay Drory <shayd@nvidia.com>

The teardown path unloads the representors; align the create error
path to do the same.

Fixes: 68c2dd59a6c7 ("net/mlx5: E-Switch, Tie rep load/unload to SD LAG state")
Signed-off-by: Shay Drory <shayd@nvidia.com>
Reviewed-by: Akiva Goldberger <agoldberger@nvidia.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/lag/shared_fdb.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/shared_fdb.c b/drivers/net/ethernet/mellanox/mlx5/core/lag/shared_fdb.c
index 6b4ad3c53f2f..424040918fa3 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/lag/shared_fdb.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/shared_fdb.c
@@ -270,6 +270,7 @@ int mlx5_lag_shared_fdb_create(struct mlx5_lag *ldev,
 			pf->sd_fdb_active = false;
 		}
 		mlx5_lag_destroy_single_fdb_filter(ldev, group_id);
+		mlx5_lag_unload_reps_from_locked(ldev, filter);
 	}
 err_add_devices:
 	mlx5_lag_add_devices_filter(ldev, filter);
-- 
2.44.0

[PATCH net V2 4/4] net/mlx5: LAG, reload IB reps of LAG master before the rest

[Tariq Toukan]

From: Shay Drory <shayd@nvidia.com>

In a shared-FDB LAG the master device creates the bond IB device; the
other LAG members do not create their own, they populate a port inside
the master's IB device. mlx5_lag_reload_ib_reps_unlocked() reloaded the
members' IB reps in iteration order, with no guarantee the master is
reloaded first. When a non-master member is reloaded before the master,
it tries to populate its port in an IB device that has not been
recreated yet.

Hence, reload the master's IB reps first, then every other member.

Fixes: 2b204cdb1206 ("net/mlx5: LAG, use xa_alloc to manage LAG device indices")
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/lag/lag.c | 44 ++++++++++++++-----
 1 file changed, 32 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
index aee5ce471eba..4984c812268c 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
@@ -1263,25 +1263,45 @@ void mlx5_lag_remove_devices(struct mlx5_lag *ldev)
 	mlx5_lag_remove_devices_filter(ldev, MLX5_LAG_FILTER_PORTS);
 }
 
+static int mlx5_lag_reload_ib_reps_idx(struct mlx5_lag *ldev, int idx,
+				       u32 flags)
+{
+	struct lag_func *pf = mlx5_lag_pf(ldev, idx);
+	struct mlx5_eswitch *esw;
+	int ret;
+
+	if (pf->dev->priv.flags & flags)
+		return 0;
+
+	esw = pf->dev->priv.eswitch;
+	mlx5_esw_reps_block(esw);
+	ret = mlx5_eswitch_reload_ib_reps(esw);
+	mlx5_esw_reps_unblock(esw);
+
+	return ret;
+}
+
 static int mlx5_lag_reload_ib_reps_unlocked(struct mlx5_lag *ldev, u32 flags,
 					    u32 filter, bool cont_on_fail)
 {
-	struct lag_func *pf;
+	int master_idx = mlx5_lag_get_dev_index_by_seq_filter(ldev, MLX5_LAG_P1,
+							     filter);
 	int ret;
 	int i;
 
+	if (master_idx < 0)
+		return -EINVAL;
+
+	ret = mlx5_lag_reload_ib_reps_idx(ldev, master_idx, flags);
+	if (ret && !cont_on_fail)
+		return ret;
+
 	mlx5_lag_for_each(i, 0, ldev, filter) {
-		pf = mlx5_lag_pf(ldev, i);
-		if (!(pf->dev->priv.flags & flags)) {
-			struct mlx5_eswitch *esw;
-
-			esw = pf->dev->priv.eswitch;
-			mlx5_esw_reps_block(esw);
-			ret = mlx5_eswitch_reload_ib_reps(esw);
-			mlx5_esw_reps_unblock(esw);
-			if (ret && !cont_on_fail)
-				return ret;
-		}
+		if (i == master_idx)
+			continue;
+		ret = mlx5_lag_reload_ib_reps_idx(ldev, i, flags);
+		if (ret && !cont_on_fail)
+			return ret;
 	}
 
 	return 0;
-- 
2.44.0

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 0 (1) · Low: 0

- [High] New unbounded `msleep(100)` wait loops in `sd_lag_init()` and
  `sd_lag_cleanup()` (drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c)…

Pre-existing issues:
- [Medium] `sd_lag_cleanup()`
  (drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c:386-412) dismantles…

--

net/mlx5: SD, serialize SD LAG init/cleanup against LAG mode changes

This makes sd_lag_init() and sd_lag_cleanup() wait for
ldev->mode_changes_in_progress to reach zero before touching ldev, folds
the SD shared-FDB teardown in mlx5_lag_disable_change() into the main
locked section, and moves mlx5_sd_eswitch_mode_set() in
mlx5_eswitch_disable() after mlx5_lag_enable_change(). A couple of
questions below about the new wait loop and about the group_id handling in
the relocated teardown loop.

[ ... ]

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
> index 2285c889c215e..aee5ce471eba7 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;
> +			}
> +		}
> +	}

[Severity: Medium]

This is a pre-existing issue and not introduced by this patch, but since
the loop is being relocated here: pf->group_id is passed to
mlx5_lag_shared_fdb_destroy() without checking that it is non-zero, and
mlx5_lag_shared_fdb_destroy() in lag/shared_fdb.c treats zero as
"everything":

	u32 filter = group_id ? group_id : MLX5_LAG_FILTER_ALL;
	...
	if (filter == MLX5_LAG_FILTER_PORTS || filter == MLX5_LAG_FILTER_ALL) {
		err = mlx5_deactivate_lag(ldev);

while lag.h documents group_id 0 as "not SD":

	u32 group_id;        /* SD group ID, 0 = not SD */
	bool sd_fdb_active;  /* set on all SD group members */

sd_lag_cleanup() zeroes group_id but never looks at sd_fdb_active, and
mlx5_lag_shared_fdb_create() sets sd_fdb_active on every member of the
group, so the primary can be left with sd_fdb_active == true and
group_id == 0. sd_lag_init()'s error path produces the same combination.

Do the normal teardown paths hide this because
mlx5_unload_one_devl_locked() calls mlx5_eswitch_disable() before
mlx5_detach_device(), so sd_fdb_active is already false by the time
sd_lag_cleanup() runs?

What happens on a manual auxiliary driver unbind, where mlx5e_remove() ->
mlx5_sd_cleanup() -> sd_lag_cleanup() runs with the eswitch still in
switchdev and the SD shared FDB armed? From there it looks like the
per-group single-FDB association, the peer-miss rules and the secondary
FDB-root alias are left installed, sd_cmd_unset_secondary() goes on to
silence the secondary TX root and destroy the alias flow table object, and
a later mlx5_lag_disable_change() reaching the loop above with
group_id == 0 would take the MLX5_LAG_FILTER_ALL / mlx5_deactivate_lag()
path rather than the SD group path.

Would it make sense for sd_lag_cleanup() to call
mlx5_lag_shared_fdb_destroy(ldev, group_id) (or at least warn when
sd_fdb_active is set) before erasing the group's lag_func entries?

> +
>  	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 4cdc50cd6f038..99cf455a61e1a 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;
> +	}
> +

[Severity: High]

Can this loop sleep while holding the locks that the mode-change thread
needs in order to make progress?

sd_lag_init() is called from mlx5_sd_init() with sd->devcom held, and
mlx5_sd_init() itself runs from adrv->probe/resume, i.e. inside the
hca_devcom_comp write semaphore taken by mlx5_attach_device() /
mlx5_register_device(). The same is true of the copy of this loop in
sd_lag_cleanup(), reached via mlx5_detach_device() -> mlx5e_suspend() /
mlx5e_remove() -> mlx5_sd_cleanup(). So the msleep() poll runs with two
devcom write semaphores held.

On the other side, mlx5_lag_disable_change() raises
mode_changes_in_progress and then drops every lock, and code running
inside that window re-takes hca_devcom_comp:

mlx5_devlink_eswitch_mode_set()
  mlx5_lag_disable_change(esw->dev)   /* mode_changes_in_progress++, locks dropped */
  esw_offloads_start()
    esw_mode_change()
      mlx5_devcom_comp_lock(esw->dev->priv.hca_devcom_comp)  /* blocks */
  mlx5_lag_enable_change(esw->dev)    /* never reached, counter stays > 0 */

and mlx5_devcom_comp_lock() is a write semaphore shared by all devices
matching the same sw system image GUID:

drivers/net/ethernet/mellanox/mlx5/core/lib/devcom.c:
void mlx5_devcom_comp_lock(struct mlx5_devcom_comp_dev *devcom)
{
	if (!devcom)
		return;
	down_write(&devcom->comp->sem);
}

The two threads can be on different devlink instances (mode change on one
PF, aux probe/remove on another PF of the same ldev), so the devlink lock
does not serialize them. msleep() is uninterruptible and the loop has no
timeout or bail-out, and since the wait is a poll on a plain int it is
also invisible to lockdep.

The commit message says the new loop is "mirroring mlx5_lag_remove_mdev()"
- mlx5_lag_remove_mdev() polls the same counter but holds no devcom
semaphore. Is the mirror still valid for callers that do hold sd->devcom
and hca_devcom_comp?

>  	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);
>

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260906071332.3759199-1-tariqt%40nvidia.com

Re: [PATCH net V2 3/4] net/mlx5: SD, unload reps on shared FDB create error path

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 0 · Low: 1

- [Low] Commit message of "net/mlx5: SD, unload reps on shared FDB create
  error path" describes the change only as symmetry ("The teardown path…

--

net/mlx5: SD, unload reps on shared FDB create error path

The SD branch of mlx5_lag_shared_fdb_create() now unloads the
representors on the error path after destroying the single FDB, matching
what mlx5_lag_shared_fdb_destroy() does.  The patch carries a Fixes: tag
for 68c2dd59a6c7 ("net/mlx5: E-Switch, Tie rep load/unload to SD LAG
state").

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/shared_fdb.c b/drivers/net/ethernet/mellanox/mlx5/core/lag/shared_fdb.c
> index 6b4ad3c53f2f3..424040918fa37 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/lag/shared_fdb.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/shared_fdb.c
> @@ -270,6 +270,7 @@ int mlx5_lag_shared_fdb_create(struct mlx5_lag *ldev,
>  			pf->sd_fdb_active = false;
>  		}
>  		mlx5_lag_destroy_single_fdb_filter(ldev, group_id);
> +		mlx5_lag_unload_reps_from_locked(ldev, filter);

[Severity: Low]
This isn't a bug in the code, but could the changelog say what state was
actually left behind before this call was added?

The commit message only describes symmetry:

    The teardown path unloads the representors; align the create error
    path to do the same.

In the SD branch, mlx5_lag_shared_fdb_create() sets sd_fdb_active for
every group member before the rescan and the reps reload:

	mlx5_lag_for_each(i, 0, ldev, filter) {
		pf = mlx5_lag_pf(ldev, i);
		pf->sd_fdb_active = true;
	}
	...
	mlx5_lag_rescan_dev_locked(ldev, dev0, true);
	err = mlx5_lag_reload_ib_reps_from_locked(ldev, 0, filter, false);
	if (err) {
		mlx5_core_err(dev0, "Failed to enable lag\n");
		goto err_rescan_drivers;
	}

Non-uplink reps are only skipped while SD LAG is inactive, per
mlx5_esw_offloads_rep_load() in eswitch_offloads.c:

	if (vport_num != MLX5_VPORT_UPLINK &&
	    mlx5_get_sd(esw->dev) && !mlx5_lag_is_active(esw->dev))
		return 0;

so reps do get loaded in that window.  If
mlx5_lag_reload_ib_reps_from_locked() then fails, err_rescan_drivers
clears pf->sd_fdb_active and calls
mlx5_lag_destroy_single_fdb_filter(), which left the VF/SF reps loaded
while SD LAG is no longer active, i.e. exactly the state that
68c2dd59a6c7 was written to avoid.

Since this is Fixes-tagged and therefore stable material, would it help
to spell that resulting state out in the changelog?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260906071332.3759199-1-tariqt%40nvidia.com

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

[Paolo Abeni]

On 9/6/26 9:13 AM, Tariq Toukan wrote:
>  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;

My reading of the clashiko report is that the above introduces a
deadlock.

While I agree that the fairness issue raised by sashiko.dev
could be deferred to net-next, I think we must avoid introducing
the mentioned deadlock.

/P

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

[Shay Drori]

On 09/09/2026 15:15, netdev-bot+sashiko@kernel.org wrote:
> External email: Use caution opening links or attachments
> 
> 
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
> 
> Critical: 0 · High: 1 · Medium: 0 (1) · Low: 0
> 
> - [High] New unbounded `msleep(100)` wait loops in `sd_lag_init()` and
>    `sd_lag_cleanup()` (drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c)…
> 
> Pre-existing issues:
> - [Medium] `sd_lag_cleanup()`
>    (drivers/net/ethernet/mellanox/mlx5/core/lib/sd.c:386-412) dismantles…
> 
> --
> 
> net/mlx5: SD, serialize SD LAG init/cleanup against LAG mode changes
> 
> This makes sd_lag_init() and sd_lag_cleanup() wait for
> ldev->mode_changes_in_progress to reach zero before touching ldev, folds
> the SD shared-FDB teardown in mlx5_lag_disable_change() into the main
> locked section, and moves mlx5_sd_eswitch_mode_set() in
> mlx5_eswitch_disable() after mlx5_lag_enable_change(). A couple of
> questions below about the new wait loop and about the group_id handling in
> the relocated teardown loop.
> 
> [ ... ]
> 
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c b/drivers/net/ethernet/mellanox/mlx5/core/lag/lag.c
>> index 2285c889c215e..aee5ce471eba7 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;
>> +                     }
>> +             }
>> +     }
> 
> [Severity: Medium]
> 
> This is a pre-existing issue and not introduced by this patch, but since
> the loop is being relocated here: pf->group_id is passed to
> mlx5_lag_shared_fdb_destroy() without checking that it is non-zero, and
> mlx5_lag_shared_fdb_destroy() in lag/shared_fdb.c treats zero as
> "everything":
> 
>          u32 filter = group_id ? group_id : MLX5_LAG_FILTER_ALL;
>          ...
>          if (filter == MLX5_LAG_FILTER_PORTS || filter == MLX5_LAG_FILTER_ALL) {
>                  err = mlx5_deactivate_lag(ldev);
> 
> while lag.h documents group_id 0 as "not SD":
> 
>          u32 group_id;        /* SD group ID, 0 = not SD */
>          bool sd_fdb_active;  /* set on all SD group members */
> 
> sd_lag_cleanup() zeroes group_id but never looks at sd_fdb_active, and
> mlx5_lag_shared_fdb_create() sets sd_fdb_active on every member of the
> group, so the primary can be left with sd_fdb_active == true and
> group_id == 0. sd_lag_init()'s error path produces the same combination.
> 
> Do the normal teardown paths hide this because
> mlx5_unload_one_devl_locked() calls mlx5_eswitch_disable() before
> mlx5_detach_device(), so sd_fdb_active is already false by the time
> sd_lag_cleanup() runs?
> 
> What happens on a manual auxiliary driver unbind, where mlx5e_remove() ->
> mlx5_sd_cleanup() -> sd_lag_cleanup() runs with the eswitch still in
> switchdev and the SD shared FDB armed? From there it looks like the
> per-group single-FDB association, the peer-miss rules and the secondary
> FDB-root alias are left installed, sd_cmd_unset_secondary() goes on to
> silence the secondary TX root and destroy the alias flow table object, and
> a later mlx5_lag_disable_change() reaching the loop above with
> group_id == 0 would take the MLX5_LAG_FILTER_ALL / mlx5_deactivate_lag()
> path rather than the SD group path.
> 
> Would it make sense for sd_lag_cleanup() to call
> mlx5_lag_shared_fdb_destroy(ldev, group_id) (or at least warn when
> sd_fdb_active is set) before erasing the group's lag_func entries?
> 
>> +
>>        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 4cdc50cd6f038..99cf455a61e1a 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;
>> +     }
>> +
> 
> [Severity: High]
> 
> Can this loop sleep while holding the locks that the mode-change thread
> needs in order to make progress?
> 
> sd_lag_init() is called from mlx5_sd_init() with sd->devcom held, and
> mlx5_sd_init() itself runs from adrv->probe/resume, i.e. inside the
> hca_devcom_comp write semaphore taken by mlx5_attach_device() /
> mlx5_register_device(). The same is true of the copy of this loop in
> sd_lag_cleanup(), reached via mlx5_detach_device() -> mlx5e_suspend() /
> mlx5e_remove() -> mlx5_sd_cleanup(). So the msleep() poll runs with two
> devcom write semaphores held.
> 
> On the other side, mlx5_lag_disable_change() raises
> mode_changes_in_progress and then drops every lock, and code running
> inside that window re-takes hca_devcom_comp:
> 
> mlx5_devlink_eswitch_mode_set()
>    mlx5_lag_disable_change(esw->dev)   /* mode_changes_in_progress++, locks dropped */
>    esw_offloads_start()
>      esw_mode_change()
>        mlx5_devcom_comp_lock(esw->dev->priv.hca_devcom_comp)  /* blocks */
>    mlx5_lag_enable_change(esw->dev)    /* never reached, counter stays > 0 */
> 
> and mlx5_devcom_comp_lock() is a write semaphore shared by all devices
> matching the same sw system image GUID:
> 
> drivers/net/ethernet/mellanox/mlx5/core/lib/devcom.c:
> void mlx5_devcom_comp_lock(struct mlx5_devcom_comp_dev *devcom)
> {
>          if (!devcom)
>                  return;
>          down_write(&devcom->comp->sem);
> }
> 
> The two threads can be on different devlink instances (mode change on one
> PF, aux probe/remove on another PF of the same ldev), so the devlink lock
> does not serialize them. msleep() is uninterruptible and the loop has no
> timeout or bail-out, and since the wait is a poll on a plain int it is
> also invisible to lockdep.
> 
> The commit message says the new loop is "mirroring mlx5_lag_remove_mdev()"
> - mlx5_lag_remove_mdev() polls the same counter but holds no devcom
> semaphore. Is the mirror still valid for callers that do hold sd->devcom
> and hca_devcom_comp?


This is a real finding and the fix needs a new approach.
I am dropping this patch.

> 
>>        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);
>>
> 
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260906071332.3759199-1-tariqt%40nvidia.com

Re: [PATCH net V2 3/4] net/mlx5: SD, unload reps on shared FDB create error path

[Shay Drori]

On 09/09/2026 15:15, netdev-bot+sashiko@kernel.org wrote:
> External email: Use caution opening links or attachments
> 
> 
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
> 
> Critical: 0 · High: 0 · Medium: 0 · Low: 1
> 
> - [Low] Commit message of "net/mlx5: SD, unload reps on shared FDB create
>    error path" describes the change only as symmetry ("The teardown path…
> 
> --
> 
> net/mlx5: SD, unload reps on shared FDB create error path
> 
> The SD branch of mlx5_lag_shared_fdb_create() now unloads the
> representors on the error path after destroying the single FDB, matching
> what mlx5_lag_shared_fdb_destroy() does.  The patch carries a Fixes: tag
> for 68c2dd59a6c7 ("net/mlx5: E-Switch, Tie rep load/unload to SD LAG
> state").
> 
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lag/shared_fdb.c b/drivers/net/ethernet/mellanox/mlx5/core/lag/shared_fdb.c
>> index 6b4ad3c53f2f3..424040918fa37 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/lag/shared_fdb.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lag/shared_fdb.c
>> @@ -270,6 +270,7 @@ int mlx5_lag_shared_fdb_create(struct mlx5_lag *ldev,
>>                        pf->sd_fdb_active = false;
>>                }
>>                mlx5_lag_destroy_single_fdb_filter(ldev, group_id);
>> +             mlx5_lag_unload_reps_from_locked(ldev, filter);
> 
> [Severity: Low]
> This isn't a bug in the code, but could the changelog say what state was
> actually left behind before this call was added?
> 
> The commit message only describes symmetry:
> 
>      The teardown path unloads the representors; align the create error
>      path to do the same.
> 
> In the SD branch, mlx5_lag_shared_fdb_create() sets sd_fdb_active for
> every group member before the rescan and the reps reload:
> 
>          mlx5_lag_for_each(i, 0, ldev, filter) {
>                  pf = mlx5_lag_pf(ldev, i);
>                  pf->sd_fdb_active = true;
>          }
>          ...
>          mlx5_lag_rescan_dev_locked(ldev, dev0, true);
>          err = mlx5_lag_reload_ib_reps_from_locked(ldev, 0, filter, false);
>          if (err) {
>                  mlx5_core_err(dev0, "Failed to enable lag\n");
>                  goto err_rescan_drivers;
>          }
> 
> Non-uplink reps are only skipped while SD LAG is inactive, per
> mlx5_esw_offloads_rep_load() in eswitch_offloads.c:
> 
>          if (vport_num != MLX5_VPORT_UPLINK &&
>              mlx5_get_sd(esw->dev) && !mlx5_lag_is_active(esw->dev))
>                  return 0;
> 
> so reps do get loaded in that window.  If
> mlx5_lag_reload_ib_reps_from_locked() then fails, err_rescan_drivers
> clears pf->sd_fdb_active and calls
> mlx5_lag_destroy_single_fdb_filter(), which left the VF/SF reps loaded
> while SD LAG is no longer active, i.e. exactly the state that
> 68c2dd59a6c7 was written to avoid.
> 
> Since this is Fixes-tagged and therefore stable material, would it help
> to spell that resulting state out in the changelog?


good finding, will spell it out in next version


> 
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260906071332.3759199-1-tariqt%40nvidia.com