* Re: [PATCH net] net/mlx5: HWS, fix modify header pool match for copy actions
2026-10-10 12:01 [PATCH net] net/mlx5: HWS, fix modify header pool match for copy actions Alexandre Cassen
@ 2026-10-10 12:03 ` netdev-bot+sinfo
2026-10-11 12:02 ` netdev-bot+sashiko
1 sibling, 0 replies; 4+ messages in thread
From: netdev-bot+sinfo @ 2026-10-10 12:03 UTC (permalink / raw)
To: Alexandre Cassen
Cc: Saeed Mahameed, Leon Romanovsky, Tariq Toukan, Mark Bloch,
Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Moshe Shemesh, Yevgeny Kliteynik, netdev, linux-rdma
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net] net/mlx5: HWS, fix modify header pool match for copy actions
2026-10-10 12:01 [PATCH net] net/mlx5: HWS, fix modify header pool match for copy actions Alexandre Cassen
2026-10-10 12:03 ` netdev-bot+sinfo
@ 2026-10-11 12:02 ` netdev-bot+sashiko
2026-10-12 10:16 ` Alexandre Cassen
1 sibling, 1 reply; 4+ messages in thread
From: netdev-bot+sashiko @ 2026-10-11 12:02 UTC (permalink / raw)
To: acassen
Cc: saeedm, leon, tariqt, mbloch, andrew+netdev, davem, edumazet,
kuba, pabeni, moshe, kliteyn, netdev, linux-rdma
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
^ permalink raw reply [flat|nested] 4+ messages in thread