[PATCH] net: ethernet: mtk_eth_soc: fix memory leak in mtk_ppe_init()

[PATCH] net: ethernet: mtk_eth_soc: fix memory leak in mtk_ppe_init()

[Yan Cangang]

    When dmam_alloc_coherent() or devm_kzalloc() failed, the rhashtable
    ppe->l2_flows isn't destroyed. Fix it.

Fixes: 33fc42de3327 ("net: ethernet: mtk_eth_soc: support creating mac address based offload entries")
Signed-off-by: Yan Cangang <nalanzeyu@gmail.com>
---
 drivers/net/ethernet/mediatek/mtk_ppe.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/mediatek/mtk_ppe.c b/drivers/net/ethernet/mediatek/mtk_ppe.c
index 2d8ca99f2467..8da4c8be59fd 100644
--- a/drivers/net/ethernet/mediatek/mtk_ppe.c
+++ b/drivers/net/ethernet/mediatek/mtk_ppe.c
@@ -737,7 +737,7 @@ struct mtk_ppe *mtk_ppe_init(struct mtk_eth *eth, void __iomem *base,
 				  MTK_PPE_ENTRIES * soc->foe_entry_size,
 				  &ppe->foe_phys, GFP_KERNEL);
 	if (!foe)
-		return NULL;
+		goto err_free_l2_flows;
 
 	ppe->foe_table = foe;
 
@@ -745,11 +745,15 @@ struct mtk_ppe *mtk_ppe_init(struct mtk_eth *eth, void __iomem *base,
 			sizeof(*ppe->foe_flow);
 	ppe->foe_flow = devm_kzalloc(dev, foe_flow_size, GFP_KERNEL);
 	if (!ppe->foe_flow)
-		return NULL;
+		goto err_free_l2_flows;
 
 	mtk_ppe_debugfs_init(ppe, index);
 
 	return ppe;
+
+err_free_l2_flows:
+	rhashtable_destroy(&ppe->l2_flows);
+	return NULL;
 }
 
 static void mtk_ppe_init_foe_table(struct mtk_ppe *ppe)
-- 
2.30.2

Re: [PATCH] net: ethernet: mtk_eth_soc: fix memory leak in mtk_ppe_init()

[Leon Romanovsky]

On Sun, Nov 13, 2022 at 07:32:39AM +0800, Yan Cangang wrote:
>     When dmam_alloc_coherent() or devm_kzalloc() failed, the rhashtable
>     ppe->l2_flows isn't destroyed. Fix it.
 ^^^^^
Please fix indentation in commit message.

> 
> Fixes: 33fc42de3327 ("net: ethernet: mtk_eth_soc: support creating mac address based offload entries")
> Signed-off-by: Yan Cangang <nalanzeyu@gmail.com>
> ---
>  drivers/net/ethernet/mediatek/mtk_ppe.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mediatek/mtk_ppe.c b/drivers/net/ethernet/mediatek/mtk_ppe.c
> index 2d8ca99f2467..8da4c8be59fd 100644
> --- a/drivers/net/ethernet/mediatek/mtk_ppe.c
> +++ b/drivers/net/ethernet/mediatek/mtk_ppe.c
> @@ -737,7 +737,7 @@ struct mtk_ppe *mtk_ppe_init(struct mtk_eth *eth, void __iomem *base,
>  				  MTK_PPE_ENTRIES * soc->foe_entry_size,
>  				  &ppe->foe_phys, GFP_KERNEL);
>  	if (!foe)
> -		return NULL;
> +		goto err_free_l2_flows;
>  
>  	ppe->foe_table = foe;
>  
> @@ -745,11 +745,15 @@ struct mtk_ppe *mtk_ppe_init(struct mtk_eth *eth, void __iomem *base,
>  			sizeof(*ppe->foe_flow);
>  	ppe->foe_flow = devm_kzalloc(dev, foe_flow_size, GFP_KERNEL);
>  	if (!ppe->foe_flow)
> -		return NULL;
> +		goto err_free_l2_flows;
>  
>  	mtk_ppe_debugfs_init(ppe, index);
>  
>  	return ppe;
> +
> +err_free_l2_flows:
> +	rhashtable_destroy(&ppe->l2_flows);

I expect the same change to be in mtk_mdio_cleanup() too.

Thanks


> +	return NULL;
>  }
>  
>  static void mtk_ppe_init_foe_table(struct mtk_ppe *ppe)
> -- 
> 2.30.2
> 

