| Message ID | 20230629024642.2228767-2-shaozhengchao@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fix two memory leak issues for mlx5en driver | expand |
On Thu, Jun 29, 2023 at 10:46:41AM +0800, Zhengchao Shao wrote: > The memory pointed to by the fs->any pointer is not freed in the error > path of mlx5e_fs_tt_redirect_any_create, which can lead to a memory leak. > Fix by freeing the memory in the error path, thereby making the error path > identical to mlx5e_fs_tt_redirect_any_destroy(). > > Fixes: 0f575c20bf06 ("net/mlx5e: Introduce Flow Steering ANY API") > Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com> Reviewed-by: Simon Horman <simon.horman@corigine.com>
On Thu, 29 Jun, 2023 10:46:41 +0800 Zhengchao Shao <shaozhengchao@huawei.com> wrote: > The memory pointed to by the fs->any pointer is not freed in the error > path of mlx5e_fs_tt_redirect_any_create, which can lead to a memory leak. > Fix by freeing the memory in the error path, thereby making the error path > identical to mlx5e_fs_tt_redirect_any_destroy(). > > Fixes: 0f575c20bf06 ("net/mlx5e: Introduce Flow Steering ANY API") > Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com> > --- > drivers/net/ethernet/mellanox/mlx5/core/en/fs_tt_redirect.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/fs_tt_redirect.c b/drivers/net/ethernet/mellanox/mlx5/core/en/fs_tt_redirect.c > index 03cb79adf912..9cf4ec931b8b 100644 > --- a/drivers/net/ethernet/mellanox/mlx5/core/en/fs_tt_redirect.c > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/fs_tt_redirect.c > @@ -594,7 +594,7 @@ int mlx5e_fs_tt_redirect_any_create(struct mlx5e_flow_steering *fs) > > err = fs_any_create_table(fs); > if (err) > - return err; > + goto err_free_any; > > err = fs_any_enable(fs); > if (err) > @@ -606,7 +606,7 @@ int mlx5e_fs_tt_redirect_any_create(struct mlx5e_flow_steering *fs) > > err_destroy_table: > fs_any_destroy_table(fs_any); > - > +err_free_any: > kfree(fs_any); > mlx5e_fs_set_any(fs, NULL); We probably should update the 'any' member reference in fs to NULL *before* free-ing fs_any. Otherwise, there is a period in time where fs is referring to a dirty pointer value in its any member field. It's not critical, but it makes logical sense in my opinion. Lets swap these two lines in this patch. > return err; Thanks for the patch, -- Rahul Rameshbabu
On 2023/6/30 2:18, Rahul Rameshbabu wrote: > On Thu, 29 Jun, 2023 10:46:41 +0800 Zhengchao Shao <shaozhengchao@huawei.com> wrote: >> The memory pointed to by the fs->any pointer is not freed in the error >> path of mlx5e_fs_tt_redirect_any_create, which can lead to a memory leak. >> Fix by freeing the memory in the error path, thereby making the error path >> identical to mlx5e_fs_tt_redirect_any_destroy(). >> >> Fixes: 0f575c20bf06 ("net/mlx5e: Introduce Flow Steering ANY API") >> Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com> >> --- >> drivers/net/ethernet/mellanox/mlx5/core/en/fs_tt_redirect.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/fs_tt_redirect.c b/drivers/net/ethernet/mellanox/mlx5/core/en/fs_tt_redirect.c >> index 03cb79adf912..9cf4ec931b8b 100644 >> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/fs_tt_redirect.c >> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/fs_tt_redirect.c >> @@ -594,7 +594,7 @@ int mlx5e_fs_tt_redirect_any_create(struct mlx5e_flow_steering *fs) >> >> err = fs_any_create_table(fs); >> if (err) >> - return err; >> + goto err_free_any; >> >> err = fs_any_enable(fs); >> if (err) >> @@ -606,7 +606,7 @@ int mlx5e_fs_tt_redirect_any_create(struct mlx5e_flow_steering *fs) >> >> err_destroy_table: >> fs_any_destroy_table(fs_any); >> - >> +err_free_any: >> kfree(fs_any); >> mlx5e_fs_set_any(fs, NULL); > Hi Rahul: > We probably should update the 'any' member reference in fs to NULL > *before* free-ing fs_any. Otherwise, there is a period in time where fs > is referring to a dirty pointer value in its any member field. It's not > critical, but it makes logical sense in my opinion. Lets swap these two > lines in this patch. > It looks good. I will send V2. Thank you. Zhengchao Shao >> return err; > > Thanks for the patch, > > -- Rahul Rameshbabu
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/fs_tt_redirect.c b/drivers/net/ethernet/mellanox/mlx5/core/en/fs_tt_redirect.c index 03cb79adf912..9cf4ec931b8b 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/en/fs_tt_redirect.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/fs_tt_redirect.c @@ -594,7 +594,7 @@ int mlx5e_fs_tt_redirect_any_create(struct mlx5e_flow_steering *fs) err = fs_any_create_table(fs); if (err) - return err; + goto err_free_any; err = fs_any_enable(fs); if (err) @@ -606,7 +606,7 @@ int mlx5e_fs_tt_redirect_any_create(struct mlx5e_flow_steering *fs) err_destroy_table: fs_any_destroy_table(fs_any); - +err_free_any: kfree(fs_any); mlx5e_fs_set_any(fs, NULL); return err;
The memory pointed to by the fs->any pointer is not freed in the error path of mlx5e_fs_tt_redirect_any_create, which can lead to a memory leak. Fix by freeing the memory in the error path, thereby making the error path identical to mlx5e_fs_tt_redirect_any_destroy(). Fixes: 0f575c20bf06 ("net/mlx5e: Introduce Flow Steering ANY API") Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com> --- drivers/net/ethernet/mellanox/mlx5/core/en/fs_tt_redirect.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)