| Message ID | 20240108152605.3712050-1-alexious@zju.edu.cn (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] net/mlx5e: fix a double-free in arfs_create_groups | expand |
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@zju.edu.cn> > Reviewed-by: Simon Horman <horms@kernel.org> 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 <alexious@zju.edu.cn> > > Reviewed-by: Simon Horman <horms@kernel.org> > > 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, alexious@zju.edu.cn 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 <alexious@zju.edu.cn> > > > Reviewed-by: Simon Horman <horms@kernel.org> > > > > 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, alexious@zju.edu.cn 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 <alexious@zju.edu.cn> > > > > Reviewed-by: Simon Horman <horms@kernel.org> > > > > > > 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!
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; }
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@zju.edu.cn> Reviewed-by: Simon Horman <horms@kernel.org> --- 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(-)