Re: [PATCH net v2 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 · 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 says the race is reachable because 'RDMA
  flow actions on the FDB don't' serialize reformat allocations.

Pre-existing issues:
- [High] mlx5_cmd_hws_packet_reformat_dealloc() releases
  MLX5_REFORMAT_TYPE_L2_TO_L3_TUNNEL reformats into the wrong pool.

--

net/mlx5: HWS, fix reformat pool creation race

This patch replaces xa_insert() in mlx5_fs_get_pr_encap_pool() with
xa_cmpxchg(). When two callers create the first reformat pool of a given
size at the same time, the loser now frees its pool and returns the
existing one 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 trigger described here actually be reached? In
drivers/infiniband/hw/mlx5/fs.c, the
MLX5_IB_METHOD_FLOW_ACTION_CREATE_PACKET_REFORMAT handler calls
mlx5_ib_flow_action_packet_reformat_valid() first, and that returns false
for FDB tables:

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

The encap types are only accepted for MLX5_IB_UAPI_FLOW_TABLE_TYPE_NIC_TX,
which mlx5_ib_ft_type_to_namespace() maps to MLX5_FLOW_NAMESPACE_EGRESS.

HWS cmds are only installed on the FDB root, because
mlx5_flow_root_namespace_set_mode() rejects anything other than FS_FT_FDB.
So RDMA requests never seem to reach mlx5_cmd_hws_packet_reformat_alloc().

The race does look reachable through TC. mlx5e_encap_put() drops
encap_tbl_lock before it calls mlx5e_encap_dealloc():

    mutex_unlock(&esw->offloads.encap_tbl_lock);

    mlx5e_encap_dealloc(priv, e);

On that path, the L2_TO_L3_TUNNEL case in
mlx5_cmd_hws_packet_reformat_dealloc() can create an el2tol2tnl pool (see
below). That can happen at the same time as a TC allocation made under
encap_tbl_lock.

Could the commit message name the path that actually reaches
mlx5_fs_get_pr_encap_pool() without serialization?

> 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;
>  
> -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: High]
This isn't a bug introduced by this patch, but one of the other callers of
mlx5_fs_get_pr_encap_pool() looks up the wrong pool for
MLX5_REFORMAT_TYPE_L2_TO_L3_TUNNEL:

drivers/net/ethernet/mellanox/mlx5/core/steering/hws/fs_hws.c: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);
		break;
    ...
}

The alloc side in mlx5_cmd_hws_packet_reformat_alloc() uses
el2tol3tnl_pools and MLX5HWS_ACTION_TYP_REFORMAT_L2_TO_TNL_L3. Should the
dealloc case use them too?

The lookup is get-or-create, so the free path can allocate a new L2 pool.
If that allocation fails, "Failed release packet-reformat" is logged and
the function returns early. Does that leak pr_data->data and the pool
index?

mlx5_fs_hws_pr_pool_release_pr()->mlx5_fs_pool_release_index() then takes
the L2 pool's pool_lock and updates the L2 pool's counters. On a fresh
pool, used_units goes negative, and the owning L3 pool's used_units is
never decremented.

When the last entry of the bulk is released, this runs against the L2
pool:

	if (bulk_free_amount == bulk->bulk_len) {
		list_del(&bulk->pool_list);
		if (fs_pool->available_units > fs_pool->threshold)
			mlx5_fs_pool_free_bulk(fs_pool, bulk);
		else
			list_add(&bulk->pool_list, &fs_pool->unused);
	}

The bulk is still linked on the L3 pool's partially_used list, and the L3
pool_lock is not held here.

mlx5e_encap_put() calls the dealloc after it drops encap_tbl_lock. Could a
concurrent L3 acquire, holding only the L3 pool_lock, race with this
list_del() and mlx5_fs_pool_free_bulk()? That could corrupt the list or
cause a use-after-free of the bulk and its hws_action.

If the bulk goes onto the L2 pool's unused list instead, could later
L2_TO_L2_TUNNEL encaps be given an L3 tunnel hws_action? The hardware
would then apply the wrong reformat.

This can be reached with an MPLSoUDP encap on a switchdev eswitch that uses
HMFS FDB steering. The code comes from aecd9d1020e3, the same commit named
in this patch's Fixes: tag.

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