Skip to contentSkip to conversation

IPv6 addrconf related fixes

PATCH In review Stephen Hemminger 6 participants 14 messages Apr 12, 2010 – Jul 28, 2010

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

cover IPv6 addrconf related fixes

These apply to net-next, the problems do not exist in earlier kernels. The problems started when I added changes to retain IPv6 addresses when link goes down.

#1 lore raw

PATCH 1/4 IPv6: keep route for tentative address

Recent changes preserve IPv6 address when link goes down (good). But would cause address to point to dead dst entry (bad). The simplest fix is to just not delete route if address is being held for later use.

net/ipv6/addrconf.c +2 -1
@@ -4046,7 +4046,8 @@ static void __ipv6_ifa_notify(int event,
 			addrconf_leave_anycast(ifp);
 		addrconf_leave_solict(ifp->idev, &ifp->addr);
 		dst_hold(&ifp->rt->u.dst);
-		if (ip6_del_rt(ifp->rt))
+
+		if (ifp->dead && ip6_del_rt(ifp->rt))
 			dst_free(&ifp->rt->u.dst);
 		break;
 	}
#2 lore raw

RE: [PATCH 1/4] IPv6: keep route for tentative address

Quoted 11 lines · Stephen Hemminger

Recent changes preserve IPv6 address when link goes down (good). But would cause address to point to dead dst entry (bad). The simplest fix is to just not delete route if address is being held for later use.

