Skip to content

Commit e1f469c

Browse files
TaeheeYookuba-moo
authored andcommitted
Revert "netns: don't disable BHs when locking "nsid_lock""
This reverts commit 8d7e5de. To protect netns id, the nsid_lock is used when netns id is being allocated and removed by peernet2id_alloc() and unhash_nsid(). The nsid_lock can be used in BH context but only spin_lock() is used in this code. Using spin_lock() instead of spin_lock_bh() can result in a deadlock in the following scenario reported by the lockdep. In order to avoid a deadlock, the spin_lock_bh() should be used instead of spin_lock() to acquire nsid_lock. Test commands: ip netns del nst ip netns add nst ip link add veth1 type veth peer name veth2 ip link set veth1 netns nst ip netns exec nst ip link add name br1 type bridge vlan_filtering 1 ip netns exec nst ip link set dev br1 up ip netns exec nst ip link set dev veth1 master br1 ip netns exec nst ip link set dev veth1 up ip netns exec nst ip link add macvlan0 link br1 up type macvlan Splat looks like: [ 33.615860][ T607] WARNING: SOFTIRQ-safe -> SOFTIRQ-unsafe lock order detected [ 33.617194][ T607] 5.9.0-rc1+ #665 Not tainted [ ... ] [ 33.670615][ T607] Chain exists of: [ 33.670615][ T607] &mc->mca_lock --> &bridge_netdev_addr_lock_key --> &net->nsid_lock [ 33.670615][ T607] [ 33.673118][ T607] Possible interrupt unsafe locking scenario: [ 33.673118][ T607] [ 33.674599][ T607] CPU0 CPU1 [ 33.675557][ T607] ---- ---- [ 33.676516][ T607] lock(&net->nsid_lock); [ 33.677306][ T607] local_irq_disable(); [ 33.678517][ T607] lock(&mc->mca_lock); [ 33.679725][ T607] lock(&bridge_netdev_addr_lock_key); [ 33.681166][ T607] <Interrupt> [ 33.681791][ T607] lock(&mc->mca_lock); [ 33.682579][ T607] [ 33.682579][ T607] *** DEADLOCK *** [ ... ] [ 33.922046][ T607] stack backtrace: [ 33.922999][ T607] CPU: 3 PID: 607 Comm: ip Not tainted 5.9.0-rc1+ #665 [ 33.924099][ T607] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1ubuntu1 04/01/2014 [ 33.925714][ T607] Call Trace: [ 33.926238][ T607] dump_stack+0x78/0xab [ 33.926905][ T607] check_irq_usage+0x70b/0x720 [ 33.927708][ T607] ? iterate_chain_key+0x60/0x60 [ 33.928507][ T607] ? check_path+0x22/0x40 [ 33.929201][ T607] ? check_noncircular+0xcf/0x180 [ 33.930024][ T607] ? __lock_acquire+0x1952/0x1f20 [ 33.930860][ T607] __lock_acquire+0x1952/0x1f20 [ 33.931667][ T607] lock_acquire+0xaf/0x3a0 [ 33.932366][ T607] ? peernet2id_alloc+0x3a/0x170 [ 33.933147][ T607] ? br_port_fill_attrs+0x54c/0x6b0 [bridge] [ 33.934140][ T607] ? br_port_fill_attrs+0x5de/0x6b0 [bridge] [ 33.935113][ T607] ? kvm_sched_clock_read+0x14/0x30 [ 33.935974][ T607] _raw_spin_lock+0x30/0x70 [ 33.936728][ T607] ? peernet2id_alloc+0x3a/0x170 [ 33.937523][ T607] peernet2id_alloc+0x3a/0x170 [ 33.938313][ T607] rtnl_fill_ifinfo+0xb5e/0x1400 [ 33.939091][ T607] rtmsg_ifinfo_build_skb+0x8a/0xf0 [ 33.939953][ T607] rtmsg_ifinfo_event.part.39+0x17/0x50 [ 33.940863][ T607] rtmsg_ifinfo+0x1f/0x30 [ 33.941571][ T607] __dev_notify_flags+0xa5/0xf0 [ 33.942376][ T607] ? __irq_work_queue_local+0x49/0x50 [ 33.943249][ T607] ? irq_work_queue+0x1d/0x30 [ 33.943993][ T607] ? __dev_set_promiscuity+0x7b/0x1a0 [ 33.944878][ T607] __dev_set_promiscuity+0x7b/0x1a0 [ 33.945758][ T607] dev_set_promiscuity+0x1e/0x50 [ 33.946582][ T607] br_port_set_promisc+0x1f/0x40 [bridge] [ 33.947487][ T607] br_manage_promisc+0x8b/0xe0 [bridge] [ 33.948388][ T607] __dev_set_promiscuity+0x123/0x1a0 [ 33.949244][ T607] __dev_set_rx_mode+0x68/0x90 [ 33.950021][ T607] dev_uc_add+0x50/0x60 [ 33.950720][ T607] macvlan_open+0x18e/0x1f0 [macvlan] [ 33.951601][ T607] __dev_open+0xd6/0x170 [ 33.952269][ T607] __dev_change_flags+0x181/0x1d0 [ 33.953056][ T607] rtnl_configure_link+0x2f/0xa0 [ 33.953884][ T607] __rtnl_newlink+0x6b9/0x8e0 [ 33.954665][ T607] ? __lock_acquire+0x95d/0x1f20 [ 33.955450][ T607] ? lock_acquire+0xaf/0x3a0 [ 33.956193][ T607] ? is_bpf_text_address+0x5/0xe0 [ 33.956999][ T607] rtnl_newlink+0x47/0x70 Acked-by: Guillaume Nault <[email protected]> Fixes: 8d7e5de ("netns: don't disable BHs when locking "nsid_lock"") Reported-by: [email protected] Signed-off-by: Taehee Yoo <[email protected]> Signed-off-by: Jakub Kicinski <[email protected]>
1 parent 8ae4dff commit e1f469c

