[PATCH net] net: microchip: vcap api: Fix possible memory leak for vcap_dup_rule()

[PATCH net] net: microchip: vcap api: Fix possible memory leak for vcap_dup_rule()

[Jinjie Ruan]

Inject fault When select CONFIG_VCAP_KUNIT_TEST, the below memory leak
occurs. If kzalloc() for duprule succeeds, but the following
kmemdup() fails, the duprule, ckf and caf memory will be leaked. So kfree
them in the error path.

unreferenced object 0xffff122744c50600 (size 192):
  comm "kunit_try_catch", pid 346, jiffies 4294896122 (age 911.812s)
  hex dump (first 32 bytes):
    10 27 00 00 04 00 00 00 1e 00 00 00 2c 01 00 00  .'..........,...
    00 00 00 00 00 00 00 00 18 06 c5 44 27 12 ff ff  ...........D'...
  backtrace:
    [<00000000394b0db8>] __kmem_cache_alloc_node+0x274/0x2f8
    [<0000000001bedc67>] kmalloc_trace+0x38/0x88
    [<00000000b0612f98>] vcap_dup_rule+0x50/0x460
    [<000000005d2d3aca>] vcap_add_rule+0x8cc/0x1038
    [<00000000eef9d0f8>] test_vcap_xn_rule_creator.constprop.0.isra.0+0x238/0x494
    [<00000000cbda607b>] vcap_api_rule_remove_in_front_test+0x1ac/0x698
    [<00000000c8766299>] kunit_try_run_case+0xe0/0x20c
    [<00000000c4fe9186>] kunit_generic_run_threadfn_adapter+0x50/0x94
    [<00000000f6864acf>] kthread+0x2e8/0x374
    [<0000000022e639b3>] ret_from_fork+0x10/0x20

Fixes: 814e7693207f ("net: microchip: vcap api: Add a storage state to a VCAP rule")
Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
 drivers/net/ethernet/microchip/vcap/vcap_api.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/microchip/vcap/vcap_api.c b/drivers/net/ethernet/microchip/vcap/vcap_api.c
index 300fe1a93dce..ef980e4e5bc2 100644
--- a/drivers/net/ethernet/microchip/vcap/vcap_api.c
+++ b/drivers/net/ethernet/microchip/vcap/vcap_api.c
@@ -1021,18 +1021,32 @@ static struct vcap_rule_internal *vcap_dup_rule(struct vcap_rule_internal *ri,
 	list_for_each_entry(ckf, &ri->data.keyfields, ctrl.list) {
 		newckf = kmemdup(ckf, sizeof(*newckf), GFP_KERNEL);
 		if (!newckf)
-			return ERR_PTR(-ENOMEM);
+			goto err;
 		list_add_tail(&newckf->ctrl.list, &duprule->data.keyfields);
 	}
 
 	list_for_each_entry(caf, &ri->data.actionfields, ctrl.list) {
 		newcaf = kmemdup(caf, sizeof(*newcaf), GFP_KERNEL);
 		if (!newcaf)
-			return ERR_PTR(-ENOMEM);
+			goto err;
 		list_add_tail(&newcaf->ctrl.list, &duprule->data.actionfields);
 	}
 
 	return duprule;
+
+err:
+	list_for_each_entry_safe(ckf, newckf, &duprule->data.keyfields, ctrl.list) {
+		list_del(&ckf->ctrl.list);
+		kfree(ckf);
+	}
+
+	list_for_each_entry_safe(caf, newcaf, &duprule->data.actionfields, ctrl.list) {
+		list_del(&caf->ctrl.list);
+		kfree(caf);
+	}
+
+	kfree(duprule);
+	return ERR_PTR(-ENOMEM);
 }
 
 static void vcap_apply_width(u8 *dst, int width, int bytes)
-- 
2.34.1

Re: [PATCH net] net: microchip: vcap api: Fix possible memory leak for vcap_dup_rule()

[Simon Horman]

