ipv6: fix RTNL assert fail in DAD

PATCH In review Stephen Hemminger 3 participants 17 messages Mar 17, 2014 – Mar 28, 2014

Subsystems (3)

Networking Network drivers IPv4/IPv6

lore

Share & tools

lore

Stephen Hemminger

author

2014-03-17 23:39 UTC #0 lore raw

PATCH 1/1 ipv6: fix RTNL assert fail in DAD

IPv6 duplicate address detection is triggering the following assertion failure when using macvlan + vif + multicast.
RTNL: assertion failed at net/core/dev.c (4496)

This happens because the DAD timer is adding a multicast address without acquiring the RTNL mutex. In order to acquire the RTNL mutex, it must be done in process context; therefore it must be in a workqueue.

Full backtrace:

[  541.030090] RTNL: assertion failed at net/core/dev.c (4496)
[  541.031143] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G           O 3.10.33-1-amd64-vyatta #1
[  541.031145] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2007
[  541.031146]  ffffffff8148a9f0 000000000000002f ffffffff813c98c1 ffff88007c4451f8
[  541.031148]  0000000000000000 0000000000000000 ffffffff813d3540 ffff88007fc03d18
[  541.031150]  0000880000000006 ffff88007c445000 ffffffffa0194160 0000000000000000
[  541.031152] Call Trace:
[  541.031153]  <IRQ>  [<ffffffff8148a9f0>] ? dump_stack+0xd/0x17
[  541.031180]  [<ffffffff813c98c1>] ? __dev_set_promiscuity+0x101/0x180
[  541.031183]  [<ffffffff813d3540>] ? __hw_addr_create_ex+0x60/0xc0
[  541.031185]  [<ffffffff813cfe1a>] ? __dev_set_rx_mode+0xaa/0xc0
[  541.031189]  [<ffffffff813d3a81>] ? __dev_mc_add+0x61/0x90
[  541.031198]  [<ffffffffa01dcf9c>] ? igmp6_group_added+0xfc/0x1a0 [ipv6]
[  541.031208]  [<ffffffff8111237b>] ? kmem_cache_alloc+0xcb/0xd0
[  541.031212]  [<ffffffffa01ddcd7>] ? ipv6_dev_mc_inc+0x267/0x300 [ipv6]
[  541.031216]  [<ffffffffa01c2fae>] ? addrconf_join_solict+0x2e/0x40 [ipv6]
[  541.031219]  [<ffffffffa01ba2e9>] ? ipv6_dev_ac_inc+0x159/0x1f0 [ipv6]
[  541.031223]  [<ffffffffa01c0772>] ? addrconf_join_anycast+0x92/0xa0 [ipv6]
[  541.031226]  [<ffffffffa01c311e>] ? __ipv6_ifa_notify+0x11e/0x1e0 [ipv6]
[  541.031229]  [<ffffffffa01c3213>] ? ipv6_ifa_notify+0x33/0x50 [ipv6]
[  541.031233]  [<ffffffffa01c36c8>] ? addrconf_dad_completed+0x28/0x100 [ipv6]
[  541.031241]  [<ffffffff81075c1d>] ? task_cputime+0x2d/0x50
[  541.031244]  [<ffffffffa01c38d6>] ? addrconf_dad_timer+0x136/0x150 [ipv6]
[  541.031247]  [<ffffffffa01c37a0>] ? addrconf_dad_completed+0x100/0x100 [ipv6]
[  541.031255]  [<ffffffff8105313a>] ? call_timer_fn.isra.22+0x2a/0x90
[  541.031258]  [<ffffffffa01c37a0>] ? addrconf_dad_completed+0x100/0x100 [ipv6]
[  541.031261]  [<ffffffff81053531>] ? run_timer_softirq+0x1a1/0x260
[  541.031267]  [<ffffffff810350cf>] ? kvm_clock_read+0x1f/0x30
[  541.031272]  [<ffffffff810132a5>] ? sched_clock+0x5/0x10
[  541.031274]  [<ffffffff81074bd5>] ? sched_clock_local+0x15/0x80
[  541.031276]  [<ffffffff8104d586>] ? __do_softirq+0xd6/0x1b0
[  541.031282]  [<ffffffff8149109c>] ? call_softirq+0x1c/0x30
[  541.031284]  [<ffffffff8100d835>] ? do_softirq+0x75/0xb0
[  541.031286]  [<ffffffff8104d7ed>] ? irq_exit+0xbd/0xc0
[  541.031290]  [<ffffffff8102e718>] ? smp_apic_timer_interrupt+0x68/0xa0
[  541.031292]  [<ffffffff814908dd>] ? apic_timer_interrupt+0x6d/0x80
[  541.031293]  <EOI>  [<ffffffff81013ee0>] ? hard_enable_TSC+0x20/0x20
[  541.031296]  [<ffffffff81035412>] ? native_safe_halt+0x2/0x10
[  541.031298]  [<ffffffff81014619>] ? arch_cpu_idle+0x9/0x30
[  541.031299]  [<ffffffff81013ee5>] ? default_idle+0x5/0x10
[  541.031302]  [<ffffffff8108263a>] ? cpu_startup_entry+0x8a/0x180
[  541.031306]  [<ffffffff818d3e3e>] ? start_kernel+0x3ac/0x3b7
[  541.031309]  [<ffffffff818d38a9>] ? repair_env_string+0x5b/0x5b
[  541.031311]  [<ffffffff818d36bc>] ? x86_64_start_kernel+0xf6/0x105

Signed-off-by Stephen Hemminger <[email protected]>

---
Patch is against -net (not net-next) because this is a bug fix. Should be applied to -stable as well.

include/net/if_inet6.h +1 -0

