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

[Shay Drori <shayd@nvidia.com>]

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