net/ipv6/addrconf.c static void __ipv6_ifa_notify(int event, · line 4053

Quoted 9 lines
net/ipv6/addrconf.c +2 -1
@@ -4046,7 +4046,8 @@ static void __ipv6_ifa_notify(int event,
 			addrconf_leave_anycast(ifp);
 		addrconf_leave_solict(ifp->idev, &ifp->addr);
 		dst_hold(&ifp->rt->u.dst);
-		if (ip6_del_rt(ifp->rt))
+
+		if (ifp->dead && ip6_del_rt(ifp->rt))
 			dst_free(&ifp->rt->u.dst);
 		break;
 	}

Stephen,

With these series of patches (1-4) applied I am no longer seeing the hangs and warnings associated with ipv6. I ran few rounds of tests with resetting the interface and loading/unloading modules.

Thanks a lot for looking into this!
Emil

#3 lore raw

Re: [PATCH 1/4] IPv6: keep route for tentative address

Quoted 24 lines · Emil S
Quoted 11 lines · Stephen Hemminger

Recent changes preserve IPv6 address when link goes down (good). But would cause address to point to dead dst entry (bad). The simplest fix is to just not delete route if address is being held for later use.

net/ipv6/addrconf.c     2010-04-11 12:25:05.349309074 -0700 static void __ipv6_ifa_notify(int event, · line 4053

Quoted 9 lines
net/ipv6/addrconf.c     2010-04-11 12:25:05.349309074 -0700 renamed from net/ipv6/addrconf.c     2010-04-11 12:19:37.938082190 -0700 +2 -1
@@ -4046,7 +4046,8 @@ static void __ipv6_ifa_notify(int event,
                       addrconf_leave_anycast(ifp);
               addrconf_leave_solict(ifp->idev, &ifp->addr);
               dst_hold(&ifp->rt->u.dst);
-             if (ip6_del_rt(ifp->rt))
+
+             if (ifp->dead && ip6_del_rt(ifp->rt))
                       dst_free(&ifp->rt->u.dst);
               break;
       }

Stephen,

Quoted 3 lines

With these series of patches (1-4) applied I am no longer seeing the hangs and warnings associated with ipv6. I ran few rounds of tests with resetting the interface and loading/unloading modules.

Looks like these patches did not make it into net-2.6 and the issue can be seen in the latest stable 2.6.34.

Any chance that the patches can be pushed to 2.6.34-stable?

Thanks,
Emil

#4 lore raw

PATCH 1/2 IPv6: keep route for tentative address

Recent changes preserve IPv6 address when link goes down (good). But would cause address to point to dead dst entry (bad). The simplest fix is to just not delete route if address is being held for later use.

(cherry picked from commit 93fa159abe50d3c55c7f83622d3f5c09b6e06f4b)

---
Patch is for 2.6.34-stable (problem doesn't exist in earlier kernel)

 net/ipv6/addrconf.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
net/ipv6/addrconf.c +2 -1
index 413054f..12e9558 100644
@@ -4047,7 +4047,8 @@ static void __ipv6_ifa_notify(int event, struct inet6_ifaddr *ifp)
 			addrconf_leave_anycast(ifp);
 		addrconf_leave_solict(ifp->idev, &ifp->addr);
 		dst_hold(&ifp->rt->u.dst);
-		if (ip6_del_rt(ifp->rt))
+
+		if (ifp->dead && ip6_del_rt(ifp->rt))
 			dst_free(&ifp->rt->u.dst);
 		break;
 	}
Signature · 1 line
1.7.0.4
#5 lore raw

PATCH 2/2 IPv6: only notify protocols if address is completely gone

The notifier for address down should only be called if address is completely gone, not just being marked as tentative on link transition. The code in net-next would case bonding/sctp/s390 to see address disappear on link down, but they would never see it reappear on link up.

(cherry picked from commit 8595805aafc8b077e01804c9a3668e9aa3510e89)

---
Patch for 2.6.34-stable, not needed on earlier or later kernels

 net/ipv6/addrconf.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)
net/ipv6/addrconf.c +3 -1
index 12e9558..844ffc5 100644
@@ -2729,7 +2729,9 @@ static int addrconf_ifdown(struct net_device *dev, int how)
 		write_unlock_bh(&idev->lock);
 
 		__ipv6_ifa_notify(RTM_DELADDR, ifa);
-		atomic_notifier_call_chain(&inet6addr_chain, NETDEV_DOWN, ifa);
+		if (ifa->dead)
+			atomic_notifier_call_chain(&inet6addr_chain,
+						   NETDEV_DOWN, ifa);
 		in6_ifa_put(ifa);
 
 		write_lock_bh(&idev->lock);
Signature · 1 line
1.7.0.4
#6 lore raw

Patch "IPv6: only notify protocols if address is completely gone" has been added to the 2.6.34-stable tree

This is a note to let you know that I've just added the patch titled

IPv6: only notify protocols if address is completely gone

to the 2.6.34-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary

The filename of the patch is:
ipv6-only-notify-protocols-if-address-is-completely-gone.patch
and it can be found in the queue-2.6.34 subdirectory.

If you, or anyone else, feels it should not be added to the stable tree, please let <[email protected]> know about it.

Quoted 1 line

From [email protected] Tue Jul 27 16:57:29 2010

From: Stephen Hemminger <[email protected]>
Date: Mon, 24 May 2010 11:33:00 -0700
Subject: IPv6: only notify protocols if address is completely gone
To: Emil S Tantilov <[email protected]>, "David S. Miller" <[email protected]>, Greg KH <[email protected]>
Cc: NetDev <[email protected]>, "Tantilov, Emil S" <[email protected]>, [email protected]
Message-ID: <20100524113300.2ef38e12@nehalam>

From: Stephen Hemminger <[email protected]>

(cherry picked from commit 8595805aafc8b077e01804c9a3668e9aa3510e89)

The notifier for address down should only be called if address is completely gone, not just being marked as tentative on link transition. The code in net-next would case bonding/sctp/s390 to see address disappear on link down, but they would never see it reappear on link up.

---
 net/ipv6/addrconf.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
net/ipv6/addrconf.c +3 -1
@@ -2729,7 +2729,9 @@ static int addrconf_ifdown(struct net_de
 		write_unlock_bh(&idev->lock);
 
 		__ipv6_ifa_notify(RTM_DELADDR, ifa);
-		atomic_notifier_call_chain(&inet6addr_chain, NETDEV_DOWN, ifa);
+		if (ifa->dead)
+			atomic_notifier_call_chain(&inet6addr_chain,
+						   NETDEV_DOWN, ifa);
 		in6_ifa_put(ifa);
 
 		write_lock_bh(&idev->lock);

Patches currently in stable-queue which might be from [email protected] are

queue-2.6.34/sky2-restore-multicast-after-restart.patch queue-2.6.34/ipv6-keep-route-for-tentative-address.patch queue-2.6.34/ipv6-only-notify-protocols-if-address-is-completely-gone.patch queue-2.6.34/ipv6-fix-null-reference-in-proxy-neighbor-discovery.patch queue-2.6.34/bridge-fdb-cleanup-runs-too-often.patch

#7 lore raw

Re: [PATCH 1/2] IPv6: keep route for tentative address

In reply to Stephen Hemminger
Quoted 10 lines · Stephen Hemminger

Recent changes preserve IPv6 address when link goes down (good). But would cause address to point to dead dst entry (bad). The simplest fix is to just not delete route if address is being held for later use.

(cherry picked from commit 93fa159abe50d3c55c7f83622d3f5c09b6e06f4b)

---
Patch is for 2.6.34-stable (problem doesn't exist in earlier kernel)

Normally I wait for David to send networking patches for the stable trees, so I would like to get David's ack on these two before I am willing to apply them.

thanks,

greg k-h

maintainer
#8 lore raw

Re: [PATCH 1/2] IPv6: keep route for tentative address

From: Greg KH <[email protected]>
Date: Mon, 24 May 2010 11:47:13 -0700

Quoted 12 lines · Stephen Hemminger
Quoted 10 lines

Recent changes preserve IPv6 address when link goes down (good). But would cause address to point to dead dst entry (bad). The simplest fix is to just not delete route if address is being held for later use.

(cherry picked from commit 93fa159abe50d3c55c7f83622d3f5c09b6e06f4b)

---
Patch is for 2.6.34-stable (problem doesn't exist in earlier kernel)

Quoted 3 lines

Normally I wait for David to send networking patches for the stable trees, so I would like to get David's ack on these two before I am willing to apply them.

These two are fine to go to -stable, Greg please integrate.

#9 lore raw

Patch "IPv6: keep route for tentative address" has been added to the 2.6.34-stable tree

In reply to Stephen Hemminger

This is a note to let you know that I've just added the patch titled

IPv6: keep route for tentative address

to the 2.6.34-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary

The filename of the patch is:
ipv6-keep-route-for-tentative-address.patch
and it can be found in the queue-2.6.34 subdirectory.

If you, or anyone else, feels it should not be added to the stable tree, please let <[email protected]> know about it.

Quoted 1 line

From [email protected] Tue Jul 27 16:56:59 2010

From: Stephen Hemminger <[email protected]>
Date: Mon, 24 May 2010 11:31:18 -0700
Subject: IPv6: keep route for tentative address
To: Emil S Tantilov <[email protected]>
Cc: NetDev <[email protected]>, [email protected], Greg KH <[email protected]>, "David S. Miller" <[email protected]>, "Tantilov, Emil S" <[email protected]>
Message-ID: <20100524113118.47cc9852@nehalam>

From: Stephen Hemminger <[email protected]>

(cherry picked from commit 93fa159abe50d3c55c7f83622d3f5c09b6e06f4b)

Recent changes preserve IPv6 address when link goes down (good). But would cause address to point to dead dst entry (bad). The simplest fix is to just not delete route if address is being held for later use.

---
 net/ipv6/addrconf.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
net/ipv6/addrconf.c +2 -1
@@ -4047,7 +4047,8 @@ static void __ipv6_ifa_notify(int event,
 			addrconf_leave_anycast(ifp);
 		addrconf_leave_solict(ifp->idev, &ifp->addr);
 		dst_hold(&ifp->rt->u.dst);
-		if (ip6_del_rt(ifp->rt))
+
+		if (ifp->dead && ip6_del_rt(ifp->rt))
 			dst_free(&ifp->rt->u.dst);
 		break;
 	}

Patches currently in stable-queue which might be from [email protected] are

queue-2.6.34/sky2-restore-multicast-after-restart.patch queue-2.6.34/ipv6-keep-route-for-tentative-address.patch queue-2.6.34/ipv6-only-notify-protocols-if-address-is-completely-gone.patch queue-2.6.34/ipv6-fix-null-reference-in-proxy-neighbor-discovery.patch queue-2.6.34/bridge-fdb-cleanup-runs-too-often.patch

#10 lore raw

PATCH 2/4 IPv6: keep tentative addresses in hash table

In reply to Stephen Hemminger

When link goes down, want address to be preserved but in a tentative state, therefore it has to stay in hash list.

net/ipv6/addrconf.c +8 -7
@@ -2703,17 +2703,18 @@ static int addrconf_ifdown(struct net_de
 			/* Flag it for later restoration when link comes up */
 			ifa->flags |= IFA_F_TENTATIVE;
 			in6_ifa_hold(ifa);
+			write_unlock_bh(&idev->lock);
 		} else {
 			list_del(&ifa->if_list);
 			ifa->dead = 1;
-		}
-		write_unlock_bh(&idev->lock);
+			write_unlock_bh(&idev->lock);
 
-		/* clear hash table */
-		spin_lock_bh(&addrconf_hash_lock);
-		hlist_del_init_rcu(&ifa->addr_lst);
-		__in6_ifa_put(ifa);
-		spin_unlock_bh(&addrconf_hash_lock);
+			/* clear hash table */
+			spin_lock_bh(&addrconf_hash_lock);
+			hlist_del_init_rcu(&ifa->addr_lst);
+			__in6_ifa_put(ifa);
+			spin_unlock_bh(&addrconf_hash_lock);
+		}
 
 		__ipv6_ifa_notify(RTM_DELADDR, ifa);
 		atomic_notifier_call_chain(&inet6addr_chain, NETDEV_DOWN, ifa);
#11 lore raw

PATCH 3/4 ipv6: additional ref count for hash list unnecessary

In reply to Stephen Hemminger

Since an address in hash list has to already have a ref count, no additional ref count is needed.

net/ipv6/addrconf.c +0 -3
@@ -675,7 +675,6 @@ ipv6_add_addr(struct inet6_dev *idev, co
 	hash = ipv6_addr_hash(addr);
 
 	hlist_add_head_rcu(&ifa->addr_lst, &inet6_addr_lst[hash]);
-	in6_ifa_hold(ifa);
 	spin_unlock(&addrconf_hash_lock);
 
 	write_lock(&idev->lock);
@@ -723,7 +722,6 @@ static void ipv6_del_addr(struct inet6_i
 
 	spin_lock_bh(&addrconf_hash_lock);
 	hlist_del_init_rcu(&ifp->addr_lst);
-	__in6_ifa_put(ifp);
 	spin_unlock_bh(&addrconf_hash_lock);
 
 	write_lock_bh(&idev->lock);
@@ -2712,7 +2710,6 @@ static int addrconf_ifdown(struct net_de
 			/* clear hash table */
 			spin_lock_bh(&addrconf_hash_lock);
 			hlist_del_init_rcu(&ifa->addr_lst);
-			__in6_ifa_put(ifa);
 			spin_unlock_bh(&addrconf_hash_lock);
 		}
 
#12 lore raw

PATCH 4/4 IPv6: only notify protocols if address is compeletely gone

In reply to Stephen Hemminger

The notifier for address down should only be called if address is completely gone, not just being marked as tentative on link transistion. The code in net-next would case bonding/sctp/s390 to see address disappear on link down, but they would never see it reappear on link up.

net/ipv6/addrconf.c +3 -1
@@ -2714,7 +2714,9 @@ static int addrconf_ifdown(struct net_de
 		}
 
 		__ipv6_ifa_notify(RTM_DELADDR, ifa);
-		atomic_notifier_call_chain(&inet6addr_chain, NETDEV_DOWN, ifa);
+		if (ifa->dead)
+			atomic_notifier_call_chain(&inet6addr_chain,
+						   NETDEV_DOWN, ifa);
 		in6_ifa_put(ifa);
 
 		write_lock_bh(&idev->lock);
maintainer
#13 lore raw
In reply to Stephen Hemminger

From: Stephen Hemminger <[email protected]>
Date: Mon, 12 Apr 2010 08:41:30 -0700

Quoted 3 lines

These apply to net-next, the problems do not exist in earlier kernels. The problems started when I added changes to retain IPv6 addresses when link goes down.

All aplied, thanks for fixing these bugs Stephen.