[PATCH] net: ipv6: Delete host routes on an ifdown

[PATCH] net: ipv6: Delete host routes on an ifdown

[David Ahern]

It was a simple idea -- save IPv6 configured addresses on a link down
so that IPv6 behaves similar to IPv4. As always the devil is in the
details and the IPv6 stack as too many behavioral differences from IPv4
making the simple idea more complicated than it needs to be.

The current implementation for keeping IPv6 addresses can panic or spit
out a warning in one of many paths:

1. IPv6 route gets an IPv4 route as its 'next' which causes a panic in
   rt6_fill_node while handling a route dump request.

2. rt->dst.obsolete is set to DST_OBSOLETE_DEAD hitting the WARN_ON in
   fib6_del

3. Panic in fib6_purge_rt because rt6i_ref count is not 1.

The root cause of all these is references related to the host route for
an address that is retained.

So, this patch deletes the host route every time the ifdown loop runs.
Since the host route is deleted and will be re-generated an up there is
no longer a need for the l3mdev fix up. On the 'admin up' side move
addrconf_permanent_addr into the NETDEV_UP event handling so that it
runs only once versus on UP and CHANGE events.

All of the current panics and warnings appear to be related to
addresses on the loopback device, but given the catastrophic nature when
a bug is triggered this patch takes the conservative approach and evicts
all host routes rather than trying to determine when it can be re-used
and when it can not. That can be a later optimizaton if desired.

Signed-off-by: David Ahern <dsa@cumulusnetworks.com>
---
Dave: I realize this goes against your preference to keep routes cached
      on a down but this patch really emphasizes my point of all the
      dark corners to be handled. 

 net/ipv6/addrconf.c | 48 +++++++++++++++---------------------------------
 1 file changed, 15 insertions(+), 33 deletions(-)

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 23cec53b568a..8ec4b3089e20 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -3176,35 +3176,9 @@ static void addrconf_gre_config(struct net_device *dev)
 }
 #endif
 
