Re: [PATCH net V2 1/4] net/mlx5: SD, serialize SD LAG init/cleanup against LAG mode changes
From: Shay Drori
Date: Thu Sep 10 2026 - 11:49:08 EST
On 09/09/2026 15:15, netdev-bot+sashiko@xxxxxxxxxx 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