On Mon, Jan 08, 2024 at 11:26:04PM +0800, Zhipeng Lu wrote:
> When `in` allocated by kvzalloc fails, arfs_create_groups will free
> ft->g and return an error. However, arfs_create_table, the only caller of
> arfs_create_groups, will hold this error and call to
> mlx5e_destroy_flow_table, in which the ft->g will be freed again.
>
> Fixes: 1cabe6b0965e ("net/mlx5e: Create aRFS flow tables")
> Signed-off-by: Zhipeng Lu <alexious@xxxxxxxxxx>
> Reviewed-by: Simon Horman <horms@xxxxxxxxxx>
When working on netdev (and probably elsewhere)
Please don't include Reviewed-by or other tags
that were explicitly supplied by someone: I don't recall
supplying the tag above so please drop it.
> ---
> Changelog:
>
> v2: free ft->g just in arfs_create_groups with a unwind ladde.
> ---
> .../net/ethernet/mellanox/mlx5/core/en_arfs.c | 17 +++++++++--------
> drivers/net/ethernet/mellanox/mlx5/core/en_fs.c | 1 -
> 2 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_arfs.c b/drivers/net/ethernet/mellanox/mlx5/core/en_arfs.c
> index bb7f86c993e5..c96f4c571b63 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_arfs.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_arfs.c
> @@ -252,13 +252,14 @@ static int arfs_create_groups(struct mlx5e_flow_table *ft,
> int err;
> u8 *mc;
>
> + ft->num_groups = 0;
> +
Although I suggested the above change, I think it
probably suitable for a separate patch. For one thing,
this is not mentioned in the patch subject. And for another,
it's probably better to change one thing at a time.
> ft->g = kcalloc(MLX5E_ARFS_NUM_GROUPS,
> sizeof(*ft->g), GFP_KERNEL);
> in = kvzalloc(inlen, GFP_KERNEL);
> if (!in || !ft->g) {
> - kfree(ft->g);
> - kvfree(in);
> - return -ENOMEM;
> + err = -ENOMEM;
> + goto free_ft;
> }
I would probably have split this up a bit:
>
> mc = MLX5_ADDR_OF(create_flow_group_in, in, match_criteria);
> @@ -278,7 +279,7 @@ static int arfs_create_groups(struct mlx5e_flow_table *ft,
> break;
> default:
> err = -EINVAL;
> - goto out;
> + goto free_ft;
> }
>
> switch (type) {
> @@ -300,7 +301,7 @@ static int arfs_create_groups(struct mlx5e_flow_table *ft,
> break;
> default:
> err = -EINVAL;
> - goto out;
> + goto free_ft;
> }
>
> MLX5_SET_CFG(in, match_criteria_enable, MLX5_MATCH_OUTER_HEADERS);
> @@ -327,7 +328,9 @@ static int arfs_create_groups(struct mlx5e_flow_table *ft,
> err:
> err = PTR_ERR(ft->g[ft->num_groups]);
> ft->g[ft->num_groups] = NULL;
> -out:
> +free_ft:
> + kfree(ft->g);
> + ft->g = NULL;
> kvfree(in);
>
> return err;
I think that I would have named the labels err_*, which
I think is more idiomatic. So combined with my suggestion
above, I suggest something like:
-err:
+err_clean_group:
err = PTR_ERR(ft->g[ft->num_groups]);
ft->g[ft->num_groups] = NULL;
-out:
+err_free_in:
kvfree(in);
+err_free_g:
+ kfree(ft->g);
+ ft->g = NULL;
return err;
> @@ -343,8 +346,6 @@ static int arfs_create_table(struct mlx5e_flow_steering *fs,
> struct mlx5_flow_table_attr ft_attr = {};
> int err;
>
> - ft->num_groups = 0;
> -
> ft_attr.max_fte = MLX5E_ARFS_TABLE_SIZE;
> ft_attr.level = MLX5E_ARFS_FT_LEVEL;
> ft_attr.prio = MLX5E_NIC_PRIO;
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_fs.c b/drivers/net/ethernet/mellanox/mlx5/core/en_fs.c
> index 777d311d44ef..7b6aa0c8b58d 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_fs.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_fs.c
> @@ -883,7 +883,6 @@ void mlx5e_fs_init_l2_addr(struct mlx5e_flow_steering *fs, struct net_device *ne
> void mlx5e_destroy_flow_table(struct mlx5e_flow_table *ft)
> {
> mlx5e_destroy_groups(ft);
> - kfree(ft->g);
> mlx5_destroy_flow_table(ft->t);
> ft->t = NULL;
Is the above still needed in some cases, and safe in all cases?
