> 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.
I apologize, but it appears that you included a "reviewed-by"
tag along with certain suggestions for version 1 of this patch
in the first review email(about 6 days before).
In response, after a short discussion, I followed some of
those suggestions and send this v2 patch.
I referred to the "Dealing with tags" section in this KernelNewbies
tips: https://kernelnewbies.org/PatchTipsAndTricks and thought
that I should include that tag in v1 email to this v2 patch.
So now I'm a little bit confused here: if the tag rule has changed
or I got some misunderstanding on existing rules? Your clarification
on this matter would be greatly appreciated.
I'll send a new version of this patch after correcting the tag
issue and taking your suggestions into consideration.
Several comments below.
>
> > ---
> > 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.
Agree, I made this change because I'd like to apply as much
suggestion as possible. And it is a better idea to leave it
to a refector patch one day.
>
> > 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:
Agree, I'll split it into two like other allocation operation
in kernel.
>
> >
> > 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:
OK, I'll change the label name to more idiomatic ones.
>
> -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?
Well, in fact the kfree(ft->g) in mlx5e_destroy_flow_table causes
double frees in different functions such as fs_udp_create_table,
not only in arfs_create_groups. But you are right, with a more
detailed check I found that in some other functions, like
accel_fs_tcp_create_table, removing such free will cause memleak.
So it could be a better idea to leave mlx5e_destroy_flow_table
as it used to be. And that follows the "one patch for one change" idea.
