Re: [PATCH net 1/2] net/mlx5: HWS, release L2 to L3 tunnel reformats to their own pool

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] net/mlx5: memory leak of pr_data->data in packet reformat dealloc
- [Medium] net/mlx5: TOCTOU race in packet reformat pool allocation
--

commit b8aadd3f3a4943df684cd9bcfb91cb735038141f
Author: Alexandre Cassen <acassen@corp.free.fr>

net/mlx5: HWS, release L2 to L3 tunnel reformats to their own pool

L2 to L3 tunnel reformats are now released to the el2tol3tnl pools.

> 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..44b311b4532d1 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
> @@ -1398,8 +1398,8 @@ static void mlx5_cmd_hws_packet_reformat_dealloc(struct mlx5_flow_root_namespace
>  						    pr_data->data_size);
>  		break;
>  	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_pool = mlx5_fs_get_pr_encap_pool(dev, &hws_pool->el2tol3tnl_pools,
> +						    MLX5HWS_ACTION_TYP_REFORMAT_L2_TO_TNL_L3,
>  						    pr_data->data_size);

[Severity: Medium]
Could this call to mlx5_fs_get_pr_encap_pool() result in a time-of-check to
time-of-use race?

If multiple threads concurrently attempt to offload TC rules requiring a new
packet reformat pool of the same size, they might race locklessly. Both
threads could evaluate xa_load() to NULL and race on xa_insert():

drivers/net/ethernet/mellanox/mlx5/core/steering/hws/fs_hws.c:mlx5_fs_get_pr_encap_pool() {
    ...
	pr_pool = xa_load(pr_pools, index);
	if (pr_pool)
		return pr_pool;
    ...
	err = xa_insert(pr_pools, index, pr_pool, GFP_KERNEL);
	if (err)
		goto cleanup_pr_pool;
    ...
}

Would the losing thread unconditionally propagate an -EBUSY error up instead
of retrying or recovering?

[Severity: Medium]
If mlx5_fs_get_pr_encap_pool() returns an error, does this code leak
pr_data->data?

Later in mlx5_cmd_hws_packet_reformat_dealloc(), the error path returns
immediately without freeing the data:

	if (!pkt_reformat->fs_hws_action.pr_data || IS_ERR(pr_pool)) {
		mlx5_core_err(ns->dev, "Failed release packet-reformat\n");
		return;
	}

Is a kfree(pr_data->data) missing in this error path prior to the return?

>  		break;
>  	case MLX5_REFORMAT_TYPE_L3_TUNNEL_TO_L2:

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