[PATCH] IPv6: Change addrconf code to deal with link changes better

[PATCH] IPv6: Change addrconf code to deal with link changes better

[Brian Haley]

I recently started noticing that sometimes I wasn't seeing all the "normal"
IPv6 packets (DAD, MLD, Router Solicit) when I was bringing-up interfaces.
I found that sometimes the link on some NICs was going up-down-up like
this:

 ADDRCONF(NETDEV_UP): eth3: link is not ready
 e1000e: eth3 NIC Link is Up 1000 Mbps Full Duplex, Flow Control: None
 ADDRCONF(NETDEV_CHANGE): eth3: link becomes ready
 (DAD queued here)
 e1000e: eth3 NIC Link is Down
 e1000e: eth3 NIC Link is Up 1000 Mbps Full Duplex, Flow Control: None

This is all tcpdump showed on a remote system:

 IP6 fe80::21f:29ff:fe59:fac9 > ff02::2: ICMP6, router solicitation, length 16
 IP6 fe80::21f:29ff:fe59:fac9 > ff02::2: ICMP6, router solicitation, length 16

The addrconf code isn't written to handle this case, and won't restart DAD.
With the patch below I get this (DAD_KICK was for debugging):

 ADDRCONF(NETDEV_UP): eth6: link is not ready
 e1000e: eth6 NIC Link is Up 1000 Mbps Full Duplex, Flow Control: None
 ADDRCONF(NETDEV_CHANGE): eth6: link becomes ready
 DAD_KICK(fe80::21f:29ff:fe59:faca): eth6
 e1000e: eth6 NIC Link is Down
 ADDRCONF(NETDEV_CHANGE): eth6: link is not ready
 e1000e: eth6 NIC Link is Up 1000 Mbps Full Duplex, Flow Control: None
 ADDRCONF(NETDEV_CHANGE): eth6: link becomes ready
 DAD_KICK(fe80::21f:29ff:fe59:faca): eth6

And corresponding tcpdump:

 IP6 :: > ff02::1:ff59:faca: ICMP6, neighbor solicitation, who has fe80::21f:29ff:fe59:faca, length 24
 IP6 :: > ff02::16: HBH ICMP6, multicast listener report v2, 1 group record(s), length 28
 IP6 fe80::21f:29ff:fe59:faca > ff02::2: ICMP6, router solicitation, length 16

I also noticed that sometimes during these blips that addrconf_dad_kick()
could be called twice, transmitting two DAD packets and resetting the
timer another second into the future, so I introduced a "Need DAD" state
to signify that DAD has not been started yet.  With both of these applied
the behavior is much better.

--

Change the IPv6 addrconf code to handle the case where the NIC link
state goes UP-DOWN-UP by removing all the autoconfigured addresses
when it goes down, so that when it comes back up they will get added
again and DAD will be triggered.  Also added a "Need DAD" state so that
we don't accidentally start it twice for the same address, sending a
duplicate packet and delaying when the address is available for use.

Signed-off-by: Brian Haley <brian.haley@hp.com>

