Patchwork Fix 2.6.34-rc1 regression in disable_ipv6 support

login
register
mail settings
Submitter Brian Haley
Date Dec. 9, 2010, 3:28 p.m.
Message ID <4D00F58A.2050307@hp.com>
Download mbox | patch
Permalink /patch/74909/
State RFC
Delegated to: David Miller
Headers show

Comments

Brian Haley - Dec. 9, 2010, 3:28 p.m.
On 12/08/2010 11:16 PM, Eric W. Biederman wrote:
> Finding the real bug is beyond me right now, but fixing the regression
> in disable_ipv6 is simple.  We can just delete ::1 when we bring down
> the loopback interface, and it will be restored automatically when we
> bring the loopback interface back up.

Hi Eric,

This would work as well, same check, different way.

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

--
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
stephen hemminger - Dec. 9, 2010, 4:27 p.m.
On Thu, 09 Dec 2010 10:28:10 -0500
Brian Haley <brian.haley@hp.com> wrote:

> On 12/08/2010 11:16 PM, Eric W. Biederman wrote:
> > Finding the real bug is beyond me right now, but fixing the regression
> > in disable_ipv6 is simple.  We can just delete ::1 when we bring down
> > the loopback interface, and it will be restored automatically when we
> > bring the loopback interface back up.
> 
> Hi Eric,
> 
> This would work as well, same check, different way.
> 
> Signed-off-by: Brian Haley <brian.haley@hp.com>
> 
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index 23cc8e1..5d16a9d 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -2728,7 +2728,8 @@ static int addrconf_ifdown(struct net_device *dev, int how)
>  		   and not link-local, then retain it. */
>  		if (!how &&
>  		    (ifa->flags&IFA_F_PERMANENT) &&
> -		    !(ipv6_addr_type(&ifa->addr) & IPV6_ADDR_LINKLOCAL)) {
> +		    !(ipv6_addr_type(&ifa->addr) &
> +		      (IPV6_ADDR_LINKLOCAL|IPV6_ADDR_LOOPBACK))) {
>  			list_move_tail(&ifa->if_list, &keep_list);
>  
>  			/* If not doing DAD on this address, just keep it. */

Checking the address type is incorrect. Any type of address can be applied to
loopback interface. If you look a couple lines there is a special case for
loopback which keeps the address.

> 		/* If just doing link down, and address is permanent
> 		   and not link-local, then retain it. */
> 		if (!how &&
> 		    (ifa->flags&IFA_F_PERMANENT) &&
> 		    !(ipv6_addr_type(&ifa->addr) & IPV6_ADDR_LINKLOCAL)) {
> 			list_move_tail(&ifa->if_list, &keep_list);
> 
> 			/* If not doing DAD on this address, just keep it. */
> 			if ((dev->flags&(IFF_NOARP|IFF_LOOPBACK)) ||
> 			    idev->cnf.accept_dad <= 0 ||
> 			    (ifa->flags & IFA_F_NODAD))

I think the problem is on coming back up, not on the down step.

--
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
Eric W. Biederman - Dec. 9, 2010, 7:09 p.m.
Brian Haley <brian.haley@hp.com> writes:

> On 12/08/2010 11:16 PM, Eric W. Biederman wrote:
>> Finding the real bug is beyond me right now, but fixing the regression
>> in disable_ipv6 is simple.  We can just delete ::1 when we bring down
>> the loopback interface, and it will be restored automatically when we
>> bring the loopback interface back up.
>
> Hi Eric,
>
> This would work as well, same check, different way.

But that looks like less of an obvious magic exception.  The only
address it is safe to ignore on is ::1, because we always restore it
when we bring the loopback interface up.

Long term we really do want to keep the loopback address.  But
that actually requires finding and fixing what is broken in
ipv6.

So let's please keep this a line that we can easily remove.  It isn't
like interfaces coming up and down are a fast path where every cycle
counts.  We just need to be reasonably efficient.

Eric
--
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
stephen hemminger - Dec. 9, 2010, 7:16 p.m.
On Thu, 09 Dec 2010 11:09:34 -0800
ebiederm@xmission.com (Eric W. Biederman) wrote:

> Brian Haley <brian.haley@hp.com> writes:
> 
> > On 12/08/2010 11:16 PM, Eric W. Biederman wrote:
> >> Finding the real bug is beyond me right now, but fixing the regression
> >> in disable_ipv6 is simple.  We can just delete ::1 when we bring down
> >> the loopback interface, and it will be restored automatically when we
> >> bring the loopback interface back up.
> >
> > Hi Eric,
> >
> > This would work as well, same check, different way.
> 
> But that looks like less of an obvious magic exception.  The only
> address it is safe to ignore on is ::1, because we always restore it
> when we bring the loopback interface up.
> 
> Long term we really do want to keep the loopback address.  But
> that actually requires finding and fixing what is broken in
> ipv6.
> 
> So let's please keep this a line that we can easily remove.  It isn't
> like interfaces coming up and down are a fast path where every cycle
> counts.  We just need to be reasonably efficient.

No but since removing address propagates up to user space daemons
like Quagga please analyze and fix the problem, don't just look
for band aid.
--
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
Eric W. Biederman - Dec. 9, 2010, 7:22 p.m.
Stephen Hemminger <shemminger@vyatta.com> writes:

> On Thu, 09 Dec 2010 10:28:10 -0500
> Brian Haley <brian.haley@hp.com> wrote:
>
>> On 12/08/2010 11:16 PM, Eric W. Biederman wrote:

>> 		/* If just doing link down, and address is permanent
>> 		   and not link-local, then retain it. */
>> 		if (!how &&
>> 		    (ifa->flags&IFA_F_PERMANENT) &&
>> 		    !(ipv6_addr_type(&ifa->addr) & IPV6_ADDR_LINKLOCAL)) {
>> 			list_move_tail(&ifa->if_list, &keep_list);
>> 
>> 			/* If not doing DAD on this address, just keep it. */
>> 			if ((dev->flags&(IFF_NOARP|IFF_LOOPBACK)) ||
>> 			    idev->cnf.accept_dad <= 0 ||
>> 			    (ifa->flags & IFA_F_NODAD))
>
> I think the problem is on coming back up, not on the down step.

Oh it is.  All addresses that you keep break if you down the loopback
interface, no matter which interface those addresses are on.

Stephen the cause of the regression in 2.6.34-rc1 that you introduced
that breaks the disable_ipv6 functionality in practice is removing
the loopback address from the loopback interface.  So I sent
a partial revert.

It is safe to do a partial revert because the loopback address is always
reprogrammed when we bring the interface back up.  But that
reprogramming only works if it doesn't error out with -EEXIST.

So by all means properly fix the ancient bug that breaks usage of all
local ipv6 addresses when the loopback interface is brought down,
and we can remove the regression fix.

However complaining about a partial revert to fix a regression you
introduced because it fixes a problem deep within the ipv6 networking
stack that the smallest modicum of testing would have revealed on your
part before you broke things seems inappropriate.

Please let's get the disable_ipv6 functionality working again (where 
in practice we don't care about preserving addresses).  Then let's
take our time and tack and fix whatever this is properly.

Eric

--
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
Eric W. Biederman - Dec. 9, 2010, 7:31 p.m.
Stephen Hemminger <shemminger@vyatta.com> writes:

> On Thu, 09 Dec 2010 11:09:34 -0800
> ebiederm@xmission.com (Eric W. Biederman) wrote:
> So let's please keep this a line that we can easily remove.  It isn't
> like interfaces coming up and down are a fast path where every cycle
> counts.  We just need to be reasonably efficient.

> No but since removing address propagates up to user space daemons
> like Quagga please analyze and fix the problem, don't just look
> for band aid.

You fix the problem.  You introduced the regression, and you didn't test
that keeping addresses actually worked.  This is not the first patch
that has been applied to fix regressions in this area.

I introduced a targeted revert of your broken change that only preserves
the one address people are least likely to change.

I know people are not downing the ipv6 loopback interface in practice
and bringing it back up because in practice on running systems because
that breaks ipv6 networking.  So quagga should not see this issue
in practice.

Right now misplaced perfectionism is being a huge enemy of creating
a kernel that actually works.

Eric

--
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
David Miller - Dec. 9, 2010, 8:20 p.m.
From: Stephen Hemminger <shemminger@vyatta.com>
Date: Thu, 9 Dec 2010 11:16:11 -0800

> No but since removing address propagates up to user space daemons
> like Quagga please analyze and fix the problem, don't just look
> for band aid.

Stephen, we lived with the previous behavior for 12+ years.

You broke stuff that did work before your change.

Putting the onus on Eric to fix it exactly how you want it to
be fixed is therefore not appropriate.

You seem to be putting exactly zero effort into trying to reproduce
the problem yourself and fixing a bug you introduced.  And hey we
have a standard way to deal with a regression when the guilty party
is uncooperative, revert.

There are therefore three choices:

1) Revert.  And this is the one I'm favoring because of how you are
   handling this issue.  The responsibility to resolve this regression
   is your's not Eric's.

   Frankly, Eric is being incredibly nice by working on trying to fix
   a bug which you introduced.

2) Accept Eric's proposed fix.

3) Figure out the real bug yourself and fix the problem the way you
   find acceptable in a reasonable, short, amount of time.

