Re: [PATCH] [v2] net/mlx5e: fix a double-free in arfs_create_groups

[alexious]

> 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.