diff --git a/include/net/if_inet6.h b/include/net/if_inet6.h
index f95ff8d..57432f1 100644
--- a/include/net/if_inet6.h
+++ b/include/net/if_inet6.h
@@ -33,6 +33,7 @@
 #ifdef __KERNEL__
 
 enum {
+	INET6_IFADDR_STATE_NEEDDAD,
 	INET6_IFADDR_STATE_DAD,
 	INET6_IFADDR_STATE_POSTDAD,
 	INET6_IFADDR_STATE_UP,
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 5bc893e..fabafc7 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -2517,13 +2517,27 @@ static int addrconf_notify(struct notifier_block *this, unsigned long event,
 			if (!idev && dev->mtu >= IPV6_MIN_MTU)
 				idev = ipv6_add_dev(dev);
 
-			if (idev) {
-				idev->if_flags |= IF_READY;
+			/*
+			 * Only start DAD if the device is ready, IF_READY
+			 * will have been set by ipv6_add_dev() if so.
+			 */
+			if (idev && idev->if_flags & IF_READY)
 				run_pending = 1;
-			}
 		} else {
 			if (!addrconf_qdisc_ok(dev)) {
-				/* device is still not ready. */
+				/*
+				 * Device is still not ready.  If we've already
+				 * configured addresses, remove them now as
+				 * we'll need to start DAD all over again.
+				 */
+				printk(KERN_INFO
+					"ADDRCONF(NETDEV_CHANGE): %s: "
+					"link is not ready\n",
+					dev->name);
+
+				if (idev && idev->if_flags & IF_READY)
+					addrconf_ifdown(dev, 0);
+
 				break;
 			}
 
@@ -2736,7 +2750,7 @@ static int addrconf_ifdown(struct net_device *dev, int how)
 
 			/* Flag it for later restoration when link comes up */
 			ifa->flags |= IFA_F_TENTATIVE;
-			ifa->state = INET6_IFADDR_STATE_DAD;
+			ifa->state = INET6_IFADDR_STATE_NEEDDAD;
 
 			write_unlock_bh(&idev->lock);
 
@@ -2847,6 +2861,7 @@ static void addrconf_dad_kick(struct inet6_ifaddr *ifp)
 		rand_num = net_random() % (idev->cnf.rtr_solicit_delay ? : 1);
 
 	ifp->probes = idev->cnf.dad_transmits;
+	ifp->state = INET6_IFADDR_STATE_DAD;
 	addrconf_mod_timer(ifp, AC_DAD, rand_num);
 }
 
@@ -2992,7 +3007,7 @@ static void addrconf_dad_run(struct inet6_dev *idev)
 	list_for_each_entry(ifp, &idev->addr_list, if_list) {
 		spin_lock(&ifp->lock);
 		if (ifp->flags & IFA_F_TENTATIVE &&
-		    ifp->state == INET6_IFADDR_STATE_DAD)
+		    ifp->state == INET6_IFADDR_STATE_NEEDDAD)
 			addrconf_dad_kick(ifp);
 		spin_unlock(&ifp->lock);
 	}

Re: [PATCH] IPv6: Change addrconf code to deal with link changes better

[Alexander Clouter]

Brian Haley <brian.haley@hp.com> wrote:
> 
> [snipped]
> 
> Change the IPv6 addrconf code to handle the case where the NIC link
> state goes UP-DOWN-UP by removing all the autoconfigured addresses
> when it goes down, so that when it comes back up they will get added
> again and DAD will be triggered.  
> 
Surely this is going to result in me sending you lots of rude emails 
everytime cursing you when my SSH connections are terminated for no real 
reason on a link flap?  Privacy addresses are handy on servers too such 
as DNS servers...

When on occasion I am forced to use Windows, those darn PuTTY disconnect 
popups I see each time I disconnect/connect from our wireless network 
nearly put axe swinging mood as it is.  Please, spare me experiencing 
this pain under Linux...

Of course I could be talking nonsense, so ignore this email and keep up 
the good work :)

Cheers

-- 
Alexander Clouter
.sigmonster says: Cole's Law:
                  	Thinly sliced cabbage.

Re: [PATCH] IPv6: Change addrconf code to deal with link changes better

[Brian Haley]

On 09/10/2010 06:27 PM, Alexander Clouter wrote:
> Brian Haley <brian.haley@hp.com> wrote:
>>
>> [snipped]
>>
>> Change the IPv6 addrconf code to handle the case where the NIC link
>> state goes UP-DOWN-UP by removing all the autoconfigured addresses
>> when it goes down, so that when it comes back up they will get added
>> again and DAD will be triggered.  
>>
> Surely this is going to result in me sending you lots of rude emails 
> everytime cursing you when my SSH connections are terminated for no real 
> reason on a link flap?  Privacy addresses are handy on servers too such 
> as DNS servers...

No, you'd be sending me some rude emails, I didn't test that particular
case, so Dave, don't apply this.  I guess we've lived with it long enough
the way it is that dropping a few packets at initialization isn't *that*
bad.  I'll see if I can figure out a better way in the meantime...

-Brian

Re: [PATCH] IPv6: Change addrconf code to deal with link changes better

[David Miller]

From: Brian Haley <brian.haley@hp.com>
Date: Fri, 10 Sep 2010 21:10:29 -0400

> No, you'd be sending me some rude emails, I didn't test that particular
> case, so Dave, don't apply this.

Ok.