ipv6: Add IFA_F_DADFAILED flag

PATCH Applied Brian Haley 5 participants 9 messages Sep 5, 2009 – Dec 2, 2009

Subsystems (3)

Networking Network drivers IPv4/IPv6

lore

Share & tools

lore

Brian Haley

author

2009-09-05 01:38 UTC #0 lore raw

PATCH 1/1 ipv6: Add IFA_F_DADFAILED flag

[Note: if this is accepted I'll send out a patch for iproute,
if you'd prefer to not use the last ifa_flag I'll send a much larger patch that does this differently :) ]

Add IFA_F_DADFAILED flag to denote an IPv6 address that has failed Duplicate Address Detection, that way tools like /sbin/ip can be more informative.

3: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qlen 1000
inet6 2001:db8::1/64 scope global tentative dadfailed
valid_lft forever preferred_lft forever

Signed-off-by Brian Haley <[email protected]>

---

include/linux/if_addr.h +1 -0

index a60c821..fd97404 100644
@@ -41,6 +41,7 @@ enum
 
 #define	IFA_F_NODAD		0x02
 #define IFA_F_OPTIMISTIC	0x04
+#define IFA_F_DADFAILED		0x08
 #define	IFA_F_HOMEADDRESS	0x10
 #define IFA_F_DEPRECATED	0x20
 #define IFA_F_TENTATIVE		0x40

net/ipv6/addrconf.c +1 -1

index 43b3c9f..6532966 100644
@@ -1376,7 +1376,7 @@ static void addrconf_dad_stop(struct inet6_ifaddr *ifp)
 	if (ifp->flags&IFA_F_PERMANENT) {
 		spin_lock_bh(&ifp->lock);
 		addrconf_del_timer(ifp);
-		ifp->flags |= IFA_F_TENTATIVE;
+		ifp->flags |= IFA_F_DADFAILED;
 		spin_unlock_bh(&ifp->lock);
 		in6_ifa_put(ifp);
 #ifdef CONFIG_IPV6_PRIVACY

Jens Rosenboom

2009-09-08 13:57 UTC #1 lore raw

Quoted 33 lines · Brian Haley

[Note: if this is accepted I'll send out a patch for iproute,
if you'd prefer to not use the last ifa_flag I'll send a much larger patch that does this differently :) ]
Add IFA_F_DADFAILED flag to denote an IPv6 address that has failed Duplicate Address Detection, that way tools like /sbin/ip can be more informative.
3: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qlen 1000
inet6 2001:db8::1/64 scope global tentative dadfailed
valid_lft forever preferred_lft forever
Signed-off-by Brian Haley <[email protected]>
---
include/linux/if_addr.h +1 -0
index a60c821..fd97404 100644
@@ -41,6 +41,7 @@ enum
 
 #define	IFA_F_NODAD		0x02
 #define IFA_F_OPTIMISTIC	0x04
+#define IFA_F_DADFAILED		0x08
 #define	IFA_F_HOMEADDRESS	0x10
 #define IFA_F_DEPRECATED	0x20
 #define IFA_F_TENTATIVE		0x40
net/ipv6/addrconf.c +0 -0
index 43b3c9f..6532966 100644

net/ipv6/addrconf.c static void addrconf_dad_stop(struct inet6_ifaddr *ifp) · line 1379

Quoted 5 lines

net/ipv6/addrconf.c +1 -1