[PATCH v2] net: ethernet: mtk_eth_soc: fix memory leak in error path

[Yan Cangang]

In mtk_ppe_init(), when dmam_alloc_coherent() or devm_kzalloc() failed,
the rhashtable ppe->l2_flows isn't destroyed. Fix it.

In mtk_probe(), when mtk_eth_offload_init() or register_netdev() failed,
have the same problem. Also, call to mtk_mdio_cleanup() is missed in this
case. Fix it.

Fixes: 33fc42de3327 ("net: ethernet: mtk_eth_soc: support creating mac address based offload entries")
Fixes: ba37b7caf1ed ("net: ethernet: mtk_eth_soc: add support for initializing the PPE")
Fixes: 502e84e2382d ("net: ethernet: mtk_eth_soc: add flow offloading support")
Signed-off-by: Yan Cangang <nalanzeyu@gmail.com>
---
v1: https://lore.kernel.org/netdev/20221112233239.824389-1-nalanzeyu@gmail.com/T/
v2:
  - clean up commit message
  - new mtk_ppe_deinit() function, call it before calling mtk_mdio_cleanup()

 drivers/net/ethernet/mediatek/mtk_eth_soc.c |  9 +++++----
 drivers/net/ethernet/mediatek/mtk_ppe.c     | 19 +++++++++++++++++--
 drivers/net/ethernet/mediatek/mtk_ppe.h     |  1 +
 3 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
index 7cd381530aa4..3f806a1358b7 100644
--- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
+++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
@@ -4143,13 +4143,13 @@ static int mtk_probe(struct platform_device *pdev)
 						   eth->soc->offload_version, i);
 			if (!eth->ppe[i]) {
 				err = -ENOMEM;
-				goto err_free_dev;
+				goto err_deinit_ppe;
 			}
 		}
 
 		err = mtk_eth_offload_init(eth);
 		if (err)
-			goto err_free_dev;
+			goto err_deinit_ppe;
 	}
 
 	for (i = 0; i < MTK_MAX_DEVS; i++) {
@@ -4159,7 +4159,7 @@ static int mtk_probe(struct platform_device *pdev)
 		err = register_netdev(eth->netdev[i]);
 		if (err) {
 			dev_err(eth->dev, "error bringing up device\n");
-			goto err_deinit_mdio;
+			goto err_deinit_ppe;
 		} else
 			netif_info(eth, probe, eth->netdev[i],
 				   "mediatek frame engine at 0x%08lx, irq %d\n",
@@ -4177,7 +4177,8 @@ static int mtk_probe(struct platform_device *pdev)
 
 	return 0;
 
-err_deinit_mdio:
+err_deinit_ppe:
+	mtk_ppe_deinit(eth);
 	mtk_mdio_cleanup(eth);
 err_free_dev:
 	mtk_free_dev(eth);
diff --git a/drivers/net/ethernet/mediatek/mtk_ppe.c b/drivers/net/ethernet/mediatek/mtk_ppe.c
index 2d8ca99f2467..784ecb2dc9fb 100644
--- a/drivers/net/ethernet/mediatek/mtk_ppe.c
+++ b/drivers/net/ethernet/mediatek/mtk_ppe.c
@@ -737,7 +737,7 @@ struct mtk_ppe *mtk_ppe_init(struct mtk_eth *eth, void __iomem *base,
 				  MTK_PPE_ENTRIES * soc->foe_entry_size,
 				  &ppe->foe_phys, GFP_KERNEL);
 	if (!foe)
-		return NULL;
+		goto err_free_l2_flows;
 
 	ppe->foe_table = foe;
 
@@ -745,11 +745,26 @@ struct mtk_ppe *mtk_ppe_init(struct mtk_eth *eth, void __iomem *base,
 			sizeof(*ppe->foe_flow);
 	ppe->foe_flow = devm_kzalloc(dev, foe_flow_size, GFP_KERNEL);
 	if (!ppe->foe_flow)
-		return NULL;
+		goto err_free_l2_flows;
 
 	mtk_ppe_debugfs_init(ppe, index);
 
 	return ppe;
+
+err_free_l2_flows:
+	rhashtable_destroy(&ppe->l2_flows);
+	return NULL;
+}
+
+void mtk_ppe_deinit(struct mtk_eth *eth)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(eth->ppe); i++) {
+		if (!eth->ppe[i])
+			return;
+		rhashtable_destroy(&eth->ppe[i]->l2_flows);
+	}
 }
 
 static void mtk_ppe_init_foe_table(struct mtk_ppe *ppe)
