2019-03-22 11:01:07

by Chengguang Xu

Subject: [PATCH net] netlink: fix a memory leak on error path

In genl_register_family(), when idr_alloc() fails,
we forget to free family->attrbuf.

Note, some callers call genl_unregister_family() to clean up
on error path, it doesn't work because the family is inserted
to the global list in the nearly last step.

Fixes: 2ae0f17df1c ("genetlink: use idr to track families").
Signed-off-by: Chengguang Xu <[email protected]>
---
net/netlink/genetlink.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index 25eeb6d2a75a..f0ec068e1d02 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -366,7 +366,7 @@ int genl_register_family(struct genl_family *family)
start, end + 1, GFP_KERNEL);
if (family->id < 0) {
err = family->id;
- goto errout_locked;
+ goto errout_free;
}

err = genl_validate_assign_mc_groups(family);
@@ -385,6 +385,7 @@ int genl_register_family(struct genl_family *family)

errout_remove:
idr_remove(&genl_fam_idr, family->id);
+errout_free:
kfree(family->attrbuf);
errout_locked:
genl_unlock_all();
--
2.20.1

2019-03-22 11:04:51

by Kirill Tkhai

Subject: Re: [PATCH net] netlink: fix a memory leak on error path

Hi, Chengguang,

On 22.03.2019 13:58, Chengguang Xu wrote:
> In genl_register_family(), when idr_alloc() fails,
> we forget to free family->attrbuf.
>
> Note, some callers call genl_unregister_family() to clean up
> on error path, it doesn't work because the family is inserted
> to the global list in the nearly last step.
>
> Fixes: 2ae0f17df1c ("genetlink: use idr to track families").
> Signed-off-by: Chengguang Xu <[email protected]>

have you seen https://patchwork.ozlabs.org/patch/1059834/ ?

Kirill
> ---
> net/netlink/genetlink.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
> index 25eeb6d2a75a..f0ec068e1d02 100644
> --- a/net/netlink/genetlink.c
> +++ b/net/netlink/genetlink.c
> @@ -366,7 +366,7 @@ int genl_register_family(struct genl_family *family)
> start, end + 1, GFP_KERNEL);
> if (family->id < 0) {
> err = family->id;
> - goto errout_locked;
> + goto errout_free;
> }
>
> err = genl_validate_assign_mc_groups(family);
> @@ -385,6 +385,7 @@ int genl_register_family(struct genl_family *family)
>
> errout_remove:
> idr_remove(&genl_fam_idr, family->id);
> +errout_free:
> kfree(family->attrbuf);
> errout_locked:
> genl_unlock_all();
> --
> 2.20.1
>

2019-03-25 01:20:18

by Chengguang Xu

Subject: Re: [PATCH net] netlink: fix a memory leak on error path

On 3/22/19 7:03 PM, Kirill Tkhai wrote:
> Hi, Chengguang,
>
> On 22.03.2019 13:58, Chengguang Xu wrote:
>> In genl_register_family(), when idr_alloc() fails,
>> we forget to free family->attrbuf.
>>
>> Note, some callers call genl_unregister_family() to clean up
>> on error path, it doesn't work because the family is inserted
>> to the global list in the nearly last step.
>>
>> Fixes: 2ae0f17df1c ("genetlink: use idr to track families").
>> Signed-off-by: Chengguang Xu <[email protected]>
> have you seen https://patchwork.ozlabs.org/patch/1059834/ ?

Sorry, I didn't notice that because I'm mainly working on linus-tree.
Please just ignore this patch.

Thanks.

2019-03-25 08:13:44

by Kirill Tkhai

Subject: Re: [PATCH net] netlink: fix a memory leak on error path

On 25.03.2019 04:19, cgxu519 wrote:
> On 3/22/19 7:03 PM, Kirill Tkhai wrote:
>> Hi, Chengguang,
>>
>> On 22.03.2019 13:58, Chengguang Xu wrote:
>>> In genl_register_family(), when idr_alloc() fails,
>>> we forget to free family->attrbuf.
>>>
>>> Note, some callers call genl_unregister_family() to clean up
>>> on error path, it doesn't work because the family is inserted
>>> to the global list in the nearly last step.
>>>
>>> Fixes: 2ae0f17df1c ("genetlink: use idr to track families").
>>> Signed-off-by: Chengguang Xu <[email protected]>
>> have you seen https://patchwork.ozlabs.org/patch/1059834/ ?
>
> Sorry, I didn't notice that because I'm mainly working on linus-tree.
> Please just ignore this patch.

Not a problem.

Some information about network development process you may find in

https://www.kernel.org/doc/Documentation/networking/netdev-FAQ.txt

Kirill