| Message ID | 20190928164843.31800-8-ap420073@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | net: fix nested device bugs | expand |
Hi,
I hadn't seen the previous patchsets of this, and looking briefly in the
archives didn't really seem to say anything about this.
However, I'm wondering now: patches 2-7 of this patchset look basically
all identical in a way:
* you set the addr_list_lock's class to a newly registered key inside
the netdev (or rather the private struct, but doesn't make a big
difference)
* you set each TX queue's _xmit_lock's class similarly
* you set the qdisc_tx_busylock/qdisc_running_key
The first two of these look pretty much completely identical.
Would it perhaps make sense to just do that for *every* netdev? Many of
those netdevs won't ever nest so it wouldn't really be useful, but I'm
not convinced it would put that much more strain on lockdep - if
anything, people are probably creating more VLANs than regular PF/VF
netdevs anyway?
I didn't see any discussion on this, but perhaps I missed it? The cost
would be a bigger netdev struct (when lockdep is enabled), but we
already have that for all the VLANs etc. it's just in the private data,
so it's not a _huge_ difference really I'd think, and this is quite a
bit of code for each device type now.
Alternatively, maybe there could just be some common helper code:
struct nested_netdev_lockdep {
struct lock_class_key xmit_lock_key;
struct lock_class_key addr_lock_key;
};
void netdev_init_nested_lockdep(struct net_device *dev,
struct netsted_netdev_lockdep *l)
{
/* ... */
}
so you just have to embed a "struct nested_netdev_lockdep" in your
private data structure and call the common code.
Or maybe make that
void netdev_init_nested_lockdep(
struct net_device *dev,
struct
netsted_netdev_lockdep *l,
struct lock_class_key
*qdisc_tx_busylock_key,
struct lock_class_key *qdisc_running_key)
so you can't really get that part wrong either?
> @@ -922,6 +938,9 @@ static void macvlan_uninit(struct net_device *dev)
> port->count -= 1;
> if (!port->count)
> macvlan_port_destroy(port->dev);
> +
> + lockdep_unregister_key(&vlan->addr_lock_key);
> + lockdep_unregister_key(&vlan->xmit_lock_key);
> }
OK, so I guess you need an equivalent "deinit" function too -
netdev_deinit_nested_lockdep() or so.
What's not really clear to me is why the qdisc locks can actually stay
the same at all levels? Can they just never nest? But then why are they
different per device type?
Thanks,
johannes
On Sun, 29 Sep 2019 at 04:15, Johannes Berg <johannes@sipsolutions.net> wrote: > > Hi, > Hi Johannes, Thank you so much for the detailed reviews! > I hadn't seen the previous patchsets of this, and looking briefly in the > archives didn't really seem to say anything about this. > > > However, I'm wondering now: patches 2-7 of this patchset look basically > all identical in a way: > * you set the addr_list_lock's class to a newly registered key inside > the netdev (or rather the private struct, but doesn't make a big > difference) > * you set each TX queue's _xmit_lock's class similarly > * you set the qdisc_tx_busylock/qdisc_running_key > > The first two of these look pretty much completely identical. > > Would it perhaps make sense to just do that for *every* netdev? Many of > those netdevs won't ever nest so it wouldn't really be useful, but I'm > not convinced it would put that much more strain on lockdep - if > anything, people are probably creating more VLANs than regular PF/VF > netdevs anyway? > > I didn't see any discussion on this, but perhaps I missed it? The cost > would be a bigger netdev struct (when lockdep is enabled), but we > already have that for all the VLANs etc. it's just in the private data, > so it's not a _huge_ difference really I'd think, and this is quite a > bit of code for each device type now. > Actually I agree with your opinion. The benefits of this way are to be able to make common helper functions. That would reduce duplicate codes and we can maintain this more easily. But I'm not sure about the overhead of this way. So I would like to ask maintainers and more reviewers about this. > Alternatively, maybe there could just be some common helper code: > > struct nested_netdev_lockdep { > struct lock_class_key xmit_lock_key; > struct lock_class_key addr_lock_key; > }; > > void netdev_init_nested_lockdep(struct net_device *dev, > struct netsted_netdev_lockdep *l) > { > /* ... */ > } > > so you just have to embed a "struct nested_netdev_lockdep" in your > private data structure and call the common code. > > Or maybe make that > > void netdev_init_nested_lockdep( > struct net_device *dev, > struct > netsted_netdev_lockdep *l, > struct lock_class_key > *qdisc_tx_busylock_key, > struct lock_class_key *qdisc_running_key) > > so you can't really get that part wrong either? > > > > @@ -922,6 +938,9 @@ static void macvlan_uninit(struct net_device *dev) > > port->count -= 1; > > if (!port->count) > > macvlan_port_destroy(port->dev); > > + > > + lockdep_unregister_key(&vlan->addr_lock_key); > > + lockdep_unregister_key(&vlan->xmit_lock_key); > > } > > OK, so I guess you need an equivalent "deinit" function too - > netdev_deinit_nested_lockdep() or so. > Using "struct nested_netdev_lockdep" looks really good. I will make common codes such as "struct nested_netdev_lockdep" and "netdev_devinit_nested_lockdep" and others in a v5 patch. > > What's not really clear to me is why the qdisc locks can actually stay > the same at all levels? Can they just never nest? But then why are they > different per device type? I didn't test about qdisc so I didn't modify code related to qdisc code. If someone reviews this, I would really appreciate. Thank you > > Thanks, > johannes >
Hi, > > I didn't see any discussion on this, but perhaps I missed it? The cost > > would be a bigger netdev struct (when lockdep is enabled), but we > > already have that for all the VLANs etc. it's just in the private data, > > so it's not a _huge_ difference really I'd think, and this is quite a > > bit of code for each device type now. > > Actually I agree with your opinion. > The benefits of this way are to be able to make common helper functions. > That would reduce duplicate codes and we can maintain this more easily. > But I'm not sure about the overhead of this way. So I would like to ask > maintainers and more reviewers about this. :-) > Using "struct nested_netdev_lockdep" looks really good. > I will make common codes such as "struct nested_netdev_lockdep" > and "netdev_devinit_nested_lockdep" and others in a v5 patch. That makes *sense*, but it seems to me that for example in virt_wifi we just missed this part completely, so addressing it in the generic code would still reduce overall code and complexity? Actually, looking at net-next, we already have netdev_lockdep_set_classes() as a macro there that handles all this. I guess having it as a macro makes some sense so it "evaporates" when lockdep isn't enabled. I'd probably try that but maybe somebody else can chime in and say what they think about applying that to *every* netdev instead though. > > What's not really clear to me is why the qdisc locks can actually stay > > the same at all levels? Can they just never nest? But then why are they > > different per device type? > > I didn't test about qdisc so I didn't modify code related to qdisc code. > If someone reviews this, I would really appreciate. I didn't really think hard about it when I wrote this ... But it seems to me the whole nesting also has to be applied here? __dev_xmit_skb: * qdisc_run_begin() * sch_direct_xmit() * HARD_TX_LOCK(dev, txq, smp_processor_id()); * dev_hard_start_xmit() // say this is VLAN * dev_queue_xmit() // on real_dev * __dev_xmit_skb // recursion on another netdev Now if you have VLAN-in-VLAN the whole thing will recurse right? johannes
On Tue, 1 Oct 2019 at 16:25, Johannes Berg <johannes@sipsolutions.net> wrote: > > Hi, > Hi, > > > I didn't see any discussion on this, but perhaps I missed it? The cost > > > would be a bigger netdev struct (when lockdep is enabled), but we > > > already have that for all the VLANs etc. it's just in the private data, > > > so it's not a _huge_ difference really I'd think, and this is quite a > > > bit of code for each device type now. > > > > Actually I agree with your opinion. > > The benefits of this way are to be able to make common helper functions. > > That would reduce duplicate codes and we can maintain this more easily. > > But I'm not sure about the overhead of this way. So I would like to ask > > maintainers and more reviewers about this. > > :-) > > > Using "struct nested_netdev_lockdep" looks really good. > > I will make common codes such as "struct nested_netdev_lockdep" > > and "netdev_devinit_nested_lockdep" and others in a v5 patch. > > That makes *sense*, but it seems to me that for example in virt_wifi we > just missed this part completely, so addressing it in the generic code > would still reduce overall code and complexity? > Yes, you're right, Virt_wifi has the same problem. I will fix this in a v5 patch! > Actually, looking at net-next, we already have > netdev_lockdep_set_classes() as a macro there that handles all this. I > guess having it as a macro makes some sense so it "evaporates" when > lockdep isn't enabled. > > > I'd probably try that but maybe somebody else can chime in and say what > they think about applying that to *every* netdev instead though. > If we place lockdep keys into "struct net_device", this macro would be a little bit modified and reused. And driver code shape will not be huge changed. I think this way is better than this v4 way. So I will try it. > > > > What's not really clear to me is why the qdisc locks can actually stay > > > the same at all levels? Can they just never nest? But then why are they > > > different per device type? > > > > I didn't test about qdisc so I didn't modify code related to qdisc code. > > If someone reviews this, I would really appreciate. > > I didn't really think hard about it when I wrote this ... > > But it seems to me the whole nesting also has to be applied here? > > __dev_xmit_skb: > * qdisc_run_begin() > * sch_direct_xmit() > * HARD_TX_LOCK(dev, txq, smp_processor_id()); > * dev_hard_start_xmit() // say this is VLAN > * dev_queue_xmit() // on real_dev > * __dev_xmit_skb // recursion on another netdev > > Now if you have VLAN-in-VLAN the whole thing will recurse right? > I have checked on this routine. Only xmit_lock(HARD_TX_LOCK) could be nested. other qdisc locks(runinng, busylock) will not be nested. This patch already handles the _xmit_lock key. so I think there is no problem. But I would like to place four lockdep keys(busylock, address, running, _xmit_lock) into "struct net_device" because of code complexity. Let me know if I misunderstood anything. > johannes > Thank you, Taehee
On Sat, 2019-10-05 at 18:13 +0900, Taehee Yoo wrote: > > If we place lockdep keys into "struct net_device", this macro would be a > little bit modified and reused. And driver code shape will not be huge > changed. I think this way is better than this v4 way. > So I will try it. What I was thinking was that if we can do this for every VLAN netdev, why shouldn't we do it for *every* netdev unconditionally? Some code could perhaps even be simplified if this was just a general part of netdev allocation. > > But it seems to me the whole nesting also has to be applied here? > > > > __dev_xmit_skb: > > * qdisc_run_begin() > > * sch_direct_xmit() > > * HARD_TX_LOCK(dev, txq, smp_processor_id()); > > * dev_hard_start_xmit() // say this is VLAN > > * dev_queue_xmit() // on real_dev > > * __dev_xmit_skb // recursion on another netdev > > > > Now if you have VLAN-in-VLAN the whole thing will recurse right? > > > > I have checked on this routine. > Only xmit_lock(HARD_TX_LOCK) could be nested. other > qdisc locks(runinng, busylock) will not be nested. OK, I still didn't check it too closely I guess, or got confused which lock I should look at. > This patch already > handles the _xmit_lock key. so I think there is no problem. Right > But I would like to place four lockdep keys(busylock, address, > running, _xmit_lock) into "struct net_device" because of code complexity. > > Let me know if I misunderstood anything. Nothing to misunderstand - I was just asking/wondering why the qdisc locks were not treated the same way. johannes
On Mon, 7 Oct 2019 at 20:41, Johannes Berg <johannes@sipsolutions.net> wrote: > Hi Johannes, > On Sat, 2019-10-05 at 18:13 +0900, Taehee Yoo wrote: > > > > If we place lockdep keys into "struct net_device", this macro would be a > > little bit modified and reused. And driver code shape will not be huge > > changed. I think this way is better than this v4 way. > > So I will try it. > > What I was thinking was that if we can do this for every VLAN netdev, > why shouldn't we do it for *every* netdev unconditionally? Some code > could perhaps even be simplified if this was just a general part of > netdev allocation. > Your opinion makes sense. I think there is no critical reason that every netdev shouldn't have own lockdep keys. By comparison, the benefits are obvious. > > > But it seems to me the whole nesting also has to be applied here? > > > > > > __dev_xmit_skb: > > > * qdisc_run_begin() > > > * sch_direct_xmit() > > > * HARD_TX_LOCK(dev, txq, smp_processor_id()); > > > * dev_hard_start_xmit() // say this is VLAN > > > * dev_queue_xmit() // on real_dev > > > * __dev_xmit_skb // recursion on another netdev > > > > > > Now if you have VLAN-in-VLAN the whole thing will recurse right? > > > > > > > I have checked on this routine. > > Only xmit_lock(HARD_TX_LOCK) could be nested. other > > qdisc locks(runinng, busylock) will not be nested. > > OK, I still didn't check it too closely I guess, or got confused which > lock I should look at. > > > This patch already > > handles the _xmit_lock key. so I think there is no problem. > > Right > > > But I would like to place four lockdep keys(busylock, address, > > running, _xmit_lock) into "struct net_device" because of code complexity. > > > > Let me know if I misunderstood anything. > > Nothing to misunderstand - I was just asking/wondering why the qdisc > locks were not treated the same way. > I'm always thankful for your detailed and careful reviews. > johannes > Thank you, Taehee Yoo
On Mon, 7 Oct 2019 at 20:41, Johannes Berg <johannes@sipsolutions.net> wrote: > Hi Johannes, > On Sat, 2019-10-05 at 18:13 +0900, Taehee Yoo wrote: > > > > If we place lockdep keys into "struct net_device", this macro would be a > > little bit modified and reused. And driver code shape will not be huge > > changed. I think this way is better than this v4 way. > > So I will try it. > > What I was thinking was that if we can do this for every VLAN netdev, > why shouldn't we do it for *every* netdev unconditionally? Some code > could perhaps even be simplified if this was just a general part of > netdev allocation. > > > > But it seems to me the whole nesting also has to be applied here? > > > > > > __dev_xmit_skb: > > > * qdisc_run_begin() > > > * sch_direct_xmit() > > > * HARD_TX_LOCK(dev, txq, smp_processor_id()); > > > * dev_hard_start_xmit() // say this is VLAN > > > * dev_queue_xmit() // on real_dev > > > * __dev_xmit_skb // recursion on another netdev > > > > > > Now if you have VLAN-in-VLAN the whole thing will recurse right? > > > > > > > I have checked on this routine. > > Only xmit_lock(HARD_TX_LOCK) could be nested. other > > qdisc locks(runinng, busylock) will not be nested. > "I have checked on this routine. Only xmit_lock(HARD_TX_LOCK) could be nested. other qdisc locks(runinng, busylock) will not be nested." I'm so sorry, I think it's not true. running lock could be nested. But lockdep warning doesn't occur because of below code. seqcount_acquire(&qdisc->running.dep_map, 0, 1, _RET_IP_); The third argument means trylock. If trylock is set, lockdep doesn't make lockdep chain. So, running could be nested but lockdep warning doesn't occur even these have the same lockdep key. You can check on /proc/lockdep and /proc/lockdep_chain > OK, I still didn't check it too closely I guess, or got confused which > lock I should look at. > > > This patch already > > handles the _xmit_lock key. so I think there is no problem. > > Right > > > But I would like to place four lockdep keys(busylock, address, > > running, _xmit_lock) into "struct net_device" because of code complexity. > > > > Let me know if I misunderstood anything. > > Nothing to misunderstand - I was just asking/wondering why the qdisc > locks were not treated the same way. > > johannes > Thank you Taehee
diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c index 940192c057b6..dae368a2e8d1 100644 --- a/drivers/net/macvlan.c +++ b/drivers/net/macvlan.c @@ -852,8 +852,6 @@ static int macvlan_do_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd) * "super class" of normal network devices; split their locks off into a * separate class since they always nest. */ -static struct lock_class_key macvlan_netdev_addr_lock_key; - #define ALWAYS_ON_OFFLOADS \ (NETIF_F_SG | NETIF_F_HW_CSUM | NETIF_F_GSO_SOFTWARE | \ NETIF_F_GSO_ROBUST | NETIF_F_GSO_ENCAP_ALL) @@ -874,12 +872,30 @@ static int macvlan_get_nest_level(struct net_device *dev) return ((struct macvlan_dev *)netdev_priv(dev))->nest_level; } -static void macvlan_set_lockdep_class(struct net_device *dev) +static void macvlan_dev_set_lockdep_one(struct net_device *dev, + struct netdev_queue *txq, + void *_unused) +{ + struct macvlan_dev *macvlan = netdev_priv(dev); + + lockdep_set_class(&txq->_xmit_lock, &macvlan->xmit_lock_key); +} + +static struct lock_class_key qdisc_tx_busylock_key; +static struct lock_class_key qdisc_running_key; + +static void macvlan_dev_set_lockdep_class(struct net_device *dev) { - netdev_lockdep_set_classes(dev); - lockdep_set_class_and_subclass(&dev->addr_list_lock, - &macvlan_netdev_addr_lock_key, - macvlan_get_nest_level(dev)); + struct macvlan_dev *macvlan = netdev_priv(dev); + + dev->qdisc_tx_busylock = &qdisc_tx_busylock_key; + dev->qdisc_running_key = &qdisc_running_key; + + lockdep_register_key(&macvlan->addr_lock_key); + lockdep_set_class(&dev->addr_list_lock, &macvlan->addr_lock_key); + + lockdep_register_key(&macvlan->xmit_lock_key); + netdev_for_each_tx_queue(dev, macvlan_dev_set_lockdep_one, NULL); } static int macvlan_init(struct net_device *dev) @@ -900,7 +916,7 @@ static int macvlan_init(struct net_device *dev) dev->gso_max_segs = lowerdev->gso_max_segs; dev->hard_header_len = lowerdev->hard_header_len; - macvlan_set_lockdep_class(dev); + macvlan_dev_set_lockdep_class(dev); vlan->pcpu_stats = netdev_alloc_pcpu_stats(struct vlan_pcpu_stats); if (!vlan->pcpu_stats) @@ -922,6 +938,9 @@ static void macvlan_uninit(struct net_device *dev) port->count -= 1; if (!port->count) macvlan_port_destroy(port->dev); + + lockdep_unregister_key(&vlan->addr_lock_key); + lockdep_unregister_key(&vlan->xmit_lock_key); } static void macvlan_dev_get_stats64(struct net_device *dev, diff --git a/include/linux/if_macvlan.h b/include/linux/if_macvlan.h index 2e55e4cdbd8a..ea5b41823287 100644 --- a/include/linux/if_macvlan.h +++ b/include/linux/if_macvlan.h @@ -31,6 +31,8 @@ struct macvlan_dev { u16 flags; int nest_level; unsigned int macaddr_count; + struct lock_class_key xmit_lock_key; + struct lock_class_key addr_lock_key; #ifdef CONFIG_NET_POLL_CONTROLLER struct netpoll *netpoll; #endif
All macvlan device has same lockdep key and subclass is initialized with nest_level. But actual nest_level value can be changed when a lower device is attached. And at this moment, the subclass should be updated but it seems to be unsafe. So this patch makes macvlan use dynamic lockdep key instead of the subclass. Test commands: ip link add bond0 type bond ip link add dummy0 type dummy ip link add macvlan0 link bond0 type macvlan mode bridge ip link add macvlan1 link dummy0 type macvlan mode bridge ip link set bond0 mtu 1000 ip link set macvlan1 master bond0 ip link set bond0 up ip link set macvlan0 up ip link set dummy0 up ip link set macvlan1 up Splat looks like: [ 30.281866] WARNING: possible recursive locking detected [ 30.282374] 5.3.0+ #3 Not tainted [ 30.282673] -------------------------------------------- [ 30.283138] ip/643 is trying to acquire lock: [ 30.283522] ffff88806750c818 (&macvlan_netdev_addr_lock_key/1){+...}, at: dev_uc_sync_multiple+0xfa/0x1a0 [ 30.284363] [ 30.284363] but task is already holding lock: [ 30.284878] ffff88806853ead8 (&macvlan_netdev_addr_lock_key/1){+...}, at: dev_set_rx_mode+0x19/0x30 [ 30.285680] [ 30.285680] other info that might help us debug this: [ 30.286274] Possible unsafe locking scenario: [ 30.286274] [ 30.286903] CPU0 [ 30.287192] ---- [ 30.287475] lock(&macvlan_netdev_addr_lock_key/1); [ 30.288121] lock(&macvlan_netdev_addr_lock_key/1); [ 30.288818] [ 30.288818] *** DEADLOCK *** [ 30.288818] [ 30.294651] May be due to missing lock nesting notation [ 30.294651] [ 30.295660] 4 locks held by ip/643: [ 30.296076] #0: ffffffff93ec7a30 (rtnl_mutex){+.+.}, at: rtnetlink_rcv_msg+0x466/0x8a0 [ 30.297030] #1: ffff88806853ead8 (&macvlan_netdev_addr_lock_key/1){+...}, at: dev_set_rx_mode+0x19/0x30 [ 30.298749] #2: ffff888063b8a3f8 (&dev_addr_list_lock_key/3){+...}, at: dev_uc_sync+0xfa/0x1a0 [ 30.299727] #3: ffffffff93b22780 (rcu_read_lock){....}, at: bond_set_rx_mode+0x5/0x3c0 [bonding] [ 30.302803] [ 30.302803] stack backtrace: [ 30.303254] CPU: 1 PID: 643 Comm: ip Not tainted 5.3.0+ #3 [ 30.303907] Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox 12/01/2006 [ 30.310458] Call Trace: [ 30.310694] dump_stack+0x7c/0xbb [ 30.311016] __lock_acquire+0x26a9/0x3df0 [ 30.311390] ? register_lock_class+0x14d0/0x14d0 [ 30.311815] lock_acquire+0x164/0x3b0 [ 30.312237] ? dev_uc_sync_multiple+0xfa/0x1a0 [ 30.312776] ? rcu_read_lock_held+0x90/0xa0 [ 30.313293] _raw_spin_lock_nested+0x2e/0x60 [ 30.313819] ? dev_uc_sync_multiple+0xfa/0x1a0 [ 30.314429] dev_uc_sync_multiple+0xfa/0x1a0 [ 30.314950] bond_set_rx_mode+0x269/0x3c0 [bonding] [ 30.315541] ? bond_init+0x6f0/0x6f0 [bonding] [ 30.316075] dev_uc_sync+0x15a/0x1a0 [ ... ] Fixes: c674ac30c549 ("macvlan: Fix lockdep warnings with stacked macvlan devices") Signed-off-by: Taehee Yoo <ap420073@gmail.com> --- v1 -> v4 : - This patch is not changed drivers/net/macvlan.c | 35 +++++++++++++++++++++++++++-------- include/linux/if_macvlan.h | 2 ++ 2 files changed, 29 insertions(+), 8 deletions(-)