| Message ID | 20230907140359.2399646-1-ruanjinjie@huawei.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 281f65d29d6da1a9b6907fb0b145aaf34f4e4822 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: microchip: vcap api: Fix possible memory leak for vcap_dup_rule() | expand |
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)
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!
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)
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(-)