* [PATCH v3] net-sysfs: Fix reference count leak
@ 2019-11-18 12:32 jouni.hogander
2019-11-18 13:27 ` Greg Kroah-Hartman
0 siblings, 1 reply; 7+ messages in thread
From: jouni.hogander @ 2019-11-18 12:32 UTC (permalink / raw)
To: netdev; +Cc: Jouni Hogander, David Miller, Greg Kroah-Hartman, Lukas Bulwahn
From: Jouni Hogander <jouni.hogander@unikie.com>
Netdev_register_kobject is calling device_initialize. In case of error
reference taken by device_initialize is not given up.
Drivers are supposed to call free_netdev in case of error. In non-error
case the last reference is given up there and device release sequence
is triggered. In error case this reference is kept and the release
sequence is never started.
Fix this reference count leak by allowing giving up the reference also
in error case in free_netdev.
Also replace BUG_ON with WARN_ON in free_netdev and in netdev_release.
This is the rootcause for couple of memory leaks reported by Syzkaller:
BUG: memory leak unreferenced object 0xffff8880675ca008 (size 256):
comm "netdev_register", pid 281, jiffies 4294696663 (age 6.808s)
hex dump (first 32 bytes):
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:
[<0000000058ca4711>] kmem_cache_alloc_trace+0x167/0x280
[<000000002340019b>] device_add+0x882/0x1750
[<000000001d588c3a>] netdev_register_kobject+0x128/0x380
[<0000000011ef5535>] register_netdevice+0xa1b/0xf00
[<000000007fcf1c99>] __tun_chr_ioctl+0x20d5/0x3dd0
[<000000006a5b7b2b>] tun_chr_ioctl+0x2f/0x40
[<00000000f30f834a>] do_vfs_ioctl+0x1c7/0x1510
[<00000000fba062ea>] ksys_ioctl+0x99/0xb0
[<00000000b1c1b8d2>] __x64_sys_ioctl+0x78/0xb0
[<00000000984cabb9>] do_syscall_64+0x16f/0x580
[<000000000bde033d>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
[<00000000e6ca2d9f>] 0xffffffffffffffff
BUG: memory leak
unreferenced object 0xffff8880668ba588 (size 8):
comm "kobject_set_nam", pid 286, jiffies 4294725297 (age 9.871s)
hex dump (first 8 bytes):
6e 72 30 00 cc be df 2b nr0....+
backtrace:
[<00000000a322332a>] __kmalloc_track_caller+0x16e/0x290
[<00000000236fd26b>] kstrdup+0x3e/0x70
[<00000000dd4a2815>] kstrdup_const+0x3e/0x50
[<0000000049a377fc>] kvasprintf_const+0x10e/0x160
[<00000000627fc711>] kobject_set_name_vargs+0x5b/0x140
[<0000000019eeab06>] dev_set_name+0xc0/0xf0
[<0000000069cb12bc>] netdev_register_kobject+0xc8/0x320
[<00000000f2e83732>] register_netdevice+0xa1b/0xf00
[<000000009e1f57cc>] __tun_chr_ioctl+0x20d5/0x3dd0
[<000000009c560784>] tun_chr_ioctl+0x2f/0x40
[<000000000d759e02>] do_vfs_ioctl+0x1c7/0x1510
[<00000000351d7c31>] ksys_ioctl+0x99/0xb0
[<000000008390040a>] __x64_sys_ioctl+0x78/0xb0
[<0000000052d196b7>] do_syscall_64+0x16f/0x580
[<0000000019af9236>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
[<00000000bc384531>] 0xffffffffffffffff
Reported-by: syzbot+ad8ca40ecd77896d51e2@syzkaller.appspotmail.com
Cc: David Miller <davem@davemloft.net>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Lukas Bulwahn <lukas.bulwahn@gmail.com>
Signed-off-by: Jouni Hogander <jouni.hogander@unikie.com>
---
v2 -> v3:
* Replaced BUG_ON with WARN_ON in free_netdev and netdev_release
v1 -> v2:
* Relying on driver calling free_netdev rather than calling
put_device directly in error path
---
net/core/dev.c | 14 +++++++-------
net/core/net-sysfs.c | 6 +++++-
2 files changed, 12 insertions(+), 8 deletions(-)
diff --git a/net/core/dev.c b/net/core/dev.c
index 99ac84ff398f..1d6c0bfb5ec5 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -9603,14 +9603,14 @@ void free_netdev(struct net_device *dev)
netdev_unregister_lockdep_key(dev);
- /* Compatibility with error handling in drivers */
- if (dev->reg_state == NETREG_UNINITIALIZED) {
- netdev_freemem(dev);
- return;
- }
+ /* reg_state is NETREG_UNINITIALIZED if there is an error in
+ * registration.
+ */
+ WARN_ON(dev->reg_state != NETREG_UNREGISTERED &&
+ dev->reg_state != NETREG_UNINITIALIZED);
- BUG_ON(dev->reg_state != NETREG_UNREGISTERED);
- dev->reg_state = NETREG_RELEASED;
+ if (dev->reg_state != NETREG_UNINITIALIZED)
+ dev->reg_state = NETREG_RELEASED;
/* will free via device release */
put_device(&dev->dev);
diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index 865ba6ca16eb..11f1e3b4b18f 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -1626,7 +1626,11 @@ static void netdev_release(struct device *d)
{
struct net_device *dev = to_net_dev(d);
- BUG_ON(dev->reg_state != NETREG_RELEASED);
+ /* reg_state is NETREG_UNINITIALIZED if there is an error in
+ * registration.
+ */
+ WARN_ON(dev->reg_state != NETREG_RELEASED &&
+ dev->reg_state != NETREG_UNINITIALIZED);
/* no need to wait for rcu grace period:
* device is dead and about to be freed.
--
2.17.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH v3] net-sysfs: Fix reference count leak
2019-11-18 12:32 [PATCH v3] net-sysfs: Fix reference count leak jouni.hogander
@ 2019-11-18 13:27 ` Greg Kroah-Hartman
2019-11-19 7:45 ` Jouni Högander
0 siblings, 1 reply; 7+ messages in thread
From: Greg Kroah-Hartman @ 2019-11-18 13:27 UTC (permalink / raw)
To: jouni.hogander; +Cc: netdev, David Miller, Lukas Bulwahn
On Mon, Nov 18, 2019 at 02:32:46PM +0200, jouni.hogander@unikie.com wrote:
> From: Jouni Hogander <jouni.hogander@unikie.com>
>
> Netdev_register_kobject is calling device_initialize. In case of error
> reference taken by device_initialize is not given up.
>
> Drivers are supposed to call free_netdev in case of error. In non-error
> case the last reference is given up there and device release sequence
> is triggered. In error case this reference is kept and the release
> sequence is never started.
>
> Fix this reference count leak by allowing giving up the reference also
> in error case in free_netdev.
>
> Also replace BUG_ON with WARN_ON in free_netdev and in netdev_release.
>
> This is the rootcause for couple of memory leaks reported by Syzkaller:
>
> BUG: memory leak unreferenced object 0xffff8880675ca008 (size 256):
> comm "netdev_register", pid 281, jiffies 4294696663 (age 6.808s)
> hex dump (first 32 bytes):
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> backtrace:
> [<0000000058ca4711>] kmem_cache_alloc_trace+0x167/0x280
> [<000000002340019b>] device_add+0x882/0x1750
> [<000000001d588c3a>] netdev_register_kobject+0x128/0x380
> [<0000000011ef5535>] register_netdevice+0xa1b/0xf00
> [<000000007fcf1c99>] __tun_chr_ioctl+0x20d5/0x3dd0
> [<000000006a5b7b2b>] tun_chr_ioctl+0x2f/0x40
> [<00000000f30f834a>] do_vfs_ioctl+0x1c7/0x1510
> [<00000000fba062ea>] ksys_ioctl+0x99/0xb0
> [<00000000b1c1b8d2>] __x64_sys_ioctl+0x78/0xb0
> [<00000000984cabb9>] do_syscall_64+0x16f/0x580
> [<000000000bde033d>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [<00000000e6ca2d9f>] 0xffffffffffffffff
>
> BUG: memory leak
> unreferenced object 0xffff8880668ba588 (size 8):
> comm "kobject_set_nam", pid 286, jiffies 4294725297 (age 9.871s)
> hex dump (first 8 bytes):
> 6e 72 30 00 cc be df 2b nr0....+
> backtrace:
> [<00000000a322332a>] __kmalloc_track_caller+0x16e/0x290
> [<00000000236fd26b>] kstrdup+0x3e/0x70
> [<00000000dd4a2815>] kstrdup_const+0x3e/0x50
> [<0000000049a377fc>] kvasprintf_const+0x10e/0x160
> [<00000000627fc711>] kobject_set_name_vargs+0x5b/0x140
> [<0000000019eeab06>] dev_set_name+0xc0/0xf0
> [<0000000069cb12bc>] netdev_register_kobject+0xc8/0x320
> [<00000000f2e83732>] register_netdevice+0xa1b/0xf00
> [<000000009e1f57cc>] __tun_chr_ioctl+0x20d5/0x3dd0
> [<000000009c560784>] tun_chr_ioctl+0x2f/0x40
> [<000000000d759e02>] do_vfs_ioctl+0x1c7/0x1510
> [<00000000351d7c31>] ksys_ioctl+0x99/0xb0
> [<000000008390040a>] __x64_sys_ioctl+0x78/0xb0
> [<0000000052d196b7>] do_syscall_64+0x16f/0x580
> [<0000000019af9236>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [<00000000bc384531>] 0xffffffffffffffff
>
> Reported-by: syzbot+ad8ca40ecd77896d51e2@syzkaller.appspotmail.com
> Cc: David Miller <davem@davemloft.net>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> Signed-off-by: Jouni Hogander <jouni.hogander@unikie.com>
> ---
> v2 -> v3:
> * Replaced BUG_ON with WARN_ON in free_netdev and netdev_release
> v1 -> v2:
> * Relying on driver calling free_netdev rather than calling
> put_device directly in error path
> ---
> net/core/dev.c | 14 +++++++-------
> net/core/net-sysfs.c | 6 +++++-
> 2 files changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 99ac84ff398f..1d6c0bfb5ec5 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -9603,14 +9603,14 @@ void free_netdev(struct net_device *dev)
>
> netdev_unregister_lockdep_key(dev);
>
> - /* Compatibility with error handling in drivers */
> - if (dev->reg_state == NETREG_UNINITIALIZED) {
> - netdev_freemem(dev);
> - return;
> - }
> + /* reg_state is NETREG_UNINITIALIZED if there is an error in
> + * registration.
> + */
> + WARN_ON(dev->reg_state != NETREG_UNREGISTERED &&
> + dev->reg_state != NETREG_UNINITIALIZED);
You "warn" about this, but do not actually handle the problem. So what
is this helping with?
Systems with panic-on-warn just rebooted, and a "normal" system saw a
traceback yet no error handling happened so why would the code even test
this?
I'm not trying to be picky here, just to think about what you are doing
with these checks please.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v3] net-sysfs: Fix reference count leak
2019-11-18 13:27 ` Greg Kroah-Hartman
@ 2019-11-19 7:45 ` Jouni Högander
2019-11-19 7:58 ` Greg Kroah-Hartman
0 siblings, 1 reply; 7+ messages in thread
From: Jouni Högander @ 2019-11-19 7:45 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: netdev, David Miller, Lukas Bulwahn
Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> On Mon, Nov 18, 2019 at 02:32:46PM +0200, jouni.hogander@unikie.com wrote:
>> From: Jouni Hogander <jouni.hogander@unikie.com>
>>
>> Netdev_register_kobject is calling device_initialize. In case of error
>> reference taken by device_initialize is not given up.
>>
>> Drivers are supposed to call free_netdev in case of error. In non-error
>> case the last reference is given up there and device release sequence
>> is triggered. In error case this reference is kept and the release
>> sequence is never started.
>>
>> Fix this reference count leak by allowing giving up the reference also
>> in error case in free_netdev.
>>
>> Also replace BUG_ON with WARN_ON in free_netdev and in netdev_release.
>>
>> This is the rootcause for couple of memory leaks reported by Syzkaller:
>>
>> BUG: memory leak unreferenced object 0xffff8880675ca008 (size 256):
>> comm "netdev_register", pid 281, jiffies 4294696663 (age 6.808s)
>> hex dump (first 32 bytes):
>> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
>> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
>> backtrace:
>> [<0000000058ca4711>] kmem_cache_alloc_trace+0x167/0x280
>> [<000000002340019b>] device_add+0x882/0x1750
>> [<000000001d588c3a>] netdev_register_kobject+0x128/0x380
>> [<0000000011ef5535>] register_netdevice+0xa1b/0xf00
>> [<000000007fcf1c99>] __tun_chr_ioctl+0x20d5/0x3dd0
>> [<000000006a5b7b2b>] tun_chr_ioctl+0x2f/0x40
>> [<00000000f30f834a>] do_vfs_ioctl+0x1c7/0x1510
>> [<00000000fba062ea>] ksys_ioctl+0x99/0xb0
>> [<00000000b1c1b8d2>] __x64_sys_ioctl+0x78/0xb0
>> [<00000000984cabb9>] do_syscall_64+0x16f/0x580
>> [<000000000bde033d>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>> [<00000000e6ca2d9f>] 0xffffffffffffffff
>>
>> BUG: memory leak
>> unreferenced object 0xffff8880668ba588 (size 8):
>> comm "kobject_set_nam", pid 286, jiffies 4294725297 (age 9.871s)
>> hex dump (first 8 bytes):
>> 6e 72 30 00 cc be df 2b nr0....+
>> backtrace:
>> [<00000000a322332a>] __kmalloc_track_caller+0x16e/0x290
>> [<00000000236fd26b>] kstrdup+0x3e/0x70
>> [<00000000dd4a2815>] kstrdup_const+0x3e/0x50
>> [<0000000049a377fc>] kvasprintf_const+0x10e/0x160
>> [<00000000627fc711>] kobject_set_name_vargs+0x5b/0x140
>> [<0000000019eeab06>] dev_set_name+0xc0/0xf0
>> [<0000000069cb12bc>] netdev_register_kobject+0xc8/0x320
>> [<00000000f2e83732>] register_netdevice+0xa1b/0xf00
>> [<000000009e1f57cc>] __tun_chr_ioctl+0x20d5/0x3dd0
>> [<000000009c560784>] tun_chr_ioctl+0x2f/0x40
>> [<000000000d759e02>] do_vfs_ioctl+0x1c7/0x1510
>> [<00000000351d7c31>] ksys_ioctl+0x99/0xb0
>> [<000000008390040a>] __x64_sys_ioctl+0x78/0xb0
>> [<0000000052d196b7>] do_syscall_64+0x16f/0x580
>> [<0000000019af9236>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>> [<00000000bc384531>] 0xffffffffffffffff
>>
>> Reported-by: syzbot+ad8ca40ecd77896d51e2@syzkaller.appspotmail.com
>> Cc: David Miller <davem@davemloft.net>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Cc: Lukas Bulwahn <lukas.bulwahn@gmail.com>
>> Signed-off-by: Jouni Hogander <jouni.hogander@unikie.com>
>> ---
>> v2 -> v3:
>> * Replaced BUG_ON with WARN_ON in free_netdev and netdev_release
>> v1 -> v2:
>> * Relying on driver calling free_netdev rather than calling
>> put_device directly in error path
>> ---
>> net/core/dev.c | 14 +++++++-------
>> net/core/net-sysfs.c | 6 +++++-
>> 2 files changed, 12 insertions(+), 8 deletions(-)
>>
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 99ac84ff398f..1d6c0bfb5ec5 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -9603,14 +9603,14 @@ void free_netdev(struct net_device *dev)
>>
>> netdev_unregister_lockdep_key(dev);
>>
>> - /* Compatibility with error handling in drivers */
>> - if (dev->reg_state == NETREG_UNINITIALIZED) {
>> - netdev_freemem(dev);
>> - return;
>> - }
>> + /* reg_state is NETREG_UNINITIALIZED if there is an error in
>> + * registration.
>> + */
>> + WARN_ON(dev->reg_state != NETREG_UNREGISTERED &&
>> + dev->reg_state != NETREG_UNINITIALIZED);
>
> You "warn" about this, but do not actually handle the problem. So what
> is this helping with?
>
Now as I have replaced BUG_ON with WARN_ON different reg_states are
actually handled. In free_netdev reference is given up and release
sequence is expected to start no matter what is the dev->reg_state. In
netdev_release memories are freed. There are just extra warnings in the
log from the functions.
> Systems with panic-on-warn just rebooted, and a "normal" system saw a
> traceback yet no error handling happened so why would the code even test
> this?
One use for panic-on-warn is the stabilation phase before "product"
quality. In this case you are interested in everything that is
unexpected. Having panic-on-warn set is easy way to make each warning in
the system visible to user and force to react on it.
Another example is my Syzkaller excercise. I gathered all the crashes
from the system and these include warnings as well. I will probably next
look at these warnings Syzkaller found and try to understand why they
are and do they need fixes.
>
> I'm not trying to be picky here, just to think about what you are doing
> with these checks please.
This state was not expected being anything else than
NETREG_UNREGISTERED. Probably error cases were not taken into account or
maybe error cases were not expected to end up to device release
sequence. Especially as cleaning up relies on normal device release
sequence on error cases this assumption is wrong.
Do you think better choise would be to remove BUG_ON completely rather
than replacing it with WARN_ON?
BR,
Jouni Högander
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v3] net-sysfs: Fix reference count leak
2019-11-19 7:45 ` Jouni Högander
@ 2019-11-19 7:58 ` Greg Kroah-Hartman
2019-11-19 9:00 ` Jouni Högander
0 siblings, 1 reply; 7+ messages in thread
From: Greg Kroah-Hartman @ 2019-11-19 7:58 UTC (permalink / raw)
To: Jouni Högander; +Cc: netdev, David Miller, Lukas Bulwahn
On Tue, Nov 19, 2019 at 09:45:02AM +0200, Jouni Högander wrote:
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
>
> > On Mon, Nov 18, 2019 at 02:32:46PM +0200, jouni.hogander@unikie.com wrote:
> >> From: Jouni Hogander <jouni.hogander@unikie.com>
> >>
> >> Netdev_register_kobject is calling device_initialize. In case of error
> >> reference taken by device_initialize is not given up.
> >>
> >> Drivers are supposed to call free_netdev in case of error. In non-error
> >> case the last reference is given up there and device release sequence
> >> is triggered. In error case this reference is kept and the release
> >> sequence is never started.
> >>
> >> Fix this reference count leak by allowing giving up the reference also
> >> in error case in free_netdev.
> >>
> >> Also replace BUG_ON with WARN_ON in free_netdev and in netdev_release.
> >>
> >> This is the rootcause for couple of memory leaks reported by Syzkaller:
> >>
> >> BUG: memory leak unreferenced object 0xffff8880675ca008 (size 256):
> >> comm "netdev_register", pid 281, jiffies 4294696663 (age 6.808s)
> >> hex dump (first 32 bytes):
> >> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> >> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> >> backtrace:
> >> [<0000000058ca4711>] kmem_cache_alloc_trace+0x167/0x280
> >> [<000000002340019b>] device_add+0x882/0x1750
> >> [<000000001d588c3a>] netdev_register_kobject+0x128/0x380
> >> [<0000000011ef5535>] register_netdevice+0xa1b/0xf00
> >> [<000000007fcf1c99>] __tun_chr_ioctl+0x20d5/0x3dd0
> >> [<000000006a5b7b2b>] tun_chr_ioctl+0x2f/0x40
> >> [<00000000f30f834a>] do_vfs_ioctl+0x1c7/0x1510
> >> [<00000000fba062ea>] ksys_ioctl+0x99/0xb0
> >> [<00000000b1c1b8d2>] __x64_sys_ioctl+0x78/0xb0
> >> [<00000000984cabb9>] do_syscall_64+0x16f/0x580
> >> [<000000000bde033d>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> >> [<00000000e6ca2d9f>] 0xffffffffffffffff
> >>
> >> BUG: memory leak
> >> unreferenced object 0xffff8880668ba588 (size 8):
> >> comm "kobject_set_nam", pid 286, jiffies 4294725297 (age 9.871s)
> >> hex dump (first 8 bytes):
> >> 6e 72 30 00 cc be df 2b nr0....+
> >> backtrace:
> >> [<00000000a322332a>] __kmalloc_track_caller+0x16e/0x290
> >> [<00000000236fd26b>] kstrdup+0x3e/0x70
> >> [<00000000dd4a2815>] kstrdup_const+0x3e/0x50
> >> [<0000000049a377fc>] kvasprintf_const+0x10e/0x160
> >> [<00000000627fc711>] kobject_set_name_vargs+0x5b/0x140
> >> [<0000000019eeab06>] dev_set_name+0xc0/0xf0
> >> [<0000000069cb12bc>] netdev_register_kobject+0xc8/0x320
> >> [<00000000f2e83732>] register_netdevice+0xa1b/0xf00
> >> [<000000009e1f57cc>] __tun_chr_ioctl+0x20d5/0x3dd0
> >> [<000000009c560784>] tun_chr_ioctl+0x2f/0x40
> >> [<000000000d759e02>] do_vfs_ioctl+0x1c7/0x1510
> >> [<00000000351d7c31>] ksys_ioctl+0x99/0xb0
> >> [<000000008390040a>] __x64_sys_ioctl+0x78/0xb0
> >> [<0000000052d196b7>] do_syscall_64+0x16f/0x580
> >> [<0000000019af9236>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> >> [<00000000bc384531>] 0xffffffffffffffff
> >>
> >> Reported-by: syzbot+ad8ca40ecd77896d51e2@syzkaller.appspotmail.com
> >> Cc: David Miller <davem@davemloft.net>
> >> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >> Cc: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> >> Signed-off-by: Jouni Hogander <jouni.hogander@unikie.com>
> >> ---
> >> v2 -> v3:
> >> * Replaced BUG_ON with WARN_ON in free_netdev and netdev_release
> >> v1 -> v2:
> >> * Relying on driver calling free_netdev rather than calling
> >> put_device directly in error path
> >> ---
> >> net/core/dev.c | 14 +++++++-------
> >> net/core/net-sysfs.c | 6 +++++-
> >> 2 files changed, 12 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/net/core/dev.c b/net/core/dev.c
> >> index 99ac84ff398f..1d6c0bfb5ec5 100644
> >> --- a/net/core/dev.c
> >> +++ b/net/core/dev.c
> >> @@ -9603,14 +9603,14 @@ void free_netdev(struct net_device *dev)
> >>
> >> netdev_unregister_lockdep_key(dev);
> >>
> >> - /* Compatibility with error handling in drivers */
> >> - if (dev->reg_state == NETREG_UNINITIALIZED) {
> >> - netdev_freemem(dev);
> >> - return;
> >> - }
> >> + /* reg_state is NETREG_UNINITIALIZED if there is an error in
> >> + * registration.
> >> + */
> >> + WARN_ON(dev->reg_state != NETREG_UNREGISTERED &&
> >> + dev->reg_state != NETREG_UNINITIALIZED);
> >
> > You "warn" about this, but do not actually handle the problem. So what
> > is this helping with?
> >
>
> Now as I have replaced BUG_ON with WARN_ON different reg_states are
> actually handled. In free_netdev reference is given up and release
> sequence is expected to start no matter what is the dev->reg_state. In
> netdev_release memories are freed. There are just extra warnings in the
> log from the functions.
>
> > Systems with panic-on-warn just rebooted, and a "normal" system saw a
> > traceback yet no error handling happened so why would the code even test
> > this?
>
> One use for panic-on-warn is the stabilation phase before "product"
> quality. In this case you are interested in everything that is
> unexpected. Having panic-on-warn set is easy way to make each warning in
> the system visible to user and force to react on it.
>
> Another example is my Syzkaller excercise. I gathered all the crashes
> from the system and these include warnings as well. I will probably next
> look at these warnings Syzkaller found and try to understand why they
> are and do they need fixes.
I know many "production" systems that run panic-on-warn so that if
something "odd" happens, the system gets rebooted and restarted all
automatically. It makes sense in a large cluster/cloud system.
> > I'm not trying to be picky here, just to think about what you are doing
> > with these checks please.
>
> This state was not expected being anything else than
> NETREG_UNREGISTERED. Probably error cases were not taken into account or
> maybe error cases were not expected to end up to device release
> sequence. Especially as cleaning up relies on normal device release
> sequence on error cases this assumption is wrong.
>
> Do you think better choise would be to remove BUG_ON completely rather
> than replacing it with WARN_ON?
I don't know this code, and I'm not the maintainer of it, so I don't
have a strong opinion. Other than if this was code I was maintaining, I
would not want it there :)
Anyway, your original fix looks sane to me, and that's probably a good
thing to just do now and then you can work on these other things.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH v3] net-sysfs: Fix reference count leak
2019-11-19 7:58 ` Greg Kroah-Hartman
@ 2019-11-19 9:00 ` Jouni Högander
0 siblings, 0 replies; 7+ messages in thread
From: Jouni Högander @ 2019-11-19 9:00 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: netdev, David Miller, Lukas Bulwahn
Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> On Tue, Nov 19, 2019 at 09:45:02AM +0200, Jouni Högander wrote:
>> Do you think better choise would be to remove BUG_ON completely rather
>> than replacing it with WARN_ON?
>
> I don't know this code, and I'm not the maintainer of it, so I don't
> have a strong opinion. Other than if this was code I was maintaining, I
> would not want it there :)
>
> Anyway, your original fix looks sane to me, and that's probably a good
> thing to just do now and then you can work on these other things.
>
> thanks,
>
> greg k-h
Thank you a lot for your help on this patch.
BR,
Jouni Högander
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v3] net-sysfs: Fix reference count leak
@ 2019-11-18 12:06 jouni.hogander
2019-11-18 12:25 ` Leon Romanovsky
0 siblings, 1 reply; 7+ messages in thread
From: jouni.hogander @ 2019-11-18 12:06 UTC (permalink / raw)
To: netdev; +Cc: Jouni Hogander, David Miller, Greg Kroah-Hartman, Lukas Bulwahn
From: Jouni Hogander <jouni.hogander@unikie.com>
Netdev_register_kobject is calling device_initialize. In case of error
reference taken by device_initialize is not given up.
Drivers are supposed to call free_netdev in case of error. In non-error
case the last reference is given up there and device release sequence
is triggered. In error case this reference is kept and the release
sequence is never started.
Fix this reference count leak by allowing giving up the reference also
in error case in free_netdev.
Also replace BUG_ON with WARN_ON in free_netdev and in netdev_release.
This is the rootcause for couple of memory leaks reported by Syzkaller:
BUG: memory leak unreferenced object 0xffff8880675ca008 (size 256):
comm "netdev_register", pid 281, jiffies 4294696663 (age 6.808s)
hex dump (first 32 bytes):
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
backtrace:
[<0000000058ca4711>] kmem_cache_alloc_trace+0x167/0x280
[<000000002340019b>] device_add+0x882/0x1750
[<000000001d588c3a>] netdev_register_kobject+0x128/0x380
[<0000000011ef5535>] register_netdevice+0xa1b/0xf00
[<000000007fcf1c99>] __tun_chr_ioctl+0x20d5/0x3dd0
[<000000006a5b7b2b>] tun_chr_ioctl+0x2f/0x40
[<00000000f30f834a>] do_vfs_ioctl+0x1c7/0x1510
[<00000000fba062ea>] ksys_ioctl+0x99/0xb0
[<00000000b1c1b8d2>] __x64_sys_ioctl+0x78/0xb0
[<00000000984cabb9>] do_syscall_64+0x16f/0x580
[<000000000bde033d>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
[<00000000e6ca2d9f>] 0xffffffffffffffff
BUG: memory leak
unreferenced object 0xffff8880668ba588 (size 8):
comm "kobject_set_nam", pid 286, jiffies 4294725297 (age 9.871s)
hex dump (first 8 bytes):
6e 72 30 00 cc be df 2b nr0....+
backtrace:
[<00000000a322332a>] __kmalloc_track_caller+0x16e/0x290
[<00000000236fd26b>] kstrdup+0x3e/0x70
[<00000000dd4a2815>] kstrdup_const+0x3e/0x50
[<0000000049a377fc>] kvasprintf_const+0x10e/0x160
[<00000000627fc711>] kobject_set_name_vargs+0x5b/0x140
[<0000000019eeab06>] dev_set_name+0xc0/0xf0
[<0000000069cb12bc>] netdev_register_kobject+0xc8/0x320
[<00000000f2e83732>] register_netdevice+0xa1b/0xf00
[<000000009e1f57cc>] __tun_chr_ioctl+0x20d5/0x3dd0
[<000000009c560784>] tun_chr_ioctl+0x2f/0x40
[<000000000d759e02>] do_vfs_ioctl+0x1c7/0x1510
[<00000000351d7c31>] ksys_ioctl+0x99/0xb0
[<000000008390040a>] __x64_sys_ioctl+0x78/0xb0
[<0000000052d196b7>] do_syscall_64+0x16f/0x580
[<0000000019af9236>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
[<00000000bc384531>] 0xffffffffffffffff
v2 -> v3:
* Replaced BUG_ON with WARN_ON in free_netdev and netdev_release
v1 -> v2:
* Relying on driver calling free_netdev rather than calling
put_device directly in error path
Reported-by: syzbot+ad8ca40ecd77896d51e2@syzkaller.appspotmail.com
Cc: David Miller <davem@davemloft.net>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Lukas Bulwahn <lukas.bulwahn@gmail.com>
Signed-off-by: Jouni Hogander <jouni.hogander@unikie.com>
---
net/core/dev.c | 14 +++++++-------
net/core/net-sysfs.c | 6 +++++-
2 files changed, 12 insertions(+), 8 deletions(-)
diff --git a/net/core/dev.c b/net/core/dev.c
index 99ac84ff398f..1d6c0bfb5ec5 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -9603,14 +9603,14 @@ void free_netdev(struct net_device *dev)
netdev_unregister_lockdep_key(dev);
- /* Compatibility with error handling in drivers */
- if (dev->reg_state == NETREG_UNINITIALIZED) {
- netdev_freemem(dev);
- return;
- }
+ /* reg_state is NETREG_UNINITIALIZED if there is an error in
+ * registration.
+ */
+ WARN_ON(dev->reg_state != NETREG_UNREGISTERED &&
+ dev->reg_state != NETREG_UNINITIALIZED);
- BUG_ON(dev->reg_state != NETREG_UNREGISTERED);
- dev->reg_state = NETREG_RELEASED;
+ if (dev->reg_state != NETREG_UNINITIALIZED)
+ dev->reg_state = NETREG_RELEASED;
/* will free via device release */
put_device(&dev->dev);
diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index 865ba6ca16eb..11f1e3b4b18f 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -1626,7 +1626,11 @@ static void netdev_release(struct device *d)
{
struct net_device *dev = to_net_dev(d);
- BUG_ON(dev->reg_state != NETREG_RELEASED);
+ /* reg_state is NETREG_UNINITIALIZED if there is an error in
+ * registration.
+ */
+ WARN_ON(dev->reg_state != NETREG_RELEASED &&
+ dev->reg_state != NETREG_UNINITIALIZED);
/* no need to wait for rcu grace period:
* device is dead and about to be freed.
--
2.17.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH v3] net-sysfs: Fix reference count leak
2019-11-18 12:06 jouni.hogander
@ 2019-11-18 12:25 ` Leon Romanovsky
0 siblings, 0 replies; 7+ messages in thread
From: Leon Romanovsky @ 2019-11-18 12:25 UTC (permalink / raw)
To: jouni.hogander; +Cc: netdev, David Miller, Greg Kroah-Hartman, Lukas Bulwahn
On Mon, Nov 18, 2019 at 02:06:50PM +0200, jouni.hogander@unikie.com wrote:
> From: Jouni Hogander <jouni.hogander@unikie.com>
>
> Netdev_register_kobject is calling device_initialize. In case of error
> reference taken by device_initialize is not given up.
>
> Drivers are supposed to call free_netdev in case of error. In non-error
> case the last reference is given up there and device release sequence
> is triggered. In error case this reference is kept and the release
> sequence is never started.
>
> Fix this reference count leak by allowing giving up the reference also
> in error case in free_netdev.
>
> Also replace BUG_ON with WARN_ON in free_netdev and in netdev_release.
>
> This is the rootcause for couple of memory leaks reported by Syzkaller:
>
> BUG: memory leak unreferenced object 0xffff8880675ca008 (size 256):
> comm "netdev_register", pid 281, jiffies 4294696663 (age 6.808s)
> hex dump (first 32 bytes):
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> backtrace:
> [<0000000058ca4711>] kmem_cache_alloc_trace+0x167/0x280
> [<000000002340019b>] device_add+0x882/0x1750
> [<000000001d588c3a>] netdev_register_kobject+0x128/0x380
> [<0000000011ef5535>] register_netdevice+0xa1b/0xf00
> [<000000007fcf1c99>] __tun_chr_ioctl+0x20d5/0x3dd0
> [<000000006a5b7b2b>] tun_chr_ioctl+0x2f/0x40
> [<00000000f30f834a>] do_vfs_ioctl+0x1c7/0x1510
> [<00000000fba062ea>] ksys_ioctl+0x99/0xb0
> [<00000000b1c1b8d2>] __x64_sys_ioctl+0x78/0xb0
> [<00000000984cabb9>] do_syscall_64+0x16f/0x580
> [<000000000bde033d>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [<00000000e6ca2d9f>] 0xffffffffffffffff
>
> BUG: memory leak
> unreferenced object 0xffff8880668ba588 (size 8):
> comm "kobject_set_nam", pid 286, jiffies 4294725297 (age 9.871s)
> hex dump (first 8 bytes):
> 6e 72 30 00 cc be df 2b nr0....+
> backtrace:
> [<00000000a322332a>] __kmalloc_track_caller+0x16e/0x290
> [<00000000236fd26b>] kstrdup+0x3e/0x70
> [<00000000dd4a2815>] kstrdup_const+0x3e/0x50
> [<0000000049a377fc>] kvasprintf_const+0x10e/0x160
> [<00000000627fc711>] kobject_set_name_vargs+0x5b/0x140
> [<0000000019eeab06>] dev_set_name+0xc0/0xf0
> [<0000000069cb12bc>] netdev_register_kobject+0xc8/0x320
> [<00000000f2e83732>] register_netdevice+0xa1b/0xf00
> [<000000009e1f57cc>] __tun_chr_ioctl+0x20d5/0x3dd0
> [<000000009c560784>] tun_chr_ioctl+0x2f/0x40
> [<000000000d759e02>] do_vfs_ioctl+0x1c7/0x1510
> [<00000000351d7c31>] ksys_ioctl+0x99/0xb0
> [<000000008390040a>] __x64_sys_ioctl+0x78/0xb0
> [<0000000052d196b7>] do_syscall_64+0x16f/0x580
> [<0000000019af9236>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [<00000000bc384531>] 0xffffffffffffffff
>
> v2 -> v3:
> * Replaced BUG_ON with WARN_ON in free_netdev and netdev_release
>
> v1 -> v2:
> * Relying on driver calling free_netdev rather than calling
> put_device directly in error path
This changelog should go after "---" line.
Thanks
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-11-19 9:00 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-11-18 12:32 [PATCH v3] net-sysfs: Fix reference count leak jouni.hogander
2019-11-18 13:27 ` Greg Kroah-Hartman
2019-11-19 7:45 ` Jouni Högander
2019-11-19 7:58 ` Greg Kroah-Hartman
2019-11-19 9:00 ` Jouni Högander
-- strict thread matches above, loose matches on Subject: below --
2019-11-18 12:06 jouni.hogander
2019-11-18 12:25 ` Leon Romanovsky
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).