@@ -59,6 +59,7 @@ struct inet6_ifaddr {
 	unsigned long		tstamp; /* updated timestamp */
 
 	struct timer_list	dad_timer;
+	struct work_struct	dad_work;
 
 	struct inet6_dev	*idev;
 	struct rt6_info		*rt;

net/ipv6/addrconf.c +26 -3

@@ -150,8 +150,10 @@ static struct rt6_info *addrconf_get_pre
 						  const struct net_device *dev,
 						  u32 flags, u32 noflags);
 
+static struct workqueue_struct *addrconf_wq;
 static void addrconf_dad_start(struct inet6_ifaddr *ifp);
 static void addrconf_dad_timer(unsigned long data);
+static void addrconf_dad_work(struct work_struct *work);
 static void addrconf_dad_completed(struct inet6_ifaddr *ifp);
 static void addrconf_dad_run(struct inet6_dev *idev);
 static void addrconf_rs_timer(unsigned long data);

@@ -851,6 +853,7 @@ ipv6_add_addr(struct inet6_dev *idev, co
 	spin_lock_init(&ifa->state_lock);
 	setup_timer(&ifa->dad_timer, addrconf_dad_timer,
 		    (unsigned long)ifa);
+	INIT_WORK(&ifa->dad_work, addrconf_dad_work);
 	INIT_HLIST_NODE(&ifa->addr_lst);
 	ifa->scope = scope;
 	ifa->prefix_len = pfxlen;

@@ -3203,6 +3206,18 @@ out:
 	read_unlock_bh(&idev->lock);
 }
 
