[PATCH net-next] net/mlx5: Fix memory leak in IPsec RoCE creation

[PATCH net-next] net/mlx5: Fix memory leak in IPsec RoCE creation

[Leon Romanovsky]

From: Patrisious Haddad <phaddad@nvidia.com>

During IPsec RoCE TX creation a struct for the flow group creation is
allocated, but never freed. Free that struct once it is no longer in use.

Fixes: 22551e77e550 ("net/mlx5: Configure IPsec steering for egress RoCEv2 traffic")
Signed-off-by: Patrisious Haddad <phaddad@nvidia.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
 .../ethernet/mellanox/mlx5/core/lib/ipsec_fs_roce.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/ipsec_fs_roce.c b/drivers/net/ethernet/mellanox/mlx5/core/lib/ipsec_fs_roce.c
index 2c53589b765d..edc6798b359e 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/lib/ipsec_fs_roce.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/ipsec_fs_roce.c
@@ -162,7 +162,7 @@ int mlx5_ipsec_fs_roce_tx_create(struct mlx5_core_dev *mdev,
 	if (IS_ERR(ft)) {
 		err = PTR_ERR(ft);
 		mlx5_core_err(mdev, "Fail to create RoCE IPsec tx ft err=%d\n", err);
-		return err;
+		goto fail_table;
 	}
 
 	roce->ft = ft;
@@ -174,22 +174,25 @@ int mlx5_ipsec_fs_roce_tx_create(struct mlx5_core_dev *mdev,
 	if (IS_ERR(g)) {
 		err = PTR_ERR(g);
 		mlx5_core_err(mdev, "Fail to create RoCE IPsec tx group err=%d\n", err);
-		goto fail;
+		goto fail_group;
 	}
 	roce->g = g;
 
 	err = ipsec_fs_roce_tx_rule_setup(mdev, roce, pol_ft);
 	if (err) {
 		mlx5_core_err(mdev, "Fail to create RoCE IPsec tx rules err=%d\n", err);
-		goto rule_fail;
+		goto fail_rule;
 	}
 
+	kvfree(in);
 	return 0;
 
-rule_fail:
+fail_rule:
 	mlx5_destroy_flow_group(roce->g);
-fail:
+fail_group:
 	mlx5_destroy_flow_table(ft);
+fail_table:
+	kvfree(in);
 	return err;
 }
 
-- 
2.39.2

Re: [PATCH net-next] net/mlx5: Fix memory leak in IPsec RoCE creation

[Jakub Kicinski]

On Sun, 19 Feb 2023 14:59:57 +0200 Leon Romanovsky wrote:
> -rule_fail:
> +fail_rule:
>  	mlx5_destroy_flow_group(roce->g);
> -fail:
> +fail_group:
>  	mlx5_destroy_flow_table(ft);
> +fail_table:
> +	kvfree(in);
>  	return err;

If you're touching all of them please name them after what they do.
Much easier to review.

Re: [PATCH net-next] net/mlx5: Fix memory leak in IPsec RoCE creation

[Leon Romanovsky]

On Mon, Feb 20, 2023 at 04:50:00PM -0800, Jakub Kicinski wrote:
> On Sun, 19 Feb 2023 14:59:57 +0200 Leon Romanovsky wrote:
> > -rule_fail:
> > +fail_rule:
> >  	mlx5_destroy_flow_group(roce->g);
> > -fail:
> > +fail_group:
> >  	mlx5_destroy_flow_table(ft);
> > +fail_table:
> > +	kvfree(in);
> >  	return err;
> 
> If you're touching all of them please name them after what they do.
> Much easier to review.

I can change it, but all mlx* drivers and randomly chosen place in ice
use label to show what fail and not what will be done. Such notation
gives an ability to refactor code without changing label names if
failed part of code is not removed.

Thanks

Re: [PATCH net-next] net/mlx5: Fix memory leak in IPsec RoCE creation

[Jakub Kicinski]

On Tue, 21 Feb 2023 08:39:40 +0200 Leon Romanovsky wrote:
> On Mon, Feb 20, 2023 at 04:50:00PM -0800, Jakub Kicinski wrote:
> > On Sun, 19 Feb 2023 14:59:57 +0200 Leon Romanovsky wrote:  
> > > -rule_fail:
> > > +fail_rule:
> > >  	mlx5_destroy_flow_group(roce->g);
> > > -fail:
> > > +fail_group:
> > >  	mlx5_destroy_flow_table(ft);
> > > +fail_table:
> > > +	kvfree(in);
> > >  	return err;  
> > 
> > If you're touching all of them please name them after what they do.
> > Much easier to review.  
> 
> I can change it, but all mlx* drivers and randomly chosen place in ice
> use label to show what fail and not what will be done. Such notation
> gives an ability to refactor code without changing label names if
> failed part of code is not removed.

Please refactor, and it'd be great if the convention was changed for
all new code in these drivers.