lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Tue, 8 Sep 2020 00:47:37 +0900
From:   Taehee Yoo <ap420073@...il.com>
To:     Guillaume Nault <gnault@...hat.com>
Cc:     David Miller <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net] netns: fix a deadlock in peernet2id_alloc()

On Mon, 7 Sep 2020 at 22:55, Guillaume Nault <gnault@...hat.com> wrote:
>

Hi Guillaume,
Thank you for the review!

> 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.
>

Okay, I will send a v2 patch, which uses the regular git revert
message template.

Thank you!
Taehee

> Apart from that,
> Acked-by: Guillaume Nault <gnault@...hat.com>
>
> > Fixes: 8d7e5dee972f ("netns: don't disable BHs when locking "nsid_lock"")
> > Reported-by: syzbot+3f960c64a104eaa2c813@...kaller.appspotmail.com
> > Signed-off-by: Taehee Yoo <ap420073@...il.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
> >
>

Powered by blists - more mailing lists