File tree

1 file changed

+11
-11
lines changed

1 file changed

+11
-11
lines changed

net/core/net_namespace.c

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -251,10 +251,10 @@ int peernet2id_alloc(struct net *net, struct net *peer, gfp_t gfp)
251251
if (refcount_read(&net->count) == 0)
252252
return NETNSA_NSID_NOT_ASSIGNED;
253253

254-
spin_lock(&net->nsid_lock);
254+
spin_lock_bh(&net->nsid_lock);
255255
id = __peernet2id(net, peer);
256256
if (id >= 0) {
257-
spin_unlock(&net->nsid_lock);
257+
spin_unlock_bh(&net->nsid_lock);
258258
return id;
259259
}
260260

@@ -264,12 +264,12 @@ int peernet2id_alloc(struct net *net, struct net *peer, gfp_t gfp)
264264
* just been idr_remove()'d from there in cleanup_net().
265265
*/
266266
if (!maybe_get_net(peer)) {
267-
spin_unlock(&net->nsid_lock);
267+
spin_unlock_bh(&net->nsid_lock);
268268
return NETNSA_NSID_NOT_ASSIGNED;
269269
}
270270

271271
id = alloc_netid(net, peer, -1);
272-
spin_unlock(&net->nsid_lock);
272+
spin_unlock_bh(&net->nsid_lock);
273273

274274
put_net(peer);
275275
if (id < 0)
@@ -534,20 +534,20 @@ static void unhash_nsid(struct net *net, struct net *last)
534534
for_each_net(tmp) {
535535
int id;
536536

537-
spin_lock(&tmp->nsid_lock);
537+
spin_lock_bh(&tmp->nsid_lock);
538538
id = __peernet2id(tmp, net);
539539
if (id >= 0)
540540
idr_remove(&tmp->netns_ids, id);
541-
spin_unlock(&tmp->nsid_lock);
541+
spin_unlock_bh(&tmp->nsid_lock);
542542
if (id >= 0)
543543
rtnl_net_notifyid(tmp, RTM_DELNSID, id, 0, NULL,
544544
GFP_KERNEL);
545545
if (tmp == last)
546546
break;
547547
}
548-
spin_lock(&net->nsid_lock);
548+
spin_lock_bh(&net->nsid_lock);
549549
idr_destroy(&net->netns_ids);
550-
spin_unlock(&net->nsid_lock);
550+
spin_unlock_bh(&net->nsid_lock);
551551
}
552552

553553
static LLIST_HEAD(cleanup_list);
@@ -760,9 +760,9 @@ static int rtnl_net_newid(struct sk_buff *skb, struct nlmsghdr *nlh,
760760
return PTR_ERR(peer);
761761
}
762762

763-
spin_lock(&net->nsid_lock);
763+
spin_lock_bh(&net->nsid_lock);
764764
if (__peernet2id(net, peer) >= 0) {
765-
spin_unlock(&net->nsid_lock);
765+
spin_unlock_bh(&net->nsid_lock);
766766
err = -EEXIST;
767767
NL_SET_BAD_ATTR(extack, nla);
768768
NL_SET_ERR_MSG(extack,
@@ -771,7 +771,7 @@ static int rtnl_net_newid(struct sk_buff *skb, struct nlmsghdr *nlh,
771771
}
772772

773773
err = alloc_netid(net, peer, nsid);
774-
spin_unlock(&net->nsid_lock);
774+
spin_unlock_bh(&net->nsid_lock);
775775
if (err >= 0) {
776776
rtnl_net_notifyid(net, RTM_NEWNSID, err, NETLINK_CB(skb).portid,
777777
nlh, GFP_KERNEL);

0 commit comments

Comments
 (0)