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

[sashiko-bot@kernel.org]

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