+/* DAD completion is handled in work queue */
+static void addrconf_dad_work(struct work_struct *work)
+{
+	struct inet6_ifaddr *ifp
+		= container_of(work, struct inet6_ifaddr, dad_work);
+
+	rtnl_lock();
+	addrconf_dad_completed(ifp);
+	rtnl_unlock();
+	in6_ifa_put(ifp);
+}
+
 static void addrconf_dad_timer(unsigned long data)
 {
 	struct inet6_ifaddr *ifp = (struct inet6_ifaddr *) data;

@@ -3234,9 +3249,8 @@ static void addrconf_dad_timer(unsigned
 		spin_unlock(&ifp->lock);
 		write_unlock(&idev->lock);
 
-		addrconf_dad_completed(ifp);
-
-		goto out;
+		queue_work(addrconf_wq, &ifp->dad_work);
+		return;
 	}
 
 	ifp->dad_probes--;

@@ -5244,6 +5258,12 @@ int __init addrconf_init(void)
 	if (err < 0)
 		goto out_addrlabel;
 
+	addrconf_wq = create_workqueue("addrconf");
+	if (!addrconf_wq) {
+		err = -ENOMEM;
+		goto out_nowq;
+	}
+
 	/* The addrconf netdev notifier requires that loopback_dev
 	 * has it's ipv6 private information allocated and setup
 	 * before it can bring up and give link-local addresses

@@ -5302,6 +5322,8 @@ errout:
 	rtnl_af_unregister(&inet6_ops);
 	unregister_netdevice_notifier(&ipv6_dev_notf);
 errlo:
+	destroy_workqueue(addrconf_wq);
+out_nowq:
 	unregister_pernet_subsys(&addrconf_ops);
 out_addrlabel:
 	ipv6_addr_label_cleanup();

@@ -5340,4 +5362,5 @@ void addrconf_cleanup(void)
 
 	del_timer(&addr_chk_timer);
 	rtnl_unlock();
+	destroy_workqueue(addrconf_wq);
 }

Hannes Frederic Sowa

2014-03-18 00:29 UTC #1 lore raw

Hi!

Quoted 21 lines · Stephen Hemminger

IPv6 duplicate address detection is triggering the following assertion failure when using macvlan + vif + multicast.
RTNL: assertion failed at net/core/dev.c (4496)
This happens because the DAD timer is adding a multicast address without acquiring the RTNL mutex. In order to acquire the RTNL mutex, it must be done in process context; therefore it must be in a workqueue.
Full backtrace:
[  541.030090] RTNL: assertion failed at net/core/dev.c (4496)
[  541.031143] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G           O 3.10.33-1-amd64-vyatta #1
[  541.031145] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2007
[  541.031146]  ffffffff8148a9f0 000000000000002f ffffffff813c98c1 ffff88007c4451f8
[  541.031148]  0000000000000000 0000000000000000 ffffffff813d3540 ffff88007fc03d18
[  541.031150]  0000880000000006 ffff88007c445000 ffffffffa0194160 0000000000000000
[  541.031152] Call Trace:
[  541.031153]  <IRQ>  [<ffffffff8148a9f0>] ? dump_stack+0xd/0x17
[  541.031180]  [<ffffffff813c98c1>] ? __dev_set_promiscuity+0x101/0x180
[  541.031183]  [<ffffffff813d3540>] ? __hw_addr_create_ex+0x60/0xc0
[  541.031185]  [<ffffffff813cfe1a>] ? __dev_set_rx_mode+0xaa/0xc0
[  541.031189]  [<ffffffff813d3a81>] ? __dev_mc_add+0x61/0x90

Quoted 8 lines

[  541.031198]  [<ffffffffa01dcf9c>] ? igmp6_group_added+0xfc/0x1a0 [ipv6]
[  541.031208]  [<ffffffff8111237b>] ? kmem_cache_alloc+0xcb/0xd0
[  541.031212]  [<ffffffffa01ddcd7>] ? ipv6_dev_mc_inc+0x267/0x300 [ipv6]
[  541.031216]  [<ffffffffa01c2fae>] ? addrconf_join_solict+0x2e/0x40 [ipv6]
[  541.031219]  [<ffffffffa01ba2e9>] ? ipv6_dev_ac_inc+0x159/0x1f0 [ipv6]
[  541.031223]  [<ffffffffa01c0772>] ? addrconf_join_anycast+0x92/0xa0 [ipv6]
[  541.031226]  [<ffffffffa01c311e>] ? __ipv6_ifa_notify+0x11e/0x1e0 [ipv6]
[  541.031229]  [<ffffffffa01c3213>] ? ipv6_ifa_notify+0x33/0x50 [ipv6]

This is the most often case but I fear there are more of them.

addrconf_verify seems unsafe, too, when removing the last ipv6 address. So does addrconf_prefix_rcv if adding first address.

I wonder if we should put the whole ipv6_ifa_notify infrastructure in a workqueue? I don't like that either and it could add subtile races.

Those races also seem possible if we only defer addrconf_join_solict, addrconf_leave_solict, addrconf_join_anycast and addrconf_leave_anycast to workqueues.

This change is certainly going into the right direction but I am not sure if we could generalize it.

Greetings,

Hannes

Stephen Hemminger

author

2014-03-19 00:54 UTC #2 lore raw

Quoted 33 lines · On Tue, 18 Mar 2014 01:29:08 +0100 Hannes Frederic Sowa…

Hi!

Quoted 21 lines · Stephen Hemminger

IPv6 duplicate address detection is triggering the following assertion failure when using macvlan + vif + multicast.
RTNL: assertion failed at net/core/dev.c (4496)
This happens because the DAD timer is adding a multicast address without acquiring the RTNL mutex. In order to acquire the RTNL mutex, it must be done in process context; therefore it must be in a workqueue.
Full backtrace:
[  541.030090] RTNL: assertion failed at net/core/dev.c (4496)
[  541.031143] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G           O 3.10.33-1-amd64-vyatta #1
[  541.031145] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2007
[  541.031146]  ffffffff8148a9f0 000000000000002f ffffffff813c98c1 ffff88007c4451f8
[  541.031148]  0000000000000000 0000000000000000 ffffffff813d3540 ffff88007fc03d18
[  541.031150]  0000880000000006 ffff88007c445000 ffffffffa0194160 0000000000000000
[  541.031152] Call Trace:
[  541.031153]  <IRQ>  [<ffffffff8148a9f0>] ? dump_stack+0xd/0x17
[  541.031180]  [<ffffffff813c98c1>] ? __dev_set_promiscuity+0x101/0x180
[  541.031183]  [<ffffffff813d3540>] ? __hw_addr_create_ex+0x60/0xc0
[  541.031185]  [<ffffffff813cfe1a>] ? __dev_set_rx_mode+0xaa/0xc0
[  541.031189]  [<ffffffff813d3a81>] ? __dev_mc_add+0x61/0x90

Quoted 8 lines

[  541.031198]  [<ffffffffa01dcf9c>] ? igmp6_group_added+0xfc/0x1a0 [ipv6]
[  541.031208]  [<ffffffff8111237b>] ? kmem_cache_alloc+0xcb/0xd0
[  541.031212]  [<ffffffffa01ddcd7>] ? ipv6_dev_mc_inc+0x267/0x300 [ipv6]
[  541.031216]  [<ffffffffa01c2fae>] ? addrconf_join_solict+0x2e/0x40 [ipv6]
[  541.031219]  [<ffffffffa01ba2e9>] ? ipv6_dev_ac_inc+0x159/0x1f0 [ipv6]
[  541.031223]  [<ffffffffa01c0772>] ? addrconf_join_anycast+0x92/0xa0 [ipv6]
[  541.031226]  [<ffffffffa01c311e>] ? __ipv6_ifa_notify+0x11e/0x1e0 [ipv6]
[  541.031229]  [<ffffffffa01c3213>] ? ipv6_ifa_notify+0x33/0x50 [ipv6]

Quoted 7 lines

This is the most often case but I fear there are more of them.
addrconf_verify seems unsafe, too, when removing the last ipv6 address. So does addrconf_prefix_rcv if adding first address.
I wonder if we should put the whole ipv6_ifa_notify infrastructure in a workqueue? I don't like that either and it could add subtile races.

That is option, might be some call chains that already have rtnl_lock held.

Quoted 7 lines

Those races also seem possible if we only defer addrconf_join_solict, addrconf_leave_solict, addrconf_join_anycast and addrconf_leave_anycast to workqueues.
This change is certainly going into the right direction but I am not sure if we could generalize it.

There is a lot of bookkeeping that happens for cases where nothing changes at the device layer. Want to avoid rtnl_lock unless there is something that is going to happen.

David Miller

maintainer

2014-03-19 04:17 UTC #3 lore raw

From: Stephen Hemminger <[email protected]>
Date: Tue, 18 Mar 2014 17:54:06 -0700

Quoted 7 lines

Quoted 2 lines · On Tue, 18 Mar 2014 01:29:08 +0100 Hannes Frederic Sowa…
I wonder if we should put the whole ipv6_ifa_notify infrastructure in a workqueue? I don't like that either and it could add subtile races.
That is option, might be some call chains that already have rtnl_lock held.

There are TAHI ipv6 conformance tests that expect state changes to be precisely synchronous.

And frankly it's pretty reasonable to send two packets back to back, one which causes the state change and one which tests if the state change happened, and expect that to work.

Stephen Hemminger

author

2014-03-19 06:58 UTC #4 lore raw

Quoted 9 lines · On Wed, 19 Mar 2014 00:17:36 -0400 (EDT) David Miller <[email protected]>…

From: Stephen Hemminger <[email protected]>
Date: Tue, 18 Mar 2014 17:54:06 -0700
Quoted 6 lines
Quoted 2 lines · On Tue, 18 Mar 2014 01:29:08 +0100 Hannes Frederic Sowa…
I wonder if we should put the whole ipv6_ifa_notify infrastructure in a workqueue? I don't like that either and it could add subtile races.

Quoted 8 lines

Quoted 1 line
That is option, might be some call chains that already have rtnl_lock held.
There are TAHI ipv6 conformance tests that expect state changes to be precisely synchronous.
And frankly it's pretty reasonable to send two packets back to back, one which causes the state change and one which tests if the state change happened, and expect that to work.

It is more the timer based state changes that are problematic because they aren't acquire RTNL.

David Miller

maintainer

2014-03-19 17:53 UTC #5 lore raw

From: Stephen Hemminger <[email protected]>
Date: Tue, 18 Mar 2014 23:58:11 -0700

Quoted 17 lines

Quoted 14 lines · On Wed, 19 Mar 2014 00:17:36 -0400 (EDT) David Miller <[email protected]>…
From: Stephen Hemminger <[email protected]>
Date: Tue, 18 Mar 2014 17:54:06 -0700
Quoted 7 lines
Quoted 2 lines · On Tue, 18 Mar 2014 01:29:08 +0100 Hannes Frederic Sowa…
I wonder if we should put the whole ipv6_ifa_notify infrastructure in a workqueue? I don't like that either and it could add subtile races.
That is option, might be some call chains that already have rtnl_lock held.
There are TAHI ipv6 conformance tests that expect state changes to be precisely synchronous.

Quoted 6 lines

Quoted 3 lines
And frankly it's pretty reasonable to send two packets back to back, one which causes the state change and one which tests if the state change happened, and expect that to work.
It is more the timer based state changes that are problematic because they aren't acquire RTNL.

Ok, the timer stuff could run from a workqueue just fine.

Hannes Frederic Sowa

2014-03-19 22:44 UTC #6 lore raw

Quoted 19 lines · David Miller

From: Stephen Hemminger <[email protected]>
Date: Tue, 18 Mar 2014 23:58:11 -0700
Quoted 16 lines
Quoted 13 lines · On Wed, 19 Mar 2014 00:17:36 -0400 (EDT) David Miller <[email protected]>…
From: Stephen Hemminger <[email protected]>
Date: Tue, 18 Mar 2014 17:54:06 -0700
Quoted 7 lines
Quoted 2 lines · On Tue, 18 Mar 2014 01:29:08 +0100 Hannes Frederic Sowa…
I wonder if we should put the whole ipv6_ifa_notify infrastructure in a workqueue? I don't like that either and it could add subtile races.
That is option, might be some call chains that already have rtnl_lock held.
There are TAHI ipv6 conformance tests that expect state changes to be precisely synchronous.

Exactly, I was afraid of those things (not exactly of TAHI breakage but of real packet loss in case of late workqueue scheduling).

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
And frankly it's pretty reasonable to send two packets back to back, one which causes the state change and one which tests if the state change happened, and expect that to work.
It is more the timer based state changes that are problematic because they aren't acquire RTNL.
Ok, the timer stuff could run from a workqueue just fine.

We have no-timer invocations, too, like addrconf_prefix_rcv. In that case the whole handling of the router advertisment should get deferred into the workqueue.

Bye,

Hannes

David Miller

maintainer

2014-03-20 03:52 UTC #7 lore raw

From: Hannes Frederic Sowa <[email protected]>
Date: Wed, 19 Mar 2014 23:44:42 +0100

Quoted 6 lines · David Miller

Quoted 1 line
Ok, the timer stuff could run from a workqueue just fine.
We have no-timer invocations, too, like addrconf_prefix_rcv. In that case the whole handling of the router advertisment should get deferred into the workqueue.

Just to be clear, you are saying that this doesn't need to be synchronous? Handling a prefix event seems like something that would in fact need to be.

Hannes Frederic Sowa

2014-03-20 06:38 UTC #8 lore raw

Quoted 6 lines · David Miller

From: Hannes Frederic Sowa <[email protected]>
Date: Wed, 19 Mar 2014 23:44:42 +0100
Quoted 3 lines · David Miller
Quoted 1 line
Ok, the timer stuff could run from a workqueue just fine.

Quoted 7 lines

Quoted 3 lines
We have no-timer invocations, too, like addrconf_prefix_rcv. In that case the whole handling of the router advertisment should get deferred into the workqueue.
Just to be clear, you are saying that this doesn't need to be synchronous? Handling a prefix event seems like something that would in fact need to be.

Here is my current analysis and proposals:

Actually, I would say that a safe entry point for starting to push further prefix event handling into a workqueue would be addrconf_dad_start.

Quoted 1 line

From there on, we need to make sure that addrconf_join_solict (which

is the first point we actually need RTNL locked) is called before we do optimistic duplicate address detection processing (this seems to be the only happens-before invariant we need to preserve here). Stephen already allocated the work_struct in inet6_ifaddr, so my suggestion would be to change Stephen's patch to use a delayed workqueue and just replace the other timer operations to use the new work_struct in inet6_ifaddr with delayed operations. Entry-point would be addrconf_dad_start which simply adds the delayed operation with 0 delay and maybe a new flag so that addrconf_dad_timer (which should be called addrconf_dad_work by then) does the work which was prior in addrconf_dad_start.

The addrconf_dad_completed handling could be under RTNL, too, so the original problem would be gone.

addrconf_verify would also need a delayed workqueue (split to addrconf_verify_rtnl and addrconf_verify is just a invocation to mod_delay_work(wq, addrconf_verify_work, 0) which calls addrconf_verify_rtnl with rtnl locked, would be my approach by only looking at the code).

That leaves us with one unsafe invocation of an rtnl-locked needed invocation in pndisc_constructor for proxy_ndp handling. Don't know what to do about that currently but didn't look to closely.

Also, to find problems like this sooner, should we propagate ASSERT_RTNL() tests up from conditional callees to their callers (e.g. __dev_set_promiscuity -> __dev_set_rx_mode -> maybe even further up the stack?).

Greetings,

Hannes

Hannes Frederic Sowa

2014-03-23 02:03 UTC #9 lore raw

In reply to Stephen Hemminger

Hi Stephen!

Quoted 7 lines · Stephen Hemminger

IPv6 duplicate address detection is triggering the following assertion failure when using macvlan + vif + multicast.
RTNL: assertion failed at net/core/dev.c (4496)
This happens because the DAD timer is adding a multicast address without acquiring the RTNL mutex. In order to acquire the RTNL mutex, it must be done in process context; therefore it must be in a workqueue.

I finally had a bit free time and cooked up this patch. I stole some snippets from yours. Of course, I'll attribute it correctly.

Just wanted to post early, so we don't waste time to work on that concurrently:

include/net/if_inet6.h +2 -1

index 9650a3f..1cdcf9c 100644
@@ -31,6 +31,7 @@
 #define IF_PREFIX_AUTOCONF	0x02
 
 enum {
+	INET6_IFADDR_STATE_PREDAD,
 	INET6_IFADDR_STATE_DAD,
 	INET6_IFADDR_STATE_POSTDAD,
 	INET6_IFADDR_STATE_UP,

@@ -58,7 +59,7 @@ struct inet6_ifaddr {
 	unsigned long		cstamp;	/* created timestamp */
 	unsigned long		tstamp; /* updated timestamp */
 
-	struct timer_list	dad_timer;
+	struct delayed_work	dad_work;
 
 	struct inet6_dev	*idev;
 	struct rt6_info		*rt;

net/ipv6/addrconf.c +111 ~~-34~~

381 lines not on this page · raw message

Hannes Frederic Sowa

2014-03-25 08:03 UTC #10 lore raw

PATCH 1/1 ipv6: move DAD and addrconf_verify processing to workqueue

addrconf_join_solict and addrconf_join_anycast may cause actions which need rtnl locked, especially on first address creation.

To get rtnl lock we need to push the code paths which depend on those calls up to workqueues, specifically addrconf_verify and the DAD processing.

Relevant backtrace:

[  541.030090] RTNL: assertion failed at net/core/dev.c (4496)
[  541.031143] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G           O 3.10.33-1-amd64-vyatta #1
[  541.031145] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2007
[  541.031146]  ffffffff8148a9f0 000000000000002f ffffffff813c98c1 ffff88007c4451f8
[  541.031148]  0000000000000000 0000000000000000 ffffffff813d3540 ffff88007fc03d18
[  541.031150]  0000880000000006 ffff88007c445000 ffffffffa0194160 0000000000000000
[  541.031152] Call Trace:
[  541.031153]  <IRQ>  [<ffffffff8148a9f0>] ? dump_stack+0xd/0x17
[  541.031180]  [<ffffffff813c98c1>] ? __dev_set_promiscuity+0x101/0x180
[  541.031183]  [<ffffffff813d3540>] ? __hw_addr_create_ex+0x60/0xc0
[  541.031185]  [<ffffffff813cfe1a>] ? __dev_set_rx_mode+0xaa/0xc0
[  541.031189]  [<ffffffff813d3a81>] ? __dev_mc_add+0x61/0x90
[  541.031198]  [<ffffffffa01dcf9c>] ? igmp6_group_added+0xfc/0x1a0 [ipv6]
[  541.031208]  [<ffffffff8111237b>] ? kmem_cache_alloc+0xcb/0xd0
[  541.031212]  [<ffffffffa01ddcd7>] ? ipv6_dev_mc_inc+0x267/0x300 [ipv6]
[  541.031216]  [<ffffffffa01c2fae>] ? addrconf_join_solict+0x2e/0x40 [ipv6]
[  541.031219]  [<ffffffffa01ba2e9>] ? ipv6_dev_ac_inc+0x159/0x1f0 [ipv6]
[  541.031223]  [<ffffffffa01c0772>] ? addrconf_join_anycast+0x92/0xa0 [ipv6]
[  541.031226]  [<ffffffffa01c311e>] ? __ipv6_ifa_notify+0x11e/0x1e0 [ipv6]
[  541.031229]  [<ffffffffa01c3213>] ? ipv6_ifa_notify+0x33/0x50 [ipv6]
[  541.031233]  [<ffffffffa01c36c8>] ? addrconf_dad_completed+0x28/0x100 [ipv6]
[  541.031241]  [<ffffffff81075c1d>] ? task_cputime+0x2d/0x50
[  541.031244]  [<ffffffffa01c38d6>] ? addrconf_dad_timer+0x136/0x150 [ipv6]
[  541.031247]  [<ffffffffa01c37a0>] ? addrconf_dad_completed+0x100/0x100 [ipv6]
[  541.031255]  [<ffffffff8105313a>] ? call_timer_fn.isra.22+0x2a/0x90
[  541.031258]  [<ffffffffa01c37a0>] ? addrconf_dad_completed+0x100/0x100 [ipv6]

Hunks and backtrace stolen from a patch by Stephen Hemminger.

Reported-by Stephen Hemminger <[email protected]>

Signed-off-by Stephen Hemminger <[email protected]>

Signed-off-by Hannes Frederic Sowa <[email protected]>

---
 include/net/if_inet6.h |   3 +-
 net/ipv6/addrconf.c    | 146 +++++++++++++++++++++++++++++++++++++------------
 2 files changed, 114 insertions(+), 35 deletions(-)

include/net/if_inet6.h +2 -1

index 9650a3f..1cdcf9c 100644
@@ -31,6 +31,7 @@
 #define IF_PREFIX_AUTOCONF	0x02
 
 enum {
+	INET6_IFADDR_STATE_PREDAD,
 	INET6_IFADDR_STATE_DAD,
 	INET6_IFADDR_STATE_POSTDAD,
 	INET6_IFADDR_STATE_UP,

@@ -58,7 +59,7 @@ struct inet6_ifaddr {
 	unsigned long		cstamp;	/* created timestamp */
 	unsigned long		tstamp; /* updated timestamp */
 
-	struct timer_list	dad_timer;
+	struct delayed_work	dad_work;
 
 	struct inet6_dev	*idev;
 	struct rt6_info		*rt;

net/ipv6/addrconf.c +112 ~~-34~~

376 lines not on this page · raw message

Signature · 1 line

1.8.5.3

Stephen Hemminger

author

2014-03-25 21:20 UTC #11 lore raw

Re: [PATCH net] ipv6: move DAD and addrconf_verify processing to workqueue

net/ipv6/addrconf.c int ipv6_chk_home_addr(struct net *net, const struct in6_addr *addr) · line 3578

Quoted 12 lines · On Tue, 25 Mar 2014 09:03:01 +0100 Hannes Frederic Sowa…

net/ipv6/addrconf.c +3 -1

-static void addrconf_verify(unsigned long foo)
+static void addrconf_verify_rtnl(void)
 {
 	unsigned long now, next, next_sec, next_sched;
 	struct inet6_ifaddr *ifp;
 	int i;
 
+	ASSERT_RTNL();
+
 	rcu_read_lock_bh();
 	spin_lock(&addrconf_verify_lock);

Since RTNL is now held in addrconf_verify(), is addrconf_verify_lock is not needed.

Since RTNL is held, rcu is not needed either.

Also, I would skip the wrapper (addrconf_verify_rtnl) and just put it all in addrconf_verify_work

Hannes Frederic Sowa

2014-03-26 00:00 UTC #12 lore raw

Re: [PATCH net] ipv6: move DAD and addrconf_verify processing to workqueue

Quoted 11 lines · Stephen Hemminger

Quoted 8 lines · On Tue, 25 Mar 2014 09:03:01 +0100 Hannes Frederic Sowa…
net/ipv6/addrconf.c +1 -1
-static void addrconf_verify(unsigned long foo)
+static void addrconf_verify_rtnl(void)
 {
 	unsigned long now, next, next_sec, next_sched;
 	struct inet6_ifaddr *ifp;
 	int i;

Quoted 6 lines

net/ipv6/addrconf.c int ipv6_chk_home_addr(struct net *net, const struct in6_addr *addr) · line 3578
Quoted 4 lines
net/ipv6/addrconf.c +2 -0
+	ASSERT_RTNL();
+
 	rcu_read_lock_bh();
 	spin_lock(&addrconf_verify_lock);
Since RTNL is now held in addrconf_verify(), is addrconf_verify_lock is not needed.

Yes, I overlooked that, thanks!

Quoted 1 line

Since RTNL is held, rcu is not needed either.

It might be possible if we add other rtnl lock invocations to the dad process, but because rcu is much more lightweight, I would leave it as is:

ipv6_del_addr removes elements from inet6_addr_lst without holding RTNL but only synchronizes on addrconf_hash_lock. In this constellation RCU is still needed.

Quoted 2 lines

Also, I would skip the wrapper (addrconf_verify_rtnl) and just put it all in addrconf_verify_work

I preferred to have the addrconf_verify invocations inline and not deferred if we e.g. do cleanups from user space to not have any races because of not yet deleted addresses. I would also prefer to leave this as is, ok?

Will send v2, thanks!

Hannes Frederic Sowa

2014-03-26 06:17 UTC #13 lore raw

PATCH v2 1/1 ipv6: move DAD and addrconf_verify processing to workqueue

addrconf_join_solict and addrconf_join_anycast may cause actions which need rtnl locked, especially on first address creation.

A new DAD state is introduced which deferres processing of the initial DAD processing into a workqueue.

To get rtnl lock we need to push the code paths which depend on those calls up to workqueues, specifically addrconf_verify and the DAD processing.

(v2)
addrconf_dad_failure needs to be queued up to the workqueue, too. This patch introduces a new DAD state and stop the DAD processing in the workqueue (this is because of the possible ipv6_del_addr processing which removes the solicited multicast address from the device).

addrconf_verify_lock is removed, too. After the transition it is not needed any more.

As we are not processing in bottom half anymore we need to be a bit more careful about disabling bottom half out when we lock spin_locks which are also used in bh.

Relevant backtrace:

[  541.030090] RTNL: assertion failed at net/core/dev.c (4496)
[  541.031143] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G           O 3.10.33-1-amd64-vyatta #1
[  541.031145] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2007
[  541.031146]  ffffffff8148a9f0 000000000000002f ffffffff813c98c1 ffff88007c4451f8
[  541.031148]  0000000000000000 0000000000000000 ffffffff813d3540 ffff88007fc03d18
[  541.031150]  0000880000000006 ffff88007c445000 ffffffffa0194160 0000000000000000
[  541.031152] Call Trace:
[  541.031153]  <IRQ>  [<ffffffff8148a9f0>] ? dump_stack+0xd/0x17
[  541.031180]  [<ffffffff813c98c1>] ? __dev_set_promiscuity+0x101/0x180
[  541.031183]  [<ffffffff813d3540>] ? __hw_addr_create_ex+0x60/0xc0
[  541.031185]  [<ffffffff813cfe1a>] ? __dev_set_rx_mode+0xaa/0xc0
[  541.031189]  [<ffffffff813d3a81>] ? __dev_mc_add+0x61/0x90
[  541.031198]  [<ffffffffa01dcf9c>] ? igmp6_group_added+0xfc/0x1a0 [ipv6]
[  541.031208]  [<ffffffff8111237b>] ? kmem_cache_alloc+0xcb/0xd0
[  541.031212]  [<ffffffffa01ddcd7>] ? ipv6_dev_mc_inc+0x267/0x300 [ipv6]
[  541.031216]  [<ffffffffa01c2fae>] ? addrconf_join_solict+0x2e/0x40 [ipv6]
[  541.031219]  [<ffffffffa01ba2e9>] ? ipv6_dev_ac_inc+0x159/0x1f0 [ipv6]
[  541.031223]  [<ffffffffa01c0772>] ? addrconf_join_anycast+0x92/0xa0 [ipv6]
[  541.031226]  [<ffffffffa01c311e>] ? __ipv6_ifa_notify+0x11e/0x1e0 [ipv6]
[  541.031229]  [<ffffffffa01c3213>] ? ipv6_ifa_notify+0x33/0x50 [ipv6]
[  541.031233]  [<ffffffffa01c36c8>] ? addrconf_dad_completed+0x28/0x100 [ipv6]
[  541.031241]  [<ffffffff81075c1d>] ? task_cputime+0x2d/0x50
[  541.031244]  [<ffffffffa01c38d6>] ? addrconf_dad_timer+0x136/0x150 [ipv6]
[  541.031247]  [<ffffffffa01c37a0>] ? addrconf_dad_completed+0x100/0x100 [ipv6]
[  541.031255]  [<ffffffff8105313a>] ? call_timer_fn.isra.22+0x2a/0x90
[  541.031258]  [<ffffffffa01c37a0>] ? addrconf_dad_completed+0x100/0x100 [ipv6]

Hunks and backtrace stolen from a patch by Stephen Hemminger.

Reported-by Stephen Hemminger <[email protected]>

Signed-off-by Stephen Hemminger <[email protected]>

Signed-off-by Hannes Frederic Sowa <[email protected]>

---
I tried to address the changelog in the commit description.

Thanks to Stephen for the review, hope that happens again, because the changes are quite a lot. ;)

I didn't remove the rcu_read_lock_bh from addrconf_verify because the address list ist synchronized via addrconf_hash_lock and not RTNL. ipv6_add_addr adds addresses there without taking RTNL and RCU lock is not that expensive and code is not that very sensible for speed.

Thanks!

 include/net/if_inet6.h |   4 +-
 net/ipv6/addrconf.c    | 190 ++++++++++++++++++++++++++++++++++++-------------
 2 files changed, 143 insertions(+), 51 deletions(-)

include/net/if_inet6.h +3 -1

index 9650a3f..b4956a5 100644
@@ -31,8 +31,10 @@
 #define IF_PREFIX_AUTOCONF	0x02
 
 enum {
+	INET6_IFADDR_STATE_PREDAD,
 	INET6_IFADDR_STATE_DAD,
 	INET6_IFADDR_STATE_POSTDAD,
+	INET6_IFADDR_STATE_ERRDAD,
 	INET6_IFADDR_STATE_UP,
 	INET6_IFADDR_STATE_DEAD,
 };

@@ -58,7 +60,7 @@ struct inet6_ifaddr {
 	unsigned long		cstamp;	/* created timestamp */
 	unsigned long		tstamp; /* updated timestamp */
 
-	struct timer_list	dad_timer;
+	struct delayed_work	dad_work;
 
 	struct inet6_dev	*idev;
 	struct rt6_info		*rt;

net/ipv6/addrconf.c +140 ~~-50~~

462 lines not on this page · raw message

Signature · 1 line

1.8.5.3

David Miller

maintainer

2014-03-27 17:15 UTC #14 lore raw

Re: [PATCH v2 net] ipv6: move DAD and addrconf_verify processing to workqueue

From: Hannes Frederic Sowa <[email protected]>
Date: Wed, 26 Mar 2014 07:17:45 +0100

net/ipv6/addrconf.c void inet6_ifa_finish_destroy(struct inet6_ifaddr *ifp) · line 759

Quoted 9 lines

net/ipv6/addrconf.c +1 -1

@@ -751,7 +753,7 @@ void inet6_ifa_finish_destroy(struct inet6_ifaddr *ifp)
 
 	in6_dev_put(ifp->idev);
 
-	if (del_timer(&ifp->dad_timer))
+	if (cancel_delayed_work(&ifp->dad_work))
 		pr_notice("Timer is still running, when freeing ifa=%p\n", ifp);
 
 	if (ifp->state != INET6_IFADDR_STATE_DEAD) {

This pr_notice() needs to be adjusted, we are no longer using a timer.

Hannes Frederic Sowa

2014-03-27 17:28 UTC #15 lore raw

PATCH v3 1/1 ipv6: move DAD and addrconf_verify processing to workqueue

addrconf_join_solict and addrconf_join_anycast may cause actions which need rtnl locked, especially on first address creation.

A new DAD state is introduced which deferres processing of the initial DAD processing into a workqueue.

To get rtnl lock we need to push the code paths which depend on those calls up to workqueues, specifically addrconf_verify and the DAD processing.

addrconf_verify_lock is removed, too. After the transition it is not needed any more.

As we are not processing in bottom half anymore we need to be a bit more careful about disabling bottom half out when we lock spin_locks which are also used in bh.

Relevant backtrace:

[  541.030090] RTNL: assertion failed at net/core/dev.c (4496)
[  541.031143] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G           O 3.10.33-1-amd64-vyatta #1
[  541.031145] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2007
[  541.031146]  ffffffff8148a9f0 000000000000002f ffffffff813c98c1 ffff88007c4451f8
[  541.031148]  0000000000000000 0000000000000000 ffffffff813d3540 ffff88007fc03d18
[  541.031150]  0000880000000006 ffff88007c445000 ffffffffa0194160 0000000000000000
[  541.031152] Call Trace:
[  541.031153]  <IRQ>  [<ffffffff8148a9f0>] ? dump_stack+0xd/0x17
[  541.031180]  [<ffffffff813c98c1>] ? __dev_set_promiscuity+0x101/0x180
[  541.031183]  [<ffffffff813d3540>] ? __hw_addr_create_ex+0x60/0xc0
[  541.031185]  [<ffffffff813cfe1a>] ? __dev_set_rx_mode+0xaa/0xc0
[  541.031189]  [<ffffffff813d3a81>] ? __dev_mc_add+0x61/0x90
[  541.031198]  [<ffffffffa01dcf9c>] ? igmp6_group_added+0xfc/0x1a0 [ipv6]
[  541.031208]  [<ffffffff8111237b>] ? kmem_cache_alloc+0xcb/0xd0
[  541.031212]  [<ffffffffa01ddcd7>] ? ipv6_dev_mc_inc+0x267/0x300 [ipv6]
[  541.031216]  [<ffffffffa01c2fae>] ? addrconf_join_solict+0x2e/0x40 [ipv6]
[  541.031219]  [<ffffffffa01ba2e9>] ? ipv6_dev_ac_inc+0x159/0x1f0 [ipv6]
[  541.031223]  [<ffffffffa01c0772>] ? addrconf_join_anycast+0x92/0xa0 [ipv6]
[  541.031226]  [<ffffffffa01c311e>] ? __ipv6_ifa_notify+0x11e/0x1e0 [ipv6]
[  541.031229]  [<ffffffffa01c3213>] ? ipv6_ifa_notify+0x33/0x50 [ipv6]
[  541.031233]  [<ffffffffa01c36c8>] ? addrconf_dad_completed+0x28/0x100 [ipv6]
[  541.031241]  [<ffffffff81075c1d>] ? task_cputime+0x2d/0x50
[  541.031244]  [<ffffffffa01c38d6>] ? addrconf_dad_timer+0x136/0x150 [ipv6]
[  541.031247]  [<ffffffffa01c37a0>] ? addrconf_dad_completed+0x100/0x100 [ipv6]
[  541.031255]  [<ffffffff8105313a>] ? call_timer_fn.isra.22+0x2a/0x90
[  541.031258]  [<ffffffffa01c37a0>] ? addrconf_dad_completed+0x100/0x100 [ipv6]

Hunks and backtrace stolen from a patch by Stephen Hemminger.

Reported-by Stephen Hemminger <[email protected]>

Signed-off-by Stephen Hemminger <[email protected]>

Signed-off-by Hannes Frederic Sowa <[email protected]>

---
v3 (only minor change):
* reword pr_notice in inet6_ifa_finish_destroy (thanks, David!)

 include/net/if_inet6.h |   4 +-
 net/ipv6/addrconf.c    | 193 ++++++++++++++++++++++++++++++++++++-------------
 2 files changed, 145 insertions(+), 52 deletions(-)

include/net/if_inet6.h +3 -1

index 9650a3f..b4956a5 100644
@@ -31,8 +31,10 @@
 #define IF_PREFIX_AUTOCONF	0x02
 
 enum {
+	INET6_IFADDR_STATE_PREDAD,
 	INET6_IFADDR_STATE_DAD,
 	INET6_IFADDR_STATE_POSTDAD,
+	INET6_IFADDR_STATE_ERRDAD,
 	INET6_IFADDR_STATE_UP,
 	INET6_IFADDR_STATE_DEAD,
 };

@@ -58,7 +60,7 @@ struct inet6_ifaddr {
 	unsigned long		cstamp;	/* created timestamp */
 	unsigned long		tstamp; /* updated timestamp */
 
-	struct timer_list	dad_timer;
+	struct delayed_work	dad_work;
 
 	struct inet6_dev	*idev;
 	struct rt6_info		*rt;

net/ipv6/addrconf.c +142 ~~-51~~

465 lines not on this page · raw message

Signature · 1 line

1.8.5.3

David Miller

maintainer Applied

2014-03-28 20:57 UTC #16 lore raw

Re: [PATCH v3 net] ipv6: move DAD and addrconf_verify processing to workqueue

From: Hannes Frederic Sowa <[email protected]>
Date: Thu, 27 Mar 2014 18:28:07 +0100

Quoted 24 lines

addrconf_join_solict and addrconf_join_anycast may cause actions which need rtnl locked, especially on first address creation.
A new DAD state is introduced which deferres processing of the initial DAD processing into a workqueue.
To get rtnl lock we need to push the code paths which depend on those calls up to workqueues, specifically addrconf_verify and the DAD processing.
(v2)
addrconf_dad_failure needs to be queued up to the workqueue, too. This patch introduces a new DAD state and stop the DAD processing in the workqueue (this is because of the possible ipv6_del_addr processing which removes the solicited multicast address from the device).
addrconf_verify_lock is removed, too. After the transition it is not needed any more.
As we are not processing in bottom half anymore we need to be a bit more careful about disabling bottom half out when we lock spin_locks which are also used in bh.
Relevant backtrace:

...

Quoted 8 lines

Hunks and backtrace stolen from a patch by Stephen Hemminger.
Reported-by Stephen Hemminger <[email protected]>
Signed-off-by Stephen Hemminger <[email protected]>
Signed-off-by Hannes Frederic Sowa <[email protected]>
---
v3 (only minor change):
* reword pr_notice in inet6_ifa_finish_destroy (thanks, David!)

Applied, thanks a lot Hannes.