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

[PATCH net v2 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 pool creation while keeping a new
  pool on error.

v2:
- Keep the pattern lookup out of the mutex and look again under it on
  a miss (Moshe).
- Add Moshe's Reviewed-by to patch 1.

v1: https://lore.kernel.org/netdev/20261008204230.3222198-1-acassen@corp.free.fr/

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  | 66 +++++++++++--------
 .../mellanox/mlx5/core/steering/hws/fs_hws.h  |  1 +
 2 files changed, 40 insertions(+), 27 deletions(-)

-- 
2.43.0

[PATCH net v2 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")
Reviewed-by: Moshe Shemesh <moshe@nvidia.com>
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

Re: [PATCH net v2 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")
> Reviewed-by: Moshe Shemesh <moshe@nvidia.com>
> 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/20261012100555.1317878-1-acassen@corp.free.fr?part=1

Re: [PATCH net v2 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 · 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 says the race is reachable because 'RDMA
  flow actions on the FDB don't' serialize reformat allocations.

Pre-existing issues:
- [High] mlx5_cmd_hws_packet_reformat_dealloc() releases
  MLX5_REFORMAT_TYPE_L2_TO_L3_TUNNEL reformats into the wrong pool.

--

net/mlx5: HWS, fix reformat pool creation race

This patch replaces xa_insert() in mlx5_fs_get_pr_encap_pool() with
xa_cmpxchg(). When two callers create the first reformat pool of a given
size at the same time, the loser now frees its pool and returns the
existing one 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 trigger described here actually be reached? In
drivers/infiniband/hw/mlx5/fs.c, the
MLX5_IB_METHOD_FLOW_ACTION_CREATE_PACKET_REFORMAT handler calls
mlx5_ib_flow_action_packet_reformat_valid() first, and that returns false
for FDB tables:

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

The encap types are only accepted for MLX5_IB_UAPI_FLOW_TABLE_TYPE_NIC_TX,
which mlx5_ib_ft_type_to_namespace() maps to MLX5_FLOW_NAMESPACE_EGRESS.

HWS cmds are only installed on the FDB root, because
mlx5_flow_root_namespace_set_mode() rejects anything other than FS_FT_FDB.
So RDMA requests never seem to reach mlx5_cmd_hws_packet_reformat_alloc().

The race does look reachable through TC. mlx5e_encap_put() drops
encap_tbl_lock before it calls mlx5e_encap_dealloc():

    mutex_unlock(&esw->offloads.encap_tbl_lock);

    mlx5e_encap_dealloc(priv, e);

On that path, the L2_TO_L3_TUNNEL case in
mlx5_cmd_hws_packet_reformat_dealloc() can create an el2tol2tnl pool (see
below). That can happen at the same time as a TC allocation made under
encap_tbl_lock.

Could the commit message name the path that actually reaches
mlx5_fs_get_pr_encap_pool() without serialization?

> 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;
>  
> -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: High]
This isn't a bug introduced by this patch, but one of the other callers of
mlx5_fs_get_pr_encap_pool() looks up the wrong pool for
MLX5_REFORMAT_TYPE_L2_TO_L3_TUNNEL:

drivers/net/ethernet/mellanox/mlx5/core/steering/hws/fs_hws.c: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);
		break;
    ...
}

The alloc side in mlx5_cmd_hws_packet_reformat_alloc() uses
el2tol3tnl_pools and MLX5HWS_ACTION_TYP_REFORMAT_L2_TO_TNL_L3. Should the
dealloc case use them too?

The lookup is get-or-create, so the free path can allocate a new L2 pool.
If that allocation fails, "Failed release packet-reformat" is logged and
the function returns early. Does that leak pr_data->data and the pool
index?

mlx5_fs_hws_pr_pool_release_pr()->mlx5_fs_pool_release_index() then takes
the L2 pool's pool_lock and updates the L2 pool's counters. On a fresh
pool, used_units goes negative, and the owning L3 pool's used_units is
never decremented.

When the last entry of the bulk is released, this runs against the L2
pool:

	if (bulk_free_amount == bulk->bulk_len) {
		list_del(&bulk->pool_list);
		if (fs_pool->available_units > fs_pool->threshold)
			mlx5_fs_pool_free_bulk(fs_pool, bulk);
		else
			list_add(&bulk->pool_list, &fs_pool->unused);
	}

The bulk is still linked on the L3 pool's partially_used list, and the L3
pool_lock is not held here.

mlx5e_encap_put() calls the dealloc after it drops encap_tbl_lock. Could a
concurrent L3 acquire, holding only the L3 pool_lock, race with this
list_del() and mlx5_fs_pool_free_bulk()? That could corrupt the list or
cause a use-after-free of the bulk and its hws_action.

If the bulk goes onto the L2 pool's unused list instead, could later
L2_TO_L2_TUNNEL encaps be given an L3 tunnel hws_action? The hardware
would then apply the wrong reformat.

This can be reached with an MPLSoUDP encap on a switchdev eswitch that uses
HMFS FDB steering. The code comes from aecd9d1020e3, the same commit named
in this patch's Fixes: tag.

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

[PATCH net v2 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.

Pools now live until cleanup so the lookup needs no lock. A miss looks
again under a mutex before creating the pool.

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  | 53 +++++++++++--------
 .../mellanox/mlx5/core/steering/hws/fs_hws.h  |  1 +
 2 files changed, 33 insertions(+), 21 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..11cced847186 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);
@@ -1457,6 +1459,23 @@ mlx5_fs_destroy_mh_pool(struct mlx5_fs_pool *pool, struct xarray *mh_pools,
 	kfree(pool);
 }
 
+static struct mlx5_fs_pool *
+mlx5_fs_find_mh_pool(struct xarray *mh_pools,
+		     struct mlx5hws_action_mh_pattern *pattern,
+		     unsigned long *cnt)
+{
+	struct mlx5_fs_pool *pool;
+	unsigned long i;
+
+	*cnt = 0;
+	xa_for_each(mh_pools, i, pool) {
+		if (mlx5_fs_hws_mh_pool_match(pool, pattern))
+			return pool;
+		(*cnt)++;
+	}
+	return NULL;
+}
+
 static int mlx5_cmd_hws_modify_header_alloc(struct mlx5_flow_root_namespace *ns,
 					    u8 namespace, u8 num_actions,
 					    void *modify_actions,
@@ -1467,33 +1486,28 @@ static int mlx5_cmd_hws_modify_header_alloc(struct mlx5_flow_root_namespace *ns,
 	struct mlx5_fs_hws_mh *mh_data = NULL;
 	struct mlx5hws_action *hws_action;
 	struct mlx5_fs_pool *pool;
-	unsigned long i, cnt = 0;
-	bool known_pattern;
+	unsigned long cnt;
 	int err;
 
 	pattern.sz = MLX5_UN_SZ_BYTES(set_add_copy_action_in_auto) * num_actions;
 	pattern.data = modify_actions;
 
-	known_pattern = false;
-	xa_for_each(&hws_pool->mh_pools, i, pool) {
-		if (mlx5_fs_hws_mh_pool_match(pool, &pattern)) {
-			known_pattern = true;
-			break;
-		}
-		cnt++;
-	}
-
-	if (!known_pattern) {
-		pool = mlx5_fs_create_mh_pool(ns->dev, &pattern,
-					      &hws_pool->mh_pools, cnt);
+	pool = mlx5_fs_find_mh_pool(&hws_pool->mh_pools, &pattern, &cnt);
+	if (!pool) {
+		mutex_lock(&hws_pool->mh_pools_lock);
+		pool = mlx5_fs_find_mh_pool(&hws_pool->mh_pools, &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);
 	}
+
 	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 +1523,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..cf068fb5bdbf 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 creation */
 	struct xarray table_dests;
 	struct xarray vport_vhca_dests;
 	struct xarray vport_dests;
-- 
2.43.0

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

[Moshe Shemesh]

On 10/12/2026 1:05 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.
> 
> Pools now live until cleanup so the lookup needs no lock. A miss looks
> again under a mutex before creating the pool.
> 
> 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>

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

Thanks.

> ---
>   .../mellanox/mlx5/core/steering/hws/fs_hws.c  | 53 +++++++++++--------
>   .../mellanox/mlx5/core/steering/hws/fs_hws.h  |  1 +
>   2 files changed, 33 insertions(+), 21 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..11cced847186 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);
> @@ -1457,6 +1459,23 @@ mlx5_fs_destroy_mh_pool(struct mlx5_fs_pool *pool, struct xarray *mh_pools,
>          kfree(pool);
>   }
> 
> +static struct mlx5_fs_pool *
> +mlx5_fs_find_mh_pool(struct xarray *mh_pools,
> +                    struct mlx5hws_action_mh_pattern *pattern,
> +                    unsigned long *cnt)
> +{
> +       struct mlx5_fs_pool *pool;
> +       unsigned long i;
> +
> +       *cnt = 0;
> +       xa_for_each(mh_pools, i, pool) {
> +               if (mlx5_fs_hws_mh_pool_match(pool, pattern))
> +                       return pool;
> +               (*cnt)++;
> +       }
> +       return NULL;
> +}
> +
>   static int mlx5_cmd_hws_modify_header_alloc(struct mlx5_flow_root_namespace *ns,
>                                              u8 namespace, u8 num_actions,
>                                              void *modify_actions,
> @@ -1467,33 +1486,28 @@ static int mlx5_cmd_hws_modify_header_alloc(struct mlx5_flow_root_namespace *ns,
>          struct mlx5_fs_hws_mh *mh_data = NULL;
>          struct mlx5hws_action *hws_action;
>          struct mlx5_fs_pool *pool;
> -       unsigned long i, cnt = 0;
> -       bool known_pattern;
> +       unsigned long cnt;
>          int err;
> 
>          pattern.sz = MLX5_UN_SZ_BYTES(set_add_copy_action_in_auto) * num_actions;
>          pattern.data = modify_actions;
> 
> -       known_pattern = false;
> -       xa_for_each(&hws_pool->mh_pools, i, pool) {
> -               if (mlx5_fs_hws_mh_pool_match(pool, &pattern)) {
> -                       known_pattern = true;
> -                       break;
> -               }
> -               cnt++;
> -       }
> -
> -       if (!known_pattern) {
> -               pool = mlx5_fs_create_mh_pool(ns->dev, &pattern,
> -                                             &hws_pool->mh_pools, cnt);
> +       pool = mlx5_fs_find_mh_pool(&hws_pool->mh_pools, &pattern, &cnt);
> +       if (!pool) {
> +               mutex_lock(&hws_pool->mh_pools_lock);
> +               pool = mlx5_fs_find_mh_pool(&hws_pool->mh_pools, &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);
>          }
> +
>          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 +1523,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..cf068fb5bdbf 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 creation */
>          struct xarray table_dests;
>          struct xarray vport_vhca_dests;
>          struct xarray vport_dests;
> --
> 2.43.0
> 

Re: [PATCH net v2 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] mlx5: hws: 32-bit truncation in modify header action match
--

commit 33535690e64312a76a1dd5c1afe587641631b515
Author: Alexandre Cassen <acassen@corp.free.fr>

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

This commit addresses a race condition during modify header pool creation
where concurrent callers might allocate pools at the same index. It fixes
this by keeping created pools until cleanup, allowing lockless lookups for
existing pools while using a mutex for creation.

> 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..11cced847186e 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
[ ... ]
> @@ -1457,6 +1459,23 @@ mlx5_fs_destroy_mh_pool(struct mlx5_fs_pool *pool, struct xarray *mh_pools,
>  	kfree(pool);
>  }
>  
> +static struct mlx5_fs_pool *
> +mlx5_fs_find_mh_pool(struct xarray *mh_pools,
> +		     struct mlx5hws_action_mh_pattern *pattern,
> +		     unsigned long *cnt)
> +{
> +	struct mlx5_fs_pool *pool;
> +	unsigned long i;
> +
> +	*cnt = 0;
> +	xa_for_each(mh_pools, i, pool) {
> +		if (mlx5_fs_hws_mh_pool_match(pool, pattern))

[Severity: High]
Does this code rely on a matching function that truncates 64-bit payload
data?

Looking at mlx5_fs_hws_mh_pool_match() in
drivers/net/ethernet/mellanox/mlx5/core/steering/hws/fs_hws_pools.c, it
casts the 64-bit modify action words down to 32 bits:

		if ((__force __be32)pattern->data[i] !=
		    (__force __be32)pool_pattern->data[i])
			return false;

Since data is an array of 64-bit elements (__be64 *data), this discards 32
bits which include fields like action_type, field, offset, and length.
Consequently, two distinct modify actions sharing the same lower 32 bits will
incorrectly match, and the driver might apply the wrong header modifications.

Can this be triggered by user-space or network admin configurations setting
up similar TC or RDMA flow rules, resulting in wrong hardware pool reuse and
corrupted traffic headers?

> +			return pool;
> +		(*cnt)++;
> +	}
> +	return NULL;
> +}
> +

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