* [PATCH net] bonding: check error value of register_netdevice() immediately
@ 2020-07-18 7:13 Taehee Yoo
2020-07-18 20:06 ` Cong Wang
0 siblings, 1 reply; 3+ messages in thread
From: Taehee Yoo @ 2020-07-18 7:13 UTC (permalink / raw)
To: davem, kuba, j.vosburgh, vfalico, andy, netdev; +Cc: amwang, ap420073
If register_netdevice() is failed, net_device should not be used
because variables are uninitialized or freed.
So, the routine should be stopped immediately.
But, bond_create() doesn't check return value of register_netdevice()
immediately. That will result in a panic because of using uninitialized
or freed memory.
Test commands:
modprobe netdev-notifier-error-inject
echo -22 > /sys/kernel/debug/notifier-error-inject/netdev/\
actions/NETDEV_REGISTER/error
modprobe bonding max_bonds=3
Splat looks like:
[ 375.028492][ T193] general protection fault, probably for non-canonical address 0x6b6b6b6b6b6b6b6b: 0000 [#1] SMP DEBUG_PAGEALLOC PTI
[ 375.033207][ T193] CPU: 2 PID: 193 Comm: kworker/2:2 Not tainted 5.8.0-rc4+ #645
[ 375.036068][ T193] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1ubuntu1 04/01/2014
[ 375.039673][ T193] Workqueue: events linkwatch_event
[ 375.041557][ T193] RIP: 0010:dev_activate+0x4a/0x340
[ 375.043381][ T193] Code: 40 a8 04 0f 85 db 00 00 00 8b 83 08 04 00 00 85 c0 0f 84 0d 01 00 00 31 d2 89 d0 48 8d 04 40 48 c1 e0 07 48 03 83 00 04 00 00 <48> 8b 48 10 f6 41 10 01 75 08 f0 80 a1 a0 01 00 00 fd 48 89 48 08
[ 375.050267][ T193] RSP: 0018:ffff9f8facfcfdd8 EFLAGS: 00010202
[ 375.052410][ T193] RAX: 6b6b6b6b6b6b6b6b RBX: ffff9f8fae6ea000 RCX: 0000000000000006
[ 375.055178][ T193] RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffff9f8fae6ea000
[ 375.057762][ T193] RBP: ffff9f8fae6ea000 R08: 0000000000000000 R09: 0000000000000000
[ 375.059810][ T193] R10: 0000000000000001 R11: 0000000000000000 R12: ffff9f8facfcfe08
[ 375.061892][ T193] R13: ffffffff883587e0 R14: 0000000000000000 R15: ffff9f8fae6ea580
[ 375.063931][ T193] FS: 0000000000000000(0000) GS:ffff9f8fbae00000(0000) knlGS:0000000000000000
[ 375.066239][ T193] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 375.067841][ T193] CR2: 00007f2f542167a0 CR3: 000000012cee6002 CR4: 00000000003606e0
[ 375.069657][ T193] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 375.071471][ T193] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 375.073269][ T193] Call Trace:
[ 375.074005][ T193] linkwatch_do_dev+0x4d/0x50
[ 375.075052][ T193] __linkwatch_run_queue+0x10b/0x200
[ 375.076244][ T193] linkwatch_event+0x21/0x30
[ 375.077274][ T193] process_one_work+0x252/0x600
[ 375.078379][ T193] ? process_one_work+0x600/0x600
[ 375.079518][ T193] worker_thread+0x3c/0x380
[ 375.080534][ T193] ? process_one_work+0x600/0x600
[ 375.081668][ T193] kthread+0x139/0x150
[ 375.082567][ T193] ? kthread_park+0x90/0x90
[ 375.083567][ T193] ret_from_fork+0x22/0x30
Fixes: 9e2e61fbf8ad ("bonding: fix potential deadlock in bond_uninit()")
Signed-off-by: Taehee Yoo <ap420073@gmail.com>
---
drivers/net/bonding/bond_main.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 004919aea5fb..f88cb097b022 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -5053,15 +5053,19 @@ int bond_create(struct net *net, const char *name)
bond_dev->rtnl_link_ops = &bond_link_ops;
res = register_netdevice(bond_dev);
+ if (res < 0) {
+ free_netdev(bond_dev);
+ rtnl_unlock();
+
+ return res;
+ }
netif_carrier_off(bond_dev);
bond_work_init_all(bond);
rtnl_unlock();
- if (res < 0)
- free_netdev(bond_dev);
- return res;
+ return 0;
}
static int __net_init bond_net_init(struct net *net)
--
2.17.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH net] bonding: check error value of register_netdevice() immediately
2020-07-18 7:13 [PATCH net] bonding: check error value of register_netdevice() immediately Taehee Yoo
@ 2020-07-18 20:06 ` Cong Wang
2020-07-19 7:41 ` Taehee Yoo
0 siblings, 1 reply; 3+ messages in thread
From: Cong Wang @ 2020-07-18 20:06 UTC (permalink / raw)
To: Taehee Yoo
Cc: David Miller, Jakub Kicinski, Jay Vosburgh, Veaceslav Falico,
Andy Gospodarek, Linux Kernel Network Developers, WANG Cong
On Sat, Jul 18, 2020 at 12:14 AM Taehee Yoo <ap420073@gmail.com> wrote:
>
> If register_netdevice() is failed, net_device should not be used
> because variables are uninitialized or freed.
> So, the routine should be stopped immediately.
> But, bond_create() doesn't check return value of register_netdevice()
> immediately. That will result in a panic because of using uninitialized
> or freed memory.
>
> Test commands:
> modprobe netdev-notifier-error-inject
> echo -22 > /sys/kernel/debug/notifier-error-inject/netdev/\
> actions/NETDEV_REGISTER/error
> modprobe bonding max_bonds=3
>
> Splat looks like:
> [ 375.028492][ T193] general protection fault, probably for non-canonical address 0x6b6b6b6b6b6b6b6b: 0000 [#1] SMP DEBUG_PAGEALLOC PTI
> [ 375.033207][ T193] CPU: 2 PID: 193 Comm: kworker/2:2 Not tainted 5.8.0-rc4+ #645
> [ 375.036068][ T193] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1ubuntu1 04/01/2014
> [ 375.039673][ T193] Workqueue: events linkwatch_event
> [ 375.041557][ T193] RIP: 0010:dev_activate+0x4a/0x340
> [ 375.043381][ T193] Code: 40 a8 04 0f 85 db 00 00 00 8b 83 08 04 00 00 85 c0 0f 84 0d 01 00 00 31 d2 89 d0 48 8d 04 40 48 c1 e0 07 48 03 83 00 04 00 00 <48> 8b 48 10 f6 41 10 01 75 08 f0 80 a1 a0 01 00 00 fd 48 89 48 08
> [ 375.050267][ T193] RSP: 0018:ffff9f8facfcfdd8 EFLAGS: 00010202
> [ 375.052410][ T193] RAX: 6b6b6b6b6b6b6b6b RBX: ffff9f8fae6ea000 RCX: 0000000000000006
> [ 375.055178][ T193] RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffff9f8fae6ea000
> [ 375.057762][ T193] RBP: ffff9f8fae6ea000 R08: 0000000000000000 R09: 0000000000000000
> [ 375.059810][ T193] R10: 0000000000000001 R11: 0000000000000000 R12: ffff9f8facfcfe08
> [ 375.061892][ T193] R13: ffffffff883587e0 R14: 0000000000000000 R15: ffff9f8fae6ea580
> [ 375.063931][ T193] FS: 0000000000000000(0000) GS:ffff9f8fbae00000(0000) knlGS:0000000000000000
> [ 375.066239][ T193] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 375.067841][ T193] CR2: 00007f2f542167a0 CR3: 000000012cee6002 CR4: 00000000003606e0
> [ 375.069657][ T193] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [ 375.071471][ T193] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> [ 375.073269][ T193] Call Trace:
> [ 375.074005][ T193] linkwatch_do_dev+0x4d/0x50
> [ 375.075052][ T193] __linkwatch_run_queue+0x10b/0x200
> [ 375.076244][ T193] linkwatch_event+0x21/0x30
> [ 375.077274][ T193] process_one_work+0x252/0x600
> [ 375.078379][ T193] ? process_one_work+0x600/0x600
> [ 375.079518][ T193] worker_thread+0x3c/0x380
> [ 375.080534][ T193] ? process_one_work+0x600/0x600
> [ 375.081668][ T193] kthread+0x139/0x150
> [ 375.082567][ T193] ? kthread_park+0x90/0x90
> [ 375.083567][ T193] ret_from_fork+0x22/0x30
>
> Fixes: 9e2e61fbf8ad ("bonding: fix potential deadlock in bond_uninit()")
I doubt this is the first offending commit. At that time, the only
thing after register_netdevice() was rtnl_unlock(). I think it is
commit e826eafa65c6f1f7c8db5a237556cebac57ebcc5 which
introduced the bug, as it is the first commit puts something between
register_netdevice() and rtnl_unlock().
But this patch itself is obviously correct.
Thanks.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH net] bonding: check error value of register_netdevice() immediately
2020-07-18 20:06 ` Cong Wang
@ 2020-07-19 7:41 ` Taehee Yoo
0 siblings, 0 replies; 3+ messages in thread
From: Taehee Yoo @ 2020-07-19 7:41 UTC (permalink / raw)
To: Cong Wang
Cc: David Miller, Jakub Kicinski, Jay Vosburgh, Veaceslav Falico,
Andy Gospodarek, Linux Kernel Network Developers
On Sun, 19 Jul 2020 at 05:06, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
Hi Cong,
Thanks a lot for your review!
> On Sat, Jul 18, 2020 at 12:14 AM Taehee Yoo <ap420073@gmail.com> wrote:
> >
> > If register_netdevice() is failed, net_device should not be used
> > because variables are uninitialized or freed.
> > So, the routine should be stopped immediately.
> > But, bond_create() doesn't check return value of register_netdevice()
> > immediately. That will result in a panic because of using uninitialized
> > or freed memory.
> >
> > Test commands:
> > modprobe netdev-notifier-error-inject
> > echo -22 > /sys/kernel/debug/notifier-error-inject/netdev/\
> > actions/NETDEV_REGISTER/error
> > modprobe bonding max_bonds=3
> >
> > Splat looks like:
> > [ 375.028492][ T193] general protection fault, probably for non-canonical address 0x6b6b6b6b6b6b6b6b: 0000 [#1] SMP DEBUG_PAGEALLOC PTI
> > [ 375.033207][ T193] CPU: 2 PID: 193 Comm: kworker/2:2 Not tainted 5.8.0-rc4+ #645
> > [ 375.036068][ T193] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1ubuntu1 04/01/2014
> > [ 375.039673][ T193] Workqueue: events linkwatch_event
> > [ 375.041557][ T193] RIP: 0010:dev_activate+0x4a/0x340
> > [ 375.043381][ T193] Code: 40 a8 04 0f 85 db 00 00 00 8b 83 08 04 00 00 85 c0 0f 84 0d 01 00 00 31 d2 89 d0 48 8d 04 40 48 c1 e0 07 48 03 83 00 04 00 00 <48> 8b 48 10 f6 41 10 01 75 08 f0 80 a1 a0 01 00 00 fd 48 89 48 08
> > [ 375.050267][ T193] RSP: 0018:ffff9f8facfcfdd8 EFLAGS: 00010202
> > [ 375.052410][ T193] RAX: 6b6b6b6b6b6b6b6b RBX: ffff9f8fae6ea000 RCX: 0000000000000006
> > [ 375.055178][ T193] RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffff9f8fae6ea000
> > [ 375.057762][ T193] RBP: ffff9f8fae6ea000 R08: 0000000000000000 R09: 0000000000000000
> > [ 375.059810][ T193] R10: 0000000000000001 R11: 0000000000000000 R12: ffff9f8facfcfe08
> > [ 375.061892][ T193] R13: ffffffff883587e0 R14: 0000000000000000 R15: ffff9f8fae6ea580
> > [ 375.063931][ T193] FS: 0000000000000000(0000) GS:ffff9f8fbae00000(0000) knlGS:0000000000000000
> > [ 375.066239][ T193] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > [ 375.067841][ T193] CR2: 00007f2f542167a0 CR3: 000000012cee6002 CR4: 00000000003606e0
> > [ 375.069657][ T193] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > [ 375.071471][ T193] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > [ 375.073269][ T193] Call Trace:
> > [ 375.074005][ T193] linkwatch_do_dev+0x4d/0x50
> > [ 375.075052][ T193] __linkwatch_run_queue+0x10b/0x200
> > [ 375.076244][ T193] linkwatch_event+0x21/0x30
> > [ 375.077274][ T193] process_one_work+0x252/0x600
> > [ 375.078379][ T193] ? process_one_work+0x600/0x600
> > [ 375.079518][ T193] worker_thread+0x3c/0x380
> > [ 375.080534][ T193] ? process_one_work+0x600/0x600
> > [ 375.081668][ T193] kthread+0x139/0x150
> > [ 375.082567][ T193] ? kthread_park+0x90/0x90
> > [ 375.083567][ T193] ret_from_fork+0x22/0x30
> >
> > Fixes: 9e2e61fbf8ad ("bonding: fix potential deadlock in bond_uninit()")
>
> I doubt this is the first offending commit. At that time, the only
> thing after register_netdevice() was rtnl_unlock(). I think it is
> commit e826eafa65c6f1f7c8db5a237556cebac57ebcc5 which
> introduced the bug, as it is the first commit puts something between
> register_netdevice() and rtnl_unlock().
>
I checked for it.
You're right.
The bug was actually introduced by commit e826eafa65c6.
So, I will send a v2 patch to change a fixes tag.
Thanks a lot!
Taehee Yoo
> But this patch itself is obviously correct.
>
> Thanks.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-07-19 7:47 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-18 7:13 [PATCH net] bonding: check error value of register_netdevice() immediately Taehee Yoo
2020-07-18 20:06 ` Cong Wang
2020-07-19 7:41 ` Taehee Yoo
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.