kernel BUG at net/core/net-sysfs.c:LINE!

[syzbot]

Hello,

syzbot found the following crash on:

HEAD commit: e382d91f Add linux-next specific files for 20190322
git tree: linux-next
console output: https://syzkaller.appspot.com/x/log.txt?x=1737671b200000
kernel config: https://syzkaller.appspot.com/x/.config?x=3d850e8b394c7a19
dashboard link: https://syzkaller.appspot.com/bug?extid=6024817a931b2830bc93
compiler: gcc (GCC) 9.0.0 20181231 (experimental)
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1795613b200000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=10f5a437200000

The bug was bisected to:

commit 6b70fc94afd165342876e53fc4b2f7d085009945
Author: Wang Hai <wang...@huawei.com>
Date: Wed Mar 20 18:25:05 2019 +0000

net-sysfs: Fix memory leak in netdev_register_kobject

bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=1522556d200000
final crash: https://syzkaller.appspot.com/x/report.txt?x=1722556d200000
console output: https://syzkaller.appspot.com/x/log.txt?x=1322556d200000

IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+602481...@syzkaller.appspotmail.com
Fixes: 6b70fc94afd1 ("net-sysfs: Fix memory leak in
netdev_register_kobject")

RAX: ffffffffffffffda RBX: 0000000000000003 RCX: 0000000000441399
RDX: 0000000020000080 RSI: 00000000000089f1 RDI: 0000000000000006
RBP: 00007ffcd20666d0 R08: 0000000000000001 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: ffffffffffffffff
R13: 0000000000000007 R14: 0000000000000000 R15: 0000000000000000
------------[ cut here ]------------
kernel BUG at net/core/net-sysfs.c:1631!
invalid opcode: 0000 [#1] PREEMPT SMP KASAN
CPU: 1 PID: 8035 Comm: syz-executor344 Not tainted 5.1.0-rc1-next-20190322
#9
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
RIP: 0010:netdev_release net/core/net-sysfs.c:1631 [inline]
RIP: 0010:netdev_release+0x92/0xb0 net/core/net-sysfs.c:1627
Code: 48 c1 ea 03 80 3c 02 00 75 29 48 8b bb 80 fa ff ff e8 12 77 20 fc 4c
89 ef e8 7a cc f5 ff 5b 41 5c 41 5d 5d c3 e8 9e 9b e8 fb <0f> 0b e8 27 b1
20 fc eb 9c e8 80 b1 20 fc eb d0 0f 1f 40 00 66 2e
RSP: 0018:ffff88808d6af718 EFLAGS: 00010293
RAX: ffff8880a87902c0 RBX: ffff88808e3612a0 RCX: ffffffff8587fc89
RDX: 0000000000000000 RSI: ffffffff8587fcd2 RDI: 0000000000000001
RBP: ffff88808d6af730 R08: ffff8880a87902c0 R09: ffff8880a8790b60
R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
R13: ffff88808e360d00 R14: 0000000000000000 R15: 0000000000000000
FS: 0000000001752880(0000) GS:ffff8880ae900000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000000 CR3: 0000000098da2000 CR4: 00000000001406e0
Call Trace:
device_release+0x7d/0x210 drivers/base/core.c:1064
kobject_cleanup lib/kobject.c:662 [inline]
kobject_release lib/kobject.c:691 [inline]
kref_put include/linux/kref.h:67 [inline]
kobject_put.cold+0x28f/0x2ec lib/kobject.c:708
put_device+0x20/0x30 drivers/base/core.c:2205
netdev_register_kobject+0x1a1/0x3c0 net/core/net-sysfs.c:1763
register_netdevice+0x878/0xff0 net/core/dev.c:8709
ip6_tnl_create2+0x1c2/0x350 net/ipv6/ip6_tunnel.c:269
ip6_tnl_create net/ipv6/ip6_tunnel.c:320 [inline]
ip6_tnl_locate+0x63f/0x8d0 net/ipv6/ip6_tunnel.c:368
ip6_tnl_ioctl+0x490/0xab0 net/ipv6/ip6_tunnel.c:1634
dev_ifsioc+0x257/0x990 net/core/dev_ioctl.c:322
dev_ioctl+0x286/0xc90 net/core/dev_ioctl.c:513
sock_ioctl+0x48b/0x610 net/socket.c:1102
vfs_ioctl fs/ioctl.c:46 [inline]
file_ioctl fs/ioctl.c:509 [inline]
do_vfs_ioctl+0xd6e/0x1390 fs/ioctl.c:696
ksys_ioctl+0xab/0xd0 fs/ioctl.c:713
__do_sys_ioctl fs/ioctl.c:720 [inline]
__se_sys_ioctl fs/ioctl.c:718 [inline]
__x64_sys_ioctl+0x73/0xb0 fs/ioctl.c:718
do_syscall_64+0x103/0x610 arch/x86/entry/common.c:290
entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x441399
Code: e8 5c ae 02 00 48 83 c4 18 c3 0f 1f 80 00 00 00 00 48 89 f8 48 89 f7
48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff
ff 0f 83 bb 0a fc ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007ffcd20666b8 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
RAX: ffffffffffffffda RBX: 0000000000000003 RCX: 0000000000441399
RDX: 0000000020000080 RSI: 00000000000089f1 RDI: 0000000000000006
RBP: 00007ffcd20666d0 R08: 0000000000000001 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: ffffffffffffffff
R13: 0000000000000007 R14: 0000000000000000 R15: 0000000000000000
Modules linked in:
---[ end trace 0dbf190846958075 ]---
RIP: 0010:netdev_release net/core/net-sysfs.c:1631 [inline]
RIP: 0010:netdev_release+0x92/0xb0 net/core/net-sysfs.c:1627
Code: 48 c1 ea 03 80 3c 02 00 75 29 48 8b bb 80 fa ff ff e8 12 77 20 fc 4c
89 ef e8 7a cc f5 ff 5b 41 5c 41 5d 5d c3 e8 9e 9b e8 fb <0f> 0b e8 27 b1
20 fc eb 9c e8 80 b1 20 fc eb d0 0f 1f 40 00 66 2e
RSP: 0018:ffff88808d6af718 EFLAGS: 00010293
RAX: ffff8880a87902c0 RBX: ffff88808e3612a0 RCX: ffffffff8587fc89
RDX: 0000000000000000 RSI: ffffffff8587fcd2 RDI: 0000000000000001
RBP: ffff88808d6af730 R08: ffff8880a87902c0 R09: ffff8880a8790b60
R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
R13: ffff88808e360d00 R14: 0000000000000000 R15: 0000000000000000
FS: 0000000001752880(0000) GS:ffff8880ae900000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000000 CR3: 0000000098da2000 CR4: 00000000001406e0


---
This bug is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzk...@googlegroups.com.

syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
For information about bisection process see: https://goo.gl/tpsmEJ#bisection
syzbot can test patches for this bug, for details see:
https://goo.gl/tpsmEJ#testing-patches

[Andy Shevchenko]

On Sat, Mar 23, 2019 at 12:32:06AM -0700, syzbot wrote:
> Hello,
>
> syzbot found the following crash on:
>
> HEAD commit: e382d91f Add linux-next specific files for 20190322
> git tree: linux-next
> console output: https://syzkaller.appspot.com/x/log.txt?x=1737671b200000
> kernel config: https://syzkaller.appspot.com/x/.config?x=3d850e8b394c7a19
> dashboard link: https://syzkaller.appspot.com/bug?extid=6024817a931b2830bc93
> compiler: gcc (GCC) 9.0.0 20181231 (experimental)
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1795613b200000
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=10f5a437200000
>
> The bug was bisected to:
>
> commit 6b70fc94afd165342876e53fc4b2f7d085009945
> Author: Wang Hai <wang...@huawei.com>
> Date: Wed Mar 20 18:25:05 2019 +0000
>
> net-sysfs: Fix memory leak in netdev_register_kobject

Nice.

I looked briefly in the flow of this report and it looks like the patch above
should be reverted.

The problem is not so easy to fix. One approach is to initialize device
(and thus kobject) somewhere in alloc_netdev() and put device in free_netdev()
respectively, but this might produce more interesting regressions.

--
With Best Regards,
Andy Shevchenko

[wanghai (M)]

thanks , Can it be fixed like this?

diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index 4ff661f..e609c8d 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -1745,16 +1745,21 @@ int netdev_register_kobject(struct net_device *ndev)

        error = device_add(dev);
        if (error)
-               return error;
+               goto error_put_device;

        error = register_queue_kobjects(ndev);
-       if (error) {
-               device_del(dev);
-               return error;
-       }
+       if (error)
+               goto error_device_del;

        pm_runtime_set_memalloc_noio(dev, true);

+       return 0;
+
+error_device_del:
+       device_del(dev);
+error_put_device:
+       ndev->reg_state = NETREG_RELEASED;
+       put_device(dev);
        return error;

[Andy Shevchenko]

On Mon, Mar 25, 2019 at 11:20:01PM +0800, wanghai (M) wrote:
> thanks , Can it be fixed like this?

I dunno. I think no, it can't.

As far as I can see the issue happened due to freeing entire network device at
the point of putting reference count to the device (struct device is embedded
into struct net_device).

When it happens the access to _any_ field of struct net_device will crash the
system.

Basically it means that put_device() should be carefully placed case-by-case,
because on real hardware the actual device is parent and usually no-one does
access to the child without need. On the contrary the tunX devices are
artificial and are controlled by the network stack.

So, it means we need to do something like

ret = register_netdev(...);
if (ret) {
put_device(&ndev->dev);
...
}

But as I mentioned, it would be tricky to not break something else.

P.S. It might be I have missed something, I'm not an expert in network stack.

[Dmitry Torokhov]

On Mon, Mar 25, 2019 at 06:10:31PM +0200, Andy Shevchenko wrote:
> On Mon, Mar 25, 2019 at 11:20:01PM +0800, wanghai (M) wrote:
> > thanks , Can it be fixed like this?
>
> I dunno. I think no, it can't.

I agree, it can't.

>
> As far as I can see the issue happened due to freeing entire network device at
> the point of putting reference count to the device (struct device is embedded
> into struct net_device).
>
> When it happens the access to _any_ field of struct net_device will crash the
> system.
>
> Basically it means that put_device() should be carefully placed case-by-case,
> because on real hardware the actual device is parent and usually no-one does
> access to the child without need. On the contrary the tunX devices are
> artificial and are controlled by the network stack.
>
> So, it means we need to do something like
>
> ret = register_netdev(...);
> if (ret) {
> put_device(&ndev->dev);
> ...
> }
>
> But as I mentioned, it would be tricky to not break something else.

I'd say that the entity that called alloc_netdev() should be the one
that calls put_device() (but the way of free_netdev()), not net/core
code. Do we have a driver that is messed up and does not do proper
cleanup?

Thanks.

--
Dmitry

[Dmitry Torokhov]

OK, looking at this some more, I think we need to set dev->reg_state =
NETREG_REGISTERED earlier, right after successful call to device_add()
as at this point the device is alive as far as device core is concerned.
The queue kobjects have to be managed separately, for that I'd pull the
code out of netdev_register_kobject() and move it into
register_netdevice() and ensure that we clean up there properly.

Thanks.

--
Dmitry

[wanghai (M)]

Can someone fix this issue? Thanks.

> .
>

[Eric Dumazet]

On 04/03/2019 08:19 PM, wanghai (M) wrote:
> Can someone fix this issue? Thanks.

What do you mean by this exactly ?

It seems your patch added a regression, so you should either revert the patch
or fix this yourself.

[Al Viro]

On Thu, Apr 04, 2019 at 11:19:03AM +0800, wanghai (M) wrote:
> Can someone fix this issue? Thanks.

Revert the bogus patch, perhaps? Because bogus it is - failure of
register_netdevice() should NOT drop the reference it's been given.
It's up to the caller and that's when the name will be freed.