On Sun, Sep 06, 2020 at 02:34:04PM +0000, Taehee Yoo wrote:
> 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
Thanks Taehee. I thought I had checked all the possible code paths
before letting BH enabled. Looks like I missed some.
Just one nit: this is a plain revert of 8d7e5dee972f ("netns:
don't disable BHs when locking "nsid_lock""). So it would be clearer to
use the regular git revert message template.
Apart from that,
Acked-by: Guillaume Nault <gna...@redhat.com>
> Fixes: 8d7e5dee972f ("netns: don't disable BHs when locking "nsid_lock"")
> Reported-by: syzbot+3f960c64a104eaa2c...@syzkaller.appspotmail.com
> Signed-off-by: Taehee Yoo <ap420...@gmail.com>
> ---
> net/core/net_namespace.c | 22 +++++++++++-----------
> 1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/net/core/net_namespace.c b/net/core/net_namespace.c
> index dcd61aca343e..944ab214e5ae 100644
> --- a/net/core/net_namespace.c
> +++ b/net/core/net_namespace.c
> @@ -251,10 +251,10 @@ int peernet2id_alloc(struct net *net, struct net *peer,
> gfp_t gfp)
> if (refcount_read(&net->count) == 0)
> return NETNSA_NSID_NOT_ASSIGNED;
>
> - spin_lock(&net->nsid_lock);
> + spin_lock_bh(&net->nsid_lock);
> id = __peernet2id(net, peer);
> if (id >= 0) {
> - spin_unlock(&net->nsid_lock);
> + spin_unlock_bh(&net->nsid_lock);
> return id;
> }
>
> @@ -264,12 +264,12 @@ int peernet2id_alloc(struct net *net, struct net *peer,
> gfp_t gfp)
> * just been idr_remove()'d from there in cleanup_net().
> */
> if (!maybe_get_net(peer)) {
> - spin_unlock(&net->nsid_lock);
> + spin_unlock_bh(&net->nsid_lock);
> return NETNSA_NSID_NOT_ASSIGNED;
> }
>
> id = alloc_netid(net, peer, -1);
> - spin_unlock(&net->nsid_lock);
> + spin_unlock_bh(&net->nsid_lock);
>
> put_net(peer);
> if (id < 0)
> @@ -534,20 +534,20 @@ static void unhash_nsid(struct net *net, struct net
> *last)
> for_each_net(tmp) {
> int id;
>
> - spin_lock(&tmp->nsid_lock);
> + spin_lock_bh(&tmp->nsid_lock);
> id = __peernet2id(tmp, net);
> if (id >= 0)
> idr_remove(&tmp->netns_ids, id);
> - spin_unlock(&tmp->nsid_lock);
> + spin_unlock_bh(&tmp->nsid_lock);
> if (id >= 0)
> rtnl_net_notifyid(tmp, RTM_DELNSID, id, 0, NULL,
> GFP_KERNEL);
> if (tmp == last)
> break;
> }
> - spin_lock(&net->nsid_lock);
> + spin_lock_bh(&net->nsid_lock);
> idr_destroy(&net->netns_ids);
> - spin_unlock(&net->nsid_lock);
> + spin_unlock_bh(&net->nsid_lock);
> }
>
> static LLIST_HEAD(cleanup_list);
> @@ -760,9 +760,9 @@ static int rtnl_net_newid(struct sk_buff *skb, struct
> nlmsghdr *nlh,
> return PTR_ERR(peer);
> }
>
> - spin_lock(&net->nsid_lock);
> + spin_lock_bh(&net->nsid_lock);
> if (__peernet2id(net, peer) >= 0) {
> - spin_unlock(&net->nsid_lock);
> + spin_unlock_bh(&net->nsid_lock);
> err = -EEXIST;
> NL_SET_BAD_ATTR(extack, nla);
> NL_SET_ERR_MSG(extack,
> @@ -771,7 +771,7 @@ static int rtnl_net_newid(struct sk_buff *skb, struct
> nlmsghdr *nlh,
> }
>
> err = alloc_netid(net, peer, nsid);
> - spin_unlock(&net->nsid_lock);
> + spin_unlock_bh(&net->nsid_lock);
> if (err >= 0) {
> rtnl_net_notifyid(net, RTM_NEWNSID, err, NETLINK_CB(skb).portid,
> nlh, GFP_KERNEL);
> --
> 2.17.1
>
