[PATCH net] net/mlx5: HWS, fix modify header pool match for copy actions

[PATCH net] net/mlx5: HWS, fix modify header pool match for copy actions

[Alexandre Cassen]

mlx5_fs_hws_mh_pool_match() compares only the first half of each action
since rules pass their values as arguments. Copy and add_field actions
keep their destination in the second half and HWS reads them from the
pattern alone. Two modify headers whose copies differ only in
destination then share a pool and the second rule writes to the wrong
field.

Use the HWS core comparison that checks these actions in full.

Found while checking a Sashiko AI review report and confirmed against
the HWS pattern code. Not reproduced. No in-kernel user puts such
actions on the FDB but RDMA flow actions can.

Fixes: b36315ca69cb ("net/mlx5: fs, add HWS modify header API function")
Signed-off-by: Alexandre Cassen <acassen@corp.free.fr>
---
Sashiko AI review: https://sashiko.dev/#/patchset/20261008204230.3222198-1-acassen@corp.free.fr?part=2

 .../mlx5/core/steering/hws/fs_hws_pools.c         | 10 +++-------
 .../mellanox/mlx5/core/steering/hws/mlx5hws.h     | 15 +++++++++++++++
 .../mellanox/mlx5/core/steering/hws/pat_arg.c     |  8 ++++----
 3 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/fs_hws_pools.c b/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/fs_hws_pools.c
