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

[netdev-bot+sashiko@kernel.org]

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