[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 1/2] net/mlx5: HWS, fix reformat pool creation race

[sashiko-bot]

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

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008204230.3222198-1-acassen@corp.free.fr?part=1

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] net/mlx5: 32-bit truncation of 64-bit modify actions in pool match
--

commit 214c1c5671cbf4fde727381eec5a1bbac14499d3
Author: Alexandre Cassen <acassen@corp.free.fr>

net/mlx5: HWS, fix modify header pool creation race

This commit adds a mutex to serialize modify header pool creation in
mlx5_cmd_hws_modify_header_alloc() to prevent concurrent callers from
failing with -EBUSY. It also removes the error path destruction of the newly
created pool so that other concurrent users can still access it.

> 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 66edb42268cbd..b387016bb5aad 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

[ ... ]

> @@ -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)) {

[Severity: High]
Does mlx5_fs_hws_mh_pool_match() incorrectly truncate 64-bit modify actions to
32 bits during comparison?

Looking at drivers/net/ethernet/mellanox/mlx5/core/steering/hws/fs_hws_pools.c:

mlx5_fs_hws_mh_pool_match() {
	...
	for (i = 0; i < num_actions; i++) {
		if ((__force __be32)pattern->data[i] !=
		    (__force __be32)pool_pattern->data[i])
			return false;
	}
	...
}

The pattern->data array elements are 64-bit PRM elements (__be64). Casting
them to __be32 compares only the lower 32 bits, completely ignoring the action
type and target fields in the upper 32 bits.

Could this cause two distinct flow modify actions that share the same lower
32 bits to evaluate as equal, resulting in them incorrectly sharing the same
modify header pool and executing the wrong actions on packets?

>  			known_pattern = true;
>  			break;
>  		}
>  		cnt++;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008204230.3222198-1-acassen@corp.free.fr?part=2

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

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 (1) · Medium: 1 (1) · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message names the unserialized trigger as "RDMA
  flow actions on the FDB".

Pre-existing issues:
- [High] In mlx5_cmd_hws_packet_reformat_dealloc()
  (drivers/net/ethernet/mellanox/mlx5/core/steering/hws/fs_hws.c), the…
- [Medium] Baseline decision is false: this is resolved later in the
  series.

--

net/mlx5: HWS, fix reformat pool creation race

This patch changes mlx5_fs_get_pr_encap_pool() to use xa_cmpxchg()
instead of xa_insert(). When two callers race to create the first
reformat pool of a given size, the loser now frees its own pool and
returns the one already published, instead of failing with -EBUSY.

> TC serializes these
> allocations under encap_tbl_lock while RDMA flow actions on the FDB
> don't.

[Severity: Medium]
Can the RDMA path actually reach mlx5_fs_get_pr_encap_pool()?

The only RDMA caller of mlx5_packet_reformat_alloc() is the
MLX5_IB_METHOD_FLOW_ACTION_CREATE_PACKET_REFORMAT handler in
drivers/infiniband/hw/mlx5/fs.c. It validates the request first:

	if (!mlx5_ib_flow_action_packet_reformat_valid(mdev, dv_prt, ft_type))
		return -EOPNOTSUPP;

mlx5_ib_flow_action_packet_reformat_valid() accepts encap types only for
NIC_TX and decap types only for NIC_RX. It returns false for FDB.

NIC_TX and NIC_RX map to the EGRESS and BYPASS namespaces, which use the
fw cmds. HWS cmds are only installed on an FDB root:

drivers/net/ethernet/mellanox/mlx5/core/fs_core.c:mlx5_flow_namespace_set_mode() {
	if (root->table_type != FS_FT_FDB)
		return -EOPNOTSUPP;
	...
	else if (mode == MLX5_FLOW_STEERING_MODE_HMFS)
		cmds = mlx5_fs_cmd_get_hws_cmds();
}

