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

[netdev-bot+sashiko@kernel.org]

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