Loopback has always been special, especially on ipv6.  When we don't
have a device to point something at, we point it at loopback.

Also destination cache entries which still have references when they
get zapped get pointed at loopback.
--
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
David Miller - Dec. 9, 2010, 8:20 p.m.
From: ebiederm@xmission.com (Eric W. Biederman)
Date: Thu, 09 Dec 2010 11:31:36 -0800

> You fix the problem.  You introduced the regression, and you didn't test
> that keeping addresses actually worked.  This is not the first patch
> that has been applied to fix regressions in this area.

Exactly, I fully agree with Eric.
--
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
stephen hemminger - Dec. 9, 2010, 10:51 p.m.
On Thu, 09 Dec 2010 12:20:33 -0800 (PST)
David Miller <davem@davemloft.net> wrote:

> From: Stephen Hemminger <shemminger@vyatta.com>
> Date: Thu, 9 Dec 2010 11:16:11 -0800
> 
> > No but since removing address propagates up to user space daemons
> > like Quagga please analyze and fix the problem, don't just look
> > for band aid.
> 
> Stephen, we lived with the previous behavior for 12+ years.
> 
> You broke stuff that did work before your change.
> 
> Putting the onus on Eric to fix it exactly how you want it to
> be fixed is therefore not appropriate.
> 
> You seem to be putting exactly zero effort into trying to reproduce
> the problem yourself and fixing a bug you introduced.  And hey we
> have a standard way to deal with a regression when the guilty party
> is uncooperative, revert.
> 
> There are therefore three choices:
> 
> 1) Revert.  And this is the one I'm favoring because of how you are
>    handling this issue.  The responsibility to resolve this regression
>    is your's not Eric's.
> 
>    Frankly, Eric is being incredibly nice by working on trying to fix
>    a bug which you introduced.
> 
> 2) Accept Eric's proposed fix.
> 
> 3) Figure out the real bug yourself and fix the problem the way you
>    find acceptable in a reasonable, short, amount of time.
> 
> Loopback has always been special, especially on ipv6.  When we don't
> have a device to point something at, we point it at loopback.
> 
> Also destination cache entries which still have references when they
> get zapped get pointed at loopback.

