* [PATCH] net-sysfs: Fix reference count leak in rx|netdev_queue_add_kobject
@ 2019-11-19 9:51 jouni.hogander
2019-11-20 1:13 ` David Miller
0 siblings, 1 reply; 6+ messages in thread
From: jouni.hogander @ 2019-11-19 9:51 UTC (permalink / raw)
To: netdev; +Cc: Jouni Hogander, David Miller, Lukas Bulwahn
From: Jouni Hogander <jouni.hogander@unikie.com>
kobject_init_and_add takes reference even when it fails. This has
to be given up by the caller in error handling. Otherwise memory
allocated by kobject_init_and_add is never freed.
Cc: David Miller <davem@davemloft.net>
Cc: Lukas Bulwahn <lukas.bulwahn@gmail.com>
Signed-off-by: Jouni Hogander <jouni.hogander@unikie.com>
---
net/core/net-sysfs.c | 24 +++++++++++++-----------
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index 865ba6ca16eb..4f404bf33e44 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -923,21 +923,23 @@ static int rx_queue_add_kobject(struct net_device *dev, int index)
error = kobject_init_and_add(kobj, &rx_queue_ktype, NULL,
"rx-%u", index);
if (error)
- return error;
+ goto err;
dev_hold(queue->dev);
if (dev->sysfs_rx_queue_group) {
error = sysfs_create_group(kobj, dev->sysfs_rx_queue_group);
- if (error) {
- kobject_put(kobj);
- return error;
- }
+ if (error)
+ goto err;
}
kobject_uevent(kobj, KOBJ_ADD);
return error;
+
+err:
+ kobject_put(kobj);
+ return error;
}
#endif /* CONFIG_SYSFS */
@@ -1461,21 +1463,21 @@ static int netdev_queue_add_kobject(struct net_device *dev, int index)
error = kobject_init_and_add(kobj, &netdev_queue_ktype, NULL,
"tx-%u", index);
if (error)
- return error;
+ goto err;
dev_hold(queue->dev);
#ifdef CONFIG_BQL
error = sysfs_create_group(kobj, &dql_group);
- if (error) {
- kobject_put(kobj);
- return error;
- }
+ if (error)
+ goto err;
#endif
kobject_uevent(kobj, KOBJ_ADD);
- return 0;
+err:
+ kobject_put(kobj);
+ return error;
}
#endif /* CONFIG_SYSFS */
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] net-sysfs: Fix reference count leak in rx|netdev_queue_add_kobject
2019-11-19 9:51 [PATCH] net-sysfs: Fix reference count leak in rx|netdev_queue_add_kobject jouni.hogander
@ 2019-11-20 1:13 ` David Miller
2019-11-20 6:59 ` Jouni Högander
0 siblings, 1 reply; 6+ messages in thread
From: David Miller @ 2019-11-20 1:13 UTC (permalink / raw)
To: jouni.hogander; +Cc: netdev, lukas.bulwahn
From: jouni.hogander@unikie.com
Date: Tue, 19 Nov 2019 11:51:21 +0200
> kobject_init_and_add takes reference even when it fails.
I see this in the comment above kobject_init_and_add() but not in the
code.
Where does the implementation of kobject_init_and_add() actually take
such a reference?
Did you discover this by code inspection, or by an actual bug that was
triggered? If by an actual bug, please provide the OOPS and/or checker
trace that indicated a leak was happening here.
I don't see anything actually wrong here because kobject_init_and_add()
doesn't actually seem to do what it's comment suggests...
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net-sysfs: Fix reference count leak in rx|netdev_queue_add_kobject
2019-11-20 1:13 ` David Miller
@ 2019-11-20 6:59 ` Jouni Högander
0 siblings, 0 replies; 6+ messages in thread
From: Jouni Högander @ 2019-11-20 6:59 UTC (permalink / raw)
To: David Miller; +Cc: netdev, lukas.bulwahn
David Miller <davem@davemloft.net> writes:
> From: jouni.hogander@unikie.com
> Date: Tue, 19 Nov 2019 11:51:21 +0200
>
>> kobject_init_and_add takes reference even when it fails.
>
> I see this in the comment above kobject_init_and_add() but not in the
> code.
>
> Where does the implementation of kobject_init_and_add() actually take
> such a reference?
kobject_init_and_add -> kobject_init -> kobject_init_internal -> kref_init
kref_init initializes ref_count as 1.
>
> Did you discover this by code inspection, or by an actual bug that was
> triggered? If by an actual bug, please provide the OOPS and/or checker
> trace that indicated a leak was happening here.
Originally found it via memory leak identified by Syzkaller. I will
submit new one with memleak dump.
>
> I don't see anything actually wrong here because kobject_init_and_add()
> doesn't actually seem to do what it's comment suggests...
See the code path above.
BR,
Jouni Högander
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net-sysfs: Fix reference count leak in rx|netdev_queue_add_kobject
2019-11-20 20:10 ` David Miller
@ 2019-11-21 3:12 ` Eric Dumazet
0 siblings, 0 replies; 6+ messages in thread
From: Eric Dumazet @ 2019-11-21 3:12 UTC (permalink / raw)
To: David Miller, jouni.hogander; +Cc: netdev, lukas.bulwahn
On 11/20/19 12:10 PM, David Miller wrote:
> From: jouni.hogander@unikie.com
> Date: Wed, 20 Nov 2019 09:08:16 +0200
>
>> From: Jouni Hogander <jouni.hogander@unikie.com>
>>
>> kobject_init_and_add takes reference even when it fails. This has
>> to be given up by the caller in error handling. Otherwise memory
>> allocated by kobject_init_and_add is never freed. Originally found
>> by Syzkaller:
>>
>> BUG: memory leak
> ...
>> Signed-off-by: Jouni Hogander <jouni.hogander@unikie.com>
>
> Applied, thank you.
>
I am late to the party, sorry.
But I do not see a Fixes: tag, and the patch seems buggy.
I question how it was really tested ? Please Jouni share your drugs :)
I would rather see this stuff for net-next, because whatever memory leak
we had for years has not raised serious concerns.
I will submit this fix, I guess...
diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index 4f404bf33e44c977c1a8ba00706817b76215b75c..ae3bcb1540ec57df311dac6847323a23a74ec960 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -1474,6 +1474,7 @@ static int netdev_queue_add_kobject(struct net_device *dev, int index)
#endif
kobject_uevent(kobj, KOBJ_ADD);
+ return 0;
err:
kobject_put(kobj);
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] net-sysfs: Fix reference count leak in rx|netdev_queue_add_kobject
2019-11-20 7:08 jouni.hogander
@ 2019-11-20 20:10 ` David Miller
2019-11-21 3:12 ` Eric Dumazet
0 siblings, 1 reply; 6+ messages in thread
From: David Miller @ 2019-11-20 20:10 UTC (permalink / raw)
To: jouni.hogander; +Cc: netdev, lukas.bulwahn
From: jouni.hogander@unikie.com
Date: Wed, 20 Nov 2019 09:08:16 +0200
> From: Jouni Hogander <jouni.hogander@unikie.com>
>
> kobject_init_and_add takes reference even when it fails. This has
> to be given up by the caller in error handling. Otherwise memory
> allocated by kobject_init_and_add is never freed. Originally found
> by Syzkaller:
>
> BUG: memory leak
...
> Signed-off-by: Jouni Hogander <jouni.hogander@unikie.com>
Applied, thank you.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] net-sysfs: Fix reference count leak in rx|netdev_queue_add_kobject
@ 2019-11-20 7:08 jouni.hogander
2019-11-20 20:10 ` David Miller
0 siblings, 1 reply; 6+ messages in thread
From: jouni.hogander @ 2019-11-20 7:08 UTC (permalink / raw)
To: netdev; +Cc: Jouni Hogander, David Miller, Lukas Bulwahn
From: Jouni Hogander <jouni.hogander@unikie.com>
kobject_init_and_add takes reference even when it fails. This has
to be given up by the caller in error handling. Otherwise memory
allocated by kobject_init_and_add is never freed. Originally found
by Syzkaller:
BUG: memory leak
unreferenced object 0xffff8880679f8b08 (size 8):
comm "netdev_register", pid 269, jiffies 4294693094 (age 12.132s)
hex dump (first 8 bytes):
72 78 2d 30 00 36 20 d4 rx-0.6 .
backtrace:
[<000000008c93818e>] __kmalloc_track_caller+0x16e/0x290
[<000000001f2e4e49>] kvasprintf+0xb1/0x140
[<000000007f313394>] kvasprintf_const+0x56/0x160
[<00000000aeca11c8>] kobject_set_name_vargs+0x5b/0x140
[<0000000073a0367c>] kobject_init_and_add+0xd8/0x170
[<0000000088838e4b>] net_rx_queue_update_kobjects+0x152/0x560
[<000000006be5f104>] netdev_register_kobject+0x210/0x380
[<00000000e31dab9d>] register_netdevice+0xa1b/0xf00
[<00000000f68b2465>] __tun_chr_ioctl+0x20d5/0x3dd0
[<000000004c50599f>] tun_chr_ioctl+0x2f/0x40
[<00000000bbd4c317>] do_vfs_ioctl+0x1c7/0x1510
[<00000000d4c59e8f>] ksys_ioctl+0x99/0xb0
[<00000000946aea81>] __x64_sys_ioctl+0x78/0xb0
[<0000000038d946e5>] do_syscall_64+0x16f/0x580
[<00000000e0aa5d8f>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
[<00000000285b3d1a>] 0xffffffffffffffff
Cc: David Miller <davem@davemloft.net>
Cc: Lukas Bulwahn <lukas.bulwahn@gmail.com>
Signed-off-by: Jouni Hogander <jouni.hogander@unikie.com>
---
net/core/net-sysfs.c | 24 +++++++++++++-----------
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index 865ba6ca16eb..4f404bf33e44 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -923,21 +923,23 @@ static int rx_queue_add_kobject(struct net_device *dev, int index)
error = kobject_init_and_add(kobj, &rx_queue_ktype, NULL,
"rx-%u", index);
if (error)
- return error;
+ goto err;
dev_hold(queue->dev);
if (dev->sysfs_rx_queue_group) {
error = sysfs_create_group(kobj, dev->sysfs_rx_queue_group);
- if (error) {
- kobject_put(kobj);
- return error;
- }
+ if (error)
+ goto err;
}
kobject_uevent(kobj, KOBJ_ADD);
return error;
+
+err:
+ kobject_put(kobj);
+ return error;
}
#endif /* CONFIG_SYSFS */
@@ -1461,21 +1463,21 @@ static int netdev_queue_add_kobject(struct net_device *dev, int index)
error = kobject_init_and_add(kobj, &netdev_queue_ktype, NULL,
"tx-%u", index);
if (error)
- return error;
+ goto err;
dev_hold(queue->dev);
#ifdef CONFIG_BQL
error = sysfs_create_group(kobj, &dql_group);
- if (error) {
- kobject_put(kobj);
- return error;
- }
+ if (error)
+ goto err;
#endif
kobject_uevent(kobj, KOBJ_ADD);
- return 0;
+err:
+ kobject_put(kobj);
+ return error;
}
#endif /* CONFIG_SYSFS */
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-11-21 3:12 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-19 9:51 [PATCH] net-sysfs: Fix reference count leak in rx|netdev_queue_add_kobject jouni.hogander
2019-11-20 1:13 ` David Miller
2019-11-20 6:59 ` Jouni Högander
2019-11-20 7:08 jouni.hogander
2019-11-20 20:10 ` David Miller
2019-11-21 3:12 ` Eric Dumazet
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).