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

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

[Tommi Rantala]

Hi,

Was fuzzing Linus v3.17-rc2-89-g59753a8 with Trinity as the root user
in qemu, when I hit the following assertion failures.

Tommi


[init] Started watchdog process, PID is 4841
[main] Main thread is alive.
[   77.229699] sctp: [Deprecated]: trinity-main (pid 4842) Use of int
in max_burst socket option deprecated.
[   77.229699] Use struct sctp_assoc_value instead
[   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
[   77.308570]  [<ffffffff8117097d>] ? sched_clock_local+0x1d/0x80
[   77.309260]  [<ffffffff810a8a27>] ? kvm_clock_read+0x27/0x40
[   77.309915]  [<ffffffff810736d9>] ? sched_clock+0x9/0x10
[   77.310537]  [<ffffffff815afff8>] ? sock_has_perm+0x168/0x1e0
[   77.311204]  [<ffffffff81170bb8>] ? sched_clock_cpu+0xa8/0xf0
[   77.311866]  [<ffffffff81170d1b>] ? local_clock+0x1b/0x30
[   77.312501]  [<ffffffff811872cd>] ? lock_release_holdtime+0x1d/0x170
[   77.313241]  [<ffffffff815b0010>] ? sock_has_perm+0x180/0x1e0
[   77.313905]  [<ffffffff815afe90>] ?
selinux_msg_queue_alloc_security+0xa0/0xa0
[   77.314746]  [<ffffffff820ce263>] ipv6_setsockopt+0x53/0xb0
[   77.315397]  [<ffffffff820d3135>] udpv6_setsockopt+0x25/0x30
[   77.316058]  [<ffffffff81f9930f>] sock_common_setsockopt+0xf/0x20
[   77.316764]  [<ffffffff81f9305e>] SyS_setsockopt+0x8e/0xd0
[   77.317406]  [<ffffffff823a47e9>] system_call_fastpath+0x16/0x1b
[main] 375 sockets created based on info from socket cachefile.
[main] Generating file descriptors
[main] Added 129 filenames from /dev
[main] Added 44048 filenames from /proc
[main] Added 18192 filenames from /sys
[main] Enabled 9 fd providers.
[watchdog] Watchdog is alive. (pid:4841)
[child3:4846] finit_module (313) returned ENOSYS, marking as inactive.
[child1:4844] kcmp (312) returned ENOSYS, marking as inactive.
[child2:4845] uselib (134) returned ENOSYS, marking as inactive.
[child1:4844] nfsservctl (180) returned ENOSYS, marking as inactive.
[child2:4845] delete_module (129:[32BIT]) returned ENOSYS, marking as inactive.
[child2:4845] init_module (175) returned ENOSYS, marking as inactive.
[   84.126609] trinity-c7: vm86 mode not supported on 64 bit kernel
[child7:4850] vm86 (166:[32BIT]) returned ENOSYS, marking as inactive.
[main] Bailing main loop because ctrl-c.
[   84.345840] RTNL: assertion failed at net/ipv6/addrconf.c (1712)
[   84.346615] CPU: 0 PID: 4842 Comm: trinity-main Not tainted 3.17.0-rc2+ #30
[   84.347426] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
[   84.348102]  ffff88003d76a618 ffff880026133d10 ffffffff8238ba79
ffff8800382d8000
[   84.349018]  ffff880026133d50 ffffffff820bd5db ffffffff81141555
ffff8800382d8220
[   84.349935]  ffff8800382d8000 00000000f76fd1e1 ffff88003d76a618
ffff8800382d8000
[   84.350848] Call Trace:
[   84.351149]  [<ffffffff8238ba79>] dump_stack+0x4d/0x66
[   84.351751]  [<ffffffff820bd5db>] addrconf_leave_solict+0x4b/0xb0
[   84.352574]  [<ffffffff81141555>] ? __local_bh_enable_ip+0xa5/0xf0
[   84.353315]  [<ffffffff820b07b3>] __ipv6_dev_ac_dec+0xc3/0x140
[   84.354019]  [<ffffffff820b08c8>] ipv6_dev_ac_dec+0x98/0xb0
[   84.354687]  [<ffffffff820b0bcd>] ipv6_sock_ac_close+0x10d/0x1a0
[   84.355410]  [<ffffffff820b0aee>] ? ipv6_sock_ac_close+0x2e/0x1a0
[   84.356147]  [<ffffffff820ae9d3>] inet6_release+0x23/0x40
[   84.356789]  [<ffffffff81f91834>] sock_release+0x14/0x80
[   84.357410]  [<ffffffff81f918ad>] sock_close+0xd/0x20
[   84.358042]  [<ffffffff8127fa91>] __fput+0x111/0x1e0
[   84.358622]  [<ffffffff8127fba9>] ____fput+0x9/0x10
[   84.359196]  [<ffffffff8115e3ee>] task_work_run+0x9e/0xd0
[   84.359825]  [<ffffffff8113f4b6>] do_exit+0x456/0xb30
[   84.360419]  [<ffffffff823a541c>] ? retint_swapgs+0x13/0x1b
[   84.361075]  [<ffffffff8113fc54>] do_group_exit+0x84/0xd0
[   84.361705]  [<ffffffff8113fcaf>] SyS_exit_group+0xf/0x10
[   84.362338]  [<ffffffff823a47e9>] system_call_fastpath+0x16/0x1b
[watchdog] [4841] Watchdog exiting because ctrl-c.
[init] Ran 775 syscalls. Successes: 179  Failures: 596

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

[Vlad Yasevich]

On 08/29/2014 11:26 AM, Tommi Rantala wrote:
> Hi,
> 
> Was fuzzing Linus v3.17-rc2-89-g59753a8 with Trinity as the root user
> in qemu, when I hit the following assertion failures.
> 
> Tommi
> 
> 
> [init] Started watchdog process, PID is 4841
> [main] Main thread is alive.
> [   77.229699] sctp: [Deprecated]: trinity-main (pid 4842) Use of int
> in max_burst socket option deprecated.
> [   77.229699] Use struct sctp_assoc_value instead
> [   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
> [   77.308570]  [<ffffffff8117097d>] ? sched_clock_local+0x1d/0x80
> [   77.309260]  [<ffffffff810a8a27>] ? kvm_clock_read+0x27/0x40
> [   77.309915]  [<ffffffff810736d9>] ? sched_clock+0x9/0x10
> [   77.310537]  [<ffffffff815afff8>] ? sock_has_perm+0x168/0x1e0
> [   77.311204]  [<ffffffff81170bb8>] ? sched_clock_cpu+0xa8/0xf0
> [   77.311866]  [<ffffffff81170d1b>] ? local_clock+0x1b/0x30
> [   77.312501]  [<ffffffff811872cd>] ? lock_release_holdtime+0x1d/0x170
> [   77.313241]  [<ffffffff815b0010>] ? sock_has_perm+0x180/0x1e0
> [   77.313905]  [<ffffffff815afe90>] ?
> selinux_msg_queue_alloc_security+0xa0/0xa0
> [   77.314746]  [<ffffffff820ce263>] ipv6_setsockopt+0x53/0xb0
> [   77.315397]  [<ffffffff820d3135>] udpv6_setsockopt+0x25/0x30
> [   77.316058]  [<ffffffff81f9930f>] sock_common_setsockopt+0xf/0x20
> [   77.316764]  [<ffffffff81f9305e>] SyS_setsockopt+0x8e/0xd0
> [   77.317406]  [<ffffffff823a47e9>] system_call_fastpath+0x16/0x1b
> [main] 375 sockets created based on info from socket cachefile.
> [main] Generating file descriptors
> [main] Added 129 filenames from /dev
> [main] Added 44048 filenames from /proc
> [main] Added 18192 filenames from /sys
> [main] Enabled 9 fd providers.
> [watchdog] Watchdog is alive. (pid:4841)
> [child3:4846] finit_module (313) returned ENOSYS, marking as inactive.
> [child1:4844] kcmp (312) returned ENOSYS, marking as inactive.
> [child2:4845] uselib (134) returned ENOSYS, marking as inactive.
> [child1:4844] nfsservctl (180) returned ENOSYS, marking as inactive.
> [child2:4845] delete_module (129:[32BIT]) returned ENOSYS, marking as inactive.
> [child2:4845] init_module (175) returned ENOSYS, marking as inactive.
> [   84.126609] trinity-c7: vm86 mode not supported on 64 bit kernel
> [child7:4850] vm86 (166:[32BIT]) returned ENOSYS, marking as inactive.
> [main] Bailing main loop because ctrl-c.
> [   84.345840] RTNL: assertion failed at net/ipv6/addrconf.c (1712)
> [   84.346615] CPU: 0 PID: 4842 Comm: trinity-main Not tainted 3.17.0-rc2+ #30
> [   84.347426] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
> [   84.348102]  ffff88003d76a618 ffff880026133d10 ffffffff8238ba79
> ffff8800382d8000
> [   84.349018]  ffff880026133d50 ffffffff820bd5db ffffffff81141555
> ffff8800382d8220
> [   84.349935]  ffff8800382d8000 00000000f76fd1e1 ffff88003d76a618
> ffff8800382d8000
> [   84.350848] Call Trace:
> [   84.351149]  [<ffffffff8238ba79>] dump_stack+0x4d/0x66
> [   84.351751]  [<ffffffff820bd5db>] addrconf_leave_solict+0x4b/0xb0
> [   84.352574]  [<ffffffff81141555>] ? __local_bh_enable_ip+0xa5/0xf0
> [   84.353315]  [<ffffffff820b07b3>] __ipv6_dev_ac_dec+0xc3/0x140
> [   84.354019]  [<ffffffff820b08c8>] ipv6_dev_ac_dec+0x98/0xb0
> [   84.354687]  [<ffffffff820b0bcd>] ipv6_sock_ac_close+0x10d/0x1a0
> [   84.355410]  [<ffffffff820b0aee>] ? ipv6_sock_ac_close+0x2e/0x1a0
> [   84.356147]  [<ffffffff820ae9d3>] inet6_release+0x23/0x40
> [   84.356789]  [<ffffffff81f91834>] sock_release+0x14/0x80
> [   84.357410]  [<ffffffff81f918ad>] sock_close+0xd/0x20
> [   84.358042]  [<ffffffff8127fa91>] __fput+0x111/0x1e0
> [   84.358622]  [<ffffffff8127fba9>] ____fput+0x9/0x10
> [   84.359196]  [<ffffffff8115e3ee>] task_work_run+0x9e/0xd0
> [   84.359825]  [<ffffffff8113f4b6>] do_exit+0x456/0xb30
> [   84.360419]  [<ffffffff823a541c>] ? retint_swapgs+0x13/0x1b
> [   84.361075]  [<ffffffff8113fc54>] do_group_exit+0x84/0xd0
> [   84.361705]  [<ffffffff8113fcaf>] SyS_exit_group+0xf/0x10
> [   84.362338]  [<ffffffff823a47e9>] system_call_fastpath+0x16/0x1b
> [watchdog] [4841] Watchdog exiting because ctrl-c.
> [init] Ran 775 syscalls. Successes: 179  Failures: 596
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Yep,  looks like ipv6_dev_ac_inc() and __ipv6_dev_ac_dec() are called
without RNTL in the socket option path and with RTNL in the address
configuration path.  So it look like this this can actually trigger
list corruptions.

-vlad

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

[Cong Wang]

[-- Attachment #1: Type: text/plain, Size: 1216 bytes --]

On Fri, Aug 29, 2014 at 8:26 AM, Tommi Rantala <tt.rantala@gmail.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.

[-- Attachment #2: ipv6-anycast.diff --]
[-- Type: text/plain, Size: 3664 bytes --]

commit 31d83db0b417f705cbb31b2159603b8b53b81ab6
Author: Cong Wang <xiyou.wangcong@gmail.com>
Date:   Fri Aug 29 11:02:15 2014 -0700

    ipv6: fix rtnl lock assertion in ipv6_sock_ac_join()
    
    Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 4298013..1ae0e74 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2077,8 +2077,8 @@ void __dev_remove_pack(struct packet_type *pt);
 void dev_add_offload(struct packet_offload *po);
 void dev_remove_offload(struct packet_offload *po);
 
-struct net_device *dev_get_by_flags_rcu(struct net *net, unsigned short flags,
-					unsigned short mask);
+struct net_device *dev_get_by_flags(struct net *net, unsigned short flags,
+				    unsigned short mask);
 struct net_device *dev_get_by_name(struct net *net, const char *name);
 struct net_device *dev_get_by_name_rcu(struct net *net, const char *name);
 struct net_device *__dev_get_by_name(struct net *net, const char *name);
diff --git a/net/core/dev.c b/net/core/dev.c
index 26d296c..73cdb03 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -897,23 +897,24 @@ struct net_device *dev_getfirstbyhwtype(struct net *net, unsigned short type)
 EXPORT_SYMBOL(dev_getfirstbyhwtype);
 
 /**
- *	dev_get_by_flags_rcu - find any device with given flags
+ *	dev_get_by_flags - find any device with given flags
  *	@net: the applicable net namespace
  *	@if_flags: IFF_* values
  *	@mask: bitmask of bits in if_flags to check
  *
  *	Search for any interface with the given flags. Returns NULL if a device
  *	is not found or a pointer to the device. Must be called inside
- *	rcu_read_lock(), and result refcount is unchanged.
+ *	rtnl_lock(), and result refcount is unchanged.
  */
 
-struct net_device *dev_get_by_flags_rcu(struct net *net, unsigned short if_flags,
+struct net_device *dev_get_by_flags(struct net *net, unsigned short if_flags,
 				    unsigned short mask)
 {
 	struct net_device *dev, *ret;
 
+	ASSERT_RTNL();
 	ret = NULL;
-	for_each_netdev_rcu(net, dev) {
+	for_each_netdev(net, dev) {
 		if (((dev->flags ^ if_flags) & mask) == 0) {
 			ret = dev;
 			break;
diff --git a/net/ipv6/anycast.c b/net/ipv6/anycast.c
index 2101832..c523c1a 100644
--- a/net/ipv6/anycast.c
+++ b/net/ipv6/anycast.c
@@ -77,7 +77,7 @@ int ipv6_sock_ac_join(struct sock *sk, int ifindex, const struct in6_addr *addr)
 	pac->acl_next = NULL;
 	pac->acl_addr = *addr;
 
-	rcu_read_lock();
+	rtnl_lock();
 	if (ifindex == 0) {
 		struct rt6_info *rt;
 
@@ -90,11 +90,11 @@ int ipv6_sock_ac_join(struct sock *sk, int ifindex, const struct in6_addr *addr)
 			goto error;
 		} else {
 			/* router, no matching interface: just pick one */
-			dev = dev_get_by_flags_rcu(net, IFF_UP,
+			dev = dev_get_by_flags(net, IFF_UP,
 						   IFF_UP | IFF_LOOPBACK);
 		}
 	} else
-		dev = dev_get_by_index_rcu(net, ifindex);
+		dev = __dev_get_by_index(net, ifindex);
 
 	if (dev == NULL) {
 		err = -ENODEV;
@@ -136,7 +136,7 @@ int ipv6_sock_ac_join(struct sock *sk, int ifindex, const struct in6_addr *addr)
 	}
 
 error:
-	rcu_read_unlock();
+	rtnl_unlock();
 	if (pac)
 		sock_kfree_s(sk, pac, sizeof(*pac));
 	return err;
@@ -171,13 +171,15 @@ int ipv6_sock_ac_drop(struct sock *sk, int ifindex, const struct in6_addr *addr)
 
 	spin_unlock_bh(&ipv6_sk_ac_lock);
 
-	rcu_read_lock();
-	dev = dev_get_by_index_rcu(net, pac->acl_ifindex);
+	rtnl_lock();
+	dev = __dev_get_by_index(net, pac->acl_ifindex);
 	if (dev)
 		ipv6_dev_ac_dec(dev, &pac->acl_addr);
-	rcu_read_unlock();
+	rtnl_unlock();
 
 	sock_kfree_s(sk, pac, sizeof(*pac));
+	if (!dev)
+		return -ENODEV;
 	return 0;
 }
 

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

[Sabrina Dubroca]

[-- Attachment #1: Type: text/plain, Size: 1723 bytes --]

2014-08-29, 11:14:48 -0700, Cong Wang wrote:
> On Fri, Aug 29, 2014 at 8:26 AM, Tommi Rantala <tt.rantala@gmail.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.


-- 
Sabrina

[-- Attachment #2: ipv6-anycast_v2.diff --]
[-- Type: text/plain, Size: 4333 bytes --]

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 429801370d0c..1ae0e745b1b1 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2077,8 +2077,8 @@ void __dev_remove_pack(struct packet_type *pt);
 void dev_add_offload(struct packet_offload *po);
 void dev_remove_offload(struct packet_offload *po);
 
-struct net_device *dev_get_by_flags_rcu(struct net *net, unsigned short flags,
-					unsigned short mask);
+struct net_device *dev_get_by_flags(struct net *net, unsigned short flags,
+				    unsigned short mask);
 struct net_device *dev_get_by_name(struct net *net, const char *name);
 struct net_device *dev_get_by_name_rcu(struct net *net, const char *name);
 struct net_device *__dev_get_by_name(struct net *net, const char *name);
diff --git a/net/core/dev.c b/net/core/dev.c
index 443b814db05b..8fede6ef4a39 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -897,23 +897,24 @@ struct net_device *dev_getfirstbyhwtype(struct net *net, unsigned short type)
 EXPORT_SYMBOL(dev_getfirstbyhwtype);
 
 /**
- *	dev_get_by_flags_rcu - find any device with given flags
+ *	dev_get_by_flags - find any device with given flags
  *	@net: the applicable net namespace
  *	@if_flags: IFF_* values
  *	@mask: bitmask of bits in if_flags to check
  *
  *	Search for any interface with the given flags. Returns NULL if a device
  *	is not found or a pointer to the device. Must be called inside
- *	rcu_read_lock(), and result refcount is unchanged.
+ *	rtnl_lock(), and result refcount is unchanged.
  */
 
-struct net_device *dev_get_by_flags_rcu(struct net *net, unsigned short if_flags,
+struct net_device *dev_get_by_flags(struct net *net, unsigned short if_flags,
 				    unsigned short mask)
 {
 	struct net_device *dev, *ret;
 
+	ASSERT_RTNL();
 	ret = NULL;
-	for_each_netdev_rcu(net, dev) {
+	for_each_netdev(net, dev) {
 		if (((dev->flags ^ if_flags) & mask) == 0) {
 			ret = dev;
 			break;
@@ -921,7 +922,7 @@ struct net_device *dev_get_by_flags_rcu(struct net *net, unsigned short if_flags
 	}
 	return ret;
 }
-EXPORT_SYMBOL(dev_get_by_flags_rcu);
+EXPORT_SYMBOL(dev_get_by_flags);
 
 /**
  *	dev_valid_name - check if name is okay for network device
diff --git a/net/ipv6/anycast.c b/net/ipv6/anycast.c
index 210183244689..6de5caa26ea4 100644
--- a/net/ipv6/anycast.c
+++ b/net/ipv6/anycast.c
@@ -77,7 +77,7 @@ int ipv6_sock_ac_join(struct sock *sk, int ifindex, const struct in6_addr *addr)
 	pac->acl_next = NULL;
 	pac->acl_addr = *addr;
 
-	rcu_read_lock();
+	rtnl_lock();
 	if (ifindex == 0) {
 		struct rt6_info *rt;
 
@@ -90,11 +90,11 @@ int ipv6_sock_ac_join(struct sock *sk, int ifindex, const struct in6_addr *addr)
 			goto error;
 		} else {
 			/* router, no matching interface: just pick one */
-			dev = dev_get_by_flags_rcu(net, IFF_UP,
+			dev = dev_get_by_flags(net, IFF_UP,
 						   IFF_UP | IFF_LOOPBACK);
 		}
 	} else
-		dev = dev_get_by_index_rcu(net, ifindex);
+		dev = __dev_get_by_index(net, ifindex);
 
 	if (dev == NULL) {
 		err = -ENODEV;
@@ -136,7 +136,7 @@ int ipv6_sock_ac_join(struct sock *sk, int ifindex, const struct in6_addr *addr)
 	}
 
 error:
-	rcu_read_unlock();
+	rtnl_unlock();
 	if (pac)
 		sock_kfree_s(sk, pac, sizeof(*pac));
 	return err;
@@ -171,13 +171,15 @@ int ipv6_sock_ac_drop(struct sock *sk, int ifindex, const struct in6_addr *addr)
 
 	spin_unlock_bh(&ipv6_sk_ac_lock);
 
-	rcu_read_lock();
-	dev = dev_get_by_index_rcu(net, pac->acl_ifindex);
+	rtnl_lock();
+	dev = __dev_get_by_index(net, pac->acl_ifindex);
 	if (dev)
 		ipv6_dev_ac_dec(dev, &pac->acl_addr);
-	rcu_read_unlock();
+	rtnl_unlock();
 
 	sock_kfree_s(sk, pac, sizeof(*pac));
+	if (!dev)
+		return -ENODEV;
 	return 0;
 }
 
@@ -198,12 +200,12 @@ void ipv6_sock_ac_close(struct sock *sk)
 	spin_unlock_bh(&ipv6_sk_ac_lock);
 
 	prev_index = 0;
-	rcu_read_lock();
+	rtnl_lock();
 	while (pac) {
 		struct ipv6_ac_socklist *next = pac->acl_next;
 
 		if (pac->acl_ifindex != prev_index) {
-			dev = dev_get_by_index_rcu(net, pac->acl_ifindex);
+			dev = __dev_get_by_index(net, pac->acl_ifindex);
 			prev_index = pac->acl_ifindex;
 		}
 		if (dev)
@@ -211,7 +213,7 @@ void ipv6_sock_ac_close(struct sock *sk)
 		sock_kfree_s(sk, pac, sizeof(*pac));
 		pac = next;
 	}
-	rcu_read_unlock();
+	rtnl_unlock();
 }
 
 static void aca_put(struct ifacaddr6 *ac)

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

[Cong Wang]

On Fri, Aug 29, 2014 at 12:53 PM, Sabrina Dubroca <sd@queasysnail.net> wrote:
> 2014-08-29, 11:14:48 -0700, Cong Wang wrote:
>> On Fri, Aug 29, 2014 at 8:26 AM, Tommi Rantala <tt.rantala@gmail.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.

You are absolutely right here.

Can I have your Signed-off-by and Tested-by before sending the patch
formally?

Thanks!

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

[Hannes Frederic Sowa]

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

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

[Sabrina Dubroca]

2014-08-29, 15:54:48 -0700, Cong Wang wrote:
> [...]
> 
> You are absolutely right here.
> 
> Can I have your Signed-off-by and Tested-by before sending the patch
> formally?
> 
> Thanks!

Sure:

Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
Tested-by: Sabrina Dubroca <sd@queasysnail.net>


Thanks,

-- 
Sabrina

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

[Sabrina Dubroca]

Hello,

2014-08-30, 03:51:29 +0200, Hannes Frederic Sowa wrote:
> Hi Sabrina,
> 
> [...]
> 
> 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

Thanks for explaining.

I had a look at what you suggested.


So, for anycast, on top of the previous patch, we'd have:

---
diff --git a/net/ipv6/anycast.c b/net/ipv6/anycast.c
index 210183244689..61dd3046b804 100644
--- a/net/ipv6/anycast.c
+++ b/net/ipv6/anycast.c
 static void aca_put(struct ifacaddr6 *ac)
@@ -233,6 +235,8 @@ int ipv6_dev_ac_inc(struct net_device *dev, const struct in6_addr *addr)
 	struct rt6_info *rt;
 	int err;
 
+	ASSERT_RTNL();
+
 	idev = in6_dev_get(dev);
 
 	if (idev == NULL)
@@ -302,6 +306,8 @@ int __ipv6_dev_ac_dec(struct inet6_dev *idev, const struct in6_addr *addr)
 {
 	struct ifacaddr6 *aca, *prev_aca;
 
+	ASSERT_RTNL();
+
 	write_lock_bh(&idev->lock);
 	prev_aca = NULL;
 	for (aca = idev->ac_list; aca; aca = aca->aca_next) {
@@ -336,6 +342,8 @@ static int ipv6_dev_ac_dec(struct net_device *dev, const struct in6_addr *addr)
 {
 	struct inet6_dev *idev = __in6_dev_get(dev);
 
+	ASSERT_RTNL();
+
 	if (idev == NULL)
 		return -ENODEV;
 	return __ipv6_dev_ac_dec(idev, addr);

---


And for multicast:
 - locking order in the patch below: rtnl -> rcu -> ipv6_sk_mc_lock
 - ipv6_sock_mc_join: maybe move all the _unlock()'s together at the end of the function
 - do we need to modify rcu_dereference_protected in ipv6_sock_mc_drop/ipv6_sock_mc_close
 - I had a look at the other codepaths that call ipv6_dev_mc_inc/dec
   - ipv6_mc_destroy_dev, dev_forward_change, ipv6_add_dev,
     addrconf_join_solict -- all take rtnl or already have an
     ASSERT_RTNL()
   - pndisc_destructor, called from pneigh_ifdown/pneigh_delete
   - pndisc_constructor, called from pneigh_lookup -- pneigh_lookup
     has ASSERT_RTNL(), but pneigh_lookup is called from ip6_forward and
     ndisc_recv_na
   - (hope I didn't miss any callers)

As far as I could see, apart maybe from pndisc_constructor, it seems
okay, but I'd like to hear your comments.


Current modifications:

---
diff --git a/net/ipv6/mcast.c b/net/ipv6/mcast.c
index 70881795da96..d73ac1ef65f2 100644
--- a/net/ipv6/mcast.c
+++ b/net/ipv6/mcast.c
@@ -172,6 +172,7 @@ int ipv6_sock_mc_join(struct sock *sk, int ifindex, const struct in6_addr *addr)
 	mc_lst->next = NULL;
 	mc_lst->addr = *addr;
 
+	rtnl_lock();
 	rcu_read_lock();
 	if (ifindex == 0) {
 		struct rt6_info *rt;
@@ -185,6 +186,7 @@ int ipv6_sock_mc_join(struct sock *sk, int ifindex, const struct in6_addr *addr)
 
 	if (dev == NULL) {
 		rcu_read_unlock();
+		rtnl_unlock();
 		sock_kfree_s(sk, mc_lst, sizeof(*mc_lst));
 		return -ENODEV;
 	}
@@ -202,6 +204,7 @@ int ipv6_sock_mc_join(struct sock *sk, int ifindex, const struct in6_addr *addr)
 
 	if (err) {
 		rcu_read_unlock();
+		rtnl_unlock();
 		sock_kfree_s(sk, mc_lst, sizeof(*mc_lst));
 		return err;
 	}
@@ -212,6 +215,7 @@ int ipv6_sock_mc_join(struct sock *sk, int ifindex, const struct in6_addr *addr)
 	spin_unlock(&ipv6_sk_mc_lock);
 
 	rcu_read_unlock();
+	rtnl_unlock();
 
 	return 0;
 }
@@ -229,6 +233,7 @@ int ipv6_sock_mc_drop(struct sock *sk, int ifindex, const struct in6_addr *addr)
 	if (!ipv6_addr_is_multicast(addr))
 		return -EINVAL;
 
+	rtnl_lock();
 	spin_lock(&ipv6_sk_mc_lock);
 	for (lnk = &np->ipv6_mc_list;
 	     (mc_lst = rcu_dereference_protected(*lnk,
@@ -252,12 +257,15 @@ int ipv6_sock_mc_drop(struct sock *sk, int ifindex, const struct in6_addr *addr)
 			} else
 				(void) ip6_mc_leave_src(sk, mc_lst, NULL);
 			rcu_read_unlock();
+			rtnl_unlock();
+
 			atomic_sub(sizeof(*mc_lst), &sk->sk_omem_alloc);
 			kfree_rcu(mc_lst, rcu);
 			return 0;
 		}
 	}
 	spin_unlock(&ipv6_sk_mc_lock);
+	rtnl_unlock();
 
 	return -EADDRNOTAVAIL;
 }
@@ -302,6 +310,7 @@ void ipv6_sock_mc_close(struct sock *sk)
 	if (!rcu_access_pointer(np->ipv6_mc_list))
 		return;
 
+	rtnl_lock();
 	spin_lock(&ipv6_sk_mc_lock);
 	while ((mc_lst = rcu_dereference_protected(np->ipv6_mc_list,
 				lockdep_is_held(&ipv6_sk_mc_lock))) != NULL) {
@@ -328,6 +337,7 @@ void ipv6_sock_mc_close(struct sock *sk)
 		spin_lock(&ipv6_sk_mc_lock);
 	}
 	spin_unlock(&ipv6_sk_mc_lock);
+	rtnl_unlock();
 }
 
 int ip6_mc_source(int add, int omode, struct sock *sk,
@@ -845,6 +855,8 @@ int ipv6_dev_mc_inc(struct net_device *dev, const struct in6_addr *addr)
 	struct ifmcaddr6 *mc;
 	struct inet6_dev *idev;
 
+	ASSERT_RTNL();
+
 	/* we need to take a reference on idev */
 	idev = in6_dev_get(dev);
 
@@ -916,6 +928,8 @@ int __ipv6_dev_mc_dec(struct inet6_dev *idev, const struct in6_addr *addr)
 {
 	struct ifmcaddr6 *ma, **map;
 
+	ASSERT_RTNL();
+
 	write_lock_bh(&idev->lock);
 	for (map = &idev->mc_list; (ma = *map) != NULL; map = &ma->next) {
 		if (ipv6_addr_equal(&ma->mca_addr, addr)) {
@@ -942,6 +956,8 @@ int ipv6_dev_mc_dec(struct net_device *dev, const struct in6_addr *addr)
 	struct inet6_dev *idev;
 	int err;
 
+	ASSERT_RTNL();
+
 	rcu_read_lock();
 
 	idev = __in6_dev_get(dev);

---


Thanks,

-- 
Sabrina

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

[Sabrina Dubroca]

2014-08-30, 12:58:21 +0200, Sabrina Dubroca wrote:
>    - pndisc_constructor, called from pneigh_lookup -- pneigh_lookup
>      has ASSERT_RTNL(), but pneigh_lookup is called from ip6_forward and
>      ndisc_recv_na

Ah, these have creat = 0, so it's fine. I missed that earlier.

Sorry for the noise.

-- 
Sabrina

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

[Hannes Frederic Sowa]

Hi,

On Sa, 2014-08-30 at 12:58 +0200, Sabrina Dubroca wrote:
> 2014-08-30, 03:51:29 +0200, Hannes Frederic Sowa wrote:
> > Hi Sabrina,
> > 
> > [...]
> > 
> > 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
> 
> Thanks for explaining.
> 
> I had a look at what you suggested.
> 
> 
> So, for anycast, on top of the previous patch, we'd have:
> 
> ---
> diff --git a/net/ipv6/anycast.c b/net/ipv6/anycast.c
> index 210183244689..61dd3046b804 100644
> --- a/net/ipv6/anycast.c
> +++ b/net/ipv6/anycast.c
>  static void aca_put(struct ifacaddr6 *ac)
> @@ -233,6 +235,8 @@ int ipv6_dev_ac_inc(struct net_device *dev, const struct in6_addr *addr)
>  	struct rt6_info *rt;
>  	int err;
>  
> +	ASSERT_RTNL();
> +
>  	idev = in6_dev_get(dev);
>  
>  	if (idev == NULL)
> @@ -302,6 +306,8 @@ int __ipv6_dev_ac_dec(struct inet6_dev *idev, const struct in6_addr *addr)
>  {
>  	struct ifacaddr6 *aca, *prev_aca;
>  
> +	ASSERT_RTNL();
> +
>  	write_lock_bh(&idev->lock);
>  	prev_aca = NULL;
>  	for (aca = idev->ac_list; aca; aca = aca->aca_next) {
> @@ -336,6 +342,8 @@ static int ipv6_dev_ac_dec(struct net_device *dev, const struct in6_addr *addr)
>  {
>  	struct inet6_dev *idev = __in6_dev_get(dev);
>  
> +	ASSERT_RTNL();
> +
>  	if (idev == NULL)
>  		return -ENODEV;
>  	return __ipv6_dev_ac_dec(idev, addr);

ASSERT_RTNL() still performs a runtime check. While those are not really
fast paths, I still think it is better to keep them to a minimum and
place them only at places where we know all code which needs to be
guarded passes by:

I would suggest to move ASSERT_RTNL to ipv6_dev_mc_inc and
__ipv6_dev_mc_dec and even remove the checks from addrconf_join_solict
and addrconf_leave_solict. Does that cover all code paths and makes
sense?

> ---
> 
> 
> And for multicast:
>  - locking order in the patch below: rtnl -> rcu -> ipv6_sk_mc_lock
>  - ipv6_sock_mc_join: maybe move all the _unlock()'s together at the end of the function
>  - do we need to modify rcu_dereference_protected in ipv6_sock_mc_drop/ipv6_sock_mc_close
>  - I had a look at the other codepaths that call ipv6_dev_mc_inc/dec
>    - ipv6_mc_destroy_dev, dev_forward_change, ipv6_add_dev,
>      addrconf_join_solict -- all take rtnl or already have an
>      ASSERT_RTNL()
>    - pndisc_destructor, called from pneigh_ifdown/pneigh_delete
>    - pndisc_constructor, called from pneigh_lookup -- pneigh_lookup
>      has ASSERT_RTNL(), but pneigh_lookup is called from ip6_forward and
>      ndisc_recv_na
>    - (hope I didn't miss any callers)
> 
> As far as I could see, apart maybe from pndisc_constructor, it seems
> okay, but I'd like to hear your comments.

The rest of the patch looks good.

Can you or Cong post a final patch with the adapted ac_join/drop
changes?

Thanks,
Hannes

[PATCH] ipv6: fix rtnl locking in setsockopt for anycast and multicast

[Sabrina Dubroca]

Calling setsockopt with IPV6_JOIN_ANYCAST or IPV6_LEAVE_ANYCAST
triggers the assertion in addrconf_join_solict()/addrconf_leave_solict()

ipv6_sock_ac_join(), ipv6_sock_ac_drop(), ipv6_sock_ac_close() need to
take RTNL before calling ipv6_dev_ac_inc/dec. Same thing with
ipv6_sock_mc_join(), ipv6_sock_mc_drop(), ipv6_sock_mc_close() before
calling ipv6_dev_mc_inc/dec.

This patch moves ASSERT_RTNL() up a level in the call stack.

Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
Reported-by: Tommi Rantala <tt.rantala@gmail.com>
---
I included Cong's Signed-off-by for the first part of the patch,
I hope that's OK.

This patch is based on -next, but since the assertion can also be
triggered on a current kernel (tested on a 3.16), I think it should
also go in stable.

 include/linux/netdevice.h |  4 ++--
 net/core/dev.c            | 11 ++++++-----
 net/ipv6/addrconf.c       | 15 +++++----------
 net/ipv6/anycast.c        | 30 +++++++++++++++++++-----------
 net/ipv6/mcast.c          | 16 ++++++++++++++++
 5 files changed, 48 insertions(+), 28 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 429801370d0c..1ae0e745b1b1 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2077,8 +2077,8 @@ void __dev_remove_pack(struct packet_type *pt);
 void dev_add_offload(struct packet_offload *po);
 void dev_remove_offload(struct packet_offload *po);
 
-struct net_device *dev_get_by_flags_rcu(struct net *net, unsigned short flags,
-					unsigned short mask);
+struct net_device *dev_get_by_flags(struct net *net, unsigned short flags,
+				    unsigned short mask);
 struct net_device *dev_get_by_name(struct net *net, const char *name);
 struct net_device *dev_get_by_name_rcu(struct net *net, const char *name);
 struct net_device *__dev_get_by_name(struct net *net, const char *name);
diff --git a/net/core/dev.c b/net/core/dev.c
index 443b814db05b..8fede6ef4a39 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -897,23 +897,24 @@ struct net_device *dev_getfirstbyhwtype(struct net *net, unsigned short type)
 EXPORT_SYMBOL(dev_getfirstbyhwtype);
 
 /**
- *	dev_get_by_flags_rcu - find any device with given flags
+ *	dev_get_by_flags - find any device with given flags
  *	@net: the applicable net namespace
  *	@if_flags: IFF_* values
  *	@mask: bitmask of bits in if_flags to check
  *
  *	Search for any interface with the given flags. Returns NULL if a device
  *	is not found or a pointer to the device. Must be called inside
- *	rcu_read_lock(), and result refcount is unchanged.
+ *	rtnl_lock(), and result refcount is unchanged.
  */
 
-struct net_device *dev_get_by_flags_rcu(struct net *net, unsigned short if_flags,
+struct net_device *dev_get_by_flags(struct net *net, unsigned short if_flags,
 				    unsigned short mask)
 {
 	struct net_device *dev, *ret;
 
+	ASSERT_RTNL();
 	ret = NULL;
-	for_each_netdev_rcu(net, dev) {
+	for_each_netdev(net, dev) {
 		if (((dev->flags ^ if_flags) & mask) == 0) {
 			ret = dev;
 			break;
@@ -921,7 +922,7 @@ struct net_device *dev_get_by_flags_rcu(struct net *net, unsigned short if_flags
 	}
 	return ret;
 }
-EXPORT_SYMBOL(dev_get_by_flags_rcu);
+EXPORT_SYMBOL(dev_get_by_flags);
 
 /**
  *	dev_valid_name - check if name is okay for network device
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 267ce3caee24..7ada65937d23 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -1690,14 +1690,12 @@ void addrconf_dad_failure(struct inet6_ifaddr *ifp)
 	addrconf_mod_dad_work(ifp, 0);
 }
 
-/* Join to solicited addr multicast group. */
-
+/* Join to solicited addr multicast group.
+ * caller must hold RTNL */
 void addrconf_join_solict(struct net_device *dev, const struct in6_addr *addr)
 {
 	struct in6_addr maddr;
 
-	ASSERT_RTNL();
-
 	if (dev->flags&(IFF_LOOPBACK|IFF_NOARP))
 		return;
 
@@ -1705,12 +1703,11 @@ void addrconf_join_solict(struct net_device *dev, const struct in6_addr *addr)
 	ipv6_dev_mc_inc(dev, &maddr);
 }
 
+/* caller must hold RTNL */
 void addrconf_leave_solict(struct inet6_dev *idev, const struct in6_addr *addr)
 {
 	struct in6_addr maddr;
 
-	ASSERT_RTNL();
-
 	if (idev->dev->flags&(IFF_LOOPBACK|IFF_NOARP))
 		return;
 
@@ -1718,12 +1715,11 @@ void addrconf_leave_solict(struct inet6_dev *idev, const struct in6_addr *addr)
 	__ipv6_dev_mc_dec(idev, &maddr);
 }
 
+/* caller must hold RTNL */
 static void addrconf_join_anycast(struct inet6_ifaddr *ifp)
 {
 	struct in6_addr addr;
 
-	ASSERT_RTNL();
-
 	if (ifp->prefix_len >= 127) /* RFC 6164 */
 		return;
 	ipv6_addr_prefix(&addr, &ifp->addr, ifp->prefix_len);
@@ -1732,12 +1728,11 @@ static void addrconf_join_anycast(struct inet6_ifaddr *ifp)
 	ipv6_dev_ac_inc(ifp->idev->dev, &addr);
 }
 
+/* caller must hold RTNL */
 static void addrconf_leave_anycast(struct inet6_ifaddr *ifp)
 {
 	struct in6_addr addr;
 
-	ASSERT_RTNL();
-
 	if (ifp->prefix_len >= 127) /* RFC 6164 */
 		return;
 	ipv6_addr_prefix(&addr, &ifp->addr, ifp->prefix_len);
diff --git a/net/ipv6/anycast.c b/net/ipv6/anycast.c
index 210183244689..572c2faede55 100644
--- a/net/ipv6/anycast.c
+++ b/net/ipv6/anycast.c
@@ -77,7 +77,7 @@ int ipv6_sock_ac_join(struct sock *sk, int ifindex, const struct in6_addr *addr)
 	pac->acl_next = NULL;
 	pac->acl_addr = *addr;
 
-	rcu_read_lock();
+	rtnl_lock();
 	if (ifindex == 0) {
 		struct rt6_info *rt;
 
@@ -90,11 +90,11 @@ int ipv6_sock_ac_join(struct sock *sk, int ifindex, const struct in6_addr *addr)
 			goto error;
 		} else {
 			/* router, no matching interface: just pick one */
-			dev = dev_get_by_flags_rcu(net, IFF_UP,
-						   IFF_UP | IFF_LOOPBACK);
+			dev = dev_get_by_flags(net, IFF_UP,
+					       IFF_UP | IFF_LOOPBACK);
 		}
 	} else
-		dev = dev_get_by_index_rcu(net, ifindex);
+		dev = __dev_get_by_index(net, ifindex);
 
 	if (dev == NULL) {
 		err = -ENODEV;
@@ -136,7 +136,7 @@ int ipv6_sock_ac_join(struct sock *sk, int ifindex, const struct in6_addr *addr)
 	}
 
 error:
-	rcu_read_unlock();
+	rtnl_unlock();
 	if (pac)
 		sock_kfree_s(sk, pac, sizeof(*pac));
 	return err;
@@ -171,13 +171,15 @@ int ipv6_sock_ac_drop(struct sock *sk, int ifindex, const struct in6_addr *addr)
 
 	spin_unlock_bh(&ipv6_sk_ac_lock);
 
-	rcu_read_lock();
-	dev = dev_get_by_index_rcu(net, pac->acl_ifindex);
+	rtnl_lock();
+	dev = __dev_get_by_index(net, pac->acl_ifindex);
 	if (dev)
 		ipv6_dev_ac_dec(dev, &pac->acl_addr);
-	rcu_read_unlock();
+	rtnl_unlock();
 
 	sock_kfree_s(sk, pac, sizeof(*pac));
+	if (!dev)
+		return -ENODEV;
 	return 0;
 }
 
@@ -198,12 +200,12 @@ void ipv6_sock_ac_close(struct sock *sk)
 	spin_unlock_bh(&ipv6_sk_ac_lock);
 
 	prev_index = 0;
-	rcu_read_lock();
+	rtnl_lock();
 	while (pac) {
 		struct ipv6_ac_socklist *next = pac->acl_next;
 
 		if (pac->acl_ifindex != prev_index) {
-			dev = dev_get_by_index_rcu(net, pac->acl_ifindex);
+			dev = __dev_get_by_index(net, pac->acl_ifindex);
 			prev_index = pac->acl_ifindex;
 		}
 		if (dev)
@@ -211,7 +213,7 @@ void ipv6_sock_ac_close(struct sock *sk)
 		sock_kfree_s(sk, pac, sizeof(*pac));
 		pac = next;
 	}
-	rcu_read_unlock();
+	rtnl_unlock();
 }
 
 static void aca_put(struct ifacaddr6 *ac)
@@ -233,6 +235,8 @@ int ipv6_dev_ac_inc(struct net_device *dev, const struct in6_addr *addr)
 	struct rt6_info *rt;
 	int err;
 
+	ASSERT_RTNL();
+
 	idev = in6_dev_get(dev);
 
 	if (idev == NULL)
@@ -302,6 +306,8 @@ int __ipv6_dev_ac_dec(struct inet6_dev *idev, const struct in6_addr *addr)
 {
 	struct ifacaddr6 *aca, *prev_aca;
 
+	ASSERT_RTNL();
+
 	write_lock_bh(&idev->lock);
 	prev_aca = NULL;
 	for (aca = idev->ac_list; aca; aca = aca->aca_next) {
@@ -336,6 +342,8 @@ static int ipv6_dev_ac_dec(struct net_device *dev, const struct in6_addr *addr)
 {
 	struct inet6_dev *idev = __in6_dev_get(dev);
 
+	ASSERT_RTNL();
+
 	if (idev == NULL)
 		return -ENODEV;
 	return __ipv6_dev_ac_dec(idev, addr);
diff --git a/net/ipv6/mcast.c b/net/ipv6/mcast.c
index 70881795da96..d73ac1ef65f2 100644
--- a/net/ipv6/mcast.c
+++ b/net/ipv6/mcast.c
@@ -172,6 +172,7 @@ int ipv6_sock_mc_join(struct sock *sk, int ifindex, const struct in6_addr *addr)
 	mc_lst->next = NULL;
 	mc_lst->addr = *addr;
 
+	rtnl_lock();
 	rcu_read_lock();
 	if (ifindex == 0) {
 		struct rt6_info *rt;
@@ -185,6 +186,7 @@ int ipv6_sock_mc_join(struct sock *sk, int ifindex, const struct in6_addr *addr)
 
 	if (dev == NULL) {
 		rcu_read_unlock();
+		rtnl_unlock();
 		sock_kfree_s(sk, mc_lst, sizeof(*mc_lst));
 		return -ENODEV;
 	}
@@ -202,6 +204,7 @@ int ipv6_sock_mc_join(struct sock *sk, int ifindex, const struct in6_addr *addr)
 
 	if (err) {
 		rcu_read_unlock();
+		rtnl_unlock();
 		sock_kfree_s(sk, mc_lst, sizeof(*mc_lst));
 		return err;
 	}
@@ -212,6 +215,7 @@ int ipv6_sock_mc_join(struct sock *sk, int ifindex, const struct in6_addr *addr)
 	spin_unlock(&ipv6_sk_mc_lock);
 
 	rcu_read_unlock();
+	rtnl_unlock();
 
 	return 0;
 }
@@ -229,6 +233,7 @@ int ipv6_sock_mc_drop(struct sock *sk, int ifindex, const struct in6_addr *addr)
 	if (!ipv6_addr_is_multicast(addr))
 		return -EINVAL;
 
+	rtnl_lock();
 	spin_lock(&ipv6_sk_mc_lock);
 	for (lnk = &np->ipv6_mc_list;
 	     (mc_lst = rcu_dereference_protected(*lnk,
@@ -252,12 +257,15 @@ int ipv6_sock_mc_drop(struct sock *sk, int ifindex, const struct in6_addr *addr)
 			} else
 				(void) ip6_mc_leave_src(sk, mc_lst, NULL);
 			rcu_read_unlock();
+			rtnl_unlock();
+
 			atomic_sub(sizeof(*mc_lst), &sk->sk_omem_alloc);
 			kfree_rcu(mc_lst, rcu);
 			return 0;
 		}
 	}
 	spin_unlock(&ipv6_sk_mc_lock);
+	rtnl_unlock();
 
 	return -EADDRNOTAVAIL;
 }
@@ -302,6 +310,7 @@ void ipv6_sock_mc_close(struct sock *sk)
 	if (!rcu_access_pointer(np->ipv6_mc_list))
 		return;
 
+	rtnl_lock();
 	spin_lock(&ipv6_sk_mc_lock);
 	while ((mc_lst = rcu_dereference_protected(np->ipv6_mc_list,
 				lockdep_is_held(&ipv6_sk_mc_lock))) != NULL) {
@@ -328,6 +337,7 @@ void ipv6_sock_mc_close(struct sock *sk)
 		spin_lock(&ipv6_sk_mc_lock);
 	}
 	spin_unlock(&ipv6_sk_mc_lock);
+	rtnl_unlock();
 }
 
 int ip6_mc_source(int add, int omode, struct sock *sk,
@@ -845,6 +855,8 @@ int ipv6_dev_mc_inc(struct net_device *dev, const struct in6_addr *addr)
 	struct ifmcaddr6 *mc;
 	struct inet6_dev *idev;
 
+	ASSERT_RTNL();
+
 	/* we need to take a reference on idev */
 	idev = in6_dev_get(dev);
 
@@ -916,6 +928,8 @@ int __ipv6_dev_mc_dec(struct inet6_dev *idev, const struct in6_addr *addr)
 {
 	struct ifmcaddr6 *ma, **map;
 
+	ASSERT_RTNL();
+
 	write_lock_bh(&idev->lock);
 	for (map = &idev->mc_list; (ma = *map) != NULL; map = &ma->next) {
 		if (ipv6_addr_equal(&ma->mca_addr, addr)) {
@@ -942,6 +956,8 @@ int ipv6_dev_mc_dec(struct net_device *dev, const struct in6_addr *addr)
 	struct inet6_dev *idev;
 	int err;
 
+	ASSERT_RTNL();
+
 	rcu_read_lock();
 
 	idev = __in6_dev_get(dev);
-- 
2.1.0

Re: [PATCH] ipv6: fix rtnl locking in setsockopt for anycast and multicast

[Hannes Frederic Sowa]

Hi,

On Mo, 2014-09-01 at 23:05 +0200, Sabrina Dubroca wrote:
> Calling setsockopt with IPV6_JOIN_ANYCAST or IPV6_LEAVE_ANYCAST
> triggers the assertion in addrconf_join_solict()/addrconf_leave_solict()
> 
> ipv6_sock_ac_join(), ipv6_sock_ac_drop(), ipv6_sock_ac_close() need to
> take RTNL before calling ipv6_dev_ac_inc/dec. Same thing with
> ipv6_sock_mc_join(), ipv6_sock_mc_drop(), ipv6_sock_mc_close() before
> calling ipv6_dev_mc_inc/dec.
> 
> This patch moves ASSERT_RTNL() up a level in the call stack.
> 
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
> Reported-by: Tommi Rantala <tt.rantala@gmail.com>
> ---
> I included Cong's Signed-off-by for the first part of the patch,
> I hope that's OK.
> 
> This patch is based on -next, but since the assertion can also be
> triggered on a current kernel (tested on a 3.16), I think it should
> also go in stable.

Ack, thus you should base the patch on the net tree.

> 
>  include/linux/netdevice.h |  4 ++--
>  net/core/dev.c            | 11 ++++++-----
>  net/ipv6/addrconf.c       | 15 +++++----------
>  net/ipv6/anycast.c        | 30 +++++++++++++++++++-----------
>  net/ipv6/mcast.c          | 16 ++++++++++++++++
>  5 files changed, 48 insertions(+), 28 deletions(-)
> 
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 429801370d0c..1ae0e745b1b1 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -2077,8 +2077,8 @@ void __dev_remove_pack(struct packet_type *pt);
>  void dev_add_offload(struct packet_offload *po);
>  void dev_remove_offload(struct packet_offload *po);
>  
> -struct net_device *dev_get_by_flags_rcu(struct net *net, unsigned short flags,
> -					unsigned short mask);
> +struct net_device *dev_get_by_flags(struct net *net, unsigned short flags,
> +				    unsigned short mask);
>  struct net_device *dev_get_by_name(struct net *net, const char *name);
>  struct net_device *dev_get_by_name_rcu(struct net *net, const char *name);
>  struct net_device *__dev_get_by_name(struct net *net, const char *name);
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 443b814db05b..8fede6ef4a39 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -897,23 +897,24 @@ struct net_device *dev_getfirstbyhwtype(struct net *net, unsigned short type)
>  EXPORT_SYMBOL(dev_getfirstbyhwtype);
>  
>  /**
> - *	dev_get_by_flags_rcu - find any device with given flags
> + *	dev_get_by_flags - find any device with given flags
>   *	@net: the applicable net namespace
>   *	@if_flags: IFF_* values
>   *	@mask: bitmask of bits in if_flags to check
>   *
>   *	Search for any interface with the given flags. Returns NULL if a device
>   *	is not found or a pointer to the device. Must be called inside
> - *	rcu_read_lock(), and result refcount is unchanged.
> + *	rtnl_lock(), and result refcount is unchanged.
>   */
>  
> -struct net_device *dev_get_by_flags_rcu(struct net *net, unsigned short if_flags,
> +struct net_device *dev_get_by_flags(struct net *net, unsigned short if_flags,
>  				    unsigned short mask)
>  {
>  	struct net_device *dev, *ret;
>  
> +	ASSERT_RTNL();
>  	ret = NULL;
> -	for_each_netdev_rcu(net, dev) {
> +	for_each_netdev(net, dev) {
>  		if (((dev->flags ^ if_flags) & mask) == 0) {
>  			ret = dev;
>  			break;
> @@ -921,7 +922,7 @@ struct net_device *dev_get_by_flags_rcu(struct net *net, unsigned short if_flags
>  	}
>  	return ret;
>  }
> -EXPORT_SYMBOL(dev_get_by_flags_rcu);
> +EXPORT_SYMBOL(dev_get_by_flags);

I don't have a very strong opinion on that, but think we shouldn't touch
this function. In general it looks like a useful one and if you force
rtnl lock on it you cannot call it from bh anymore. I think we should
keep rcu locking here and in the anycast code. It shouldn't matter much.

>  /**
>   *	dev_valid_name - check if name is okay for network device
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index 267ce3caee24..7ada65937d23 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -1690,14 +1690,12 @@ void addrconf_dad_failure(struct inet6_ifaddr *ifp)
>  	addrconf_mod_dad_work(ifp, 0);
>  }
>  
> -/* Join to solicited addr multicast group. */
> -
> +/* Join to solicited addr multicast group.
> + * caller must hold RTNL */
>  void addrconf_join_solict(struct net_device *dev, const struct in6_addr *addr)
>  {
>  	struct in6_addr maddr;
>  
> -	ASSERT_RTNL();
> -
>  	if (dev->flags&(IFF_LOOPBACK|IFF_NOARP))
>  		return;
>  
> @@ -1705,12 +1703,11 @@ void addrconf_join_solict(struct net_device *dev, const struct in6_addr *addr)
>  	ipv6_dev_mc_inc(dev, &maddr);
>  }
>  
> +/* caller must hold RTNL */
>  void addrconf_leave_solict(struct inet6_dev *idev, const struct in6_addr *addr)
>  {
>  	struct in6_addr maddr;
>  
> -	ASSERT_RTNL();
> -
>  	if (idev->dev->flags&(IFF_LOOPBACK|IFF_NOARP))
>  		return;
>  
> @@ -1718,12 +1715,11 @@ void addrconf_leave_solict(struct inet6_dev *idev, const struct in6_addr *addr)
>  	__ipv6_dev_mc_dec(idev, &maddr);
>  }
>  
> +/* caller must hold RTNL */
>  static void addrconf_join_anycast(struct inet6_ifaddr *ifp)
>  {
>  	struct in6_addr addr;
>  
> -	ASSERT_RTNL();
> -
>  	if (ifp->prefix_len >= 127) /* RFC 6164 */
>  		return;
>  	ipv6_addr_prefix(&addr, &ifp->addr, ifp->prefix_len);
> @@ -1732,12 +1728,11 @@ static void addrconf_join_anycast(struct inet6_ifaddr *ifp)
>  	ipv6_dev_ac_inc(ifp->idev->dev, &addr);
>  }
>  
> +/* caller must hold RTNL */
>  static void addrconf_leave_anycast(struct inet6_ifaddr *ifp)
>  {
>  	struct in6_addr addr;
>  
> -	ASSERT_RTNL();
> -
>  	if (ifp->prefix_len >= 127) /* RFC 6164 */
>  		return;
>  	ipv6_addr_prefix(&addr, &ifp->addr, ifp->prefix_len);
> diff --git a/net/ipv6/anycast.c b/net/ipv6/anycast.c
> index 210183244689..572c2faede55 100644
> --- a/net/ipv6/anycast.c
> +++ b/net/ipv6/anycast.c
> @@ -77,7 +77,7 @@ int ipv6_sock_ac_join(struct sock *sk, int ifindex, const struct in6_addr *addr)
>  	pac->acl_next = NULL;
>  	pac->acl_addr = *addr;
>  
> -	rcu_read_lock();
> +	rtnl_lock();

We would need to keep rcu_read_lock inside rtnl_lock if you decide to
keep dev_get_by_flags_rcu around.

>  	if (ifindex == 0) {
>  		struct rt6_info *rt;
>  
> @@ -90,11 +90,11 @@ int ipv6_sock_ac_join(struct sock *sk, int ifindex, const struct in6_addr *addr)
>  			goto error;
>  		} else {
>  			/* router, no matching interface: just pick one */
> -			dev = dev_get_by_flags_rcu(net, IFF_UP,
> -						   IFF_UP | IFF_LOOPBACK);
> +			dev = dev_get_by_flags(net, IFF_UP,
> +					       IFF_UP | IFF_LOOPBACK);
>  		}
>  	} else
> -		dev = dev_get_by_index_rcu(net, ifindex);
> +		dev = __dev_get_by_index(net, ifindex);
>  
>  	if (dev == NULL) {
>  		err = -ENODEV;
> @@ -136,7 +136,7 @@ int ipv6_sock_ac_join(struct sock *sk, int ifindex, const struct in6_addr *addr)
>  	}
>  
>  error:
> -	rcu_read_unlock();
> +	rtnl_unlock();

...here, too.

>  	if (pac)
>  		sock_kfree_s(sk, pac, sizeof(*pac));
>  	return err;
> @@ -171,13 +171,15 @@ int ipv6_sock_ac_drop(struct sock *sk, int ifindex, const struct in6_addr *addr)
>  
>  	spin_unlock_bh(&ipv6_sk_ac_lock);
>  
> -	rcu_read_lock();
> -	dev = dev_get_by_index_rcu(net, pac->acl_ifindex);
> +	rtnl_lock();
> +	dev = __dev_get_by_index(net, pac->acl_ifindex);
>  	if (dev)
>  		ipv6_dev_ac_dec(dev, &pac->acl_addr);
> -	rcu_read_unlock();
> +	rtnl_unlock();
>  
>  	sock_kfree_s(sk, pac, sizeof(*pac));
> +	if (!dev)
> +		return -ENODEV;
>  	return 0;
>  }
>  
> @@ -198,12 +200,12 @@ void ipv6_sock_ac_close(struct sock *sk)
>  	spin_unlock_bh(&ipv6_sk_ac_lock);
>  
>  	prev_index = 0;
> -	rcu_read_lock();
> +	rtnl_lock();
>  	while (pac) {
>  		struct ipv6_ac_socklist *next = pac->acl_next;
>  
>  		if (pac->acl_ifindex != prev_index) {
> -			dev = dev_get_by_index_rcu(net, pac->acl_ifindex);
> +			dev = __dev_get_by_index(net, pac->acl_ifindex);
>  			prev_index = pac->acl_ifindex;
>  		}
>  		if (dev)
> @@ -211,7 +213,7 @@ void ipv6_sock_ac_close(struct sock *sk)
>  		sock_kfree_s(sk, pac, sizeof(*pac));
>  		pac = next;
>  	}
> -	rcu_read_unlock();
> +	rtnl_unlock();
>  }
>  
>  static void aca_put(struct ifacaddr6 *ac)
> @@ -233,6 +235,8 @@ int ipv6_dev_ac_inc(struct net_device *dev, const struct in6_addr *addr)
>  	struct rt6_info *rt;
>  	int err;
>  
> +	ASSERT_RTNL();
> +
>  	idev = in6_dev_get(dev);
>  
>  	if (idev == NULL)
> @@ -302,6 +306,8 @@ int __ipv6_dev_ac_dec(struct inet6_dev *idev, const struct in6_addr *addr)
>  {
>  	struct ifacaddr6 *aca, *prev_aca;
>  
> +	ASSERT_RTNL();
> +
>  	write_lock_bh(&idev->lock);
>  	prev_aca = NULL;
>  	for (aca = idev->ac_list; aca; aca = aca->aca_next) {
> @@ -336,6 +342,8 @@ static int ipv6_dev_ac_dec(struct net_device *dev, const struct in6_addr *addr)
>  {
>  	struct inet6_dev *idev = __in6_dev_get(dev);
>  
> +	ASSERT_RTNL();
> +
>  	if (idev == NULL)
>  		return -ENODEV;
>  	return __ipv6_dev_ac_dec(idev, addr);
> diff --git a/net/ipv6/mcast.c b/net/ipv6/mcast.c
> index 70881795da96..d73ac1ef65f2 100644
> --- a/net/ipv6/mcast.c
> +++ b/net/ipv6/mcast.c
> @@ -172,6 +172,7 @@ int ipv6_sock_mc_join(struct sock *sk, int ifindex, const struct in6_addr *addr)
>  	mc_lst->next = NULL;
>  	mc_lst->addr = *addr;
>  
> +	rtnl_lock();
>  	rcu_read_lock();
>  	if (ifindex == 0) {
>  		struct rt6_info *rt;
> @@ -185,6 +186,7 @@ int ipv6_sock_mc_join(struct sock *sk, int ifindex, const struct in6_addr *addr)
>  
>  	if (dev == NULL) {
>  		rcu_read_unlock();
> +		rtnl_unlock();
>  		sock_kfree_s(sk, mc_lst, sizeof(*mc_lst));
>  		return -ENODEV;
>  	}
> @@ -202,6 +204,7 @@ int ipv6_sock_mc_join(struct sock *sk, int ifindex, const struct in6_addr *addr)
>  
>  	if (err) {
>  		rcu_read_unlock();
> +		rtnl_unlock();
>  		sock_kfree_s(sk, mc_lst, sizeof(*mc_lst));
>  		return err;
>  	}
> @@ -212,6 +215,7 @@ int ipv6_sock_mc_join(struct sock *sk, int ifindex, const struct in6_addr *addr)
>  	spin_unlock(&ipv6_sk_mc_lock);
>  
>  	rcu_read_unlock();
> +	rtnl_unlock();
>  
>  	return 0;
>  }
> @@ -229,6 +233,7 @@ int ipv6_sock_mc_drop(struct sock *sk, int ifindex, const struct in6_addr *addr)
>  	if (!ipv6_addr_is_multicast(addr))
>  		return -EINVAL;
>  
> +	rtnl_lock();
>  	spin_lock(&ipv6_sk_mc_lock);
>  	for (lnk = &np->ipv6_mc_list;
>  	     (mc_lst = rcu_dereference_protected(*lnk,
> @@ -252,12 +257,15 @@ int ipv6_sock_mc_drop(struct sock *sk, int ifindex, const struct in6_addr *addr)
>  			} else
>  				(void) ip6_mc_leave_src(sk, mc_lst, NULL);
>  			rcu_read_unlock();
> +			rtnl_unlock();
> +
>  			atomic_sub(sizeof(*mc_lst), &sk->sk_omem_alloc);
>  			kfree_rcu(mc_lst, rcu);
>  			return 0;
>  		}
>  	}
>  	spin_unlock(&ipv6_sk_mc_lock);
> +	rtnl_unlock();
>  
>  	return -EADDRNOTAVAIL;
>  }
> @@ -302,6 +310,7 @@ void ipv6_sock_mc_close(struct sock *sk)
>  	if (!rcu_access_pointer(np->ipv6_mc_list))
>  		return;
>  
> +	rtnl_lock();
>  	spin_lock(&ipv6_sk_mc_lock);
>  	while ((mc_lst = rcu_dereference_protected(np->ipv6_mc_list,
>  				lockdep_is_held(&ipv6_sk_mc_lock))) != NULL) {
> @@ -328,6 +337,7 @@ void ipv6_sock_mc_close(struct sock *sk)
>  		spin_lock(&ipv6_sk_mc_lock);
>  	}
>  	spin_unlock(&ipv6_sk_mc_lock);
> +	rtnl_unlock();
>  }
>  
>  int ip6_mc_source(int add, int omode, struct sock *sk,
> @@ -845,6 +855,8 @@ int ipv6_dev_mc_inc(struct net_device *dev, const struct in6_addr *addr)
>  	struct ifmcaddr6 *mc;
>  	struct inet6_dev *idev;
>  
> +	ASSERT_RTNL();
> +
>  	/* we need to take a reference on idev */
>  	idev = in6_dev_get(dev);
>  
> @@ -916,6 +928,8 @@ int __ipv6_dev_mc_dec(struct inet6_dev *idev, const struct in6_addr *addr)
>  {
>  	struct ifmcaddr6 *ma, **map;
>  
> +	ASSERT_RTNL();
> +
>  	write_lock_bh(&idev->lock);
>  	for (map = &idev->mc_list; (ma = *map) != NULL; map = &ma->next) {
>  		if (ipv6_addr_equal(&ma->mca_addr, addr)) {
> @@ -942,6 +956,8 @@ int ipv6_dev_mc_dec(struct net_device *dev, const struct in6_addr *addr)
>  	struct inet6_dev *idev;
>  	int err;
>  
> +	ASSERT_RTNL();
> +

Minor nit:
This one is not necessary and will be guarded by the __ipv6_dev_mc_dec
one.

>  	rcu_read_lock();
>  
>  	idev = __in6_dev_get(dev);

Rest looks good to me, thanks,
Hannes

[PATCH net v2] ipv6: fix rtnl locking in setsockopt for anycast and multicast

[Sabrina Dubroca]

Calling setsockopt with IPV6_JOIN_ANYCAST or IPV6_LEAVE_ANYCAST
triggers the assertion in addrconf_join_solict()/addrconf_leave_solict()

ipv6_sock_ac_join(), ipv6_sock_ac_drop(), ipv6_sock_ac_close() need to
take RTNL before calling ipv6_dev_ac_inc/dec. Same thing with
ipv6_sock_mc_join(), ipv6_sock_mc_drop(), ipv6_sock_mc_close() before
calling ipv6_dev_mc_inc/dec.

This patch moves ASSERT_RTNL() up a level in the call stack.

Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
Reported-by: Tommi Rantala <tt.rantala@gmail.com>
---
As was said earlier, this should go in stable.

v2:
 - based on net
 - keep dev_get_by_flags_rcu and RCU in ipv6_sock_ac_*
 - remove two ASSERT_RTNL() that are not necessary

Thank you for your help, Hannes!

 net/ipv6/addrconf.c | 15 +++++----------
 net/ipv6/anycast.c  | 12 ++++++++++++
 net/ipv6/mcast.c    | 14 ++++++++++++++
 3 files changed, 31 insertions(+), 10 deletions(-)

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 0b239fc1816e..aa0e135b808c 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -1690,14 +1690,12 @@ void addrconf_dad_failure(struct inet6_ifaddr *ifp)
 	addrconf_mod_dad_work(ifp, 0);
 }
 
-/* Join to solicited addr multicast group. */
-
+/* Join to solicited addr multicast group.
+ * caller must hold RTNL */
 void addrconf_join_solict(struct net_device *dev, const struct in6_addr *addr)
 {
 	struct in6_addr maddr;
 
-	ASSERT_RTNL();
-
 	if (dev->flags&(IFF_LOOPBACK|IFF_NOARP))
 		return;
 
@@ -1705,12 +1703,11 @@ void addrconf_join_solict(struct net_device *dev, const struct in6_addr *addr)
 	ipv6_dev_mc_inc(dev, &maddr);
 }
 
+/* caller must hold RTNL */
 void addrconf_leave_solict(struct inet6_dev *idev, const struct in6_addr *addr)
 {
 	struct in6_addr maddr;
 
-	ASSERT_RTNL();
-
 	if (idev->dev->flags&(IFF_LOOPBACK|IFF_NOARP))
 		return;
 
@@ -1718,12 +1715,11 @@ void addrconf_leave_solict(struct inet6_dev *idev, const struct in6_addr *addr)
 	__ipv6_dev_mc_dec(idev, &maddr);
 }
 
+/* caller must hold RTNL */
 static void addrconf_join_anycast(struct inet6_ifaddr *ifp)
 {
 	struct in6_addr addr;
 
-	ASSERT_RTNL();
-
 	if (ifp->prefix_len >= 127) /* RFC 6164 */
 		return;
 	ipv6_addr_prefix(&addr, &ifp->addr, ifp->prefix_len);
@@ -1732,12 +1728,11 @@ static void addrconf_join_anycast(struct inet6_ifaddr *ifp)
 	ipv6_dev_ac_inc(ifp->idev->dev, &addr);
 }
 
+/* caller must hold RTNL */
 static void addrconf_leave_anycast(struct inet6_ifaddr *ifp)
 {
 	struct in6_addr addr;
 
-	ASSERT_RTNL();
-
 	if (ifp->prefix_len >= 127) /* RFC 6164 */
 		return;
 	ipv6_addr_prefix(&addr, &ifp->addr, ifp->prefix_len);
diff --git a/net/ipv6/anycast.c b/net/ipv6/anycast.c
index 210183244689..45b9d81d91e8 100644
--- a/net/ipv6/anycast.c
+++ b/net/ipv6/anycast.c
@@ -77,6 +77,7 @@ int ipv6_sock_ac_join(struct sock *sk, int ifindex, const struct in6_addr *addr)
 	pac->acl_next = NULL;
 	pac->acl_addr = *addr;
 
+	rtnl_lock();
 	rcu_read_lock();
 	if (ifindex == 0) {
 		struct rt6_info *rt;
@@ -137,6 +138,7 @@ int ipv6_sock_ac_join(struct sock *sk, int ifindex, const struct in6_addr *addr)
 
 error:
 	rcu_read_unlock();
+	rtnl_unlock();
 	if (pac)
 		sock_kfree_s(sk, pac, sizeof(*pac));
 	return err;
@@ -171,13 +173,17 @@ int ipv6_sock_ac_drop(struct sock *sk, int ifindex, const struct in6_addr *addr)
 
 	spin_unlock_bh(&ipv6_sk_ac_lock);
 
+	rtnl_lock();
 	rcu_read_lock();
 	dev = dev_get_by_index_rcu(net, pac->acl_ifindex);
 	if (dev)
 		ipv6_dev_ac_dec(dev, &pac->acl_addr);
 	rcu_read_unlock();
+	rtnl_unlock();
 
 	sock_kfree_s(sk, pac, sizeof(*pac));
+	if (!dev)
+		return -ENODEV;
 	return 0;
 }
 
@@ -198,6 +204,7 @@ void ipv6_sock_ac_close(struct sock *sk)
 	spin_unlock_bh(&ipv6_sk_ac_lock);
 
 	prev_index = 0;
+	rtnl_lock();
 	rcu_read_lock();
 	while (pac) {
 		struct ipv6_ac_socklist *next = pac->acl_next;
@@ -212,6 +219,7 @@ void ipv6_sock_ac_close(struct sock *sk)
 		pac = next;
 	}
 	rcu_read_unlock();
+	rtnl_unlock();
 }
 
 static void aca_put(struct ifacaddr6 *ac)
@@ -233,6 +241,8 @@ int ipv6_dev_ac_inc(struct net_device *dev, const struct in6_addr *addr)
 	struct rt6_info *rt;
 	int err;
 
+	ASSERT_RTNL();
+
 	idev = in6_dev_get(dev);
 
 	if (idev == NULL)
@@ -302,6 +312,8 @@ int __ipv6_dev_ac_dec(struct inet6_dev *idev, const struct in6_addr *addr)
 {
 	struct ifacaddr6 *aca, *prev_aca;
 
+	ASSERT_RTNL();
+
 	write_lock_bh(&idev->lock);
 	prev_aca = NULL;
 	for (aca = idev->ac_list; aca; aca = aca->aca_next) {
diff --git a/net/ipv6/mcast.c b/net/ipv6/mcast.c
index 617f0958e164..a23b655a7627 100644
--- a/net/ipv6/mcast.c
+++ b/net/ipv6/mcast.c
@@ -172,6 +172,7 @@ int ipv6_sock_mc_join(struct sock *sk, int ifindex, const struct in6_addr *addr)
 	mc_lst->next = NULL;
 	mc_lst->addr = *addr;
 
+	rtnl_lock();
 	rcu_read_lock();
 	if (ifindex == 0) {
 		struct rt6_info *rt;
@@ -185,6 +186,7 @@ int ipv6_sock_mc_join(struct sock *sk, int ifindex, const struct in6_addr *addr)
 
 	if (dev == NULL) {
 		rcu_read_unlock();
+		rtnl_unlock();
 		sock_kfree_s(sk, mc_lst, sizeof(*mc_lst));
 		return -ENODEV;
 	}
@@ -202,6 +204,7 @@ int ipv6_sock_mc_join(struct sock *sk, int ifindex, const struct in6_addr *addr)
 
 	if (err) {
 		rcu_read_unlock();
+		rtnl_unlock();
 		sock_kfree_s(sk, mc_lst, sizeof(*mc_lst));
 		return err;
 	}
@@ -212,6 +215,7 @@ int ipv6_sock_mc_join(struct sock *sk, int ifindex, const struct in6_addr *addr)
 	spin_unlock(&ipv6_sk_mc_lock);
 
 	rcu_read_unlock();
+	rtnl_unlock();
 
 	return 0;
 }
@@ -229,6 +233,7 @@ int ipv6_sock_mc_drop(struct sock *sk, int ifindex, const struct in6_addr *addr)
 	if (!ipv6_addr_is_multicast(addr))
 		return -EINVAL;
 
+	rtnl_lock();
 	spin_lock(&ipv6_sk_mc_lock);
 	for (lnk = &np->ipv6_mc_list;
 	     (mc_lst = rcu_dereference_protected(*lnk,
@@ -252,12 +257,15 @@ int ipv6_sock_mc_drop(struct sock *sk, int ifindex, const struct in6_addr *addr)
 			} else
 				(void) ip6_mc_leave_src(sk, mc_lst, NULL);
 			rcu_read_unlock();
+			rtnl_unlock();
+
 			atomic_sub(sizeof(*mc_lst), &sk->sk_omem_alloc);
 			kfree_rcu(mc_lst, rcu);
 			return 0;
 		}
 	}
 	spin_unlock(&ipv6_sk_mc_lock);
+	rtnl_unlock();
 
 	return -EADDRNOTAVAIL;
 }
@@ -302,6 +310,7 @@ void ipv6_sock_mc_close(struct sock *sk)
 	if (!rcu_access_pointer(np->ipv6_mc_list))
 		return;
 
+	rtnl_lock();
 	spin_lock(&ipv6_sk_mc_lock);
 	while ((mc_lst = rcu_dereference_protected(np->ipv6_mc_list,
 				lockdep_is_held(&ipv6_sk_mc_lock))) != NULL) {
@@ -328,6 +337,7 @@ void ipv6_sock_mc_close(struct sock *sk)
 		spin_lock(&ipv6_sk_mc_lock);
 	}
 	spin_unlock(&ipv6_sk_mc_lock);
+	rtnl_unlock();
 }
 
 int ip6_mc_source(int add, int omode, struct sock *sk,
@@ -845,6 +855,8 @@ int ipv6_dev_mc_inc(struct net_device *dev, const struct in6_addr *addr)
 	struct ifmcaddr6 *mc;
 	struct inet6_dev *idev;
 
+	ASSERT_RTNL();
+
 	/* we need to take a reference on idev */
 	idev = in6_dev_get(dev);
 
@@ -916,6 +928,8 @@ int __ipv6_dev_mc_dec(struct inet6_dev *idev, const struct in6_addr *addr)
 {
 	struct ifmcaddr6 *ma, **map;
 
+	ASSERT_RTNL();
+
 	write_lock_bh(&idev->lock);
 	for (map = &idev->mc_list; (ma=*map) != NULL; map = &ma->next) {
 		if (ipv6_addr_equal(&ma->mca_addr, addr)) {
-- 
2.1.0

Re: [PATCH net v2] ipv6: fix rtnl locking in setsockopt for anycast and multicast

[Hannes Frederic Sowa]

On Di, 2014-09-02 at 10:29 +0200, Sabrina Dubroca wrote:
> Calling setsockopt with IPV6_JOIN_ANYCAST or IPV6_LEAVE_ANYCAST
> triggers the assertion in addrconf_join_solict()/addrconf_leave_solict()
> 
> ipv6_sock_ac_join(), ipv6_sock_ac_drop(), ipv6_sock_ac_close() need to
> take RTNL before calling ipv6_dev_ac_inc/dec. Same thing with
> ipv6_sock_mc_join(), ipv6_sock_mc_drop(), ipv6_sock_mc_close() before
> calling ipv6_dev_mc_inc/dec.
> 
> This patch moves ASSERT_RTNL() up a level in the call stack.
> 
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
> Reported-by: Tommi Rantala <tt.rantala@gmail.com>
> ---
> As was said earlier, this should go in stable.
> 
> v2:
>  - based on net
>  - keep dev_get_by_flags_rcu and RCU in ipv6_sock_ac_*
>  - remove two ASSERT_RTNL() that are not necessary
> 
> Thank you for your help, Hannes!

Thanks for fixing! ;)

Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>

Re: [PATCH net v2] ipv6: fix rtnl locking in setsockopt for anycast and multicast

[Cong Wang]

On Tue, Sep 2, 2014 at 1:29 AM, Sabrina Dubroca <sd@queasysnail.net> wrote:
>
> v2:
>  - based on net
>  - keep dev_get_by_flags_rcu and RCU in ipv6_sock_ac_*
>  - remove two ASSERT_RTNL() that are not necessary

There is no point to keep RCU here. Hannes' reply doesn't make
any sense.

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

[Cong Wang]

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.

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.

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

[Hannes Frederic Sowa]

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.

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

Bye,
Hannes

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

[Cong Wang]

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.

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

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

[Eric Dumazet]

On Tue, 2014-09-02 at 11:04 -0700, Cong Wang wrote:
> On Tue, Sep 2, 2014 at 10:58 AM, Hannes Frederic Sowa

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

Point is : you did not check if some stable versions had more callers.

Its very nice you checked current version, but it is not enough for a
stable candidate.

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

[Cong Wang]

On Tue, Sep 2, 2014 at 11:11 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Tue, 2014-09-02 at 11:04 -0700, Cong Wang wrote:
>> On Tue, Sep 2, 2014 at 10:58 AM, Hannes Frederic Sowa
>
>> > 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.
>
> Point is : you did not check if some stable versions had more callers.
>
> Its very nice you checked current version, but it is not enough for a
> stable candidate.

That is what we do when backporting patches, I can do that if David asks
me to backport it, but you know for netdev that is David's work.

(I am not saying I don't want to help him, I just want to point out the fact.
I am very pleased to help David for stable backports as long as he asks)

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

[Hannes Frederic Sowa]

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

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

[Eric Dumazet]

On Tue, 2014-09-02 at 11:15 -0700, Cong Wang wrote:

> That is what we do when backporting patches, I can do that if David asks
> me to backport it, but you know for netdev that is David's work.
> 
> (I am not saying I don't want to help him, I just want to point out the fact.
> I am very pleased to help David for stable backports as long as he asks)

Problem is : your patch submission do not identify bug origin.

You claim you want to help, but you do not provide the basic thing that
_really_ helps.

The proper way to identify bug origin is to add in the headers one
line :

Fixes: 12-digit-SHA1 ("patch title")

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

[Cong Wang]

On Tue, Sep 2, 2014 at 11:21 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Tue, 2014-09-02 at 11:15 -0700, Cong Wang wrote:
>
>> That is what we do when backporting patches, I can do that if David asks
>> me to backport it, but you know for netdev that is David's work.
>>
>> (I am not saying I don't want to help him, I just want to point out the fact.
>> I am very pleased to help David for stable backports as long as he asks)
>
> Problem is : your patch submission do not identify bug origin.
>
> You claim you want to help, but you do not provide the basic thing that
> _really_ helps.
>
> The proper way to identify bug origin is to add in the headers one
> line :
>
> Fixes: 12-digit-SHA1 ("patch title")
>

Since when "Fixes:" tag becomes mandatory for a stable patch?
At least netdev-FAQ is not updated. ;-) I 100% agree "Fixes:"
is helpful when backporting patches, but it is not mandatory currently.

For this patch, I was too lazy to dig the history, it looks like this is
caused by the following commit:

commit c15b1ccadb323ea50023e8f1cca2954129a62b51
Author: Hannes Frederic Sowa <hannes@stressinduktion.org>
Date:   Thu Mar 27 18:28:07 2014 +0100

    ipv6: move DAD and addrconf_verify processing to workqueue

Thanks.

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

[Cong Wang]

On Tue, Sep 2, 2014 at 11:18 AM, Hannes Frederic Sowa
<hannes@stressinduktion.org> wrote:
> 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.
>

Sure, I never doubt adding ASSERT_RTNL() is helpful, I just still think
this should be for net-next, or at least a separated patch. I don't want
my patch to be blamed in others' "Fixes:". :)

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

[Hannes Frederic Sowa]

On Di, 2014-09-02 at 11:40 -0700, Cong Wang wrote:
> On Tue, Sep 2, 2014 at 11:18 AM, Hannes Frederic Sowa
> <hannes@stressinduktion.org> wrote:
> > 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.
> >
> 
> Sure, I never doubt adding ASSERT_RTNL() is helpful, I just still think
> this should be for net-next, or at least a separated patch. I don't want
> my patch to be blamed in others' "Fixes:". :)

Come on, that's why we have community review. Nobody blames anyone
because of added regressions. It's more a fault of the community then,
and it works out fairly good I think! Even others are keen on fixing
your bugs sometimes. ;)

If fixes tag is well researched, it won't point to the addition of
ASSERT_RTNL() but your patch would help to discover a bug somewhere else
in the stack.

I think for this patch a fixes-tag is hard to find because it is hard to
find because it dates back to the beginning of the git history IMHO.

Bye,
Hannes

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

[Vlad Yasevich]

On 09/02/2014 02:15 PM, Cong Wang wrote:
> On Tue, Sep 2, 2014 at 11:11 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> On Tue, 2014-09-02 at 11:04 -0700, Cong Wang wrote:
>>> On Tue, Sep 2, 2014 at 10:58 AM, Hannes Frederic Sowa
>>
>>>> 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.
>>
>> Point is : you did not check if some stable versions had more callers.
>>
>> Its very nice you checked current version, but it is not enough for a
>> stable candidate.
> 
> That is what we do when backporting patches, I can do that if David asks
> me to backport it, but you know for netdev that is David's work.
> 
> (I am not saying I don't want to help him, I just want to point out the fact.
> I am very pleased to help David for stable backports as long as he asks)

Instead of helping after the fact, why not arrange the patches so that it's
not such a big issue.  Leave the _rcu variant alone.  Add an _rtnl variant
of the function and use that in the patch.  Have a follow-on patch that
removes the _rcu variant all by itself.  This way backports become easier,
and if anyone wants the _rcu variant back, all they have to do is revert
a very simple commit.

-vlad

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

[Cong Wang]

On Tue, Sep 2, 2014 at 12:02 PM, Hannes Frederic Sowa
<hannes@stressinduktion.org> wrote:
>
> If fixes tag is well researched, it won't point to the addition of
> ASSERT_RTNL() but your patch would help to discover a bug somewhere else
> in the stack.
>
> I think for this patch a fixes-tag is hard to find because it is hard to
> find because it dates back to the beginning of the git history IMHO.
>

As I replied to Eric, this warning is probably caused by
the following commit:

commit c15b1ccadb323ea50023e8f1cca2954129a62b51
Author: Hannes Frederic Sowa <hannes@stressinduktion.org>
Date:   Thu Mar 27 18:28:07 2014 +0100

    ipv6: move DAD and addrconf_verify processing to workqueue


HOWEVER, you can definitely argue that the code without your
ASSERT_RTNL() was already broken, it's a little hard to tell without
digging more, that might date back to the beginning of git as you said.

For safety, I think we can simply assume it's that commit to be fixed
so that we don't fix older kernels until someone really reports a bug.

Re: [PATCH net v2] ipv6: fix rtnl locking in setsockopt for anycast and multicast

[David Miller]

From: Sabrina Dubroca <sd@queasysnail.net>
Date: Tue, 2 Sep 2014 10:29:29 +0200

> Calling setsockopt with IPV6_JOIN_ANYCAST or IPV6_LEAVE_ANYCAST
> triggers the assertion in addrconf_join_solict()/addrconf_leave_solict()
> 
> ipv6_sock_ac_join(), ipv6_sock_ac_drop(), ipv6_sock_ac_close() need to
> take RTNL before calling ipv6_dev_ac_inc/dec. Same thing with
> ipv6_sock_mc_join(), ipv6_sock_mc_drop(), ipv6_sock_mc_close() before
> calling ipv6_dev_mc_inc/dec.
> 
> This patch moves ASSERT_RTNL() up a level in the call stack.
> 
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
> Reported-by: Tommi Rantala <tt.rantala@gmail.com>

Applied and queued up for -stable, thanks.

Re: [PATCH net v2] ipv6: fix rtnl locking in setsockopt for anycast and multicast

[Cong Wang]

On Fri, Sep 5, 2014 at 11:53 AM, David Miller <davem@davemloft.net> wrote:
> From: Sabrina Dubroca <sd@queasysnail.net>
> Date: Tue, 2 Sep 2014 10:29:29 +0200
>
>> Calling setsockopt with IPV6_JOIN_ANYCAST or IPV6_LEAVE_ANYCAST
>> triggers the assertion in addrconf_join_solict()/addrconf_leave_solict()
>>
>> ipv6_sock_ac_join(), ipv6_sock_ac_drop(), ipv6_sock_ac_close() need to
>> take RTNL before calling ipv6_dev_ac_inc/dec. Same thing with
>> ipv6_sock_mc_join(), ipv6_sock_mc_drop(), ipv6_sock_mc_close() before
>> calling ipv6_dev_mc_inc/dec.
>>
>> This patch moves ASSERT_RTNL() up a level in the call stack.
>>
>> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
>> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
>> Reported-by: Tommi Rantala <tt.rantala@gmail.com>
>
> Applied and queued up for -stable, thanks.

I believe you applied a wrong version, at least the following
is not correct:

+       if (!dev)
+               return -ENODEV;

Sabrina took that from my draft patch, but they all don't
realize this is wrong.

(I did provide a correct version which is just ignored by you.)

Re: [PATCH net v2] ipv6: fix rtnl locking in setsockopt for anycast and multicast

[Hannes Frederic Sowa]

On Fr, 2014-09-05 at 11:58 -0700, Cong Wang wrote:
> On Fri, Sep 5, 2014 at 11:53 AM, David Miller <davem@davemloft.net> wrote:
> > From: Sabrina Dubroca <sd@queasysnail.net>
> > Date: Tue, 2 Sep 2014 10:29:29 +0200
> >
> >> Calling setsockopt with IPV6_JOIN_ANYCAST or IPV6_LEAVE_ANYCAST
> >> triggers the assertion in addrconf_join_solict()/addrconf_leave_solict()
> >>
> >> ipv6_sock_ac_join(), ipv6_sock_ac_drop(), ipv6_sock_ac_close() need to
> >> take RTNL before calling ipv6_dev_ac_inc/dec. Same thing with
> >> ipv6_sock_mc_join(), ipv6_sock_mc_drop(), ipv6_sock_mc_close() before
> >> calling ipv6_dev_mc_inc/dec.
> >>
> >> This patch moves ASSERT_RTNL() up a level in the call stack.
> >>
> >> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> >> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
> >> Reported-by: Tommi Rantala <tt.rantala@gmail.com>
> >
> > Applied and queued up for -stable, thanks.
> 
> I believe you applied a wrong version, at least the following
> is not correct:
> 
> +       if (!dev)
> +               return -ENODEV;
> 
> Sabrina took that from my draft patch, but they all don't
> realize this is wrong.
> 
> (I did provide a correct version which is just ignored by you.)

What games are you playing? You know how patches are processed by David
and I even let him the choice by pointing out a problem in your patch so
that you could an update and send v2.

I really feel miffed about your behavior!

Anyway, I saw the hunk adding the return -ENODEV and didn't see any
problems with it. Sure it might be better if it would gone into a
separate patch. Can you elaborate what problems you see?

Thanks,
Hannes

Re: [PATCH net v2] ipv6: fix rtnl locking in setsockopt for anycast and multicast

[David Miller]

From: Cong Wang <cwang@twopensource.com>
Date: Fri, 5 Sep 2014 11:58:31 -0700

> On Fri, Sep 5, 2014 at 11:53 AM, David Miller <davem@davemloft.net> wrote:
>> From: Sabrina Dubroca <sd@queasysnail.net>
>> Date: Tue, 2 Sep 2014 10:29:29 +0200
>>
>>> Calling setsockopt with IPV6_JOIN_ANYCAST or IPV6_LEAVE_ANYCAST
>>> triggers the assertion in addrconf_join_solict()/addrconf_leave_solict()
>>>
>>> ipv6_sock_ac_join(), ipv6_sock_ac_drop(), ipv6_sock_ac_close() need to
>>> take RTNL before calling ipv6_dev_ac_inc/dec. Same thing with
>>> ipv6_sock_mc_join(), ipv6_sock_mc_drop(), ipv6_sock_mc_close() before
>>> calling ipv6_dev_mc_inc/dec.
>>>
>>> This patch moves ASSERT_RTNL() up a level in the call stack.
>>>
>>> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
>>> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
>>> Reported-by: Tommi Rantala <tt.rantala@gmail.com>
>>
>> Applied and queued up for -stable, thanks.
> 
> I believe you applied a wrong version, at least the following
> is not correct:
> 
> +       if (!dev)
> +               return -ENODEV;
> 
> Sabrina took that from my draft patch, but they all don't
> realize this is wrong.
> 
> (I did provide a correct version which is just ignored by you.)

Not ignored, but rather it was hard to interpret the situation due to
poor communication in the feedback emails.

The onus is on you guys to communicate things precisely so that I
understand what patch is in what state.

Re: [PATCH net v2] ipv6: fix rtnl locking in setsockopt for anycast and multicast

[Cong Wang]

On Fri, Sep 5, 2014 at 12:12 PM, Hannes Frederic Sowa
<hannes@stressinduktion.org> wrote:
>
> What games are you playing? You know how patches are processed by David
> and I even let him the choice by pointing out a problem in your patch so
> that you could an update and send v2.

I assume David goes over all the discussion before applying any patch.
So in the original discussion, obviously I disagree with your point on RCU,
and sent my own version.

I do understand David may miss something due to massive emails in
this list, but you make no point here nor I understand your broken English
"let him the choice...".

>
> I really feel miffed about your behavior!
>

I SEE. Please do redirect my email to your /dev/null. Thanks a lot!

> Anyway, I saw the hunk adding the return -ENODEV and didn't see any
> problems with it. Sure it might be better if it would gone into a
> separate patch. Can you elaborate what problems you see?
>

Yes, it should be not in this patch at the very very least.

Re: [PATCH net v2] ipv6: fix rtnl locking in setsockopt for anycast and multicast

[David Miller]

From: Cong Wang <cwang@twopensource.com>
Date: Fri, 5 Sep 2014 12:23:37 -0700

> On Fri, Sep 5, 2014 at 12:12 PM, Hannes Frederic Sowa
> <hannes@stressinduktion.org> wrote:
>>
>> What games are you playing? You know how patches are processed by David
>> and I even let him the choice by pointing out a problem in your patch so
>> that you could an update and send v2.
> 
> I assume David goes over all the discussion before applying any patch.
> So in the original discussion, obviously I disagree with your point on RCU,
> and sent my own version.

I thought the retention of RCU locking was reasonable, and that your
feedback was something I disagreed with.

This is different from ignoring your feedback.

Re: [PATCH net v2] ipv6: fix rtnl locking in setsockopt for anycast and multicast

[Cong Wang]

On Fri, Sep 5, 2014 at 12:25 PM, David Miller <davem@davemloft.net> wrote:
>
> I thought the retention of RCU locking was reasonable, and that your
> feedback was something I disagreed with.
>
> This is different from ignoring your feedback.

The point is we don't need to backport the patch that far, as I already
mentioned, up to  commit c15b1ccadb323ea
("ipv6: move DAD and addrconf_verify processing to workqueue"),
simply because no one reports any bug.

So we don't have to adjust the patch just for stable in this case.