@@ -1376,7 +1376,7 @@ static void addrconf_dad_stop(struct inet6_ifaddr *ifp)
 	if (ifp->flags&IFA_F_PERMANENT) {
 		spin_lock_bh(&ifp->lock);
 		addrconf_del_timer(ifp);
-		ifp->flags |= IFA_F_TENTATIVE;
+		ifp->flags |= IFA_F_DADFAILED;

I think you still have to set IFA_F_TENTATIVE here, too, otherwise ipv6_dev_get_saddr() will use this address.

Brian Haley

author

2009-09-08 15:18 UTC #2 lore raw

Quoted 11 lines · Jens Rosenboom

net/ipv6/addrconf.c static void addrconf_dad_stop(struct inet6_ifaddr *ifp) · line 1379
Quoted 8 lines
net/ipv6/addrconf.c +1 -1
@@ -1376,7 +1376,7 @@ static void addrconf_dad_stop(struct inet6_ifaddr *ifp)
 	if (ifp->flags&IFA_F_PERMANENT) {
 		spin_lock_bh(&ifp->lock);
 		addrconf_del_timer(ifp);
-		ifp->flags |= IFA_F_TENTATIVE;
+		ifp->flags |= IFA_F_DADFAILED;
I think you still have to set IFA_F_TENTATIVE here, too, otherwise ipv6_dev_get_saddr() will use this address.

The tentative bit is still set from when this address was added back in ipv6_add_addr() from what I can tell, re-setting it here is actually unnecessary. At least /sbin/ip was still showing it set during my testing.

-Brian

Jens Rosenboom

2009-09-08 15:43 UTC #3 lore raw

Quoted 13 lines · Brian Haley

Quoted 11 lines · Jens Rosenboom
net/ipv6/addrconf.c static void addrconf_dad_stop(struct inet6_ifaddr *ifp) · line 1379
Quoted 8 lines
net/ipv6/addrconf.c +1 -1
@@ -1376,7 +1376,7 @@ static void addrconf_dad_stop(struct inet6_ifaddr *ifp)
 	if (ifp->flags&IFA_F_PERMANENT) {
 		spin_lock_bh(&ifp->lock);
 		addrconf_del_timer(ifp);
-		ifp->flags |= IFA_F_TENTATIVE;
+		ifp->flags |= IFA_F_DADFAILED;
I think you still have to set IFA_F_TENTATIVE here, too, otherwise ipv6_dev_get_saddr() will use this address.

Quoted 4 lines

The tentative bit is still set from when this address was added back in ipv6_add_addr() from what I can tell, re-setting it here is actually unnecessary. At least /sbin/ip was still showing it set during my testing.

There is the possibility of a race when the dad_timer expires at the same time the NA triggering DAD failure is received. There isn't a big chance to see that during real world testing, though.

Brian Haley

author

2009-09-10 00:42 UTC #4 lore raw

Quoted 12 lines · Jens Rosenboom

Quoted 11 lines · Brian Haley
Quoted 10 lines · Jens Rosenboom
net/ipv6/addrconf.c static void addrconf_dad_stop(struct inet6_ifaddr *ifp) · line 1379
Quoted 8 lines
net/ipv6/addrconf.c +1 -1
@@ -1376,7 +1376,7 @@ static void addrconf_dad_stop(struct inet6_ifaddr *ifp)
 	if (ifp->flags&IFA_F_PERMANENT) {
 		spin_lock_bh(&ifp->lock);
 		addrconf_del_timer(ifp);
-		ifp->flags |= IFA_F_TENTATIVE;
+		ifp->flags |= IFA_F_DADFAILED;
I think you still have to set IFA_F_TENTATIVE here, too, otherwise ipv6_dev_get_saddr() will use this address.

Quoted 8 lines

Quoted 4 lines
The tentative bit is still set from when this address was added back in ipv6_add_addr() from what I can tell, re-setting it here is actually unnecessary. At least /sbin/ip was still showing it set during my testing.
There is the possibility of a race when the dad_timer expires at the same time the NA triggering DAD failure is received. There isn't a big chance to see that during real world testing, though.

Ok, how does this look? I changed it to set the tentative flag as it did before, plus clear the dad_failed flag if the device got restarted, triggering DAD to happen again for any tentative address, that was an oversight on my part.

I'd still like to know if using this last ifa_flag is going to be an issue, I actually finished a similar patch that uses a new IFA_ADDRFLAGS structure to pass in/out this additional info.

-Brian

include/linux/if_addr.h +1 -0

index a60c821..fd97404 100644
@@ -41,6 +41,7 @@ enum
 
 #define	IFA_F_NODAD		0x02
 #define IFA_F_OPTIMISTIC	0x04
+#define IFA_F_DADFAILED		0x08
 #define	IFA_F_HOMEADDRESS	0x10
 #define IFA_F_DEPRECATED	0x20
 #define IFA_F_TENTATIVE		0x40

net/ipv6/addrconf.c +7 -5

index 43b3c9f..c9b3690 100644
@@ -1371,12 +1371,14 @@ struct inet6_ifaddr *ipv6_get_ifaddr(struct net *net, const struct in6_addr *add
 
 /* Gets referenced address, destroys ifaddr */
 
-static void addrconf_dad_stop(struct inet6_ifaddr *ifp)
+static void addrconf_dad_stop(struct inet6_ifaddr *ifp, int dad_failed)
 {
 	if (ifp->flags&IFA_F_PERMANENT) {
 		spin_lock_bh(&ifp->lock);
 		addrconf_del_timer(ifp);
 		ifp->flags |= IFA_F_TENTATIVE;
+		if (dad_failed)
+			ifp->flags |= IFA_F_DADFAILED;
 		spin_unlock_bh(&ifp->lock);
 		in6_ifa_put(ifp);
 #ifdef CONFIG_IPV6_PRIVACY

@@ -1422,7 +1424,7 @@ void addrconf_dad_failure(struct inet6_ifaddr *ifp)
 		}
 	}
 
-	addrconf_dad_stop(ifp);
+	addrconf_dad_stop(ifp, 1);
 }
 
 /* Join to solicited addr multicast group. */

@@ -2778,7 +2780,7 @@ static void addrconf_dad_start(struct inet6_ifaddr *ifp, u32 flags)
 	    idev->cnf.accept_dad < 1 ||
 	    !(ifp->flags&IFA_F_TENTATIVE) ||
 	    ifp->flags & IFA_F_NODAD) {
-		ifp->flags &= ~(IFA_F_TENTATIVE|IFA_F_OPTIMISTIC);
+		ifp->flags &= ~(IFA_F_TENTATIVE|IFA_F_OPTIMISTIC|IFA_F_DADFAILED);
 		spin_unlock_bh(&ifp->lock);
 		read_unlock_bh(&idev->lock);

@@ -2795,7 +2797,7 @@ static void addrconf_dad_start(struct inet6_ifaddr *ifp, u32 flags)
 		 * - otherwise, kill it.
 		 */
 		in6_ifa_hold(ifp);
-		addrconf_dad_stop(ifp);
+		addrconf_dad_stop(ifp, 0);
 		return;
 	}

@@ -2829,7 +2831,7 @@ static void addrconf_dad_timer(unsigned long data)
 		 * DAD was successful
 		 */
 
-		ifp->flags &= ~(IFA_F_TENTATIVE|IFA_F_OPTIMISTIC);
+		ifp->flags &= ~(IFA_F_TENTATIVE|IFA_F_OPTIMISTIC|IFA_F_DADFAILED);
 		spin_unlock_bh(&ifp->lock);
 		read_unlock_bh(&idev->lock);

Jens Rosenboom

2009-09-10 16:11 UTC #5 lore raw

Quoted 18 lines · Brian Haley

Quoted 17 lines · Jens Rosenboom
Quoted 15 lines · Brian Haley
Quoted 10 lines · Jens Rosenboom
net/ipv6/addrconf.c static void addrconf_dad_stop(struct inet6_ifaddr *ifp) · line 1379
Quoted 8 lines
net/ipv6/addrconf.c +1 -1
@@ -1376,7 +1376,7 @@ static void addrconf_dad_stop(struct inet6_ifaddr *ifp)
 	if (ifp->flags&IFA_F_PERMANENT) {
 		spin_lock_bh(&ifp->lock);
 		addrconf_del_timer(ifp);
-		ifp->flags |= IFA_F_TENTATIVE;
+		ifp->flags |= IFA_F_DADFAILED;
I think you still have to set IFA_F_TENTATIVE here, too, otherwise ipv6_dev_get_saddr() will use this address.
The tentative bit is still set from when this address was added back in ipv6_add_addr() from what I can tell, re-setting it here is actually unnecessary. At least /sbin/ip was still showing it set during my testing.

Quoted 8 lines

Quoted 3 lines
There is the possibility of a race when the dad_timer expires at the same time the NA triggering DAD failure is received. There isn't a big chance to see that during real world testing, though.
Ok, how does this look? I changed it to set the tentative flag as it did before, plus clear the dad_failed flag if the device got restarted, triggering DAD to happen again for any tentative address, that was an oversight on my part.

Looks fine to me so far, can you also send the patch for userspace? That would making testing this a bit easier. ;-)

Quoted 3 lines

I'd still like to know if using this last ifa_flag is going to be an issue, I actually finished a similar patch that uses a new IFA_ADDRFLAGS structure to pass in/out this additional info.

IMHO you should stick to this version, if any future feature needs another bit, it may happen also to need two of them and so will need a new structure then anyway, but why not keep it simple for now?

Brian Haley

author

2009-09-10 19:14 UTC #6 lore raw

Hi Jens,

Quoted 7 lines · Jens Rosenboom

Quoted 4 lines
Ok, how does this look? I changed it to set the tentative flag as it did before, plus clear the dad_failed flag if the device got restarted, triggering DAD to happen again for any tentative address, that was an oversight on my part.
Looks fine to me so far, can you also send the patch for userspace? That would making testing this a bit easier. ;-)

Iproute2 patch below, I'll re-post both once you have a chance to test.

Quoted 7 lines

Quoted 3 lines
I'd still like to know if using this last ifa_flag is going to be an issue, I actually finished a similar patch that uses a new IFA_ADDRFLAGS structure to pass in/out this additional info.
IMHO you should stick to this version, if any future feature needs another bit, it may happen also to need two of them and so will need a new structure then anyway, but why not keep it simple for now?

I'll leave it for now, I might just post as an RFC to get some feedback on it.

Thanks,

-Brian

include/linux/if_addr.h +1 -0

index a60c821..fd97404 100644
@@ -41,6 +41,7 @@ enum
 
 #define	IFA_F_NODAD		0x02
 #define IFA_F_OPTIMISTIC	0x04
+#define IFA_F_DADFAILED		0x08
 #define	IFA_F_HOMEADDRESS	0x10
 #define IFA_F_DEPRECATED	0x20
 #define IFA_F_TENTATIVE		0x40

ip/ipaddress.c +4 -0

index 267ecb3..97c7a8b 100644
@@ -508,6 +508,10 @@ int print_addrinfo(const struct sockaddr_nl *who, struct nlmsghdr *n,
 		fprintf(fp, "dynamic ");
 	} else
 		ifa->ifa_flags &= ~IFA_F_PERMANENT;
+	if (ifa->ifa_flags&IFA_F_DADFAILED) {
+		ifa->ifa_flags &= ~IFA_F_DADFAILED;
+		fprintf(fp, "dadfailed ");
+	}
 	if (ifa->ifa_flags)
 		fprintf(fp, "flags %02x ", ifa->ifa_flags);
 	if (rta_tb[IFA_LABEL])

Stephen Hemminger

Applied

2009-12-02 00:01 UTC #7 lore raw

Quoted 45 lines · On Thu, 10 Sep 2009 15:14:22 -0400 Brian Haley <[email protected]> wrote:

Hi Jens,
Quoted 7 lines · Jens Rosenboom
Quoted 4 lines
Ok, how does this look? I changed it to set the tentative flag as it did before, plus clear the dad_failed flag if the device got restarted, triggering DAD to happen again for any tentative address, that was an oversight on my part.
Looks fine to me so far, can you also send the patch for userspace? That would making testing this a bit easier. ;-)
Iproute2 patch below, I'll re-post both once you have a chance to test.
Quoted 7 lines
Quoted 3 lines
I'd still like to know if using this last ifa_flag is going to be an issue, I actually finished a similar patch that uses a new IFA_ADDRFLAGS structure to pass in/out this additional info.
IMHO you should stick to this version, if any future feature needs another bit, it may happen also to need two of them and so will need a new structure then anyway, but why not keep it simple for now?
I'll leave it for now, I might just post as an RFC to get some feedback on it.
Thanks,
(quoted diff) +0 -1
-Brian
include/linux/if_addr.h +1 -0
index a60c821..fd97404 100644
@@ -41,6 +41,7 @@ enum
 
 #define	IFA_F_NODAD		0x02
 #define IFA_F_OPTIMISTIC	0x04
+#define IFA_F_DADFAILED		0x08
 #define	IFA_F_HOMEADDRESS	0x10
 #define IFA_F_DEPRECATED	0x20
 #define IFA_F_TENTATIVE		0x40
ip/ipaddress.c +0 -0
index 267ecb3..97c7a8b 100644

ip/ipaddress.c int print_addrinfo(const struct sockaddr_nl *who, struct nlmsghdr *n, · line 517

Quoted 10 lines

ip/ipaddress.c +4 -0

@@ -508,6 +508,10 @@ int print_addrinfo(const struct sockaddr_nl *who, struct nlmsghdr *n,
 		fprintf(fp, "dynamic ");
 	} else
 		ifa->ifa_flags &= ~IFA_F_PERMANENT;
+	if (ifa->ifa_flags&IFA_F_DADFAILED) {
+		ifa->ifa_flags &= ~IFA_F_DADFAILED;
+		fprintf(fp, "dadfailed ");
+	}
 	if (ifa->ifa_flags)
 		fprintf(fp, "flags %02x ", ifa->ifa_flags);
 	if (rta_tb[IFA_LABEL])

Applied to iproute (for 2.6.32) with original message changelog

David Miller

maintainer

2009-09-11 19:37 UTC #8 lore raw

In reply to Brian Haley

From: Brian Haley <[email protected]>
Date: Fri, 04 Sep 2009 21:38:07 -0400

Quoted 9 lines

[Note: if this is accepted I'll send out a patch for iproute,
if you'd prefer to not use the last ifa_flag I'll send a much larger patch that does this differently :) ]
Add IFA_F_DADFAILED flag to denote an IPv6 address that has failed Duplicate Address Detection, that way tools like /sbin/ip can be more informative.

Quoted 5 lines

3: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qlen 1000
inet6 2001:db8::1/64 scope global tentative dadfailed
valid_lft forever preferred_lft forever
Signed-off-by Brian Haley <[email protected]>

I applied the most recent iteration of this patch using the above commit message, thanks Brian.