[PATCH] net: Fix tentative IPv6 address due to DAD looping back

[PATCH] net: Fix tentative IPv6 address due to DAD looping back

[Meng, Jilin]

we should ignore the looped-back IPv6 DAD packet to avoid configuration failure.
This occurs when a bonding interface with roundrobin mode is being configured an IPv6 address 
while the switch side isn't configured bonding/channel yet.

Signed-off-by: Jilin Meng <jilin.meng@emc.com>
Signed-off-by: Yinglin Sun <yinglin.sun@emc.com>
Signed-off-by: Lakshmanan Prithviraj <lakshmanan.prithviraj@emc.com>
---
--- linux/net/ipv6/ndisc.c.orig	2013-03-12 17:16:18.000000000 +0800
+++ linux/net/ipv6/ndisc.c	2013-03-21 10:37:06.000000000 +0800
@@ -757,6 +757,24 @@ static void ndisc_recv_ns(struct sk_buff
 
 		if (ifp->flags & (IFA_F_TENTATIVE|IFA_F_OPTIMISTIC)) {
 			if (dad) {
+				if (dev->type == ARPHRD_ETHER) {
+					const unsigned char *sadr;
+					sadr = skb_mac_header(skb);
+					if (sadr[6] == dev->dev_addr[0] &&
+					    sadr[7] == dev->dev_addr[1] &&
+					    sadr[8] == dev->dev_addr[2] &&
+					    sadr[9] == dev->dev_addr[3] &&
+					    sadr[10] == dev->dev_addr[4] &&
+					    sadr[11] == dev->dev_addr[5]) {
+					/*
+					 * looped-back to us. This occurs when
+					 * a bonding interface with roundrobin
+					 * mode is being configured while the
+					 * switch side isn't configured yet.
+					 */
+						goto out;
+				}
+
 				/*
 				 * We are colliding with another node
 				 * who is doing DAD

Re: [PATCH] net: Fix tentative IPv6 address due to DAD looping back

[YOSHIFUJI Hideaki]

Meng, Jilin wrote:
> we should ignore the looped-back IPv6 DAD packet to avoid configuration failure.
> This occurs when a bonding interface with roundrobin mode is being configured an IPv6 address 
> while the switch side isn't configured bonding/channel yet.
> 
> Signed-off-by: Jilin Meng <jilin.meng@emc.com>
> Signed-off-by: Yinglin Sun <yinglin.sun@emc.com>
> Signed-off-by: Lakshmanan Prithviraj <lakshmanan.prithviraj@emc.com>
> ---
> --- linux/net/ipv6/ndisc.c.orig	2013-03-12 17:16:18.000000000 +0800
> +++ linux/net/ipv6/ndisc.c	2013-03-21 10:37:06.000000000 +0800
> @@ -757,6 +757,24 @@ static void ndisc_recv_ns(struct sk_buff
>  
>  		if (ifp->flags & (IFA_F_TENTATIVE|IFA_F_OPTIMISTIC)) {
>  			if (dad) {
> +				if (dev->type == ARPHRD_ETHER) {
> +					const unsigned char *sadr;
> +					sadr = skb_mac_header(skb);
> +					if (sadr[6] == dev->dev_addr[0] &&
> +					    sadr[7] == dev->dev_addr[1] &&
> +					    sadr[8] == dev->dev_addr[2] &&
> +					    sadr[9] == dev->dev_addr[3] &&
> +					    sadr[10] == dev->dev_addr[4] &&
> +					    sadr[11] == dev->dev_addr[5]) {
> +					/*
> +					 * looped-back to us. This occurs when
> +					 * a bonding interface with roundrobin
> +					 * mode is being configured while the
> +					 * switch side isn't configured yet.
> +					 */
> +						goto out;
> +				}
> +
>  				/*
>  				 * We are colliding with another node
>  				 * who is doing DAD

I would say that it is a configuration error.

--yoshfuji

RE: [PATCH] net: Fix tentative IPv6 address due to DAD looping back

[Meng, Jilin]

Yoshfuji,

Partially you are correct. 
But at least this fix removes the limitation that we must configure the switch for bonding first then host in this case.

-Jilin

-----Original Message-----
From: YOSHIFUJI Hideaki [mailto:yoshfuji@linux-ipv6.org] 
Sent: Friday, March 22, 2013 11:40 AM
To: Meng, Jilin
Cc: davem@davemloft.net; kuznet@ms2.inr.ac.ru; jmorris@namei.org; kaber@trash.net; Sun, Yinglin; Prithviraj, Lakshmanan; netdev@vger.kernel.org
Subject: Re: [PATCH] net: Fix tentative IPv6 address due to DAD looping back

Meng, Jilin wrote:
> we should ignore the looped-back IPv6 DAD packet to avoid configuration failure.
> This occurs when a bonding interface with roundrobin mode is being 
> configured an IPv6 address while the switch side isn't configured bonding/channel yet.
> 
> Signed-off-by: Jilin Meng <jilin.meng@emc.com>
> Signed-off-by: Yinglin Sun <yinglin.sun@emc.com>
> Signed-off-by: Lakshmanan Prithviraj <lakshmanan.prithviraj@emc.com>
> ---
> --- linux/net/ipv6/ndisc.c.orig	2013-03-12 17:16:18.000000000 +0800
> +++ linux/net/ipv6/ndisc.c	2013-03-21 10:37:06.000000000 +0800
> @@ -757,6 +757,24 @@ static void ndisc_recv_ns(struct sk_buff
>  
>  		if (ifp->flags & (IFA_F_TENTATIVE|IFA_F_OPTIMISTIC)) {
>  			if (dad) {
> +				if (dev->type == ARPHRD_ETHER) {
> +					const unsigned char *sadr;
> +					sadr = skb_mac_header(skb);
> +					if (sadr[6] == dev->dev_addr[0] &&
> +					    sadr[7] == dev->dev_addr[1] &&
> +					    sadr[8] == dev->dev_addr[2] &&
> +					    sadr[9] == dev->dev_addr[3] &&
> +					    sadr[10] == dev->dev_addr[4] &&
> +					    sadr[11] == dev->dev_addr[5]) {
> +					/*
> +					 * looped-back to us. This occurs when
> +					 * a bonding interface with roundrobin
> +					 * mode is being configured while the
> +					 * switch side isn't configured yet.
> +					 */
> +						goto out;
> +				}
> +
>  				/*
>  				 * We are colliding with another node
>  				 * who is doing DAD

I would say that it is a configuration error.

--yoshfuji

Re: [PATCH] net: Fix tentative IPv6 address due to DAD looping back

[Sergei Shtylyov]

Hello.

On 22-03-2013 7:20, Meng, Jilin wrote:

> we should ignore the looped-back IPv6 DAD packet to avoid configuration failure.
> This occurs when a bonding interface with roundrobin mode is being configured an IPv6 address
> while the switch side isn't configured bonding/channel yet.

> Signed-off-by: Jilin Meng <jilin.meng@emc.com>
> Signed-off-by: Yinglin Sun <yinglin.sun@emc.com>
> Signed-off-by: Lakshmanan Prithviraj <lakshmanan.prithviraj@emc.com>
> ---
> --- linux/net/ipv6/ndisc.c.orig	2013-03-12 17:16:18.000000000 +0800
> +++ linux/net/ipv6/ndisc.c	2013-03-21 10:37:06.000000000 +0800
> @@ -757,6 +757,24 @@ static void ndisc_recv_ns(struct sk_buff
>
>   		if (ifp->flags & (IFA_F_TENTATIVE|IFA_F_OPTIMISTIC)) {
>   			if (dad) {
> +				if (dev->type == ARPHRD_ETHER) {
> +					const unsigned char *sadr;

    Empty line after declaration wouldn't hurt.

> +					sadr = skb_mac_header(skb);
> +					if (sadr[6] == dev->dev_addr[0] &&
> +					    sadr[7] == dev->dev_addr[1] &&
> +					    sadr[8] == dev->dev_addr[2] &&
> +					    sadr[9] == dev->dev_addr[3] &&
> +					    sadr[10] == dev->dev_addr[4] &&
> +					    sadr[11] == dev->dev_addr[5]) {

    Don't we have memcmp() in Linux?

WBR, Sergei

Re: [PATCH] net: Fix tentative IPv6 address due to DAD looping back

[Eric Dumazet]

On Fri, 2013-03-22 at 18:37 +0400, Sergei Shtylyov wrote:

>     Don't we have memcmp() in Linux?
> 

Dont use slow memcmp() in this context but fast
ether_addr_equal_64bits()

RE: [PATCH] net: Fix tentative IPv6 address due to DAD looping back

[Meng, Jilin]

Thanks Sergei for introducing a fast function.

-----Original Message-----
From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On Behalf Of Eric Dumazet
Sent: Friday, March 22, 2013 10:48 PM
To: Sergei Shtylyov
Cc: Meng, Jilin; davem@davemloft.net; kuznet@ms2.inr.ac.ru; jmorris@namei.org; yoshfuji@linux-ipv6.org; kaber@trash.net; Sun, Yinglin; Prithviraj, Lakshmanan; netdev@vger.kernel.org
Subject: Re: [PATCH] net: Fix tentative IPv6 address due to DAD looping back

On Fri, 2013-03-22 at 18:37 +0400, Sergei Shtylyov wrote:

>     Don't we have memcmp() in Linux?
> 

Dont use slow memcmp() in this context but fast
ether_addr_equal_64bits()



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

Re: [PATCH] net: Fix tentative IPv6 address due to DAD looping back

[David Miller]

From: "Meng, Jilin" <Jilin.Meng@emc.com>
Date: Thu, 11 Apr 2013 03:13:42 +0000

> Thanks Sergei for introducing a fast function.

Do not top post.

Quote the email you are responding to, and then put your response
after the quoted content, not before.

[PATCH] net: Fix tentative IPv6 address due to DAD looping back

[Meng, Jilin]

This version accepted David's comments of coding style, Eric's comments of ether_addr_equal_64bits(), and Hannes' comments of eth_hdr(), etc.

we should ignore the looped-back IPv6 DAD packet to avoid configuration failure.
This occurs when a bonding interface with roundrobin mode is being configured an IPv6 address while the switch side isn't configured bonding/channel yet.

Signed-off-by: Jilin Meng <jilin.meng@emc.com>
Signed-off-by: Yinglin Sun <yinglin.sun@emc.com>
Signed-off-by: Lakshmanan Prithviraj <lakshmanan.prithviraj@emc.com>
---
--- linux/net/ipv6/ndisc.c.orig	2013-03-12 17:16:18.000000000 +0800
+++ linux/net/ipv6/ndisc.c	2013-04-12 14:44:31.000000000 +0800
@@ -71,6 +71,7 @@
 
 #include <linux/netfilter.h>
 #include <linux/netfilter_ipv6.h>
+#include <linux/etherdevice.h>
 
 /* Set to 3 to get tracing... */
 #define ND_DEBUG 1
@@ -757,6 +758,16 @@ static void ndisc_recv_ns(struct sk_buff
 
 		if (ifp->flags & (IFA_F_TENTATIVE|IFA_F_OPTIMISTIC)) {
 			if (dad) {
+				/* To avoid the DAD looped-back to us.
+				 * This occurs when a bonding interface with
+				 * roundrobin mode is being configured while
+				 * the switch side isn't configured yet.
+				 */
+				if (dev->type == ARPHRD_ETHER &&
+				    ether_addr_equal_64bits(
+				      eth_hdr(skb)->h_source, dev->dev_addr))
+					goto out;
+
 				/*
 				 * We are colliding with another node
 				 * who is doing DAD

Re: [PATCH] net: Fix tentative IPv6 address due to DAD looping back

[David Miller]

From: "Meng, Jilin" <Jilin.Meng@emc.com>
Date: Fri, 12 Apr 2013 07:45:54 +0000

> This version accepted David's comments of coding style, Eric's comments of ether_addr_equal_64bits(), and Hannes' comments of eth_hdr(), etc.
> 
> we should ignore the looped-back IPv6 DAD packet to avoid configuration failure.
> This occurs when a bonding interface with roundrobin mode is being configured an IPv6 address while the switch side isn't configured bonding/channel yet.

Please format your commit messages properly, specifically fix these
long lines of text that extend past 80 columns.

Just because it looks properly formatted in your email client, doesn't
mean it actually is.

Please, therefore, make sure that lines do not extend past 80 columns
without a newline.

Re: [PATCH] net: Fix tentative IPv6 address due to DAD looping back

[Hannes Frederic Sowa]

On Fri, Apr 12, 2013 at 07:45:54AM +0000, Meng, Jilin wrote:
> This version accepted David's comments of coding style, Eric's comments of ether_addr_equal_64bits(), and Hannes' comments of eth_hdr(), etc.
> 
> we should ignore the looped-back IPv6 DAD packet to avoid configuration failure.
> This occurs when a bonding interface with roundrobin mode is being configured an IPv6 address while the switch side isn't configured bonding/channel yet.

RFC4862 5.4.5 definitely states that if DAD detects duplicate mac
addresses IPv6 SHOULD be disabled on that interface. Your patch
deliberately omits this all the time.

What do you think if we hook up a function which does reenable ipv6 on
an interface as soon as 'disable_ipv6' is switched to '0'? So you could
control reacquiring the ipv6 address from userspace without ifup&ifdown
and the risk of disturbing ipv4 connections? This seems to be the
easiest solution (but have not checked thoroughly).

Otherwise we should think about a loop detection in the bonding driver,
which I think, is very hard to get right. Think about more advanced
setups with bonded ports into a bridge and the suppression of spanning
tree bpdus (nobody knows where the looping does actually occur).

Thanks,

  Hannes

Re: [PATCH] net: Fix tentative IPv6 address due to DAD looping back

[Hannes Frederic Sowa]

On Fri, Apr 12, 2013 at 07:51:41PM +0200, Hannes Frederic Sowa wrote:
> On Fri, Apr 12, 2013 at 07:45:54AM +0000, Meng, Jilin wrote:
> > This version accepted David's comments of coding style, Eric's comments of ether_addr_equal_64bits(), and Hannes' comments of eth_hdr(), etc.
> > 
> > we should ignore the looped-back IPv6 DAD packet to avoid configuration failure.
> > This occurs when a bonding interface with roundrobin mode is being configured an IPv6 address while the switch side isn't configured bonding/channel yet.
> [...]
> What do you think if we hook up a function which does reenable ipv6 on
> an interface as soon as 'disable_ipv6' is switched to '0'? So you could
> control reacquiring the ipv6 address from userspace without ifup&ifdown
> and the risk of disturbing ipv4 connections? This seems to be the
> easiest solution (but have not checked thoroughly).
> [...]

I just looked into it and it does already worked as expected. Didn't know why
I assumed that there was only a disable handler. So you can resolve this issue
from userspace by a timer or a small program which listens for loopback
traffic in userspace and do DAD after the loop is resolved. I think this is a
more sensible solution.

Thanks,

  Hannes

[PATCH] net: Fix tentative IPv6 address due to DAD looping back

[Meng, Jilin]

we should ignore the looped-back IPv6 DAD packet to avoid configuration failure.
This occurs when a bonding interface with roundrobin mode is being configured an IPv6 address while the switch side isn't configured bonding/channel yet.

Signed-off-by: Jilin Meng <jilin.meng@emc.com>
Signed-off-by: Yinglin Sun <yinglin.sun@emc.com>
Signed-off-by: Lakshmanan Prithviraj <lakshmanan.prithviraj@emc.com>
---
--- linux/net/ipv6/ndisc.c.orig	2013-03-12 17:16:18.000000000 +0800
+++ linux/net/ipv6/ndisc.c	2013-03-21 10:37:06.000000000 +0800
@@ -757,6 +757,24 @@ static void ndisc_recv_ns(struct sk_buff
 
 		if (ifp->flags & (IFA_F_TENTATIVE|IFA_F_OPTIMISTIC)) {
 			if (dad) {
+				if (dev->type == ARPHRD_ETHER) {
+					const unsigned char *sadr;
+					sadr = skb_mac_header(skb);
+					if (sadr[6] == dev->dev_addr[0] &&
+					    sadr[7] == dev->dev_addr[1] &&
+					    sadr[8] == dev->dev_addr[2] &&
+					    sadr[9] == dev->dev_addr[3] &&
+					    sadr[10] == dev->dev_addr[4] &&
+					    sadr[11] == dev->dev_addr[5]) {
+					/*
+					 * looped-back to us. This occurs when
+					 * a bonding interface with roundrobin
+					 * mode is being configured while the
+					 * switch side isn't configured yet.
+					 */
+						goto out;
+				}
+
 				/*
 				 * We are colliding with another node
 				 * who is doing DAD

[PATCH] net: Fix tentative IPv6 address due to DAD looping back

[Meng, Jilin]

we should ignore the looped-back IPv6 DAD packet to avoid configuration failure.
This occurs when a bonding interface with roundrobin mode is being configured an IPv6 address while the switch side isn't configured bonding/channel yet.

Signed-off-by: Jilin Meng <jilin.meng@emc.com>
Signed-off-by: Yinglin Sun <yinglin.sun@emc.com>
Signed-off-by: Lakshmanan Prithviraj <lakshmanan.prithviraj@emc.com>
---
--- linux/net/ipv6/ndisc.c.orig	2013-03-12 17:16:18.000000000 +0800
+++ linux/net/ipv6/ndisc.c	2013-04-11 09:00:45.000000000 +0800
@@ -71,6 +71,7 @@
 
 #include <linux/netfilter.h>
 #include <linux/netfilter_ipv6.h>
+#include <linux/etherdevice.h>
 
 /* Set to 3 to get tracing... */
 #define ND_DEBUG 1
@@ -757,6 +758,19 @@ static void ndisc_recv_ns(struct sk_buff
 
 		if (ifp->flags & (IFA_F_TENTATIVE|IFA_F_OPTIMISTIC)) {
 			if (dad) {
+				if (dev->type == ARPHRD_ETHER) {
+					const unsigned char *sadr;
+					sadr = skb_mac_header(skb);
+					if (ether_addr_equal_64bits(sadr + 6, dev->dev_addr))
+					/*
+					 * looped-back to us. This occurs when
+					 * a bonding interface with roundrobin
+					 * mode is being configured while the
+					 * switch side isn't configured yet.
+					 */
+						goto out;
+				}
+
 				/*
 				 * We are colliding with another node
 				 * who is doing DAD

Re: [PATCH] net: Fix tentative IPv6 address due to DAD looping back

[David Miller]

From: "Meng, Jilin" <Jilin.Meng@emc.com>
Date: Thu, 11 Apr 2013 03:06:23 +0000

> we should ignore the looped-back IPv6 DAD packet to avoid configuration failure.
> This occurs when a bonding interface with roundrobin mode is being configured an IPv6 address while the switch side isn't configured bonding/channel yet.

Please don't submit long lines in your commit message, place
a newline before the 80th column.

> +				if (dev->type == ARPHRD_ETHER) {
> +					const unsigned char *sadr;
> +					sadr = skb_mac_header(skb);

Add an empty line between the "sadr" variable declaration and the
assignment.

> +					if (ether_addr_equal_64bits(sadr + 6, dev->dev_addr))
> +					/*
> +					 * looped-back to us. This occurs when
> +					 * a bonding interface with roundrobin
> +					 * mode is being configured while the
> +					 * switch side isn't configured yet.
> +					 */
> +						goto out;

This had improper coding style in at least 2 different aspects.

First, either put the comment before the if() statement, or if you
absolutely must put it inside the basic block of the if() statement,
add braces.

Second, the comment is improperly formatted for the networking.  It
should be formatted:

	/* Like
	 * this.
	 */

Finally, what do you mean when you ask for your message to be
"recalled"?  That's awkward and doesn't mean anything to us.  Where
else have you seen people use that terminology on a mailing list?

Instead, just say "please ignore this patch, I have a new version coming".

Then, when you post the new patch, tell us what changed from the previous
submission.

Not a good patch submission at all, and I haven't even thought about
the logic of your change at all.

RE: [PATCH] net: Fix tentative IPv6 address due to DAD looping back

[Meng, Jilin]

>-----Original Message-----
>From: David Miller [mailto:davem@davemloft.net] 
>Sent: Thursday, April 11, 2013 11:15 AM
>To: Meng, Jilin
>Cc: kuznet@ms2.inr.ac.ru; jmorris@namei.org; yoshfuji@linux-ipv6.org; kaber@trash.net; Sun, Yinglin; Prithviraj, Lakshmanan; netdev@vger.kernel.org
>Subject: Re: [PATCH] net: Fix tentative IPv6 address due to DAD looping back
>
>From: "Meng, Jilin" <Jilin.Meng@emc.com>
>Date: Thu, 11 Apr 2013 03:06:23 +0000
>
>> we should ignore the looped-back IPv6 DAD packet to avoid configuration failure.
>> This occurs when a bonding interface with roundrobin mode is being configured an IPv6 address while the switch side isn't configured bonding/channel yet.
>
>Please don't submit long lines in your commit message, place a newline before the 80th column.
>
>> +				if (dev->type == ARPHRD_ETHER) {
>> +					const unsigned char *sadr;
>> +					sadr = skb_mac_header(skb);
>
>Add an empty line between the "sadr" variable declaration and the assignment.
>
>> +					if (ether_addr_equal_64bits(sadr + 6, dev->dev_addr))
>> +					/*
>> +					 * looped-back to us. This occurs when
>> +					 * a bonding interface with roundrobin
>> +					 * mode is being configured while the
>> +					 * switch side isn't configured yet.
>> +					 */
>> +						goto out;
>
>This had improper coding style in at least 2 different aspects.
>
>First, either put the comment before the if() statement, or if you absolutely must put it inside the basic block of the if() statement, add braces.
>
>Second, the comment is improperly formatted for the networking.  It should be formatted:
>
>	/* Like
>	 * this.
>	 */
>
>Finally, what do you mean when you ask for your message to be "recalled"?  That's awkward and doesn't mean anything to us.  Where else have you seen people use that terminology on a mailing list?
>
>Instead, just say "please ignore this patch, I have a new version coming".
>
>Then, when you post the new patch, tell us what changed from the previous submission.
>
>Not a good patch submission at all, and I haven't even thought about the logic of your change at all.
>

Thanks David. I learned much from you.

-Jilin

Re: [PATCH] net: Fix tentative IPv6 address due to DAD looping back

[Hannes Frederic Sowa]

On Thu, Apr 11, 2013 at 03:06:23AM +0000, Meng, Jilin wrote:
> we should ignore the looped-back IPv6 DAD packet to avoid configuration failure.
> This occurs when a bonding interface with roundrobin mode is being configured an IPv6 address while the switch side isn't configured bonding/channel yet.

I am very uncomfortable with this change in ndisc layer. I would
prefer DAD kicking in and disabling ipv6 on that interface in such a
situation. The likelihood that an interface with the same mac address
has the same ll ipv6 address is very high and DAD must guard that.

> Signed-off-by: Jilin Meng <jilin.meng@emc.com>
> Signed-off-by: Yinglin Sun <yinglin.sun@emc.com>
> Signed-off-by: Lakshmanan Prithviraj <lakshmanan.prithviraj@emc.com>
> ---
> --- linux/net/ipv6/ndisc.c.orig	2013-03-12 17:16:18.000000000 +0800
> +++ linux/net/ipv6/ndisc.c	2013-04-11 09:00:45.000000000 +0800
> @@ -71,6 +71,7 @@
>  
>  #include <linux/netfilter.h>
>  #include <linux/netfilter_ipv6.h>
> +#include <linux/etherdevice.h>
>  
>  /* Set to 3 to get tracing... */
>  #define ND_DEBUG 1
> @@ -757,6 +758,19 @@ static void ndisc_recv_ns(struct sk_buff
>  
>  		if (ifp->flags & (IFA_F_TENTATIVE|IFA_F_OPTIMISTIC)) {
>  			if (dad) {
> +				if (dev->type == ARPHRD_ETHER) {
> +					const unsigned char *sadr;
> +					sadr = skb_mac_header(skb);
> +					if (ether_addr_equal_64bits(sadr + 6, dev->dev_addr))

Minor improvment:

struct ethhdr *eth = eth_hdr(skb);
if (ether_addr_equal_64bits(eth->h_source, dev->dev_addr))