Date:   Fri, 10 Jan 2020 15:02:16 +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 13:43, Cong Wang <xiyou.wangcong@...il.com> wrote:
>
> On Thu, Jan 9, 2020 at 7:06 PM Taehee Yoo <ap420073@...il.com> wrote:
> >
> > 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.
>
> Oh, but why? :) As I said, at least for the addr lock case, we can always
> pass subclass in the same order as they are called if we switch it back
> to static keys.
>

ndo_get_lock_subclass() was used to calculate subclass which was used by
netif_addr_lock_nested().

-static inline void netif_addr_lock_nested(struct net_device *dev)
-{
-       int subclass = SINGLE_DEPTH_NESTING;
-
-       if (dev->netdev_ops->ndo_get_lock_subclass)
-               subclass = dev->netdev_ops->ndo_get_lock_subclass(dev);
-
-       spin_lock_nested(&dev->addr_list_lock, subclass);
-}

The most important thing about nested lock is to get the correct subclass.
nest_level was used as subclass and this was calculated by
->ndo_get_lock_subclass().
But, ->ndo_get_lock_subclass() didn't calculate correct subclass.
After "master" and "nomaster" operations, nest_level should be updated
recursively, but it didn't. So incorrect subclass was used.

team3 <-- subclass 0

"ip link set team3 master team2"

team2 <-- subclass 0
team3 <-- subclass 1

"ip link set team2 master team1"

team1 <-- subclass 0
team3 <-- subclass 1
team3 <-- subclass 1

"ip link set team1 master team0"

team0 <-- subclass 0
team1 <-- subclass 1
team3 <-- subclass 1
team3 <-- subclass 1

After "master" and "nomaster" operation, subclass values of all lower or
upper interfaces would be changed. But ->ndo_get_lock_subclass()
didn't update subclass recursively, lockdep warning appeared.
In order to fix this, I had two ways.
1. use dynamic keys instead of static keys.
2. fix ndo_get_lock_subclass().

The reason why I adopted using dynamic keys instead of fixing
->ndo_get_lock_subclass() is that the ->ndo_get_lock_subclass() isn't
a common helper function.
So, driver writers should implement ->ndo_get_lock_subclass().
If we use dynamic keys, ->ndo_get_lock_subclass() code could be removed.

> >
> > >
> > > > 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.
>
> Yeah, I am aware.
>
>
> >
> > > 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.
>
> I am trying to fix this syzbot warning, not to address the key limit.
>
> The reason is that I think dynamic keys are not necessary and
> not able to be used safely in this case. What I am still not sure
> is whether using subclass (with static keys) could address the
> lockdep issue you fixed with dynamic keys.
>
> Thanks!

What I fixed problems with dynamic lockdep keys could be fixed by
nested lock too. I think if the subclass value synchronization routine
works well, there will be no problem.

Thanks a lot!

Powered by blists - more mailing lists