The unserialized caller that does reach these pools seems to be the TC
free path, mlx5e_encap_put()->mlx5e_encap_dealloc(). That call runs after
encap_tbl_lock has been dropped:

	mutex_unlock(&esw->offloads.encap_tbl_lock);

	mlx5e_encap_dealloc(priv, e);

That path only creates a pool because of the L2_TO_L3_TUNNEL lookup in
mlx5_cmd_hws_packet_reformat_dealloc(), described below.

Could the commit message name the concurrent creator that actually
exists?

> 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 5a172c572a68f..66edb42268cbd 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

[ ... ]

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

[Severity: High]
This is a pre-existing issue and was not introduced by this patch, but
does mlx5_cmd_hws_packet_reformat_dealloc() return L2_TO_L3_TUNNEL
reformats to the wrong pool?

The alloc side takes the bulk from the L2->L3 xarray:

mlx5_cmd_hws_packet_reformat_alloc() {
	case MLX5_REFORMAT_TYPE_L2_TO_L3_TUNNEL:
		...
		pr_pool = mlx5_fs_get_pr_encap_pool(dev, &hws_pool->el2tol3tnl_pools,
						    MLX5HWS_ACTION_TYP_REFORMAT_L2_TO_TNL_L3,
						    params->size);
}

The dealloc side looks it up in the L2->L2 xarray instead:

mlx5_cmd_hws_packet_reformat_dealloc() {
	case MLX5_REFORMAT_TYPE_L2_TO_L3_TUNNEL:
		pr_pool = mlx5_fs_get_pr_encap_pool(dev, &hws_pool->el2tol2tnl_pools,
						    MLX5HWS_ACTION_TYP_REFORMAT_L2_TO_TNL_L2,
						    pr_data->data_size);
}

mlx5_fs_get_pr_encap_pool() creates and publishes a pool on a miss. So
the free path can create an empty L2->L2 pool, or adopt an existing one,
and release the bulk into it.

mlx5_fs_pool_release_index() then updates that pool's available_units
and used_units, and used_units can go negative. It also calls list_del()
or list_add() on a bulk that is linked on the L3 pool's lists, while
holding only the L2 pool's pool_lock.

The bulk can end up on the L2 pool's unused list. Could a later
VXLAN/NVGRE/L2-tunnel encap of the same header size then be served from
it, and program an L2_TO_TNL_L3 hws_action in hardware?

mlx5e_encap_put() calls mlx5e_encap_dealloc() outside encap_tbl_lock.
Can this list manipulation also race with an allocation that holds only
the L3 pool_lock?

A single thread seems able to trigger this on an HMFS switchdev eswitch:

1. Add and delete a TC MPLSoUDP encap rule.
2. Add and delete a second MPLSoUDP rule with the same header size.
3. Add a VXLAN encap rule with that header size.

Should the dealloc case use el2tol3tnl_pools and
MLX5HWS_ACTION_TYP_REFORMAT_L2_TO_TNL_L3? That change might also remove
the unserialized pool creation this patch addresses.

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

[Severity: Medium]
This isn't a bug introduced by this patch, but the modify header pools in
the same file have a similar create-on-miss race.

mlx5_cmd_hws_modify_header_alloc() scans mh_pools without a lock and uses
the running count as the new index:

	xa_for_each(&hws_pool->mh_pools, i, pool) {
		if (mlx5_fs_hws_mh_pool_match(pool, &pattern)) {
			known_pattern = true;
			break;
		}
		cnt++;
	}

mlx5_fs_create_mh_pool() still uses xa_insert(). Two concurrent first
creators can therefore pick the same index, and the second one fails
with -EBUSY.

The error path can also destroy a pool that another caller has already
found:

destroy_pool:
	if (!known_pattern)
		mlx5_fs_destroy_mh_pool(pool, &hws_pool->mh_pools, cnt);

The next patch in this series, "net/mlx5: HWS, fix modify header pool
creation race", appears to fix this. It takes a new mh_pools_lock around
the lookup and creation, and it removes the destroy_pool path.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261008204230.3222198-1-acassen%40corp.free.fr