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

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]

Date:	Sat, 30 Aug 2014 03:51:29 +0200
From:	Hannes Frederic Sowa <hannes@...essinduktion.org>
To:	Sabrina Dubroca <sd@...asysnail.net>
Cc:	Cong Wang <cwang@...pensource.com>,
	Tommi Rantala <tt.rantala@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Alexey Kuznetsov <kuznet@....inr.ac.ru>,
	James Morris <jmorris@...ei.org>,
	Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
	Patrick McHardy <kaber@...sh.net>,
	netdev <netdev@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>, trinity@...r.kernel.org,
	Dave Jones <davej@...hat.com>
Subject: Re: RTNL: assertion failed at net/ipv6/addrconf.c (1699)

Hi Sabrina,

On Fr, 2014-08-29 at 21:53 +0200, Sabrina Dubroca wrote:
> 2014-08-29, 11:14:48 -0700, Cong Wang wrote:
> > On Fri, Aug 29, 2014 at 8:26 AM, Tommi Rantala <tt.rantala@...il.com> wrote:
> > > [   77.297196] RTNL: assertion failed at net/ipv6/addrconf.c (1699)
> > > [   77.298080] CPU: 0 PID: 4842 Comm: trinity-main Not tainted 3.17.0-rc2+ #30
> > > [   77.299039] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
> > > [   77.299789]  ffff88003d76a618 ffff880026133c50 ffffffff8238ba79
> > > ffff880037c84520
> > > [   77.300829]  ffff880026133c90 ffffffff820bd52b 0000000000000000
> > > ffffffff82d86c40
> > > [   77.301869]  0000000000000000 00000000f76fd1e1 ffff8800382d8000
> > > ffff8800382d8220
> > > [   77.302906] Call Trace:
> > > [   77.303246]  [<ffffffff8238ba79>] dump_stack+0x4d/0x66
> > > [   77.303928]  [<ffffffff820bd52b>] addrconf_join_solict+0x4b/0xb0
> > > [   77.304731]  [<ffffffff820b031b>] ipv6_dev_ac_inc+0x2bb/0x330
> > > [   77.305498]  [<ffffffff820b0060>] ? ac6_seq_start+0x260/0x260
> > > [   77.306257]  [<ffffffff820b05fe>] ipv6_sock_ac_join+0x26e/0x360
> > > [   77.307046]  [<ffffffff820b0429>] ? ipv6_sock_ac_join+0x99/0x360
> > > [   77.307798]  [<ffffffff820cdd60>] do_ipv6_setsockopt.isra.5+0xa70/0xf20
> > 
> > 
> > I think we should just use rtnl_lock() instead of rcu_read_lock() there,
> > it is not a hot path worth optimization.
> > 
> > Please try the attached patch.
> 
> note: it doesn't build as it is now, it needs:
> 
> -EXPORT_SYMBOL(dev_get_by_flags_rcu);
> +EXPORT_SYMBOL(dev_get_by_flags);
> 
> 
> I just tried your patch with a basic test program (open
> socket/join/leave/close and open socket/join/close).
> 
> I think you need to modify ipv6_sock_ac_close as well, or you can still
> trigger the assertion when closing the socket without leaving first.
> 
> Modified patch attached.

Sorry, just had time to look at this.

The reason is not to have list corruption but that the calls down to
ndo_set_rx_mode expect rtnl to be locked by the drivers. Filter lists
are locked by addr_list_lock and that's why I think we never saw any
problems with that, but drivers expect rtnl locked for those calls.

But this problem also affects multicast join, so patch seems incomplete
to me (and for that matter ssm multicast join, too).

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.

So just adding rtnl_lock add appropriate places seems to be ok to me,
but still need to review parts of the ssm code.

Also we should move ASSERT_RTNL checks from addrconf_join_solict to
ipv6_dev_mc_inc/dec.

Thanks,
Hannes


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html