Quit being a grinch. I am working on it, just don't know the answer.
I want to try a couple solutions, so far Eric's looks okay, just want
to make sure that it doesn't break anything.

You are over reacting. Doing on the fly re-enabling of ipv6 is a corner case.
The problem was only discovered a couple of days ago, it is not like
the world is burning down.
stephen hemminger - Dec. 10, 2010, 4:02 a.m.
On Thu, 09 Dec 2010 10:28:10 -0500
Brian Haley <brian.haley@hp.com> wrote:

> On 12/08/2010 11:16 PM, Eric W. Biederman wrote:
> > Finding the real bug is beyond me right now, but fixing the regression
> > in disable_ipv6 is simple.  We can just delete ::1 when we bring down
> > the loopback interface, and it will be restored automatically when we
> > bring the loopback interface back up.
> 
> Hi Eric,
> 
> This would work as well, same check, different way.
> 
> Signed-off-by: Brian Haley <brian.haley@hp.com>
> 
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index 23cc8e1..5d16a9d 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -2728,7 +2728,8 @@ static int addrconf_ifdown(struct net_device *dev, int how)
>  		   and not link-local, then retain it. */
>  		if (!how &&
>  		    (ifa->flags&IFA_F_PERMANENT) &&
> -		    !(ipv6_addr_type(&ifa->addr) & IPV6_ADDR_LINKLOCAL)) {
> +		    !(ipv6_addr_type(&ifa->addr) &
> +		      (IPV6_ADDR_LINKLOCAL|IPV6_ADDR_LOOPBACK))) {
>  			list_move_tail(&ifa->if_list, &keep_list);
>  
>  			/* If not doing DAD on this address, just keep it. */

This patch is cleaner but still fails for the non ::1 address case.

 # ip addr add 2000:db8::1/64 dev lo
 # ping6 ::1                                 -- works
 # ping6 2000:db8::1                         --works
 # ip li set dev lo down
 # ping6 ::1                                 -- does not work (expected)
 # ping6 2000:db8::1                         -- ditto
 # ip il set dev lo up
 # ping6 ::1                                 -- works (good)
 # ping6 2000:db8::1                         -- fails (network unreachable)


Looks like connected route is not getting restored correctly, probably
because loopback has NOARP flag set. 

The problem may not be true for just loopback, it may also apply to
tunnels.
--
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

Patch

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 23cc8e1..5d16a9d 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -2728,7 +2728,8 @@  static int addrconf_ifdown(struct net_device *dev, int how)
 		   and not link-local, then retain it. */
 		if (!how &&
 		    (ifa->flags&IFA_F_PERMANENT) &&
-		    !(ipv6_addr_type(&ifa->addr) & IPV6_ADDR_LINKLOCAL)) {
+		    !(ipv6_addr_type(&ifa->addr) &
+		      (IPV6_ADDR_LINKLOCAL|IPV6_ADDR_LOOPBACK))) {
 			list_move_tail(&ifa->if_list, &keep_list);
 
 			/* If not doing DAD on this address, just keep it. */