[PATCH net v2 0/2] net/mlx5: HWS, fix modify header pattern compare

[PATCH net v2 0/2] net/mlx5: HWS, fix modify header pattern compare

[Alexandre Cassen]

The fs_hws modify header pools and the HWS pattern cache decide whether
two modify headers can share a pattern. Two bugs let different patterns
compare equal:

- On big-endian the HWS core comparison checks the data dword of each
  action instead of its control dword.
- The fs_hws pool match ignores the destination of copy and add_field
  actions.

Patch 2 moves the pool match to the HWS core comparison that patch 1
fixes.

v2:
- Add patch 1 for the big-endian compare (Sashiko).
- Reword patch 2 to stay true on big-endian.

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

Alexandre Cassen (2):
  net/mlx5: HWS, fix modify header pattern compare on big-endian
  net/mlx5: HWS, fix modify header pool match for copy actions

 .../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     | 12 ++++++------
 3 files changed, 24 insertions(+), 13 deletions(-)

-- 
2.43.0

[PATCH net v2 1/2] net/mlx5: HWS, fix modify header pattern compare on big-endian

[Alexandre Cassen]

mlx5hws_pat_compare_pattern() casts each __be64 action to __be32 to
compare only its control dword. On big-endian this keeps the data dword
instead, which HWS clears from the pattern of set and add actions. Two
such actions writing the same value into different fields then share a
pattern and the second rule modifies the wrong field.

Read the first dword of each action from memory instead. Little-endian
behavior is unchanged.

Discovered by an AI code review agent [1] and confirmed by code
inspection. Not reproduced for lack of big-endian hardware running HWS.

Fixes: aefc15a0fa1c ("net/mlx5: HWS, added modify header pattern and args handling")
Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261010120147.135825-1-acassen%40corp.free.fr [1]
Signed-off-by: Alexandre Cassen <acassen@corp.free.fr>
---
 .../net/ethernet/mellanox/mlx5/core/steering/hws/pat_arg.c    | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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..512299b59fd0 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
@@ -116,8 +116,8 @@ static bool mlx5hws_pat_compare_pattern(int cur_num_of_actions,
 				return false;
 		} else {
 			/* Compare just the control, not the values */
-			if ((__force __be32)actions[i] !=
-			    (__force __be32)cur_actions[i])
+			if (*(__be32 *)&actions[i] !=
+			    *(__be32 *)&cur_actions[i])
 				return false;
 		}
 	}
-- 
2.43.0

Re: [PATCH net v2 1/2] net/mlx5: HWS, fix modify header pattern compare on big-endian

[sashiko-bot]

> mlx5hws_pat_compare_pattern() casts each __be64 action to __be32 to
> compare only its control dword. On big-endian this keeps the data dword
> instead, which HWS clears from the pattern of set and add actions. Two
> such actions writing the same value into different fields then share a
> pattern and the second rule modifies the wrong field.
> 
> Read the first dword of each action from memory instead. Little-endian
> behavior is unchanged.
> 
> Discovered by an AI code review agent [1] and confirmed by code
> inspection. Not reproduced for lack of big-endian hardware running HWS.
> 
> Fixes: aefc15a0fa1c ("net/mlx5: HWS, added modify header pattern and args handling")
> Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261010120147.135825-1-acassen%40corp.free.fr [1]
> 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/20261012101937.1335231-1-acassen@corp.free.fr?part=1

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

[Alexandre Cassen]

mlx5_fs_hws_mh_pool_match() compares a single dword of each action
since rules pass their values as arguments. Copy and add_field actions
keep their destination in the second dword 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>
---
 .../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 512299b59fd0..a49115f04025 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 v2 2/2] net/mlx5: HWS, fix modify header pool match for copy actions

[sashiko-bot]

> mlx5_fs_hws_mh_pool_match() compares a single dword of each action
> since rules pass their values as arguments. Copy and add_field actions
> keep their destination in the second dword 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 has reviewed this patch and found no issues. It looks great!

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

Re: [PATCH net v2 0/2] net/mlx5: HWS, fix modify header pattern compare

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