netdev - Re: WARNING: bad unlock balance in sch_direct

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 linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMArcTUFK6TUYP+zwD3009m126fz+S-cAT5CN5pZ3C5axErh8g@mail.gmail.com>
Date:   Fri, 10 Jan 2020 12:06:37 +0900
From:   Taehee Yoo <ap420073@...il.com>
To:     Cong Wang <xiyou.wangcong@...il.com>
Cc:     syzbot <syzbot+4ec99438ed7450da6272@...kaller.appspotmail.com>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: WARNING: bad unlock balance in sch_direct_xmit

On Fri, 10 Jan 2020 at 08:38, Cong Wang <xiyou.wangcong@...il.com> wrote:
>
> On Wed, Jan 8, 2020 at 3:43 AM Taehee Yoo <ap420073@...il.com> wrote:
> >
> > On Wed, 8 Jan 2020 at 09:34, Cong Wang <xiyou.wangcong@...il.com> wrote:
> > >
> > > On Tue, Jan 7, 2020 at 3:31 AM Taehee Yoo <ap420073@...il.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!