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 <[email protected]>
Reviewed-by: Simon Horman <[email protected]>
---
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;
+
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;
}
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;
@@ -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;
}
--
2.34.1
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 <[email protected]>
> Reviewed-by: Simon Horman <[email protected]>
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?
> 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 <[email protected]>
> > Reviewed-by: Simon Horman <[email protected]>
>
> 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.
On Wed, Jan 10, 2024 at 09:23:24PM +0800, [email protected] wrote:
> > 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 <[email protected]>
> > > Reviewed-by: Simon Horman <[email protected]>
> >
> > 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).
Yes, sorry. My statement above is not correct:
I now see that I did supply the tag.
> 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 think in this case my statement above was incorrect,
and indeed the rule above is correct AFAIK
But it probably would have been best not to include the tag
in v2, because there were significant changes between v1 and v2.
> I'll send a new version of this patch after correcting the tag
> issue and taking your suggestions into consideration.
>
> Several comments below.
..
> > > @@ -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.
Right, I think it would be best to focus on fixing arfs_create_groups().
And making sure that neither leaks nor double frees occur. And I think
that at this point that includes ensuring ft->g is NULL if it has been freed.
> On Wed, Jan 10, 2024 at 09:23:24PM +0800, [email protected] wrote:
> > > 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 <[email protected]>
> > > > Reviewed-by: Simon Horman <[email protected]>
> > >
> > > 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).
>
> Yes, sorry. My statement above is not correct:
> I now see that I did supply the tag.
Never mind, you did give a lot of constructive suggestion proberly to every patch.
Forgetting about a tag, well, it can't be helped.
> > 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.
>
...
>
> Right, I think it would be best to focus on fixing arfs_create_groups().
> And making sure that neither leaks nor double frees occur. And I think
> that at this point that includes ensuring ft->g is NULL if it has been freed.
A v3 version of this patch was sent just now, which is focusing on arfs_create_groups itself.
Have a nice day!