Re: [PATCH net 1/2] net/mlx5: HWS, fix reformat pool creation race

[netdev-bot+sashiko@kernel.org]

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

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

- [Medium] The commit message names the unserialized trigger as "RDMA
  flow actions on the FDB".

Pre-existing issues:
- [High] In mlx5_cmd_hws_packet_reformat_dealloc()
  (drivers/net/ethernet/mellanox/mlx5/core/steering/hws/fs_hws.c), the…
- [Medium] Baseline decision is false: this is resolved later in the
  series.

--

net/mlx5: HWS, fix reformat pool creation race

This patch changes mlx5_fs_get_pr_encap_pool() to use xa_cmpxchg()
instead of xa_insert(). When two callers race to create the first
reformat pool of a given size, the loser now frees its own pool and
returns the one already published, instead of failing with -EBUSY.

> TC serializes these
> allocations under encap_tbl_lock while RDMA flow actions on the FDB
> don't.

[Severity: Medium]
Can the RDMA path actually reach mlx5_fs_get_pr_encap_pool()?

The only RDMA caller of mlx5_packet_reformat_alloc() is the
MLX5_IB_METHOD_FLOW_ACTION_CREATE_PACKET_REFORMAT handler in
drivers/infiniband/hw/mlx5/fs.c. It validates the request first:

	if (!mlx5_ib_flow_action_packet_reformat_valid(mdev, dv_prt, ft_type))
		return -EOPNOTSUPP;

mlx5_ib_flow_action_packet_reformat_valid() accepts encap types only for
NIC_TX and decap types only for NIC_RX. It returns false for FDB.

NIC_TX and NIC_RX map to the EGRESS and BYPASS namespaces, which use the
fw cmds. HWS cmds are only installed on an FDB root:

drivers/net/ethernet/mellanox/mlx5/core/fs_core.c:mlx5_flow_namespace_set_mode() {
	if (root->table_type != FS_FT_FDB)
		return -EOPNOTSUPP;
	...
	else if (mode == MLX5_FLOW_STEERING_MODE_HMFS)
		cmds = mlx5_fs_cmd_get_hws_cmds();
}

The unserialized caller that does reach these pools seems to be the TC
free path, mlx5e_encap_put()->mlx5e_encap_dealloc(). That call runs after
encap_tbl_lock has been dropped:

	mutex_unlock(&esw->offloads.encap_tbl_lock);

	mlx5e_encap_dealloc(priv, e);

That path only creates a pool because of the L2_TO_L3_TUNNEL lookup in
mlx5_cmd_hws_packet_reformat_dealloc(), described below.

Could the commit message name the concurrent creator that actually
exists?

> 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 5a172c572a68f..66edb42268cbd 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

[ ... ]

> @@ -1242,13 +1242,14 @@ mlx5_fs_get_pr_encap_pool(struct mlx5_core_dev *dev, struct xarray *pr_pools,
>  	err = mlx5_fs_hws_pr_pool_init(pr_pool, dev, size, reformat_type);
>  	if (err)
>  		goto free_pr_pool;
> -	err = xa_insert(pr_pools, index, pr_pool, GFP_KERNEL);
> -	if (err)
> -		goto cleanup_pr_pool;
> -	return pr_pool;
> +	old = xa_cmpxchg(pr_pools, index, NULL, pr_pool, GFP_KERNEL);
> +	if (!old)
> +		return pr_pool;

[Severity: High]
This is a pre-existing issue and was not introduced by this patch, but
does mlx5_cmd_hws_packet_reformat_dealloc() return L2_TO_L3_TUNNEL
reformats to the wrong pool?

The alloc side takes the bulk from the L2->L3 xarray:

mlx5_cmd_hws_packet_reformat_alloc() {
	case MLX5_REFORMAT_TYPE_L2_TO_L3_TUNNEL:
		...
		pr_pool = mlx5_fs_get_pr_encap_pool(dev, &hws_pool->el2tol3tnl_pools,
						    MLX5HWS_ACTION_TYP_REFORMAT_L2_TO_TNL_L3,
						    params->size);
}

The dealloc side looks it up in the L2->L2 xarray instead:

mlx5_cmd_hws_packet_reformat_dealloc() {
	case MLX5_REFORMAT_TYPE_L2_TO_L3_TUNNEL:
		pr_pool = mlx5_fs_get_pr_encap_pool(dev, &hws_pool->el2tol2tnl_pools,
						    MLX5HWS_ACTION_TYP_REFORMAT_L2_TO_TNL_L2,
						    pr_data->data_size);
}

mlx5_fs_get_pr_encap_pool() creates and publishes a pool on a miss. So
the free path can create an empty L2->L2 pool, or adopt an existing one,
and release the bulk into it.

mlx5_fs_pool_release_index() then updates that pool's available_units
and used_units, and used_units can go negative. It also calls list_del()
or list_add() on a bulk that is linked on the L3 pool's lists, while
holding only the L2 pool's pool_lock.

The bulk can end up on the L2 pool's unused list. Could a later
VXLAN/NVGRE/L2-tunnel encap of the same header size then be served from
it, and program an L2_TO_TNL_L3 hws_action in hardware?

mlx5e_encap_put() calls mlx5e_encap_dealloc() outside encap_tbl_lock.
Can this list manipulation also race with an allocation that holds only
the L3 pool_lock?

A single thread seems able to trigger this on an HMFS switchdev eswitch:

1. Add and delete a TC MPLSoUDP encap rule.
2. Add and delete a second MPLSoUDP rule with the same header size.
3. Add a VXLAN encap rule with that header size.

Should the dealloc case use el2tol3tnl_pools and
MLX5HWS_ACTION_TYP_REFORMAT_L2_TO_TNL_L3? That change might also remove
the unserialized pool creation this patch addresses.

>  
> -cleanup_pr_pool:
>  	mlx5_fs_hws_pr_pool_cleanup(pr_pool);
> +	kfree(pr_pool);
> +	return xa_is_err(old) ? ERR_PTR(xa_err(old)) : old;
> +
>  free_pr_pool:
>  	kfree(pr_pool);
>  	return ERR_PTR(err);

[Severity: Medium]
This isn't a bug introduced by this patch, but the modify header pools in
the same file have a similar create-on-miss race.

mlx5_cmd_hws_modify_header_alloc() scans mh_pools without a lock and uses
the running count as the new index:

	xa_for_each(&hws_pool->mh_pools, i, pool) {
		if (mlx5_fs_hws_mh_pool_match(pool, &pattern)) {
			known_pattern = true;
			break;
		}
		cnt++;
	}

mlx5_fs_create_mh_pool() still uses xa_insert(). Two concurrent first
creators can therefore pick the same index, and the second one fails
with -EBUSY.

The error path can also destroy a pool that another caller has already
found:

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

The next patch in this series, "net/mlx5: HWS, fix modify header pool
creation race", appears to fix this. It takes a new mh_pools_lock around
the lookup and creation, and it removes the destroy_pool path.

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