index 86828e329187..cd888756e942 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/fs_hws_pools.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/fs_hws_pools.c
@@ -389,7 +389,7 @@ bool mlx5_fs_hws_mh_pool_match(struct mlx5_fs_pool *mh_pool,
 			       struct mlx5hws_action_mh_pattern *pattern)
 {
 	struct mlx5hws_action_mh_pattern *pool_pattern;
-	int num_actions, i;
+	int num_actions;
 
 	pool_pattern = mh_pool->pool_ctx;
 	if (WARN_ON_ONCE(!pool_pattern))
@@ -398,12 +398,8 @@ bool mlx5_fs_hws_mh_pool_match(struct mlx5_fs_pool *mh_pool,
 	if (pattern->sz != pool_pattern->sz)
 		return false;
 	num_actions = pattern->sz / MLX5_UN_SZ_BYTES(set_add_copy_action_in_auto);
-	for (i = 0; i < num_actions; i++) {
-		if ((__force __be32)pattern->data[i] !=
-		    (__force __be32)pool_pattern->data[i])
-			return false;
-	}
-	return true;
+	return mlx5hws_pat_compare_pattern(num_actions, pool_pattern->data,
+					   num_actions, pattern->data);
 }
 
 struct mlx5hws_action *mlx5_fc_get_hws_action(struct mlx5hws_context *ctx,
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/mlx5hws.h b/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/mlx5hws.h
index 1ad7a50d938b..13e9cdbc5d47 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/mlx5hws.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/mlx5hws.h
@@ -688,6 +688,21 @@ mlx5hws_action_create_modify_header(struct mlx5hws_context *ctx,
 				    u32 log_bulk_size,
 				    u32 flags);
 
+/**
+ * mlx5hws_pat_compare_pattern - Compare two modify header patterns.
+ *
+ * @cur_num_of_actions: Number of actions in "cur_actions" array.
+ * @cur_actions: First pattern actions.
+ * @num_of_actions: Number of actions in "actions" array.
+ * @actions: Second pattern actions.
+ *
+ * Return: true if the patterns differ only in values, false otherwise.
+ */
+bool mlx5hws_pat_compare_pattern(int cur_num_of_actions,
+				 __be64 cur_actions[],
+				 int num_of_actions,
+				 __be64 actions[]);
+
 /**
  * mlx5hws_action_create_aso_meter - Create direct rule ASO flow meter action.
  *
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/pat_arg.c b/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/pat_arg.c
index 9af449c2b4d5..0120d511c694 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/pat_arg.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/pat_arg.c
@@ -96,10 +96,10 @@ void mlx5hws_pat_uninit_pattern_cache(struct mlx5hws_pattern_cache *cache)
 	kfree(cache);
 }
 
-static bool mlx5hws_pat_compare_pattern(int cur_num_of_actions,
-					__be64 cur_actions[],
-					int num_of_actions,
-					__be64 actions[])
+bool mlx5hws_pat_compare_pattern(int cur_num_of_actions,
+				 __be64 cur_actions[],
+				 int num_of_actions,
+				 __be64 actions[])
 {
 	int i;
 
-- 
2.43.0

Re: [PATCH net] net/mlx5: HWS, fix modify header pool match for copy actions

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

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

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] net/mlx5: HWS, fix modify header pool match for copy actions

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 (1) · Medium: 0 · 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).

- [High] On big-endian CPUs, the comparator this fix now relies on still
  lets modify headers with different control words share a pool.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but
  mlx5_cmd_hws_modify_header_alloc() (fs_hws.c:1459-1515) has a use-…

--

net/mlx5: HWS, fix modify header pool match for copy actions

This patch changes mlx5_fs_hws_mh_pool_match() to call the HWS core
helper mlx5hws_pat_compare_pattern() in place of its own loop. The goal
is to compare COPY and ADD_FIELD actions in full. The helper is no
longer static, and its prototype with kernel-doc is added to mlx5hws.h.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/fs_hws_pools.c b/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/fs_hws_pools.c
> index 86828e329187d..cd888756e9421 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/fs_hws_pools.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/steering/hws/fs_hws_pools.c
> @@ -389,7 +389,7 @@ bool mlx5_fs_hws_mh_pool_match(struct mlx5_fs_pool *mh_pool,
>  			       struct mlx5hws_action_mh_pattern *pattern)
>  {
>  	struct mlx5hws_action_mh_pattern *pool_pattern;
> -	int num_actions, i;
> +	int num_actions;
>  
>  	pool_pattern = mh_pool->pool_ctx;
>  	if (WARN_ON_ONCE(!pool_pattern))

[Severity: High]
This is a pre-existing issue, but can mh_pool already be freed when it is
dereferenced here?

mlx5_cmd_hws_modify_header_alloc() walks hws_pool->mh_pools without any
lock:

    xa_for_each(&hws_pool->mh_pools, i, pool) {
        if (mlx5_fs_hws_mh_pool_match(pool, &pattern)) {

mlx5_fs_create_mh_pool() publishes the new pool with xa_insert() before
mlx5_fs_hws_mh_pool_acquire_mh() and kmemdup() run. If either of those
fails, the error path frees the pool right away:

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

mlx5_fs_destroy_mh_pool() {
    xa_erase(mh_pools, index);
    mlx5_fs_hws_mh_pool_cleanup(pool);
    kfree(pool);
}

Nothing here takes a refcount or waits for an RCU grace period.

Suppose a second caller got the same pool pointer from xa_for_each(). It
could then read the freed pool->pool_ctx, pool_pattern->sz and
pool_pattern->data in mlx5_fs_hws_mh_pool_match(). If the match succeeds,
it would call mlx5_fs_hws_mh_pool_acquire_mh() on the freed pool and store
the dangling pointer in modify_hdr->fs_hws_action.fs_pool.

Concurrent callers look reachable. mlx5_tc_ct_entry_create_mod_hdr() calls
mlx5_modify_header_alloc() directly, without a mod_hdr_tbl lock. It runs
from the WQ_UNBOUND nf_ft_offload_add workqueue under
down_read(&flowtable->flow_block_lock), so several CT entries can be
offloaded in parallel.

This patch only changes the comparison inside the matcher, so the race is
no more or less exposed than before.

[ ... ]

> @@ -398,12 +398,8 @@ bool mlx5_fs_hws_mh_pool_match(struct mlx5_fs_pool *mh_pool,
>  	if (pattern->sz != pool_pattern->sz)
>  		return false;
>  	num_actions = pattern->sz / MLX5_UN_SZ_BYTES(set_add_copy_action_in_auto);
> -	for (i = 0; i < num_actions; i++) {
> -		if ((__force __be32)pattern->data[i] !=
> -		    (__force __be32)pool_pattern->data[i])
> -			return false;
> -	}
> -	return true;
> +	return mlx5hws_pat_compare_pattern(num_actions, pool_pattern->data,
> +					   num_actions, pattern->data);

[Severity: High]
On big-endian hosts, does mlx5hws_pat_compare_pattern() actually compare
the control dword?

For actions other than COPY and ADD_FIELD, it does this:

    /* Compare just the control, not the values */
    if ((__force __be32)actions[i] !=
        (__force __be32)cur_actions[i])
        return false;

The elements are __be64, so this cast keeps the low 32 bits of the loaded
value:

- On little-endian, those are memory bytes 0-3. That is PRM dword 0, which
  holds action_type, field, offset and length.
- On big-endian, they are bytes 4-7. That is dword 1, which holds the
  immediate data for SET and ADD.

The device never sees that data dword in the pattern. From
mlx5hws_cmd_header_modify_pattern_create():

    if (type != MLX5_MODIFICATION_TYPE_COPY &&
        type != MLX5_MODIFICATION_TYPE_ADD_FIELD)
        /* Action typ-copy use all bytes for control */
        MLX5_SET(set_action_in, &pattern_data[i], data, 0);

So on big-endian, two SET or ADD patterns that write the same value into
different fields would compare equal. mlx5_cmd_hws_modify_header_alloc()
would then reuse the first pool's mh_data->bulk->hws_action, and the
second rule would write to the wrong field. That is the symptom the commit
message describes.

Also, the helper picks the full 64-bit compare based only on the opcode of
the requested action:

    u8 action_id =
        MLX5_GET(set_action_in, &actions[i], action_type);

Could an existing COPY pool then match an incoming SET whose data dword
equals the COPY's dst dword? In that case the NIC would run a COPY instead
of the SET. That would leave a copy action pool mismatch open on
big-endian.

MLX5_HW_STEERING does not seem to depend on !CPU_BIG_ENDIAN.

The commit message says the old loop "compares only the first half of each
action". The new kernel-doc in mlx5hws.h says "Return: true if the
patterns differ only in values". Neither statement holds on big-endian.

The truncating cast was already in the old fs_hws loop and in
mlx5hws_pat_find_cached_pattern(). This patch, however, sends pool
matching through the helper and documents it as a control-only compare.

Would it be safer to read the first dword explicitly, for example
((__be32 *)&actions[i])[0], or to use MLX5_GET on the control fields?

>  }
>  
>  struct mlx5hws_action *mlx5_fc_get_hws_action(struct mlx5hws_context *ctx,

[ ... ]

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