Re: RTNL: assertion failed at net/ipv6/addrconf.c (1699)

From: Hannes Frederic Sowa <hannes@stressinduktion.org>
To: Cong Wang <cwang@twopensource.com>
Cc: Sabrina Dubroca <sd@queasysnail.net>,
	Tommi Rantala <tt.rantala@gmail.com>,
	"David S. Miller" <davem@davemloft.net>,
	Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>,
	James Morris <jmorris@namei.org>,
	Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>,
	Patrick McHardy <kaber@trash.net>,
	netdev <netdev@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	trinity@vger.kernel.org, Dave Jones <davej@redhat.com>
Subject: Re: RTNL: assertion failed at net/ipv6/addrconf.c (1699)
Date: Tue, 02 Sep 2014 20:18:34 +0200	[thread overview]
Message-ID: <1409681914.978561.162800349.5A7FA784@webmail.messagingengine.com> (raw)
In-Reply-To: <CAHA+R7Mwy1SCpEUgGf9BdLEro6N-3t89KU=xL0iuzgOppXELrQ@mail.gmail.com>

On Tue, Sep 2, 2014, at 20:04, Cong Wang wrote:
> On Tue, Sep 2, 2014 at 10:58 AM, Hannes Frederic Sowa
> <hannes@stressinduktion.org> wrote:
> > Hi Cong,
> >
> > On Tue, Sep 2, 2014, at 18:50, Cong Wang wrote:
> >> On Fri, Aug 29, 2014 at 6:51 PM, Hannes Frederic Sowa
> >> <hannes@stressinduktion.org> wrote:
> >> >
> >> > Also rtnl_lock and rcu_read_lock compose in that order, so we don't need
> >> > to change dev_get_by_flags, but as this is the only user it sure is
> >> > possible. RCU locked version is just easier composeable, so I wouldn't
> >> > touch that if needed in future, just also take rcu lock as before.
> >>
> >> There is no point to keep RCU read lock if we have rtnl lock,
> >> I don't know why you don't want to change dev_get_by_flags(),
> >> it is pretty easy to do since it only has one caller.
> >
> > I definitely don't have a problem cleaning this up in net-next. I wanted
> > a minimal patch for stable because I didn't check history where and when
> > additional users of dev_get_by_flags_rcu were removed.
> 
> `git grep` should show you we only have one caller. Apparently we don't
> care about any out-of-tree module.

Sure, I don't care about out-of-tree modules either. I just wanted to
make it easier to backport. Current patch is almost headache free to
backport.

> >> Even if you really need RCU in future, you are always welcome
> >> to bring it back when you do, sorry we should never be blocked by
> >> code NOT merged yet.
> >>
> >> >
> >> > Also we should move ASSERT_RTNL checks from addrconf_join_solict to
> >> > ipv6_dev_mc_inc/dec.
> >> >
> >>
> >> Make it another patch.
> >
> > It is just one logical change, moving ASSERT_RTNLs to places where they
> > better catch invalid callstacks.
> >
> 
> Conflicts with what you claimed above. :)

Those ASSERT_RTNLs were misplaced and only caught the callers mostly
from addrconf.c. I don't mind getting reports from stable kernel users
and fixing those, too (or help fixing those). ASSERT_RTNL is not
dangerous.

We had a long history in not correctly using rtnl lock in ipv6/multicast
code and those wrongfully placed ASSERT_RTNLs were my bad when I fixed
the duplicate address detection handling.

If enough multicast addresses are subscribed to an interface we might
again get those splats because enabling promisc mode on an interface
will also check for rtnl lock.

Bye,
Hannes