diff --git a/drivers/net/ethernet/mediatek/mtk_ppe.h b/drivers/net/ethernet/mediatek/mtk_ppe.h
index 0b7a67a958e4..a09c32539bcc 100644
--- a/drivers/net/ethernet/mediatek/mtk_ppe.h
+++ b/drivers/net/ethernet/mediatek/mtk_ppe.h
@@ -304,6 +304,7 @@ struct mtk_ppe {
 
 struct mtk_ppe *mtk_ppe_init(struct mtk_eth *eth, void __iomem *base,
 			     int version, int index);
+void mtk_ppe_deinit(struct mtk_eth *eth);
 void mtk_ppe_start(struct mtk_ppe *ppe);
 int mtk_ppe_stop(struct mtk_ppe *ppe);
 
-- 
2.30.2

Re: [PATCH v2] net: ethernet: mtk_eth_soc: fix memory leak in error path

[Leon Romanovsky]

On Wed, Nov 16, 2022 at 01:14:07PM +0800, Yan Cangang wrote:
> In mtk_ppe_init(), when dmam_alloc_coherent() or devm_kzalloc() failed,
> the rhashtable ppe->l2_flows isn't destroyed. Fix it.
> 
> In mtk_probe(), when mtk_eth_offload_init() or register_netdev() failed,
> have the same problem. Also, call to mtk_mdio_cleanup() is missed in this
> case. Fix it.
> 
> Fixes: 33fc42de3327 ("net: ethernet: mtk_eth_soc: support creating mac address based offload entries")
> Fixes: ba37b7caf1ed ("net: ethernet: mtk_eth_soc: add support for initializing the PPE")
> Fixes: 502e84e2382d ("net: ethernet: mtk_eth_soc: add flow offloading support")
> Signed-off-by: Yan Cangang <nalanzeyu@gmail.com>
> ---
> v1: https://lore.kernel.org/netdev/20221112233239.824389-1-nalanzeyu@gmail.com/T/
> v2:
>   - clean up commit message
>   - new mtk_ppe_deinit() function, call it before calling mtk_mdio_cleanup()

Subject line should include target [PATCH net v2] ....

> 
>  drivers/net/ethernet/mediatek/mtk_eth_soc.c |  9 +++++----
>  drivers/net/ethernet/mediatek/mtk_ppe.c     | 19 +++++++++++++++++--
>  drivers/net/ethernet/mediatek/mtk_ppe.h     |  1 +
>  3 files changed, 23 insertions(+), 6 deletions(-)

The change seems ok, but please be aware that mtk_eth_offload_init()
calls to rhashtable_init() which is not destroyed too.

Thanks,
Reviewed-by: Leon Romanovsky <leonro@nvidia.com>

Re: [PATCH v2] net: ethernet: mtk_eth_soc: fix memory leak in error path

[Jakub Kicinski]

On Wed, 16 Nov 2022 13:14:07 +0800 Yan Cangang wrote:
> In mtk_ppe_init(), when dmam_alloc_coherent() or devm_kzalloc() failed,
> the rhashtable ppe->l2_flows isn't destroyed. Fix it.
> 
> In mtk_probe(), when mtk_eth_offload_init() or register_netdev() failed,
> have the same problem. Also, call to mtk_mdio_cleanup() is missed in this
> case. Fix it.
> 
> Fixes: 33fc42de3327 ("net: ethernet: mtk_eth_soc: support creating mac address based offload entries")
> Fixes: ba37b7caf1ed ("net: ethernet: mtk_eth_soc: add support for initializing the PPE")
> Fixes: 502e84e2382d ("net: ethernet: mtk_eth_soc: add flow offloading support")
> Signed-off-by: Yan Cangang <nalanzeyu@gmail.com>

Please break this fix up into a series of two patches, first one fixing
the issue in ba37b7caf1ed and 502e84e2382d, and second one fixing the
issue in 33fc42de3327. Those are separate fixes, and should be
backported differently (the former is needed in LTS).

Please repost with the subject changed as Leon suggested and do not
repost in this thread, start a new thread.