On Thu, Sep 07, 2023 at 10:03:58PM +0800, Jinjie Ruan wrote:
> Inject fault When select CONFIG_VCAP_KUNIT_TEST, the below memory leak
> occurs. If kzalloc() for duprule succeeds, but the following
> kmemdup() fails, the duprule, ckf and caf memory will be leaked. So kfree
> them in the error path.
> 
> unreferenced object 0xffff122744c50600 (size 192):
>   comm "kunit_try_catch", pid 346, jiffies 4294896122 (age 911.812s)
>   hex dump (first 32 bytes):
>     10 27 00 00 04 00 00 00 1e 00 00 00 2c 01 00 00  .'..........,...
>     00 00 00 00 00 00 00 00 18 06 c5 44 27 12 ff ff  ...........D'...
>   backtrace:
>     [<00000000394b0db8>] __kmem_cache_alloc_node+0x274/0x2f8
>     [<0000000001bedc67>] kmalloc_trace+0x38/0x88
>     [<00000000b0612f98>] vcap_dup_rule+0x50/0x460
>     [<000000005d2d3aca>] vcap_add_rule+0x8cc/0x1038
>     [<00000000eef9d0f8>] test_vcap_xn_rule_creator.constprop.0.isra.0+0x238/0x494
>     [<00000000cbda607b>] vcap_api_rule_remove_in_front_test+0x1ac/0x698
>     [<00000000c8766299>] kunit_try_run_case+0xe0/0x20c
>     [<00000000c4fe9186>] kunit_generic_run_threadfn_adapter+0x50/0x94
>     [<00000000f6864acf>] kthread+0x2e8/0x374
>     [<0000000022e639b3>] ret_from_fork+0x10/0x20
> 
> Fixes: 814e7693207f ("net: microchip: vcap api: Add a storage state to a VCAP rule")
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> ---
>  drivers/net/ethernet/microchip/vcap/vcap_api.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/microchip/vcap/vcap_api.c b/drivers/net/ethernet/microchip/vcap/vcap_api.c
> index 300fe1a93dce..ef980e4e5bc2 100644
> --- a/drivers/net/ethernet/microchip/vcap/vcap_api.c
> +++ b/drivers/net/ethernet/microchip/vcap/vcap_api.c
> @@ -1021,18 +1021,32 @@ static struct vcap_rule_internal *vcap_dup_rule(struct vcap_rule_internal *ri,
>  	list_for_each_entry(ckf, &ri->data.keyfields, ctrl.list) {
>  		newckf = kmemdup(ckf, sizeof(*newckf), GFP_KERNEL);
>  		if (!newckf)
> -			return ERR_PTR(-ENOMEM);
> +			goto err;
>  		list_add_tail(&newckf->ctrl.list, &duprule->data.keyfields);
>  	}
>  
>  	list_for_each_entry(caf, &ri->data.actionfields, ctrl.list) {
>  		newcaf = kmemdup(caf, sizeof(*newcaf), GFP_KERNEL);
>  		if (!newcaf)
> -			return ERR_PTR(-ENOMEM);
> +			goto err;
>  		list_add_tail(&newcaf->ctrl.list, &duprule->data.actionfields);
>  	}
>  
>  	return duprule;
> +
> +err:

Hi Jinjie Ruan,

I think it would be slightly more idiomatic, and in keeping with the
prevailing style of this file to call the label out_free. But I don't feel
strongly about that. And clearly it would have no effect on the logic.

That not withstanding, this looks good to me.

Reviewed-by: Simon Horman <horms@kernel.org>

> +	list_for_each_entry_safe(ckf, newckf, &duprule->data.keyfields, ctrl.list) {
> +		list_del(&ckf->ctrl.list);
> +		kfree(ckf);
> +	}
> +
> +	list_for_each_entry_safe(caf, newcaf, &duprule->data.actionfields, ctrl.list) {
> +		list_del(&caf->ctrl.list);
> +		kfree(caf);
> +	}
> +
> +	kfree(duprule);
> +	return ERR_PTR(-ENOMEM);
>  }
>  
>  static void vcap_apply_width(u8 *dst, int width, int bytes)

Re: [PATCH net] net: microchip: vcap api: Fix possible memory leak for vcap_dup_rule()

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by David S. Miller <davem@davemloft.net>:

On Thu, 7 Sep 2023 22:03:58 +0800 you wrote:
> Inject fault When select CONFIG_VCAP_KUNIT_TEST, the below memory leak
> occurs. If kzalloc() for duprule succeeds, but the following
> kmemdup() fails, the duprule, ckf and caf memory will be leaked. So kfree
> them in the error path.
> 
> unreferenced object 0xffff122744c50600 (size 192):
>   comm "kunit_try_catch", pid 346, jiffies 4294896122 (age 911.812s)
>   hex dump (first 32 bytes):
>     10 27 00 00 04 00 00 00 1e 00 00 00 2c 01 00 00  .'..........,...
>     00 00 00 00 00 00 00 00 18 06 c5 44 27 12 ff ff  ...........D'...
>   backtrace:
>     [<00000000394b0db8>] __kmem_cache_alloc_node+0x274/0x2f8
>     [<0000000001bedc67>] kmalloc_trace+0x38/0x88
>     [<00000000b0612f98>] vcap_dup_rule+0x50/0x460
>     [<000000005d2d3aca>] vcap_add_rule+0x8cc/0x1038
>     [<00000000eef9d0f8>] test_vcap_xn_rule_creator.constprop.0.isra.0+0x238/0x494
>     [<00000000cbda607b>] vcap_api_rule_remove_in_front_test+0x1ac/0x698
>     [<00000000c8766299>] kunit_try_run_case+0xe0/0x20c
>     [<00000000c4fe9186>] kunit_generic_run_threadfn_adapter+0x50/0x94
>     [<00000000f6864acf>] kthread+0x2e8/0x374
>     [<0000000022e639b3>] ret_from_fork+0x10/0x20
> 
> [...]

Here is the summary with links:
  - [net] net: microchip: vcap api: Fix possible memory leak for vcap_dup_rule()
    https://git.kernel.org/netdev/net/c/281f65d29d6d

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html