-#if IS_ENABLED(CONFIG_NET_L3_MASTER_DEV)
-/* If the host route is cached on the addr struct make sure it is associated
- * with the proper table. e.g., enslavement can change and if so the cached
- * host route needs to move to the new table.
- */
-static void l3mdev_check_host_rt(struct inet6_dev *idev,
-				  struct inet6_ifaddr *ifp)
-{
-	if (ifp->rt) {
-		u32 tb_id = l3mdev_fib_table(idev->dev) ? : RT6_TABLE_LOCAL;
-
-		if (tb_id != ifp->rt->rt6i_table->tb6_id) {
-			ip6_del_rt(ifp->rt);
-			ifp->rt = NULL;
-		}
-	}
-}
-#else
-static void l3mdev_check_host_rt(struct inet6_dev *idev,
-				  struct inet6_ifaddr *ifp)
-{
-}
-#endif
-
 static int fixup_permanent_addr(struct inet6_dev *idev,
 				struct inet6_ifaddr *ifp)
 {
-	l3mdev_check_host_rt(idev, ifp);
-
 	if (!ifp->rt) {
 		struct rt6_info *rt;
 
@@ -3304,6 +3278,9 @@ static int addrconf_notify(struct notifier_block *this, unsigned long event,
 			break;
 
 		if (event == NETDEV_UP) {
+			/* restore routes for permanent addresses */
+			addrconf_permanent_addr(dev);
+
 			if (!addrconf_qdisc_ok(dev)) {
 				/* device is not ready yet. */
 				pr_info("ADDRCONF(NETDEV_UP): %s: link is not ready\n",
@@ -3337,9 +3314,6 @@ static int addrconf_notify(struct notifier_block *this, unsigned long event,
 			run_pending = 1;
 		}
 
-		/* restore routes for permanent addresses */
-		addrconf_permanent_addr(dev);
-
 		switch (dev->type) {
 #if IS_ENABLED(CONFIG_IPV6_SIT)
 		case ARPHRD_SIT:
@@ -3556,6 +3530,8 @@ static int addrconf_ifdown(struct net_device *dev, int how)
 
 	INIT_LIST_HEAD(&del_list);
 	list_for_each_entry_safe(ifa, tmp, &idev->addr_list, if_list) {
+		struct rt6_info *rt = NULL;
+
 		addrconf_del_dad_work(ifa);
 
 		write_unlock_bh(&idev->lock);
@@ -3568,6 +3544,9 @@ static int addrconf_ifdown(struct net_device *dev, int how)
 			ifa->state = 0;
 			if (!(ifa->flags & IFA_F_NODAD))
 				ifa->flags |= IFA_F_TENTATIVE;
+
+			rt = ifa->rt;
+			ifa->rt = NULL;
 		} else {
 			state = ifa->state;
 			ifa->state = INET6_IFADDR_STATE_DEAD;
@@ -3578,6 +3557,9 @@ static int addrconf_ifdown(struct net_device *dev, int how)
 
 		spin_unlock_bh(&ifa->lock);
 
+		if (rt)
+			ip6_del_rt(rt);
+
 		if (state != INET6_IFADDR_STATE_DEAD) {
 			__ipv6_ifa_notify(RTM_DELADDR, ifa);
 			inet6addr_notifier_call_chain(NETDEV_DOWN, ifa);
@@ -5343,10 +5325,10 @@ static void __ipv6_ifa_notify(int event, struct inet6_ifaddr *ifp)
 			if (rt)
 				ip6_del_rt(rt);
 		}
-		dst_hold(&ifp->rt->dst);
-
-		ip6_del_rt(ifp->rt);
-
+		if (ifp->rt) {
+			dst_hold(&ifp->rt->dst);
+			ip6_del_rt(ifp->rt);
+		}
 		rt_genid_bump_ipv6(net);
 		break;
 	}
-- 
2.1.4

Re: [PATCH] net: ipv6: Delete host routes on an ifdown

[David Miller]

From: David Ahern <dsa@cumulusnetworks.com>
Date: Thu, 21 Apr 2016 20:56:12 -0700

> Dave: I realize this goes against your preference to keep routes cached
>       on a down but this patch really emphasizes my point of all the
>       dark corners to be handled. 

I've learned my lesson, I never should have put the original patch in
to begin with.

And you're so sloppy here, you didn't even bother adding a Fixes: tag.

The patch at the root of all of this has terrible semantics, and it's
added tons of regressions.  All of which goes against our most basic
principles.

I've been patient enough, but I've now reverting everything, sorry.

It's unfortunate if you have things which depend upon this, but too
bad.  This change has been extremely disruptive for a lot of people,
including me.

You're going to have to work really hard to ever get me to consider
applying patches which do this again.

Re: [PATCH] net: ipv6: Delete host routes on an ifdown

[David Ahern]

On 4/25/16 1:26 PM, David Miller wrote:
> From: David Ahern <dsa@cumulusnetworks.com>
> Date: Thu, 21 Apr 2016 20:56:12 -0700
>
>> Dave: I realize this goes against your preference to keep routes cached
>>        on a down but this patch really emphasizes my point of all the
>>        dark corners to be handled.
>
> I've learned my lesson, I never should have put the original patch in
> to begin with.
>
> And you're so sloppy here, you didn't even bother adding a Fixes: tag.
>
> The patch at the root of all of this has terrible semantics, and it's
> added tons of regressions.  All of which goes against our most basic
> principles.
>
> I've been patient enough, but I've now reverting everything, sorry.
>
> It's unfortunate if you have things which depend upon this, but too
> bad.  This change has been extremely disruptive for a lot of people,
> including me.
>
> You're going to have to work really hard to ever get me to consider
> applying patches which do this again.
>

It's unfortunate you want to take that action. Last week I came across a 
prior attempt by Stephen to do this same thing -- keep IPv6 addresses. 
That prior attempt was reverted by commit 73a8bd74e261. Cumulus, 
Brocade, and others clearly want this capability.

You have made many comments about the pains due to differences between 
IPv4 and IPv6. I deal with those pains daily. It takes time to close 
those differences and unfortunately there have been and will be bumps in 
the road. If you look back at the commits on top of the original I 
responded quickly to the 1 bug report from Andrey and the rest of the 
followups have come from me (or the delta from Mike at Brocade) which 
are a result of our range of testing with this sysctl enabled.

Re: [PATCH] net: ipv6: Delete host routes on an ifdown

[David Miller]

From: David Ahern <dsa@cumulusnetworks.com>
Date: Mon, 25 Apr 2016 13:40:26 -0600

> It's unfortunate you want to take that action. Last week I came across
> a prior attempt by Stephen to do this same thing -- keep IPv6
> addresses. That prior attempt was reverted by commit
> 73a8bd74e261. Cumulus, Brocade, and others clearly want this
> capability.

But nobody has implemented it correctly, it doesn't matter who wants
the feature.  That's why it keeps getting reverted.

Also, this testing you are talking about should have happened long
before you submitted that first patch that introduced all of these
regressions.  My observations tell me that the bulk of the testing
happened afterwards and that's why all the regressions are popping up
now.

Re: [PATCH] net: ipv6: Delete host routes on an ifdown

[David Ahern]

On 4/25/16 2:42 PM, David Miller wrote:
> From: David Ahern <dsa@cumulusnetworks.com>
> Date: Mon, 25 Apr 2016 13:40:26 -0600
>
>> It's unfortunate you want to take that action. Last week I came across
>> a prior attempt by Stephen to do this same thing -- keep IPv6
>> addresses. That prior attempt was reverted by commit
>> 73a8bd74e261. Cumulus, Brocade, and others clearly want this
>> capability.
>
> But nobody has implemented it correctly, it doesn't matter who wants
> the feature.  That's why it keeps getting reverted.
>
> Also, this testing you are talking about should have happened long
> before you submitted that first patch that introduced all of these
> regressions.  My observations tell me that the bulk of the testing
> happened afterwards and that's why all the regressions are popping up
> now.
>

My testing when submitting the patch was host level: Add an address, 
while(1) (link up, link down), delete an address, etc.

Once it was committed to our kernel it started getting hit with a range 
of L3 deployment scenarios with many nodes and networking config files 
are uploaded and jumped between on real switch hardware - no reboot but 
'networking reload' on the fly. Jumping between different deployments 
with different sets addresses, routes, vrf devices, bridges, bonds, etc.

Your objection seems to be 'all these regressions' but beyond the ref 
count from Andrey all of the bug reports have come from me with 1 from 
Mike, another invested party wanting this to happen. I am the one who 
spent the hours dealing with the kernel panics. My patch, my bug, my 
time wasted coming up with the delta patch. Rather than focusing on my 
mistakes, why not see the commitment on following through with this change?

Re: [PATCH] net: ipv6: Delete host routes on an ifdown

[Roopa Prabhu]

On 4/25/16, 1:42 PM, David Miller wrote:
> From: David Ahern <dsa@cumulusnetworks.com>
> Date: Mon, 25 Apr 2016 13:40:26 -0600
>
>> It's unfortunate you want to take that action. Last week I came across
>> a prior attempt by Stephen to do this same thing -- keep IPv6
>> addresses. That prior attempt was reverted by commit
>> 73a8bd74e261. Cumulus, Brocade, and others clearly want this
>> capability.
> But nobody has implemented it correctly, it doesn't matter who wants
> the feature.  That's why it keeps getting reverted.
>
> Also, this testing you are talking about should have happened long
> before you submitted that first patch that introduced all of these
> regressions.  My observations tell me that the bulk of the testing
> happened afterwards and that's why all the regressions are popping up
> now.
sorry if it seems that way. But we have been testing several versions of this patch
internally. davidA has been throwing it at all of our internal tests just to make sure
it gets all the testing it needs before 4.6 goes out. This last fix was something
that I think got introduced in one of the later versions during re-implementing
bits of it based on feedback. And one of our new recent tests under stress
caught it and we rushed the fix out.

thanks,
Roopa

Re: [PATCH] net: ipv6: Delete host routes on an ifdown

[Mike Manning]

On 04/25/2016 11:03 PM, David Ahern wrote:
> On 4/25/16 2:42 PM, David Miller wrote:
>> From: David Ahern <dsa@cumulusnetworks.com>
>> Date: Mon, 25 Apr 2016 13:40:26 -0600
>>
>>> It's unfortunate you want to take that action. Last week I came across
>>> a prior attempt by Stephen to do this same thing -- keep IPv6
>>> addresses. That prior attempt was reverted by commit
>>> 73a8bd74e261. Cumulus, Brocade, and others clearly want this
>>> capability.
>>
>> But nobody has implemented it correctly, it doesn't matter who wants
>> the feature.  That's why it keeps getting reverted.
>>
>> Also, this testing you are talking about should have happened long
>> before you submitted that first patch that introduced all of these
>> regressions.  My observations tell me that the bulk of the testing
>> happened afterwards and that's why all the regressions are popping up
>> now.
>>
> 
> My testing when submitting the patch was host level: Add an address, while(1) (link up, link down), delete an address, etc.
> 
> Once it was committed to our kernel it started getting hit with a range of L3 deployment scenarios with many nodes and networking config files are uploaded and jumped between on real switch hardware - no reboot but 'networking reload' on the fly. Jumping between different deployments with different sets addresses, routes, vrf devices, bridges, bonds, etc.
> 
> Your objection seems to be 'all these regressions' but beyond the ref count from Andrey all of the bug reports have come from me with 1 from Mike, another invested party wanting this to happen. I am the one who spent the hours dealing with the kernel panics. My patch, my bug, my time wasted coming up with the delta patch. Rather than focusing on my mistakes, why not see the commitment on following through with this change?

It would be great if this could be reconsidered, also bearing in mind that any potential regressions do not have any impact with the default setting of keep_addr_on_down disabled. Or if not, to at least identify what the shortcomings of this solution are for future reference.

I confirm we have been using David's original patch for not flushing IPv6 addresses since it was submitted last year, as for routers it is unacceptable to have IPv6 addresses disappear on link down (although we can work around this to some extent).

When the revised patch and the immediate follow-up fix by David were recently merged for the 4.6 kernel, the only regression I found for ethernet interfaces by changing to the new fix was that local addresses were being retained on link down. This bug was only introduced as a result of a review comment, and David's subsequent fix avoided keeping local addrs (I suggested a complementary fix to avoid fixing them up, as a crash was observed without this in some cases).

Now with David's fix for a vulnerability with loopback interfaces in place and testing looking fine, it seems a shame to give up.

Re: [PATCH] net: ipv6: Delete host routes on an ifdown

[David Miller]

From: David Ahern <dsa@cumulusnetworks.com>
Date: Mon, 25 Apr 2016 16:03:40 -0600

> Rather than focusing on my mistakes, why not see the commitment on
> following through with this change?

I do not question the amount of time and effort invested.

I question whether the change was truly ready yet, and my
conclusion right now is that it is not ready for 4.6.0-final

So I reverted instead of waiting for the other shoe to drop.

Thanks.

Re: [PATCH] net: ipv6: Delete host routes on an ifdown

[David Miller]

From: Mike Manning <mmanning@brocade.com>
Date: Tue, 26 Apr 2016 01:57:20 +0100

> It would be great if this could be reconsidered

If you guys start ganging up on me, I will set you all to ignore.
This is my last warning.

Please respect my decision and try to shore up this change properly
for 4.7.0

Thank you.