* Re: [Bridge] WARNING: bad unlock balance in sch_direct_xmit
@ 2020-01-05 22:58 ` syzbot
0 siblings, 0 replies; 20+ messages in thread
From: syzbot @ 2020-01-05 22:58 UTC (permalink / raw)
To: a, alex.aring, allison, andrew, andy, ap420073, ast, b.a.t.m.a.n,
bridge, cleech, daniel, davem, dsa, f.fainelli, fw, gregkh,
gustavo, haiyangz, info, j.vosburgh, j, jakub.kicinski, jhs,
jiri, johan.hedberg, johannes.berg, jwi, kstewart, kvalo, kys,
linmiaohe, linux-bluetooth, linux-hams, linux-hyperv,
linux-kernel, linux-ppp, linux-wireless, linux-wpan, liuhangbin,
marcel, mareklindner, mkubecek, mmanning, netdev, nikolay,
oss-drivers, paulus, ralf, roopa, sashal
syzbot has bisected this bug to:
commit ab92d68fc22f9afab480153bd82a20f6e2533769
Author: Taehee Yoo <ap420073@gmail.com>
Date: Mon Oct 21 18:47:51 2019 +0000
net: core: add generic lockdep keys
bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=15e88ec6e00000
start commit: 36487907 Merge branch 'akpm' (patches from Andrew)
git tree: upstream
final crash: https://syzkaller.appspot.com/x/report.txt?x=17e88ec6e00000
console output: https://syzkaller.appspot.com/x/log.txt?x=13e88ec6e00000
kernel config: https://syzkaller.appspot.com/x/.config?x=f2f3ef188b7e16cf
dashboard link: https://syzkaller.appspot.com/bug?extid=4ec99438ed7450da6272
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1722c5c1e00000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=167aee3ee00000
Reported-by: syzbot+4ec99438ed7450da6272@syzkaller.appspotmail.com
Fixes: ab92d68fc22f ("net: core: add generic lockdep keys")
For information about bisection process see: https://goo.gl/tpsmEJ#bisection
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: WARNING: bad unlock balance in sch_direct_xmit
@ 2020-01-05 22:58 ` syzbot
0 siblings, 0 replies; 20+ messages in thread
From: syzbot @ 2020-01-05 22:58 UTC (permalink / raw)
To: a, alex.aring, allison, andrew, andy, ap420073, ast, b.a.t.m.a.n,
bridge, cleech, daniel, davem, dsa, f.fainelli, fw, gregkh,
gustavo, haiyangz, info, j.vosburgh, j, jakub.kicinski, jhs,
jiri, johan.hedberg, johannes.berg, jwi, kstewart, kvalo, kys,
linmiaohe, linux-bluetooth, linux-hams, linux-hyperv,
linux-kernel
syzbot has bisected this bug to:
commit ab92d68fc22f9afab480153bd82a20f6e2533769
Author: Taehee Yoo <ap420073@gmail.com>
Date: Mon Oct 21 18:47:51 2019 +0000
net: core: add generic lockdep keys
bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=15e88ec6e00000
start commit: 36487907 Merge branch 'akpm' (patches from Andrew)
git tree: upstream
final crash: https://syzkaller.appspot.com/x/report.txt?x=17e88ec6e00000
console output: https://syzkaller.appspot.com/x/log.txt?x=13e88ec6e00000
kernel config: https://syzkaller.appspot.com/x/.config?x=f2f3ef188b7e16cf
dashboard link: https://syzkaller.appspot.com/bug?extid=4ec99438ed7450da6272
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1722c5c1e00000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=167aee3ee00000
Reported-by: syzbot+4ec99438ed7450da6272@syzkaller.appspotmail.com
Fixes: ab92d68fc22f ("net: core: add generic lockdep keys")
For information about bisection process see: https://goo.gl/tpsmEJ#bisection
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: WARNING: bad unlock balance in sch_direct_xmit
@ 2020-01-05 22:58 ` syzbot
0 siblings, 0 replies; 20+ messages in thread
From: syzbot @ 2020-01-05 22:58 UTC (permalink / raw)
To: a, alex.aring, allison, andrew, andy, ap420073, ast, b.a.t.m.a.n,
bridge, cleech, daniel, davem, dsa, f.fainelli, fw, gregkh,
gustavo, haiyangz, info, j.vosburgh, j, jakub.kicinski, jhs,
jiri, johan.hedberg, johannes.berg, jwi, kstewart, kvalo, kys,
linmiaohe, linux-bluetooth, linux-hams, linux-hyperv,
linux-kernel, linux-ppp, linux-wireless, linux-wpan, liuhangbin,
marcel, mareklindner, mkubecek, mmanning, netdev, nikolay,
oss-drivers, paulus, ralf, roopa, sashal
syzbot has bisected this bug to:
commit ab92d68fc22f9afab480153bd82a20f6e2533769
Author: Taehee Yoo <ap420073@gmail.com>
Date: Mon Oct 21 18:47:51 2019 +0000
net: core: add generic lockdep keys
bisection log: https://syzkaller.appspot.com/x/bisect.txt?x\x15e88ec6e00000
start commit: 36487907 Merge branch 'akpm' (patches from Andrew)
git tree: upstream
final crash: https://syzkaller.appspot.com/x/report.txt?x\x17e88ec6e00000
console output: https://syzkaller.appspot.com/x/log.txt?x\x13e88ec6e00000
kernel config: https://syzkaller.appspot.com/x/.config?xòf3ef188b7e16cf
dashboard link: https://syzkaller.appspot.com/bug?extidNc99438ed7450da6272
syz repro: https://syzkaller.appspot.com/x/repro.syz?x\x1722c5c1e00000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x\x167aee3ee00000
Reported-by: syzbot+4ec99438ed7450da6272@syzkaller.appspotmail.com
Fixes: ab92d68fc22f ("net: core: add generic lockdep keys")
For information about bisection process see: https://goo.gl/tpsmEJ#bisection
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: WARNING: bad unlock balance in sch_direct_xmit
2020-01-05 22:58 ` syzbot
` (2 preceding siblings ...)
(?)
@ 2020-01-07 5:36 ` Cong Wang
2020-01-07 11:30 ` Taehee Yoo
-1 siblings, 1 reply; 20+ messages in thread
From: Cong Wang @ 2020-01-07 5:36 UTC (permalink / raw)
To: syzbot; +Cc: Taehee Yoo, Linux Kernel Network Developers
Hi, Taehee
On Sun, Jan 5, 2020 at 2:59 PM syzbot
<syzbot+4ec99438ed7450da6272@syzkaller.appspotmail.com> wrote:
>
> syzbot has bisected this bug to:
>
> commit ab92d68fc22f9afab480153bd82a20f6e2533769
> Author: Taehee Yoo <ap420073@gmail.com>
> Date: Mon Oct 21 18:47:51 2019 +0000
>
> net: core: add generic lockdep keys
Why netdev_update_lockdep_key() is needed?
It causes the bug here because the unregister and register are not
atomic although under RTNL, fast path could still lock with one key
and unlock with another key after the previous got unregistered.
From my understand of lockdep here, as long as the device itself
is not changed, it doesn't need to update those keys.
Thanks.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: WARNING: bad unlock balance in sch_direct_xmit
2020-01-07 5:36 ` Cong Wang
@ 2020-01-07 11:30 ` Taehee Yoo
2020-01-08 0:34 ` Cong Wang
0 siblings, 1 reply; 20+ messages in thread
From: Taehee Yoo @ 2020-01-07 11:30 UTC (permalink / raw)
To: Cong Wang; +Cc: syzbot, Linux Kernel Network Developers
On Tue, 7 Jan 2020 at 14:36, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> Hi, Taehee
>
Hi, Cong!
Thank you for your diagnosis.
> On Sun, Jan 5, 2020 at 2:59 PM syzbot
> <syzbot+4ec99438ed7450da6272@syzkaller.appspotmail.com> wrote:
> >
> > syzbot has bisected this bug to:
> >
> > commit ab92d68fc22f9afab480153bd82a20f6e2533769
> > Author: Taehee Yoo <ap420073@gmail.com>
> > Date: Mon Oct 21 18:47:51 2019 +0000
> >
> > net: core: add generic lockdep keys
>
> Why netdev_update_lockdep_key() is needed?
>
> It causes the bug here because the unregister and register are not
> atomic although under RTNL, fast path could still lock with one key
> and unlock with another key after the previous got unregistered.
>
> From my understand of lockdep here, as long as the device itself
> is not changed, it doesn't need to update those keys.
>
> Thanks.
The goal of netdev_update_lockdep_key() is to avoid wrong lockdep splat.
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 02916f43bf63..cea5ef66b813 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -2448,7 +2448,7 @@ static int do_set_master(struct net_device *dev,
int ifindex,
err = ops->ndo_del_slave(upper_dev, dev);
if (err)
return err;
- netdev_update_lockdep_key(dev);
+ //netdev_update_lockdep_key(dev);
} else {
return -EOPNOTSUPP;
}
Test commands:
ip link add team0 type team
ip link add team1 type team
ip link set team0 master team1
ip link set team0 nomaster
ip link set team1 master team0
Splats:
[ 105.484781][ T1178] ======================================================
[ 105.485894][ T1178] WARNING: possible circular locking dependency detected
[ 105.486791][ T1178] 5.5.0-rc2+ #264 Not tainted
[ 105.487369][ T1178] ------------------------------------------------------
[ 105.488130][ T1178] ip/1178 is trying to acquire lock:
[ 105.488948][ T1178] ffff8880521d9280
(&dev->addr_list_lock_key#4){+...}, at:
dev_uc_sync_multiple+0x95/0x120
[ 105.490336][ T1178]
[ 105.490336][ T1178] but task is already holding lock:
[ 105.491710][ T1178] ffff88806aa29280
(&dev->addr_list_lock_key#3){+...}, at: team_add_slave+0x165d/0x1972
[team]
[ 105.493471][ T1178]
[ 105.493471][ T1178] which lock already depends on the new lock.
[ 105.493471][ T1178]
[ 105.495423][ T1178]
[ 105.495423][ T1178] the existing dependency chain (in reverse order) is:
[ 105.496809][ T1178]
[ 105.496809][ T1178] -> #1 (&dev->addr_list_lock_key#3){+...}:
[ 105.497747][ T1178] _raw_spin_lock+0x30/0x70
[ 105.498201][ T1178] dev_uc_sync_multiple+0x95/0x120
[ 105.498733][ T1178] team_add_slave+0x1668/0x1972 [team]
[ 105.499293][ T1178] do_setlink+0xaab/0x2ef0
[ 105.499755][ T1178] __rtnl_newlink+0x9c5/0x1270
[ 105.500239][ T1178] rtnl_newlink+0x65/0x90
[ 105.500713][ T1178] rtnetlink_rcv_msg+0x4a8/0x890
[ 105.501269][ T1178] netlink_rcv_skb+0x121/0x350
[ 105.501799][ T1178] netlink_unicast+0x421/0x600
[ 105.502327][ T1178] netlink_sendmsg+0x65a/0xb90
[ 105.502890][ T1178] ____sys_sendmsg+0x5ce/0x7a0
[ 105.503469][ T1178] ___sys_sendmsg+0x10f/0x1b0
[ 105.504115][ T1178] __sys_sendmsg+0xc6/0x150
[ 105.504746][ T1178] do_syscall_64+0x99/0x4f0
[ 105.505391][ T1178] entry_SYSCALL_64_after_hwframe+0x49/0xbe
[ 105.506206][ T1178]
[ 105.506206][ T1178] -> #0 (&dev->addr_list_lock_key#4){+...}:
[ 105.507238][ T1178] __lock_acquire+0x2d8d/0x3de0
[ 105.507907][ T1178] lock_acquire+0x164/0x3b0
[ 105.508536][ T1178] _raw_spin_lock+0x30/0x70
[ 105.509180][ T1178] dev_uc_sync_multiple+0x95/0x120
[ 105.509825][ T1178] team_add_slave+0x1668/0x1972 [team]
[ 105.510451][ T1178] do_setlink+0xaab/0x2ef0
[ 105.510961][ T1178] __rtnl_newlink+0x9c5/0x1270
[ 105.511525][ T1178] rtnl_newlink+0x65/0x90
[ 105.512026][ T1178] rtnetlink_rcv_msg+0x4a8/0x890
[ 105.512618][ T1178] netlink_rcv_skb+0x121/0x350
[ 105.513158][ T1178] netlink_unicast+0x421/0x600
[ 105.513843][ T1178] netlink_sendmsg+0x65a/0xb90
[ 105.514524][ T1178] ____sys_sendmsg+0x5ce/0x7a0
[ 105.515186][ T1178] ___sys_sendmsg+0x10f/0x1b0
[ 105.515852][ T1178] __sys_sendmsg+0xc6/0x150
[ 105.516493][ T1178] do_syscall_64+0x99/0x4f0
[ 105.517190][ T1178] entry_SYSCALL_64_after_hwframe+0x49/0xbe
[ 105.518005][ T1178]
[ 105.518005][ T1178] other info that might help us debug this:
[ 105.518005][ T1178]
[ 105.519317][ T1178] Possible unsafe locking scenario:
[ 105.519317][ T1178]
[ 105.520268][ T1178] CPU0 CPU1
[ 105.520958][ T1178] ---- ----
[ 105.521640][ T1178] lock(&dev->addr_list_lock_key#3);
[ 105.522866][ T1178]
lock(&dev->addr_list_lock_key#4);
[ 105.523613][ T1178]
lock(&dev->addr_list_lock_key#3);
[ 105.524303][ T1178] lock(&dev->addr_list_lock_key#4);
[ 105.524890][ T1178]
[ 105.524890][ T1178] *** DEADLOCK ***
[ 105.524890][ T1178]
[ 105.525624][ T1178] 3 locks held by ip/1178:
[ 105.526010][ T1178] #0: ffffffffb0cc0b70 (rtnl_mutex){+.+.}, at:
rtnetlink_rcv_msg+0x457/0x890
[ 105.526864][ T1178] #1: ffff88806aa29c80
(team->team_lock_key#2){+.+.}, at: team_add_slave+0x89/0x1972 [team]
[ 105.527857][ T1178] #2: ffff88806aa29280
(&dev->addr_list_lock_key#3){+...}, at: team_add_slave+0x165d/0x1972
[team]
[ 105.528914][ T1178]
[ 105.528914][ T1178] stack backtrace:
[ 105.529505][ T1178] CPU: 3 PID: 1178 Comm: ip Not tainted 5.5.0-rc2+ #264
[ 105.530202][ T1178] Hardware name: innotek GmbH
VirtualBox/VirtualBox, BIOS VirtualBox 12/01/2006
[ 105.531417][ T1178] Call Trace:
[ 105.531824][ T1178] dump_stack+0x96/0xdb
[ 105.532333][ T1178] check_noncircular+0x371/0x450
[ 105.532963][ T1178] ? print_circular_bug.isra.36+0x310/0x310
[ 105.533575][ T1178] ? stack_trace_save+0x82/0xb0
[ 105.534058][ T1178] ? hlock_class+0x130/0x130
[ 105.534549][ T1178] ? __lock_acquire+0x2d8d/0x3de0
[ 105.535036][ T1178] __lock_acquire+0x2d8d/0x3de0
[ 105.535545][ T1178] ? register_lock_class+0x14d0/0x14d0
[ 105.536034][ T1178] ? find_held_lock+0x39/0x1d0
[ 105.536469][ T1178] lock_acquire+0x164/0x3b0
[ 105.536958][ T1178] ? dev_uc_sync_multiple+0x95/0x120
[ 105.537736][ T1178] _raw_spin_lock+0x30/0x70
[ 105.538398][ T1178] ? dev_uc_sync_multiple+0x95/0x120
[ 105.539260][ T1178] dev_uc_sync_multiple+0x95/0x120
[ 105.540213][ T1178] team_add_slave+0x1668/0x1972 [team]
[ 105.541113][ T1178] ? team_init+0x7b0/0x7b0 [team]
[ 105.541760][ T1178] ? mutex_is_locked+0x13/0x50
[ 105.542359][ T1178] ? rtnl_is_locked+0x11/0x20
[ 105.542984][ T1178] ? netdev_master_upper_dev_get+0xf/0x120
[ 105.543734][ T1178] do_setlink+0xaab/0x2ef0
[ 105.544296][ T1178] ? is_bpf_text_address+0x81/0xe0
[ ... ]
After "ip link set team0 master team1", the "team1 -> team0" locking path
will be recorded in lockdep key of both team1 and team0.
Then, if "ip link set team1 master team0" is executed, "team0 -> team1"
locking path also will be recorded in lockdep key. At this moment,
lockdep will catch possible deadlock situation and it prints the above
warning message. But, both "team0 -> team1" and "team1 -> team0"
will not be existing concurrently. so the above message is actually wrong.
In order to avoid this message, a recorded locking path should be
removed. So, both lockdep_unregister_key() and lockdep_register_key()
are needed.
Yes, netdev_update_lockdep_key() is not atomic so
"WARNING: bad unlock balance in sch_direct_xmit"
message could be printed.
Thank you so much!
Taehee Yoo
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: WARNING: bad unlock balance in sch_direct_xmit
2020-01-07 11:30 ` Taehee Yoo
@ 2020-01-08 0:34 ` Cong Wang
2020-01-08 11:42 ` Taehee Yoo
0 siblings, 1 reply; 20+ messages in thread
From: Cong Wang @ 2020-01-08 0:34 UTC (permalink / raw)
To: Taehee Yoo; +Cc: syzbot, Linux Kernel Network Developers
On Tue, Jan 7, 2020 at 3:31 AM Taehee Yoo <ap420073@gmail.com> wrote:
> After "ip link set team0 master team1", the "team1 -> team0" locking path
> will be recorded in lockdep key of both team1 and team0.
> Then, if "ip link set team1 master team0" is executed, "team0 -> team1"
> locking path also will be recorded in lockdep key. At this moment,
> lockdep will catch possible deadlock situation and it prints the above
> warning message. But, both "team0 -> team1" and "team1 -> team0"
> will not be existing concurrently. so the above message is actually wrong.
> In order to avoid this message, a recorded locking path should be
> removed. So, both lockdep_unregister_key() and lockdep_register_key()
> are needed.
>
So, after you move the key down to each netdevice, they are now treated
as different locks. Is this stacked device scenario the reason why you
move it to per-netdevice? If so, I wonder why not just use nested locks?
Like:
netif_addr_nested_lock(upper, 0);
netif_addr_nested_lock(lower, 1);
netif_addr_nested_unlock(lower);
netif_addr_nested_unlock(upper);
For this case, they could still share a same key.
Thanks for the details!
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: WARNING: bad unlock balance in sch_direct_xmit
2020-01-08 0:34 ` Cong Wang
@ 2020-01-08 11:42 ` Taehee Yoo
2020-01-09 23:38 ` Cong Wang
2020-01-13 11:08 ` Dmitry Vyukov
0 siblings, 2 replies; 20+ messages in thread
From: Taehee Yoo @ 2020-01-08 11:42 UTC (permalink / raw)
To: Cong Wang; +Cc: syzbot, Linux Kernel Network Developers
On Wed, 8 Jan 2020 at 09:34, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> On Tue, Jan 7, 2020 at 3:31 AM Taehee Yoo <ap420073@gmail.com> wrote:
> > After "ip link set team0 master team1", the "team1 -> team0" locking path
> > will be recorded in lockdep key of both team1 and team0.
> > Then, if "ip link set team1 master team0" is executed, "team0 -> team1"
> > locking path also will be recorded in lockdep key. At this moment,
> > lockdep will catch possible deadlock situation and it prints the above
> > warning message. But, both "team0 -> team1" and "team1 -> team0"
> > will not be existing concurrently. so the above message is actually wrong.
> > In order to avoid this message, a recorded locking path should be
> > removed. So, both lockdep_unregister_key() and lockdep_register_key()
> > are needed.
> >
>
> So, after you move the key down to each netdevice, they are now treated
> as different locks. Is this stacked device scenario the reason why you
> move it to per-netdevice? If so, I wonder why not just use nested locks?
> Like:
>
> netif_addr_nested_lock(upper, 0);
> netif_addr_nested_lock(lower, 1);
> netif_addr_nested_unlock(lower);
> netif_addr_nested_unlock(upper);
>
> For this case, they could still share a same key.
>
> Thanks for the details!
Yes, the reason for using dynamic lockdep key is to avoid lockdep
warning in stacked device scenario.
But, the addr_list_lock case is a little bit different.
There was a bug in netif_addr_lock_nested() that
"dev->netdev_ops->ndo_get_lock_subclass" isn't updated after "master"
and "nomaster" command.
So, the wrong subclass is used, so lockdep warning message was printed.
There were some ways to fix this problem, using dynamic key is just one
of them. I think using the correct subclass in netif_addr_lock_nested()
is also a correct way to fix that problem. Another minor reason was that
the subclass is limited by 8. but dynamic key has no limitation.
Unfortunately, dynamic key has a problem too.
lockdep limits the maximum number of lockdep keys.
$cat /proc/lockdep_stats
lock-classes: 1228 [max: 8192]
So, If so many network interfaces are created, they use so many
lockdep keys. If so, lockdep will stop.
This is the cause of "BUG: MAX_LOCKDEP_KEYS too low!".
Thank you!
Taehee Yoo
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: WARNING: bad unlock balance in sch_direct_xmit
2020-01-08 11:42 ` Taehee Yoo
@ 2020-01-09 23:38 ` Cong Wang
2020-01-10 3:06 ` Taehee Yoo
2020-01-13 11:08 ` Dmitry Vyukov
1 sibling, 1 reply; 20+ messages in thread
From: Cong Wang @ 2020-01-09 23:38 UTC (permalink / raw)
To: Taehee Yoo; +Cc: syzbot, Linux Kernel Network Developers
On Wed, Jan 8, 2020 at 3:43 AM Taehee Yoo <ap420073@gmail.com> wrote:
>
> On Wed, 8 Jan 2020 at 09:34, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >
> > On Tue, Jan 7, 2020 at 3:31 AM Taehee Yoo <ap420073@gmail.com> wrote:
> > > After "ip link set team0 master team1", the "team1 -> team0" locking path
> > > will be recorded in lockdep key of both team1 and team0.
> > > Then, if "ip link set team1 master team0" is executed, "team0 -> team1"
> > > locking path also will be recorded in lockdep key. At this moment,
> > > lockdep will catch possible deadlock situation and it prints the above
> > > warning message. But, both "team0 -> team1" and "team1 -> team0"
> > > will not be existing concurrently. so the above message is actually wrong.
> > > In order to avoid this message, a recorded locking path should be
> > > removed. So, both lockdep_unregister_key() and lockdep_register_key()
> > > are needed.
> > >
> >
> > So, after you move the key down to each netdevice, they are now treated
> > as different locks. Is this stacked device scenario the reason why you
> > move it to per-netdevice? If so, I wonder why not just use nested locks?
> > Like:
> >
> > netif_addr_nested_lock(upper, 0);
> > netif_addr_nested_lock(lower, 1);
> > netif_addr_nested_unlock(lower);
> > netif_addr_nested_unlock(upper);
> >
> > For this case, they could still share a same key.
> >
> > Thanks for the details!
>
> Yes, the reason for using dynamic lockdep key is to avoid lockdep
> warning in stacked device scenario.
> But, the addr_list_lock case is a little bit different.
> There was a bug in netif_addr_lock_nested() that
> "dev->netdev_ops->ndo_get_lock_subclass" isn't updated after "master"
> and "nomaster" command.
> So, the wrong subclass is used, so lockdep warning message was printed.
Hmm? I never propose netdev_ops->ndo_get_lock_subclass(), and
the subclasses are always 0,1, no matter which is the master device,
so it doesn't need a ops.
> There were some ways to fix this problem, using dynamic key is just one
> of them. I think using the correct subclass in netif_addr_lock_nested()
> is also a correct way to fix that problem. Another minor reason was that
> the subclass is limited by 8. but dynamic key has no limitation.
Yeah, but in practice I believe 8 is sufficient for stacked devices.
>
> Unfortunately, dynamic key has a problem too.
> lockdep limits the maximum number of lockdep keys.
Right, and also the problem reported by syzbot, that is not safe
during unregister and register.
Anyway, do you think we should revert back to the static keys
and use subclass to address the lockdep issue instead?
Thanks!
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: WARNING: bad unlock balance in sch_direct_xmit
2020-01-09 23:38 ` Cong Wang
@ 2020-01-10 3:06 ` Taehee Yoo
2020-01-10 4:43 ` Cong Wang
0 siblings, 1 reply; 20+ messages in thread
From: Taehee Yoo @ 2020-01-10 3:06 UTC (permalink / raw)
To: Cong Wang; +Cc: syzbot, Linux Kernel Network Developers
On Fri, 10 Jan 2020 at 08:38, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> On Wed, Jan 8, 2020 at 3:43 AM Taehee Yoo <ap420073@gmail.com> wrote:
> >
> > On Wed, 8 Jan 2020 at 09:34, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > >
> > > On Tue, Jan 7, 2020 at 3:31 AM Taehee Yoo <ap420073@gmail.com> wrote:
> > > > After "ip link set team0 master team1", the "team1 -> team0" locking path
> > > > will be recorded in lockdep key of both team1 and team0.
> > > > Then, if "ip link set team1 master team0" is executed, "team0 -> team1"
> > > > locking path also will be recorded in lockdep key. At this moment,
> > > > lockdep will catch possible deadlock situation and it prints the above
> > > > warning message. But, both "team0 -> team1" and "team1 -> team0"
> > > > will not be existing concurrently. so the above message is actually wrong.
> > > > In order to avoid this message, a recorded locking path should be
> > > > removed. So, both lockdep_unregister_key() and lockdep_register_key()
> > > > are needed.
> > > >
> > >
> > > So, after you move the key down to each netdevice, they are now treated
> > > as different locks. Is this stacked device scenario the reason why you
> > > move it to per-netdevice? If so, I wonder why not just use nested locks?
> > > Like:
> > >
> > > netif_addr_nested_lock(upper, 0);
> > > netif_addr_nested_lock(lower, 1);
> > > netif_addr_nested_unlock(lower);
> > > netif_addr_nested_unlock(upper);
> > >
> > > For this case, they could still share a same key.
> > >
> > > Thanks for the details!
> >
> > Yes, the reason for using dynamic lockdep key is to avoid lockdep
> > warning in stacked device scenario.
> > But, the addr_list_lock case is a little bit different.
> > There was a bug in netif_addr_lock_nested() that
> > "dev->netdev_ops->ndo_get_lock_subclass" isn't updated after "master"
> > and "nomaster" command.
> > So, the wrong subclass is used, so lockdep warning message was printed.
>
> Hmm? I never propose netdev_ops->ndo_get_lock_subclass(), and
> the subclasses are always 0,1, no matter which is the master device,
> so it doesn't need a ops.
>
It's just the reason why the dynamic lockdep key was adopted instead of
a nested lock.
>
> > There were some ways to fix this problem, using dynamic key is just one
> > of them. I think using the correct subclass in netif_addr_lock_nested()
> > is also a correct way to fix that problem. Another minor reason was that
> > the subclass is limited by 8. but dynamic key has no limitation.
>
> Yeah, but in practice I believe 8 is sufficient for stacked devices.
>
I agree with this.
>
> >
> > Unfortunately, dynamic key has a problem too.
> > lockdep limits the maximum number of lockdep keys.
>
>
> Right, and also the problem reported by syzbot, that is not safe
> during unregister and register.
>
qdisc_xmit_lock_key has this problem.
But, I'm not sure about addr_list_lock_key.
If addr_list_lock is used outside of RTNL, it has this problem.
If it isn't used outside of RTNL, it doesn't have this problem.
> Anyway, do you think we should revert back to the static keys
> and use subclass to address the lockdep issue instead?
>
> Thanks!
I agree with this to reduce the number of dynamic lockdep keys.
Thanks a lot!
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: WARNING: bad unlock balance in sch_direct_xmit
2020-01-10 3:06 ` Taehee Yoo
@ 2020-01-10 4:43 ` Cong Wang
2020-01-10 6:02 ` Taehee Yoo
0 siblings, 1 reply; 20+ messages in thread
From: Cong Wang @ 2020-01-10 4:43 UTC (permalink / raw)
To: Taehee Yoo; +Cc: syzbot, Linux Kernel Network Developers
On Thu, Jan 9, 2020 at 7:06 PM Taehee Yoo <ap420073@gmail.com> wrote:
>
> On Fri, 10 Jan 2020 at 08:38, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >
> > On Wed, Jan 8, 2020 at 3:43 AM Taehee Yoo <ap420073@gmail.com> wrote:
> > >
> > > On Wed, 8 Jan 2020 at 09:34, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > > >
> > > > On Tue, Jan 7, 2020 at 3:31 AM Taehee Yoo <ap420073@gmail.com> wrote:
> > > > > After "ip link set team0 master team1", the "team1 -> team0" locking path
> > > > > will be recorded in lockdep key of both team1 and team0.
> > > > > Then, if "ip link set team1 master team0" is executed, "team0 -> team1"
> > > > > locking path also will be recorded in lockdep key. At this moment,
> > > > > lockdep will catch possible deadlock situation and it prints the above
> > > > > warning message. But, both "team0 -> team1" and "team1 -> team0"
> > > > > will not be existing concurrently. so the above message is actually wrong.
> > > > > In order to avoid this message, a recorded locking path should be
> > > > > removed. So, both lockdep_unregister_key() and lockdep_register_key()
> > > > > are needed.
> > > > >
> > > >
> > > > So, after you move the key down to each netdevice, they are now treated
> > > > as different locks. Is this stacked device scenario the reason why you
> > > > move it to per-netdevice? If so, I wonder why not just use nested locks?
> > > > Like:
> > > >
> > > > netif_addr_nested_lock(upper, 0);
> > > > netif_addr_nested_lock(lower, 1);
> > > > netif_addr_nested_unlock(lower);
> > > > netif_addr_nested_unlock(upper);
> > > >
> > > > For this case, they could still share a same key.
> > > >
> > > > Thanks for the details!
> > >
> > > Yes, the reason for using dynamic lockdep key is to avoid lockdep
> > > warning in stacked device scenario.
> > > But, the addr_list_lock case is a little bit different.
> > > There was a bug in netif_addr_lock_nested() that
> > > "dev->netdev_ops->ndo_get_lock_subclass" isn't updated after "master"
> > > and "nomaster" command.
> > > So, the wrong subclass is used, so lockdep warning message was printed.
> >
> > Hmm? I never propose netdev_ops->ndo_get_lock_subclass(), and
> > the subclasses are always 0,1, no matter which is the master device,
> > so it doesn't need a ops.
> >
>
> It's just the reason why the dynamic lockdep key was adopted instead of
> a nested lock.
Oh, but why? :) As I said, at least for the addr lock case, we can always
pass subclass in the same order as they are called if we switch it back
to static keys.
>
> >
> > > There were some ways to fix this problem, using dynamic key is just one
> > > of them. I think using the correct subclass in netif_addr_lock_nested()
> > > is also a correct way to fix that problem. Another minor reason was that
> > > the subclass is limited by 8. but dynamic key has no limitation.
> >
> > Yeah, but in practice I believe 8 is sufficient for stacked devices.
> >
>
> I agree with this.
>
> >
> > >
> > > Unfortunately, dynamic key has a problem too.
> > > lockdep limits the maximum number of lockdep keys.
> >
> >
> > Right, and also the problem reported by syzbot, that is not safe
> > during unregister and register.
> >
>
> qdisc_xmit_lock_key has this problem.
> But, I'm not sure about addr_list_lock_key.
> If addr_list_lock is used outside of RTNL, it has this problem.
> If it isn't used outside of RTNL, it doesn't have this problem.
Yeah, I am aware.
>
> > Anyway, do you think we should revert back to the static keys
> > and use subclass to address the lockdep issue instead?
> >
> > Thanks!
>
> I agree with this to reduce the number of dynamic lockdep keys.
I am trying to fix this syzbot warning, not to address the key limit.
The reason is that I think dynamic keys are not necessary and
not able to be used safely in this case. What I am still not sure
is whether using subclass (with static keys) could address the
lockdep issue you fixed with dynamic keys.
Thanks!
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: WARNING: bad unlock balance in sch_direct_xmit
2020-01-10 4:43 ` Cong Wang
@ 2020-01-10 6:02 ` Taehee Yoo
2020-01-11 21:53 ` Cong Wang
0 siblings, 1 reply; 20+ messages in thread
From: Taehee Yoo @ 2020-01-10 6:02 UTC (permalink / raw)
To: Cong Wang; +Cc: syzbot, Linux Kernel Network Developers
On Fri, 10 Jan 2020 at 13:43, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> On Thu, Jan 9, 2020 at 7:06 PM Taehee Yoo <ap420073@gmail.com> wrote:
> >
> > On Fri, 10 Jan 2020 at 08:38, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > >
> > > On Wed, Jan 8, 2020 at 3:43 AM Taehee Yoo <ap420073@gmail.com> wrote:
> > > >
> > > > On Wed, 8 Jan 2020 at 09:34, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > > > >
> > > > > On Tue, Jan 7, 2020 at 3:31 AM Taehee Yoo <ap420073@gmail.com> wrote:
> > > > > > After "ip link set team0 master team1", the "team1 -> team0" locking path
> > > > > > will be recorded in lockdep key of both team1 and team0.
> > > > > > Then, if "ip link set team1 master team0" is executed, "team0 -> team1"
> > > > > > locking path also will be recorded in lockdep key. At this moment,
> > > > > > lockdep will catch possible deadlock situation and it prints the above
> > > > > > warning message. But, both "team0 -> team1" and "team1 -> team0"
> > > > > > will not be existing concurrently. so the above message is actually wrong.
> > > > > > In order to avoid this message, a recorded locking path should be
> > > > > > removed. So, both lockdep_unregister_key() and lockdep_register_key()
> > > > > > are needed.
> > > > > >
> > > > >
> > > > > So, after you move the key down to each netdevice, they are now treated
> > > > > as different locks. Is this stacked device scenario the reason why you
> > > > > move it to per-netdevice? If so, I wonder why not just use nested locks?
> > > > > Like:
> > > > >
> > > > > netif_addr_nested_lock(upper, 0);
> > > > > netif_addr_nested_lock(lower, 1);
> > > > > netif_addr_nested_unlock(lower);
> > > > > netif_addr_nested_unlock(upper);
> > > > >
> > > > > For this case, they could still share a same key.
> > > > >
> > > > > Thanks for the details!
> > > >
> > > > Yes, the reason for using dynamic lockdep key is to avoid lockdep
> > > > warning in stacked device scenario.
> > > > But, the addr_list_lock case is a little bit different.
> > > > There was a bug in netif_addr_lock_nested() that
> > > > "dev->netdev_ops->ndo_get_lock_subclass" isn't updated after "master"
> > > > and "nomaster" command.
> > > > So, the wrong subclass is used, so lockdep warning message was printed.
> > >
> > > Hmm? I never propose netdev_ops->ndo_get_lock_subclass(), and
> > > the subclasses are always 0,1, no matter which is the master device,
> > > so it doesn't need a ops.
> > >
> >
> > It's just the reason why the dynamic lockdep key was adopted instead of
> > a nested lock.
>
> Oh, but why? :) As I said, at least for the addr lock case, we can always
> pass subclass in the same order as they are called if we switch it back
> to static keys.
>
ndo_get_lock_subclass() was used to calculate subclass which was used by
netif_addr_lock_nested().
-static inline void netif_addr_lock_nested(struct net_device *dev)
-{
- int subclass = SINGLE_DEPTH_NESTING;
-
- if (dev->netdev_ops->ndo_get_lock_subclass)
- subclass = dev->netdev_ops->ndo_get_lock_subclass(dev);
-
- spin_lock_nested(&dev->addr_list_lock, subclass);
-}
The most important thing about nested lock is to get the correct subclass.
nest_level was used as subclass and this was calculated by
->ndo_get_lock_subclass().
But, ->ndo_get_lock_subclass() didn't calculate correct subclass.
After "master" and "nomaster" operations, nest_level should be updated
recursively, but it didn't. So incorrect subclass was used.
team3 <-- subclass 0
"ip link set team3 master team2"
team2 <-- subclass 0
team3 <-- subclass 1
"ip link set team2 master team1"
team1 <-- subclass 0
team3 <-- subclass 1
team3 <-- subclass 1
"ip link set team1 master team0"
team0 <-- subclass 0
team1 <-- subclass 1
team3 <-- subclass 1
team3 <-- subclass 1
After "master" and "nomaster" operation, subclass values of all lower or
upper interfaces would be changed. But ->ndo_get_lock_subclass()
didn't update subclass recursively, lockdep warning appeared.
In order to fix this, I had two ways.
1. use dynamic keys instead of static keys.
2. fix ndo_get_lock_subclass().
The reason why I adopted using dynamic keys instead of fixing
->ndo_get_lock_subclass() is that the ->ndo_get_lock_subclass() isn't
a common helper function.
So, driver writers should implement ->ndo_get_lock_subclass().
If we use dynamic keys, ->ndo_get_lock_subclass() code could be removed.
> >
> > >
> > > > There were some ways to fix this problem, using dynamic key is just one
> > > > of them. I think using the correct subclass in netif_addr_lock_nested()
> > > > is also a correct way to fix that problem. Another minor reason was that
> > > > the subclass is limited by 8. but dynamic key has no limitation.
> > >
> > > Yeah, but in practice I believe 8 is sufficient for stacked devices.
> > >
> >
> > I agree with this.
> >
> > >
> > > >
> > > > Unfortunately, dynamic key has a problem too.
> > > > lockdep limits the maximum number of lockdep keys.
> > >
> > >
> > > Right, and also the problem reported by syzbot, that is not safe
> > > during unregister and register.
> > >
> >
> > qdisc_xmit_lock_key has this problem.
> > But, I'm not sure about addr_list_lock_key.
> > If addr_list_lock is used outside of RTNL, it has this problem.
> > If it isn't used outside of RTNL, it doesn't have this problem.
>
> Yeah, I am aware.
>
>
> >
> > > Anyway, do you think we should revert back to the static keys
> > > and use subclass to address the lockdep issue instead?
> > >
> > > Thanks!
> >
> > I agree with this to reduce the number of dynamic lockdep keys.
>
> I am trying to fix this syzbot warning, not to address the key limit.
>
> The reason is that I think dynamic keys are not necessary and
> not able to be used safely in this case. What I am still not sure
> is whether using subclass (with static keys) could address the
> lockdep issue you fixed with dynamic keys.
>
> Thanks!
What I fixed problems with dynamic lockdep keys could be fixed by
nested lock too. I think if the subclass value synchronization routine
works well, there will be no problem.
Thanks a lot!
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: WARNING: bad unlock balance in sch_direct_xmit
2020-01-10 6:02 ` Taehee Yoo
@ 2020-01-11 21:53 ` Cong Wang
2020-01-11 23:28 ` Cong Wang
2020-01-13 10:13 ` Taehee Yoo
0 siblings, 2 replies; 20+ messages in thread
From: Cong Wang @ 2020-01-11 21:53 UTC (permalink / raw)
To: Taehee Yoo; +Cc: syzbot, Linux Kernel Network Developers
On Thu, Jan 9, 2020 at 10:02 PM Taehee Yoo <ap420073@gmail.com> wrote:
> ndo_get_lock_subclass() was used to calculate subclass which was used by
> netif_addr_lock_nested().
>
> -static inline void netif_addr_lock_nested(struct net_device *dev)
> -{
> - int subclass = SINGLE_DEPTH_NESTING;
> -
> - if (dev->netdev_ops->ndo_get_lock_subclass)
> - subclass = dev->netdev_ops->ndo_get_lock_subclass(dev);
> -
> - spin_lock_nested(&dev->addr_list_lock, subclass);
> -}
>
> The most important thing about nested lock is to get the correct subclass.
> nest_level was used as subclass and this was calculated by
> ->ndo_get_lock_subclass().
> But, ->ndo_get_lock_subclass() didn't calculate correct subclass.
> After "master" and "nomaster" operations, nest_level should be updated
> recursively, but it didn't. So incorrect subclass was used.
>
> team3 <-- subclass 0
>
> "ip link set team3 master team2"
>
> team2 <-- subclass 0
> team3 <-- subclass 1
>
> "ip link set team2 master team1"
>
> team1 <-- subclass 0
> team3 <-- subclass 1
> team3 <-- subclass 1
>
> "ip link set team1 master team0"
>
> team0 <-- subclass 0
> team1 <-- subclass 1
> team3 <-- subclass 1
> team3 <-- subclass 1
>
> After "master" and "nomaster" operation, subclass values of all lower or
> upper interfaces would be changed. But ->ndo_get_lock_subclass()
> didn't update subclass recursively, lockdep warning appeared.
> In order to fix this, I had two ways.
> 1. use dynamic keys instead of static keys.
> 2. fix ndo_get_lock_subclass().
>
> The reason why I adopted using dynamic keys instead of fixing
> ->ndo_get_lock_subclass() is that the ->ndo_get_lock_subclass() isn't
> a common helper function.
> So, driver writers should implement ->ndo_get_lock_subclass().
> If we use dynamic keys, ->ndo_get_lock_subclass() code could be removed.
>
The details you provide here are really helpful for me to understand
the reasons behind your changes. Let me think about this and see how
I could address both problems. This appears to be harder than I originally
thought.
>
> What I fixed problems with dynamic lockdep keys could be fixed by
> nested lock too. I think if the subclass value synchronization routine
> works well, there will be no problem.
Great! We are on the same page.
Thanks for all the information and the reproducer too!
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: WARNING: bad unlock balance in sch_direct_xmit
2020-01-11 21:53 ` Cong Wang
@ 2020-01-11 23:28 ` Cong Wang
2020-01-13 10:53 ` Taehee Yoo
2020-01-13 10:13 ` Taehee Yoo
1 sibling, 1 reply; 20+ messages in thread
From: Cong Wang @ 2020-01-11 23:28 UTC (permalink / raw)
To: Taehee Yoo; +Cc: syzbot, Linux Kernel Network Developers
On Sat, Jan 11, 2020 at 1:53 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> The details you provide here are really helpful for me to understand
> the reasons behind your changes. Let me think about this and see how
> I could address both problems. This appears to be harder than I originally
> thought.
Do you think the following patch will make everyone happy?
diff --git a/net/core/dev.c b/net/core/dev.c
index 0ad39c87b7fd..7e885d069707 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -9177,22 +9177,10 @@ static void
netdev_unregister_lockdep_key(struct net_device *dev)
void netdev_update_lockdep_key(struct net_device *dev)
{
- struct netdev_queue *queue;
- int i;
-
- lockdep_unregister_key(&dev->qdisc_xmit_lock_key);
lockdep_unregister_key(&dev->addr_list_lock_key);
-
- lockdep_register_key(&dev->qdisc_xmit_lock_key);
lockdep_register_key(&dev->addr_list_lock_key);
lockdep_set_class(&dev->addr_list_lock, &dev->addr_list_lock_key);
- for (i = 0; i < dev->num_tx_queues; i++) {
- queue = netdev_get_tx_queue(dev, i);
-
- lockdep_set_class(&queue->_xmit_lock,
- &dev->qdisc_xmit_lock_key);
- }
}
EXPORT_SYMBOL(netdev_update_lockdep_key);
I think as long as we don't take _xmit_lock nestedly, it is fine. And
most (or all?) of the software netdev's are already lockless, so I can't
think of any case we take more than one _xmit_lock on TX path.
I tested it with the syzbot reproducer and your set master/nomaster
commands, I don't get any lockdep splat.
What do you think?
Thanks!
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: WARNING: bad unlock balance in sch_direct_xmit
2020-01-11 23:28 ` Cong Wang
@ 2020-01-13 10:53 ` Taehee Yoo
2020-01-14 19:39 ` Cong Wang
0 siblings, 1 reply; 20+ messages in thread
From: Taehee Yoo @ 2020-01-13 10:53 UTC (permalink / raw)
To: Cong Wang; +Cc: syzbot, Linux Kernel Network Developers
On Sun, 12 Jan 2020 at 08:28, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> On Sat, Jan 11, 2020 at 1:53 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > The details you provide here are really helpful for me to understand
> > the reasons behind your changes. Let me think about this and see how
> > I could address both problems. This appears to be harder than I originally
> > thought.
>
> Do you think the following patch will make everyone happy?
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 0ad39c87b7fd..7e885d069707 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -9177,22 +9177,10 @@ static void
> netdev_unregister_lockdep_key(struct net_device *dev)
>
> void netdev_update_lockdep_key(struct net_device *dev)
> {
> - struct netdev_queue *queue;
> - int i;
> -
> - lockdep_unregister_key(&dev->qdisc_xmit_lock_key);
> lockdep_unregister_key(&dev->addr_list_lock_key);
> -
> - lockdep_register_key(&dev->qdisc_xmit_lock_key);
> lockdep_register_key(&dev->addr_list_lock_key);
>
> lockdep_set_class(&dev->addr_list_lock, &dev->addr_list_lock_key);
> - for (i = 0; i < dev->num_tx_queues; i++) {
> - queue = netdev_get_tx_queue(dev, i);
> -
> - lockdep_set_class(&queue->_xmit_lock,
> - &dev->qdisc_xmit_lock_key);
> - }
> }
> EXPORT_SYMBOL(netdev_update_lockdep_key);
>
> I think as long as we don't take _xmit_lock nestedly, it is fine. And
> most (or all?) of the software netdev's are already lockless, so I can't
> think of any case we take more than one _xmit_lock on TX path.
>
> I tested it with the syzbot reproducer and your set master/nomaster
> commands, I don't get any lockdep splat.
>
> What do you think?
>
> Thanks!
I have tested this approach and I have no found any problem.
As you said, most of virtual interfaces are already lockless.
So, generally lockdep warning will not occur.
I found two virtual interfaces that they don't have LLTX and they also
could be upper interface. Interfaces are "rmnet" and virt_wifi" type.
My test case is here.
[Before]
master0(bond or team or bridge)
|
slave0(rmnet or virt_wifi)
|
master1
|
slave1
|
master2
|
veth
[After]
master0(bond or team or bridge)
|
slave1(rmnet or virt_wifi)
|
master2
|
slave0
|
master1
|
veth
In this test, the ordering of slave1 and slave0 will be changed.
But, rmnet and virt_wifi type interface couldn't be slave of bond, team,
and bridge type interface. So, This graph will not be made.
So, I agree with this approach.
Thank you so much!
Taehee Yoo
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: WARNING: bad unlock balance in sch_direct_xmit
2020-01-13 10:53 ` Taehee Yoo
@ 2020-01-14 19:39 ` Cong Wang
0 siblings, 0 replies; 20+ messages in thread
From: Cong Wang @ 2020-01-14 19:39 UTC (permalink / raw)
To: Taehee Yoo; +Cc: syzbot, Linux Kernel Network Developers
On Mon, Jan 13, 2020 at 2:53 AM Taehee Yoo <ap420073@gmail.com> wrote:
>
> On Sun, 12 Jan 2020 at 08:28, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >
> > On Sat, Jan 11, 2020 at 1:53 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > > The details you provide here are really helpful for me to understand
> > > the reasons behind your changes. Let me think about this and see how
> > > I could address both problems. This appears to be harder than I originally
> > > thought.
> >
> > Do you think the following patch will make everyone happy?
> >
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index 0ad39c87b7fd..7e885d069707 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -9177,22 +9177,10 @@ static void
> > netdev_unregister_lockdep_key(struct net_device *dev)
> >
> > void netdev_update_lockdep_key(struct net_device *dev)
> > {
> > - struct netdev_queue *queue;
> > - int i;
> > -
> > - lockdep_unregister_key(&dev->qdisc_xmit_lock_key);
> > lockdep_unregister_key(&dev->addr_list_lock_key);
> > -
> > - lockdep_register_key(&dev->qdisc_xmit_lock_key);
> > lockdep_register_key(&dev->addr_list_lock_key);
> >
> > lockdep_set_class(&dev->addr_list_lock, &dev->addr_list_lock_key);
> > - for (i = 0; i < dev->num_tx_queues; i++) {
> > - queue = netdev_get_tx_queue(dev, i);
> > -
> > - lockdep_set_class(&queue->_xmit_lock,
> > - &dev->qdisc_xmit_lock_key);
> > - }
> > }
> > EXPORT_SYMBOL(netdev_update_lockdep_key);
> >
> > I think as long as we don't take _xmit_lock nestedly, it is fine. And
> > most (or all?) of the software netdev's are already lockless, so I can't
> > think of any case we take more than one _xmit_lock on TX path.
> >
> > I tested it with the syzbot reproducer and your set master/nomaster
> > commands, I don't get any lockdep splat.
> >
> > What do you think?
> >
> > Thanks!
>
> I have tested this approach and I have no found any problem.
> As you said, most of virtual interfaces are already lockless.
> So, generally lockdep warning will not occur.
> I found two virtual interfaces that they don't have LLTX and they also
> could be upper interface. Interfaces are "rmnet" and virt_wifi" type.
>
> My test case is here.
>
> [Before]
> master0(bond or team or bridge)
> |
> slave0(rmnet or virt_wifi)
> |
> master1
> |
> slave1
> |
> master2
> |
> veth
>
> [After]
> master0(bond or team or bridge)
> |
> slave1(rmnet or virt_wifi)
> |
> master2
> |
> slave0
> |
> master1
> |
> veth
>
> In this test, the ordering of slave1 and slave0 will be changed.
> But, rmnet and virt_wifi type interface couldn't be slave of bond, team,
> and bridge type interface. So, This graph will not be made.
> So, I agree with this approach.
Thanks for reviewing it and auditing more network devices.
I will send out the patch formally.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: WARNING: bad unlock balance in sch_direct_xmit
2020-01-11 21:53 ` Cong Wang
2020-01-11 23:28 ` Cong Wang
@ 2020-01-13 10:13 ` Taehee Yoo
1 sibling, 0 replies; 20+ messages in thread
From: Taehee Yoo @ 2020-01-13 10:13 UTC (permalink / raw)
To: Cong Wang; +Cc: syzbot, Linux Kernel Network Developers
On Sun, 12 Jan 2020 at 06:53, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> On Thu, Jan 9, 2020 at 10:02 PM Taehee Yoo <ap420073@gmail.com> wrote:
> > ndo_get_lock_subclass() was used to calculate subclass which was used by
> > netif_addr_lock_nested().
> >
> > -static inline void netif_addr_lock_nested(struct net_device *dev)
> > -{
> > - int subclass = SINGLE_DEPTH_NESTING;
> > -
> > - if (dev->netdev_ops->ndo_get_lock_subclass)
> > - subclass = dev->netdev_ops->ndo_get_lock_subclass(dev);
> > -
> > - spin_lock_nested(&dev->addr_list_lock, subclass);
> > -}
> >
> > The most important thing about nested lock is to get the correct subclass.
> > nest_level was used as subclass and this was calculated by
> > ->ndo_get_lock_subclass().
> > But, ->ndo_get_lock_subclass() didn't calculate correct subclass.
> > After "master" and "nomaster" operations, nest_level should be updated
> > recursively, but it didn't. So incorrect subclass was used.
> >
> > team3 <-- subclass 0
> >
> > "ip link set team3 master team2"
> >
> > team2 <-- subclass 0
> > team3 <-- subclass 1
> >
> > "ip link set team2 master team1"
> >
> > team1 <-- subclass 0
> > team3 <-- subclass 1
> > team3 <-- subclass 1
> >
> > "ip link set team1 master team0"
> >
> > team0 <-- subclass 0
> > team1 <-- subclass 1
> > team3 <-- subclass 1
> > team3 <-- subclass 1
> >
> > After "master" and "nomaster" operation, subclass values of all lower or
> > upper interfaces would be changed. But ->ndo_get_lock_subclass()
> > didn't update subclass recursively, lockdep warning appeared.
> > In order to fix this, I had two ways.
> > 1. use dynamic keys instead of static keys.
> > 2. fix ndo_get_lock_subclass().
> >
> > The reason why I adopted using dynamic keys instead of fixing
> > ->ndo_get_lock_subclass() is that the ->ndo_get_lock_subclass() isn't
> > a common helper function.
> > So, driver writers should implement ->ndo_get_lock_subclass().
> > If we use dynamic keys, ->ndo_get_lock_subclass() code could be removed.
> >
>
> The details you provide here are really helpful for me to understand
> the reasons behind your changes. Let me think about this and see how
> I could address both problems. This appears to be harder than I originally
> thought.
>
> >
> > What I fixed problems with dynamic lockdep keys could be fixed by
> > nested lock too. I think if the subclass value synchronization routine
> > works well, there will be no problem.
>
> Great! We are on the same page.
>
> Thanks for all the information and the reproducer too!
I really glad my explanation helps you!
Thank you so much!
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: WARNING: bad unlock balance in sch_direct_xmit
2020-01-08 11:42 ` Taehee Yoo
2020-01-09 23:38 ` Cong Wang
@ 2020-01-13 11:08 ` Dmitry Vyukov
1 sibling, 0 replies; 20+ messages in thread
From: Dmitry Vyukov @ 2020-01-13 11:08 UTC (permalink / raw)
To: Taehee Yoo; +Cc: Cong Wang, syzbot, Linux Kernel Network Developers
On Wed, Jan 8, 2020 at 12:44 PM Taehee Yoo <ap420073@gmail.com> wrote:
>
> On Wed, 8 Jan 2020 at 09:34, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >
> > On Tue, Jan 7, 2020 at 3:31 AM Taehee Yoo <ap420073@gmail.com> wrote:
> > > After "ip link set team0 master team1", the "team1 -> team0" locking path
> > > will be recorded in lockdep key of both team1 and team0.
> > > Then, if "ip link set team1 master team0" is executed, "team0 -> team1"
> > > locking path also will be recorded in lockdep key. At this moment,
> > > lockdep will catch possible deadlock situation and it prints the above
> > > warning message. But, both "team0 -> team1" and "team1 -> team0"
> > > will not be existing concurrently. so the above message is actually wrong.
> > > In order to avoid this message, a recorded locking path should be
> > > removed. So, both lockdep_unregister_key() and lockdep_register_key()
> > > are needed.
> > >
> >
> > So, after you move the key down to each netdevice, they are now treated
> > as different locks. Is this stacked device scenario the reason why you
> > move it to per-netdevice? If so, I wonder why not just use nested locks?
> > Like:
> >
> > netif_addr_nested_lock(upper, 0);
> > netif_addr_nested_lock(lower, 1);
> > netif_addr_nested_unlock(lower);
> > netif_addr_nested_unlock(upper);
> >
> > For this case, they could still share a same key.
> >
> > Thanks for the details!
>
> Yes, the reason for using dynamic lockdep key is to avoid lockdep
> warning in stacked device scenario.
> But, the addr_list_lock case is a little bit different.
> There was a bug in netif_addr_lock_nested() that
> "dev->netdev_ops->ndo_get_lock_subclass" isn't updated after "master"
> and "nomaster" command.
> So, the wrong subclass is used, so lockdep warning message was printed.
> There were some ways to fix this problem, using dynamic key is just one
> of them. I think using the correct subclass in netif_addr_lock_nested()
> is also a correct way to fix that problem. Another minor reason was that
> the subclass is limited by 8. but dynamic key has no limitation.
>
> Unfortunately, dynamic key has a problem too.
> lockdep limits the maximum number of lockdep keys.
> $cat /proc/lockdep_stats
> lock-classes: 1228 [max: 8192]
>
> So, If so many network interfaces are created, they use so many
> lockdep keys. If so, lockdep will stop.
> This is the cause of "BUG: MAX_LOCKDEP_KEYS too low!".
Hi Taehee, Cong,
We actually have some serious problems with lockdep limits recently:
https://groups.google.com/g/syzkaller-bugs/c/O9pFzd9KABU/m/KCuRo3w5CgAJ
I wonder if it's related to what you mentioned... I will CC you on
that other thread.
^ permalink raw reply [flat|nested] 20+ messages in thread