[PATCH net 0/2] net/mlx5: HWS, fix pool creation races

[PATCH net 0/2] net/mlx5: HWS, fix pool creation races

[Alexandre Cassen]

This series fixes two pool creation races in fs_hws hit when two
callers create the first pool for a reformat size or a modify header
pattern at once:

- Reformat pools keep the pool inserted first.
- Modify header pools serialize the pattern lookup with the pool
  creation while keeping a new pool on error.

Alexandre Cassen (2):
  net/mlx5: HWS, fix reformat pool creation race
  net/mlx5: HWS, fix modify header pool creation race

 .../mellanox/mlx5/core/steering/hws/fs_hws.c  | 34 +++++++++----------
 .../mellanox/mlx5/core/steering/hws/fs_hws.h  |  1 +
 2 files changed, 18 insertions(+), 17 deletions(-)

-- 
2.43.0

[PATCH net 1/2] net/mlx5: HWS, fix reformat pool creation race

[Alexandre Cassen]

mlx5_fs_get_pr_encap_pool() inserts a new pool when the lookup misses.
Two callers allocating the first reformat of a size at once both
create one and the second fails with -EBUSY. TC serializes these
allocations under encap_tbl_lock while RDMA flow actions on the FDB
don't.

Sashiko AI review of an earlier series spotted this issue.

Tested for regressions on ConnectX-7 with firmware 28.48.1000.

Fixes: aecd9d1020e3 ("net/mlx5: fs, add HWS packet reformat API function")
Signed-off-by: Alexandre Cassen <acassen@corp.free.fr>
---
 .../mellanox/mlx5/core/steering/hws/fs_hws.c        | 13 +++++++------
 1 file changed, 7 insertions(+), 6 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 5a172c572a68..66edb42268cb 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
@@ -1228,7 +1228,7 @@ static struct mlx5_fs_pool *
 mlx5_fs_get_pr_encap_pool(struct mlx5_core_dev *dev, struct xarray *pr_pools,
 			  enum mlx5hws_action_type reformat_type, size_t size)
 {
-	struct mlx5_fs_pool *pr_pool;
+	struct mlx5_fs_pool *pr_pool, *old;
 	unsigned long index = size;
 	int err;
 
@@ -1242,13 +1242,14 @@ mlx5_fs_get_pr_encap_pool(struct mlx5_core_dev *dev, struct xarray *pr_pools,
 	err = mlx5_fs_hws_pr_pool_init(pr_pool, dev, size, reformat_type);
 	if (err)
 		goto free_pr_pool;
-	err = xa_insert(pr_pools, index, pr_pool, GFP_KERNEL);
-	if (err)
-		goto cleanup_pr_pool;
-	return pr_pool;
+	old = xa_cmpxchg(pr_pools, index, NULL, pr_pool, GFP_KERNEL);
+	if (!old)
+		return pr_pool;
 
-cleanup_pr_pool:
 	mlx5_fs_hws_pr_pool_cleanup(pr_pool);
+	kfree(pr_pool);
+	return xa_is_err(old) ? ERR_PTR(xa_err(old)) : old;
+
 free_pr_pool:
 	kfree(pr_pool);
 	return ERR_PTR(err);
-- 
2.43.0

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

[Alexandre Cassen]

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

Re: [PATCH net 0/2] net/mlx5: HWS, fix pool creation races

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

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

[Alexandre Cassen]

(replying to Sashiko... strange feeling)

Adding back original recipients. Sashiko review at:
https://sashiko.dev/#/patchset/20261008204230.3222198-1-acassen@corp.free.fr?part=2


On 10/9/26 22:43, sashiko-bot@kernel.org wrote:
> Does mlx5_fs_hws_mh_pool_match() incorrectly truncate 64-bit modify actions to
> 32 bits during comparison?
This code predates the patch. On little-endian the cast keeps the first
half of each action. That half holds the type and field. Dropping the
value in the second half is intended since each rule passes its values
as arguments. mlx5hws_pat_compare_pattern() does the same.

but there is a real bug. Copy and add_field actions keep their 
destination in the second half. The core compares them in full but the 
pool match does not. Two copies that differ only in destination then
share a pool and the second rule writes to the wrong field. RDMA flow
actions can trigger it from userspace.

I will send a separate fix for this.

-- Alexandre

Re: [PATCH net 1/2] net/mlx5: HWS, fix reformat pool creation race

[Moshe Shemesh]

On 10/8/2026 11:42 PM, Alexandre Cassen wrote:
> 
> mlx5_fs_get_pr_encap_pool() inserts a new pool when the lookup misses.
> Two callers allocating the first reformat of a size at once both
> create one and the second fails with -EBUSY. TC serializes these
> allocations under encap_tbl_lock while RDMA flow actions on the FDB
> don't.
> 
> Sashiko AI review of an earlier series spotted this issue.
> 
> Tested for regressions on ConnectX-7 with firmware 28.48.1000.
> 
> Fixes: aecd9d1020e3 ("net/mlx5: fs, add HWS packet reformat API function")
> Signed-off-by: Alexandre Cassen <acassen@corp.free.fr>

Reviewed-by: Moshe Shemesh <moshe@nvidia.com>

Thanks.
> ---
>   .../mellanox/mlx5/core/steering/hws/fs_hws.c        | 13 +++++++------
>   1 file changed, 7 insertions(+), 6 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 5a172c572a68..66edb42268cb 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
> @@ -1228,7 +1228,7 @@ static struct mlx5_fs_pool *
>   mlx5_fs_get_pr_encap_pool(struct mlx5_core_dev *dev, struct xarray *pr_pools,
>                            enum mlx5hws_action_type reformat_type, size_t size)
>   {
> -       struct mlx5_fs_pool *pr_pool;
> +       struct mlx5_fs_pool *pr_pool, *old;
>          unsigned long index = size;
>          int err;
> 
> @@ -1242,13 +1242,14 @@ mlx5_fs_get_pr_encap_pool(struct mlx5_core_dev *dev, struct xarray *pr_pools,
>          err = mlx5_fs_hws_pr_pool_init(pr_pool, dev, size, reformat_type);
>          if (err)
>                  goto free_pr_pool;
> -       err = xa_insert(pr_pools, index, pr_pool, GFP_KERNEL);
> -       if (err)
> -               goto cleanup_pr_pool;
> -       return pr_pool;
> +       old = xa_cmpxchg(pr_pools, index, NULL, pr_pool, GFP_KERNEL);
> +       if (!old)
> +               return pr_pool;
> 
> -cleanup_pr_pool:
>          mlx5_fs_hws_pr_pool_cleanup(pr_pool);
> +       kfree(pr_pool);
> +       return xa_is_err(old) ? ERR_PTR(xa_err(old)) : old;
> +
>   free_pr_pool:
>          kfree(pr_pool);
>          return ERR_PTR(err);
> --
> 2.43.0
> 

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

[Moshe Shemesh]

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
> 

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

[Alexandre Cassen]

Thanks for the review Moshe,


On 10/11/26 09:57, Moshe Shemesh wrote:
> Please keep the lookup out of the lock and re-check under it only on a
> miss, roughly:
Agreed, your approach is best, I will update and resend a v2 around it.

I was also considering a lockless variant where xa_alloc() picks the 
index, but 2 callers racing on a new pattern then both create a pool... 
the spare pool still allocates a full bulk of 64k entries in the host 
and device memory for a single rule. Your suggestion is better here...

-- Alexandre