Re: [PATCH net 2/2] net/mlx5: HWS, fix modify header pool creation race

[Moshe Shemesh <moshe@nvidia.com>]

On 10/8/2026 11:42 PM, Alexandre Cassen wrote:
> 
> mlx5_cmd_hws_modify_header_alloc() creates a pool at the index after
> the last one if no pool matches the pattern. TC and CT allocate modify
> headers concurrently, meaning two callers can take the same index and
> the second call fails with -EBUSY.
> 
> Its error path also destroys the pool it just created although another
> caller may already use it. Keep such a pool since cleanup releases it.
> 
> Tested for regressions on ConnectX-7 with firmware 28.48.1000.
> 
> Fixes: b36315ca69cb ("net/mlx5: fs, add HWS modify header API function")
> Signed-off-by: Alexandre Cassen <acassen@corp.free.fr>
> ---
>   .../mellanox/mlx5/core/steering/hws/fs_hws.c  | 21 +++++++++----------
>   .../mellanox/mlx5/core/steering/hws/fs_hws.h  |  1 +
>   2 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/fs_hws.c b/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/fs_hws.c
> index 66edb42268cb..b387016bb5aa 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/fs_hws.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/fs_hws.c
> @@ -63,6 +63,7 @@ static int mlx5_fs_init_hws_actions_pool(struct mlx5_core_dev *dev,
>          xa_init(&hws_pool->el2tol3tnl_pools);
>          xa_init(&hws_pool->el2tol2tnl_pools);
>          xa_init(&hws_pool->mh_pools);
> +       mutex_init(&hws_pool->mh_pools_lock);
>          xa_init(&hws_pool->table_dests);
>          xa_init(&hws_pool->vport_dests);
>          xa_init(&hws_pool->vport_vhca_dests);
> @@ -111,6 +112,7 @@ static void mlx5_fs_cleanup_hws_actions_pool(struct mlx5_fs_hws_context *fs_ctx)
>          xa_for_each(&hws_pool->mh_pools, i, pool)
>                  mlx5_fs_destroy_mh_pool(pool, &hws_pool->mh_pools, i);
>          xa_destroy(&hws_pool->mh_pools);
> +       mutex_destroy(&hws_pool->mh_pools_lock);
>          xa_for_each(&hws_pool->el2tol2tnl_pools, i, pool)
>                  mlx5_fs_destroy_pr_pool(pool, &hws_pool->el2tol2tnl_pools, i);
>          xa_destroy(&hws_pool->el2tol2tnl_pools);
> @@ -1474,6 +1476,7 @@ static int mlx5_cmd_hws_modify_header_alloc(struct mlx5_flow_root_namespace *ns,
>          pattern.sz = MLX5_UN_SZ_BYTES(set_add_copy_action_in_auto) * num_actions;
>          pattern.data = modify_actions;
> 
> +       mutex_lock(&hws_pool->mh_pools_lock);

The lock covers more than it has to. The pattern lookup is a linear
scan over all pools with a word by word compare per pool, and it runs
on every rule insertion that needs a modify header allocation. Those 
scans run in parallel today. Taking the mutex around the whole lookup 
serializes them on a single namespace wide lock, while only the 
create-if-missing step needs serializing.

Please keep the lookup out of the lock and re-check under it only on a
miss, roughly:
	pool = mlx5_fs_find_mh_pool(hws_pool, &pattern, &cnt);
	if (!pool) {
		mutex_lock(&hws_pool->mh_pools_lock);
		pool = mlx5_fs_find_mh_pool(hws_pool, &pattern, &cnt);
		if (!pool)
			pool = mlx5_fs_create_mh_pool(ns->dev, &pattern,
						      &hws_pool->mh_pools,
						      cnt);
		mutex_unlock(&hws_pool->mh_pools_lock);
		if (IS_ERR(pool))
			return PTR_ERR(pool);
	}

Thanks, Moshe.

>          known_pattern = false;
>          xa_for_each(&hws_pool->mh_pools, i, pool) {
>                  if (mlx5_fs_hws_mh_pool_match(pool, &pattern)) {
> @@ -1483,17 +1486,16 @@ static int mlx5_cmd_hws_modify_header_alloc(struct mlx5_flow_root_namespace *ns,
>                  cnt++;
>          }
> 
> -       if (!known_pattern) {
> +       if (!known_pattern)
>                  pool = mlx5_fs_create_mh_pool(ns->dev, &pattern,
>                                                &hws_pool->mh_pools, cnt);
> -               if (IS_ERR(pool))
> -                       return PTR_ERR(pool);
> -       }
> +       mutex_unlock(&hws_pool->mh_pools_lock);
> +       if (IS_ERR(pool))
> +               return PTR_ERR(pool);
> +
>          mh_data = mlx5_fs_hws_mh_pool_acquire_mh(pool);
> -       if (IS_ERR(mh_data)) {
> -               err = PTR_ERR(mh_data);
> -               goto destroy_pool;
> -       }
> +       if (IS_ERR(mh_data))
> +               return PTR_ERR(mh_data);
>          hws_action = mh_data->bulk->hws_action;
>          mh_data->data = kmemdup(pattern.data, pattern.sz, GFP_KERNEL);
>          if (!mh_data->data) {
> @@ -1509,9 +1511,6 @@ static int mlx5_cmd_hws_modify_header_alloc(struct mlx5_flow_root_namespace *ns,
> 
>   release_mh:
>          mlx5_fs_hws_mh_pool_release_mh(pool, mh_data);
> -destroy_pool:
> -       if (!known_pattern)
> -               mlx5_fs_destroy_mh_pool(pool, &hws_pool->mh_pools, cnt);
>          return err;
>   }
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/fs_hws.h b/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/fs_hws.h
> index 20cdacd8f12e..ee37e2e6d68a 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/fs_hws.h
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/fs_hws.h
> @@ -19,6 +19,7 @@ struct mlx5_fs_hws_actions_pool {
>          struct xarray el2tol3tnl_pools;
>          struct xarray el2tol2tnl_pools;
>          struct xarray mh_pools;
> +       struct mutex mh_pools_lock; /* serializes pool lookup and creation */
>          struct xarray table_dests;
>          struct xarray vport_vhca_dests;
>          struct xarray vport_dests;
> --
> 2.43.0
>