echo > 0 .../disable_ipv6 broken in 2.6.37-rc4

echo > 0 .../disable_ipv6 broken in 2.6.37-rc4

[Eric W. Biederman]

In 2.6.37-rc4 ipv6 can be disabled not enabled.
The last kernel I have tested and know this works on is 2.6.33.

To reproduce:
   ~ # ip link set lo up
   ~ # ping6 ::1
   PING ::1(::1) 56 data bytes
   64 bytes from ::1: icmp_seq=1 ttl=64 time=0.026 ms
   ^C
   --- ::1 ping statistics ---
   1 packets transmitted, 1 received, 0% packet loss, time 782ms
   rtt min/avg/max/mdev = 0.026/0.026/0.026/0.000 ms
   ~ # echo 1 > /proc/sys/net/ipv6/conf/lo/disable_ipv6 
   ~ # ping6 ::1
   connect: Network is unreachable
   ~ # echo 0 > /proc/sys/net/ipv6/conf/lo/disable_ipv6 
   ~ # ping6 ::1
   connect: Network is unreachable


I intend to poke at this a little more but at the moment
I am drawing a blank at what is going on.

I have tried reverting the last change to the ipv6 logic
and that doesn't make a difference.

   commit 64e724f62ab743d55229cd5e27ec8b068b68eb16
   Author: Brian Haley <brian.haley@hp.com>
   Date:   Tue Jul 20 10:34:30 2010 +0000
   
       ipv6: Don't add routes to ipv6 disabled interfaces.
       
       If the interface has IPv6 disabled, don't add a multicast or
       link-local route since we won't be adding a link-local address.
       
       Reported-by: Mahesh Kelkar <maheshkelkar@gmail.com>
       Signed-off-by: Brian Haley <brian.haley@hp.com>
       Signed-off-by: David S. Miller <davem@davemloft.net>

I intend to keep poking at this but if anyone can figure this out
before I do I would be greatly appreciative.

Eric

Re: echo > 0 .../disable_ipv6 broken in 2.6.37-rc4

[Lorenzo Colitti]

On Sun, Dec 5, 2010 at 4:24 PM, Eric W. Biederman <ebiederm@xmission.com> wrote:
> In 2.6.37-rc4 ipv6 can be disabled not enabled.
> The last kernel I have tested and know this works on is 2.6.33.

I'm pretty sure I could successfully re-enable IPv6 on non-loopback
interfaces (wlan0, eth0) on net-2.6 pulled a little after 2.6.37-rc1.
I didn't try lo though. Does re-enabling IPv6 on a "real" interface
still work?

Re: echo > 0 .../disable_ipv6 broken in 2.6.37-rc4

[Eric W. Biederman]

Lorenzo Colitti <lorenzo@google.com> writes:

> On Sun, Dec 5, 2010 at 4:24 PM, Eric W. Biederman <ebiederm@xmission.com> wrote:
>> In 2.6.37-rc4 ipv6 can be disabled not enabled.
>> The last kernel I have tested and know this works on is 2.6.33.
>
> I'm pretty sure I could successfully re-enable IPv6 on non-loopback
> interfaces (wlan0, eth0) on net-2.6 pulled a little after 2.6.37-rc1.
> I didn't try lo though. Does re-enabling IPv6 on a "real" interface
> still work?

Interesting that case seems to work.

Eric

Re: echo > 0 .../disable_ipv6 broken in 2.6.37-rc4

[Eric W. Biederman]

ebiederm@xmission.com (Eric W. Biederman) writes:

> Lorenzo Colitti <lorenzo@google.com> writes:
>
>> On Sun, Dec 5, 2010 at 4:24 PM, Eric W. Biederman <ebiederm@xmission.com> wrote:
>>> In 2.6.37-rc4 ipv6 can be disabled not enabled.
>>> The last kernel I have tested and know this works on is 2.6.33.
>>
>> I'm pretty sure I could successfully re-enable IPv6 on non-loopback
>> interfaces (wlan0, eth0) on net-2.6 pulled a little after 2.6.37-rc1.
>> I didn't try lo though. Does re-enabling IPv6 on a "real" interface
>> still work?
>
> Interesting that case seems to work.


It isn't the address state remove on ifdown either.

Lorenzo reverting your patch 2de795707294972f6c34bae9de713e502c431296
has no affect so you are cleared.

Darn those were the easy possibilities, now I have to stop and actually
pay attention to what the code is doing to debug this.

Eric

Re: echo > 0 .../disable_ipv6 broken in 2.6.37-rc4

[Brian Haley]

On 12/05/2010 07:24 PM, Eric W. Biederman wrote:
> 
> In 2.6.37-rc4 ipv6 can be disabled not enabled.
> The last kernel I have tested and know this works on is 2.6.33.
> 
> To reproduce:
>    ~ # ip link set lo up
>    ~ # ping6 ::1
>    PING ::1(::1) 56 data bytes
>    64 bytes from ::1: icmp_seq=1 ttl=64 time=0.026 ms
>    ^C
>    --- ::1 ping statistics ---
>    1 packets transmitted, 1 received, 0% packet loss, time 782ms
>    rtt min/avg/max/mdev = 0.026/0.026/0.026/0.000 ms
>    ~ # echo 1 > /proc/sys/net/ipv6/conf/lo/disable_ipv6 
>    ~ # ping6 ::1
>    connect: Network is unreachable
>    ~ # echo 0 > /proc/sys/net/ipv6/conf/lo/disable_ipv6 
>    ~ # ping6 ::1
>    connect: Network is unreachable
> 
> 
> I intend to poke at this a little more but at the moment
> I am drawing a blank at what is going on.

It should just be calling addrconf_notify() with either NETDEV_UP
or NETDEV_DOWN.  Does the address not come back?  Or the route?

> I intend to keep poking at this but if anyone can figure this out
> before I do I would be greatly appreciative.

I'm pulling the latest tree now, my 2.6.32.24 system is running fine, so
it's something after that.

-Brian

Re: echo > 0 .../disable_ipv6 broken in 2.6.37-rc4

[Eric W. Biederman]

Brian Haley <brian.haley@hp.com> writes:

> On 12/05/2010 07:24 PM, Eric W. Biederman wrote:
>> 
>> In 2.6.37-rc4 ipv6 can be disabled not enabled.
>> The last kernel I have tested and know this works on is 2.6.33.
>> 
>> To reproduce:
>>    ~ # ip link set lo up
>>    ~ # ping6 ::1
>>    PING ::1(::1) 56 data bytes
>>    64 bytes from ::1: icmp_seq=1 ttl=64 time=0.026 ms
>>    ^C
>>    --- ::1 ping statistics ---
>>    1 packets transmitted, 1 received, 0% packet loss, time 782ms
>>    rtt min/avg/max/mdev = 0.026/0.026/0.026/0.000 ms
>>    ~ # echo 1 > /proc/sys/net/ipv6/conf/lo/disable_ipv6 
>>    ~ # ping6 ::1
>>    connect: Network is unreachable
>>    ~ # echo 0 > /proc/sys/net/ipv6/conf/lo/disable_ipv6 
>>    ~ # ping6 ::1
>>    connect: Network is unreachable
>> 
>> 
>> I intend to poke at this a little more but at the moment
>> I am drawing a blank at what is going on.
>
> It should just be calling addrconf_notify() with either NETDEV_UP
> or NETDEV_DOWN.  Does the address not come back?  Or the route?

The address never went away, and I don't think we have a route to ::1.

Playing with this a little more if I delete the address and then bounce
the interface ping ::1 works again.  So something is just not getting
reinitialized.

Unfortunately I don't see anything obvious.  I'm still scratching my
head.

>> I intend to keep poking at this but if anyone can figure this out
>> before I do I would be greatly appreciative.
>
> I'm pulling the latest tree now, my 2.6.32.24 system is running fine, so
> it's something after that.

Agreed.  I don't have problems on 2.6.33 either, but because of overload
I haven't been doing regular testing of the kernels inbetween.

Eric

Re: echo > 0 .../disable_ipv6 broken in 2.6.37-rc4

[Brian Haley]

On 12/08/2010 04:29 PM, Eric W. Biederman wrote:
> Brian Haley <brian.haley@hp.com> writes:
> 
>> On 12/05/2010 07:24 PM, Eric W. Biederman wrote:
>>>
>>> In 2.6.37-rc4 ipv6 can be disabled not enabled.
>>> The last kernel I have tested and know this works on is 2.6.33.
>>>
>>> To reproduce:
>>>    ~ # ip link set lo up
>>>    ~ # ping6 ::1
>>>    PING ::1(::1) 56 data bytes
>>>    64 bytes from ::1: icmp_seq=1 ttl=64 time=0.026 ms
>>>    ^C
>>>    --- ::1 ping statistics ---
>>>    1 packets transmitted, 1 received, 0% packet loss, time 782ms
>>>    rtt min/avg/max/mdev = 0.026/0.026/0.026/0.000 ms
>>>    ~ # echo 1 > /proc/sys/net/ipv6/conf/lo/disable_ipv6 
>>>    ~ # ping6 ::1
>>>    connect: Network is unreachable
>>>    ~ # echo 0 > /proc/sys/net/ipv6/conf/lo/disable_ipv6 
>>>    ~ # ping6 ::1
<snip>

>> I'm pulling the latest tree now, my 2.6.32.24 system is running fine, so
>> it's something after that.
> 
> Agreed.  I don't have problems on 2.6.33 either, but because of overload
> I haven't been doing regular testing of the kernels inbetween.

This got broken in 2.6.34-rc1, and the most obvious culprit is this,
although I haven't bisected it:

commit dc2b99f71ef477a31020511876ab4403fb7c4420
Author: stephen hemminger <shemminger@vyatta.com>
Date:   Mon Feb 8 19:48:05 2010 +0000

    IPv6: keep permanent addresses on admin down

    Permanent IPV6 addresses should not be removed when the link is
    set to admin down, only when device is removed.

    When link is lost permanent addresses should be marked as tentative
    so that when link comes back they are subject to duplicate address
    detection (if DAD was enabled for that address).

    Other routing systems keep manually configured IPv6 addresses
    when link is set down.

Even though there was a bugfix update, it didn't help.

I unfortunately won't be able to look at this more until at least Friday,
I couldn't come up with a quick patch just looking quickly at addrconf_ifdown().

-Brian

Re: echo > 0 .../disable_ipv6 broken in 2.6.37-rc4

[Eric W. Biederman]

Brian Haley <brian.haley@hp.com> writes:

> This got broken in 2.6.34-rc1, and the most obvious culprit is this,
> although I haven't bisected it:
>
> commit dc2b99f71ef477a31020511876ab4403fb7c4420
> Author: stephen hemminger <shemminger@vyatta.com>
> Date:   Mon Feb 8 19:48:05 2010 +0000
>
>     IPv6: keep permanent addresses on admin down
>
>     Permanent IPV6 addresses should not be removed when the link is
>     set to admin down, only when device is removed.
>
>     When link is lost permanent addresses should be marked as tentative
>     so that when link comes back they are subject to duplicate address
>     detection (if DAD was enabled for that address).
>
>     Other routing systems keep manually configured IPv6 addresses
>     when link is set down.
>
> Even though there was a bugfix update, it didn't help.
>
> I unfortunately won't be able to look at this more until at least Friday,
> I couldn't come up with a quick patch just looking quickly at
> addrconf_ifdown().

This is almost certainly it.
ip link set lo down
ip link set lo up

Is enough to break ping6 ::1.

I get the feeling the loopback address was not actually tested and
there is something different about it.

Eric

Re: echo > 0 .../disable_ipv6 broken in 2.6.37-rc4

[Stephen Hemminger]

On Wed, 08 Dec 2010 15:13:30 -0800
ebiederm@xmission.com (Eric W. Biederman) wrote:

> Brian Haley <brian.haley@hp.com> writes:
> 
> > This got broken in 2.6.34-rc1, and the most obvious culprit is this,
> > although I haven't bisected it:
> >
> > commit dc2b99f71ef477a31020511876ab4403fb7c4420
> > Author: stephen hemminger <shemminger@vyatta.com>
> > Date:   Mon Feb 8 19:48:05 2010 +0000
> >
> >     IPv6: keep permanent addresses on admin down
> >
> >     Permanent IPV6 addresses should not be removed when the link is
> >     set to admin down, only when device is removed.
> >
> >     When link is lost permanent addresses should be marked as tentative
> >     so that when link comes back they are subject to duplicate address
> >     detection (if DAD was enabled for that address).
> >
> >     Other routing systems keep manually configured IPv6 addresses
> >     when link is set down.
> >
> > Even though there was a bugfix update, it didn't help.
> >
> > I unfortunately won't be able to look at this more until at least Friday,
> > I couldn't come up with a quick patch just looking quickly at
> > addrconf_ifdown().
> 
> This is almost certainly it.
> ip link set lo down
> ip link set lo up
> 
> Is enough to break ping6 ::1.
> 
> I get the feeling the loopback address was not actually tested and
> there is something different about it.

Loopback is already handled as special case in addrconf. Look at IFF_LOOPBACK
perhaps there is a logic error there.


-- 

Re: echo > 0 .../disable_ipv6 broken in 2.6.37-rc4

[Eric W. Biederman]

Stephen Hemminger <shemminger@vyatta.com> writes:

> On Wed, 08 Dec 2010 15:13:30 -0800
> ebiederm@xmission.com (Eric W. Biederman) wrote:
>
>> Brian Haley <brian.haley@hp.com> writes:
>> 
>> > This got broken in 2.6.34-rc1, and the most obvious culprit is this,
>> > although I haven't bisected it:
>> >
>> > commit dc2b99f71ef477a31020511876ab4403fb7c4420
>> > Author: stephen hemminger <shemminger@vyatta.com>
>> > Date:   Mon Feb 8 19:48:05 2010 +0000
>> >
>> >     IPv6: keep permanent addresses on admin down
>> >
>> >     Permanent IPV6 addresses should not be removed when the link is
>> >     set to admin down, only when device is removed.
>> >
>> >     When link is lost permanent addresses should be marked as tentative
>> >     so that when link comes back they are subject to duplicate address
>> >     detection (if DAD was enabled for that address).
>> >
>> >     Other routing systems keep manually configured IPv6 addresses
>> >     when link is set down.
>> >
>> > Even though there was a bugfix update, it didn't help.
>> >
>> > I unfortunately won't be able to look at this more until at least Friday,
>> > I couldn't come up with a quick patch just looking quickly at
>> > addrconf_ifdown().
>> 
>> This is almost certainly it.
>> ip link set lo down
>> ip link set lo up
>> 
>> Is enough to break ping6 ::1.
>> 
>> I get the feeling the loopback address was not actually tested and
>> there is something different about it.
>
> Loopback is already handled as special case in addrconf. Look at IFF_LOOPBACK
> perhaps there is a logic error there.

Playing with this some more I have come up with a totally evil testcase.

If I bring lo down, and back up again I cannot talk to local addresses
through the loopback device, that I could before.

Somewhere in route flushing, and route cache something has gone wrong,
making configurations that existed before the loopback interface was
brought down not work after the loopback interface is brought up.

I can fix any this problem by deleting and re-adding the address
but seems wrong, and this does seem related to the loopback case.

A nondad address goes up and down just fine on any interface I
want except on lo.  And pinging any local address seems to break
when I bring lo up and down.

Eric


# ip link set lo up
# ip link add veth0 type veth peer name veth1
# ip link set veth0 up
# ip link set veth1 up
# ip -6 addr add fdff:ffff:ffff:ffff::1/128 dev veth0 nodad
# ping6 fdff:ffff:ffff:ffff::1    
PING fdff:ffff:ffff:ffff::1(fdff:ffff:ffff:ffff::1) 56 data bytes
64 bytes from fdff:ffff:ffff:ffff::1: icmp_seq=1 ttl=64 time=0.039 ms
64 bytes from fdff:ffff:ffff:ffff::1: icmp_seq=2 ttl=64 time=0.046 ms
^C
--- fdff:ffff:ffff:ffff::1 ping statistics ---
2 packets transmitted, 2 received, 0% packet loss, time 1466ms
rtt min/avg/max/mdev = 0.039/0.042/0.046/0.007 ms
# ip link set lo down
# ip link set lo up
# ping6 fdff:ffff:ffff:ffff::1
PING fdff:ffff:ffff:ffff::1(fdff:ffff:ffff:ffff::1) 56 data bytes
ping: sendmsg: Network is unreachable
ping: sendmsg: Network is unreachable
ping: sendmsg: Network is unreachable
ping: sendmsg: Network is unreachable
ping: sendmsg: Network is unreachable
ping: sendmsg: Network is unreachable
ping: sendmsg: Network is unreachable
^C
--- fdff:ffff:ffff:ffff::1 ping statistics ---
10 packets transmitted, 0 received, 100% packet loss, time 9250ms


# ip -6 addr del fdff:ffff:ffff:ffff::1/128 dev veth0 nodad
# ip -6 addr add fdff:ffff:ffff:ffff::1/128 dev veth0 nodad
# ping6 fdff:ffff:ffff:ffff::1
PING fdff:ffff:ffff:ffff::1(fdff:ffff:ffff:ffff::1) 56 data bytes
64 bytes from fdff:ffff:ffff:ffff::1: icmp_seq=1 ttl=64 time=0.032 ms
64 bytes from fdff:ffff:ffff:ffff::1: icmp_seq=2 ttl=64 time=0.060 ms
^C
--- fdff:ffff:ffff:ffff::1 ping statistics ---
2 packets transmitted, 2 received, 0% packet loss, time 1002ms
rtt min/avg/max/mdev = 0.032/0.046/0.060/0.014 ms
# ip link set veth0 down
# ip link set veth0 up
# ping6 fdff:ffff:ffff:ffff::1
PING fdff:ffff:ffff:ffff::1(fdff:ffff:ffff:ffff::1) 56 data bytes
64 bytes from fdff:ffff:ffff:ffff::1: icmp_seq=1 ttl=64 time=0.034 ms
64 bytes from fdff:ffff:ffff:ffff::1: icmp_seq=2 ttl=64 time=0.052 ms
64 bytes from fdff:ffff:ffff:ffff::1: icmp_seq=3 ttl=64 time=0.062 ms
64 bytes from fdff:ffff:ffff:ffff::1: icmp_seq=4 ttl=64 time=0.066 ms
64 bytes from fdff:ffff:ffff:ffff::1: icmp_seq=5 ttl=64 time=0.066 ms
^C
--- fdff:ffff:ffff:ffff::1 ping statistics ---
5 packets transmitted, 5 received, 0% packet loss, time 4045ms
rtt min/avg/max/mdev = 0.034/0.056/0.066/0.012 ms
# ip link set lo down
# ip link set lo up
# ping6 fdff:ffff:ffff:ffff::1
connect: Network is unreachable
# ping6 fdff:ffff:ffff:ffff::1
connect: Network is unreachable
# ip -6 addr show
13: lo: <LOOPBACK,UP,LOWER_UP> mtu 16436 
    inet6 ::1/128 scope host 
       valid_lft forever preferred_lft forever
15: veth1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qlen 1000
    inet6 fe80::c403:15ff:fe12:f05c/64 scope link 
       valid_lft forever preferred_lft forever
16: veth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qlen 1000
    inet6 fdff:ffff:ffff:ffff::1/128 scope global nodad 
       valid_lft forever preferred_lft forever
    inet6 fe80::c01b:5ff:fe39:510b/64 scope link 
       valid_lft forever preferred_lft forever
# ip link show
13: lo: <LOOPBACK,UP,LOWER_UP> mtu 16436 qdisc noqueue state UNKNOWN 
    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
15: veth1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast state UP qlen 1000
    link/ether c6:03:15:12:f0:5c brd ff:ff:ff:ff:ff:ff
16: veth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast state UP qlen 1000
    link/ether c2:1b:05:39:51:0b brd ff:ff:ff:ff:ff:ff

Re: echo > 0 .../disable_ipv6 broken in 2.6.37-rc4

[Eric W. Biederman]

ebiederm@xmission.com (Eric W. Biederman) writes:

> Playing with this some more I have come up with a totally evil testcase.
>
> If I bring lo down, and back up again I cannot talk to local addresses
> through the loopback device, that I could before.

I just tested 2.6.32.23 and I cannot ping local ipv6 addresses after
the loopback interface is brought administratively down and back up.

So it looks like we have some old bugs getting in the way an recent
changes that are reasonable.  I don't expect that it is by design
that bouncing the loopback interface breaks the ipv6 stack.

Eric

[PATCH] Fix 2.6.34-rc1 regression in disable_ipv6 support

[Eric W. Biederman]

In a number of instances it is desirable to have systems that run with
ipv6 disabled, but where enabling ipv6 remains an option.  This
has been broken since 2.6.34-rc1.

The problem is failure mode is that after
# ip link set lo up
# echo 1 > /proc/sys/net/ipv6/conf/lo/disable_ipv6
# echo 0 > /proc/sys/net/ipv6/conf/lo/disable_ipv6
# ping6 ::1

The ping and anything similar operations that use the
ipv6 loopback address fail with network not reachable.

This failure mode appears to start with:
   commit dc2b99f71ef477a31020511876ab4403fb7c4420
   Author: stephen hemminger <shemminger@vyatta.com>
   Date:   Mon Feb 8 19:48:05 2010 +0000
   
       IPv6: keep permanent addresses on admin down
       
       Permanent IPV6 addresses should not be removed when the link is
       set to admin down, only when device is removed.
       
       When link is lost permanent addresses should be marked as tentative
       so that when link comes back they are subject to duplicate address
       detection (if DAD was enabled for that address).
       
       Other routing systems keep manually configured IPv6 addresses
       when link is set down.
       
       Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
       Signed-off-by: David S. Miller <davem@davemloft.net>

Keeping ipv6 addresses on network interfaces when we bring those
interfaces down is not the fundamental problem.  The real problem is
that there is something about routes and route-caches that make local
ipv6 address unreachable after the loopback interface is brought down,
and then brought up again, and it has been that way since at least
2.6.32.

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.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
Index: linux-2.6.37-rc5.x86_64/net/ipv6/addrconf.c
===================================================================
--- linux-2.6.37-rc5.x86_64.orig/net/ipv6/addrconf.c
+++ linux-2.6.37-rc5.x86_64/net/ipv6/addrconf.c
@@ -2727,6 +2727,7 @@ static int addrconf_ifdown(struct net_de
 		/* If just doing link down, and address is permanent
 		   and not link-local, then retain it. */
 		if (!how &&
+		    !ipv6_addr_loopback(&ifa->addr) &&
 		    (ifa->flags&IFA_F_PERMANENT) &&
 		    !(ipv6_addr_type(&ifa->addr) & IPV6_ADDR_LINKLOCAL)) {
 			list_move_tail(&ifa->if_list, &keep_list);

Re: [PATCH] Fix 2.6.34-rc1 regression in disable_ipv6 support

[Brian Haley]

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. */

Re: [PATCH] Fix 2.6.34-rc1 regression in disable_ipv6 support

[Stephen Hemminger]

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.

Re: [PATCH] Fix 2.6.34-rc1 regression in disable_ipv6 support

[Eric W. Biederman]

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

Re: [PATCH] Fix 2.6.34-rc1 regression in disable_ipv6 support

[Eric W. Biederman]

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

Re: [PATCH] Fix 2.6.34-rc1 regression in disable_ipv6 support

[Stephen Hemminger]

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.

Re: [PATCH] Fix 2.6.34-rc1 regression in disable_ipv6 support

[Eric W. Biederman]

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

Re: [PATCH] Fix 2.6.34-rc1 regression in disable_ipv6 support

[David Miller]

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.

Re: [PATCH] Fix 2.6.34-rc1 regression in disable_ipv6 support

[David Miller]

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.

Re: [PATCH] Fix 2.6.34-rc1 regression in disable_ipv6 support

[Stephen Hemminger]

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.


-- 

[RFC] ipv6: don't flush routes when setting loopback down

[Stephen Hemminger]

When loopback device is being brought down, then keep the route table
entries because they are special. The entries in the local table for
linklocal routes and ::1 address should not be purged.

This is a sub optimal solution to the problem and should be replaced
by a better fix in future.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

---
patch versus current net-next tree, but if this acceptable
it should be applied to net-2.6 as well.

--- a/net/ipv6/addrconf.c	2010-12-16 10:29:34.035408392 -0800
+++ b/net/ipv6/addrconf.c	2010-12-16 10:30:37.366834482 -0800
@@ -2669,7 +2669,9 @@ static int addrconf_ifdown(struct net_de
 
 	ASSERT_RTNL();
 
-	rt6_ifdown(net, dev);
+	/* Flush routes if device is being removed or it is not loopback */
+	if (how || !(dev->flags & IFF_LOOPBACK))
+		rt6_ifdown(net, dev);
 	neigh_ifdown(&nd_tbl, dev);
 
 	idev = __in6_dev_get(dev);

Re: [RFC] ipv6: don't flush routes when setting loopback down

[Eric W. Biederman]

Stephen Hemminger <shemminger@vyatta.com> writes:

> When loopback device is being brought down, then keep the route table
> entries because they are special. The entries in the local table for
> linklocal routes and ::1 address should not be purged.
>
> This is a sub optimal solution to the problem and should be replaced
> by a better fix in future.

I will test this and let you know how it goes from my side.

I would really like to get a fix for this in before -rc7,
as this bug makes the kernel unusable for me.

Eric

Re: [RFC] ipv6: don't flush routes when setting loopback down

[Eric W. Biederman]

Stephen Hemminger <shemminger@vyatta.com> writes:

> When loopback device is being brought down, then keep the route table
> entries because they are special. The entries in the local table for
> linklocal routes and ::1 address should not be purged.
>
> This is a sub optimal solution to the problem and should be replaced
> by a better fix in future.
>
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

Stephen thanks for this.  This patch looks good to me.  I just tested
this against 2.6.37-rc6 and my simple tests show it to be working
without problems.

Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>


> ---
> patch versus current net-next tree, but if this acceptable
> it should be applied to net-2.6 as well.
>
> --- a/net/ipv6/addrconf.c	2010-12-16 10:29:34.035408392 -0800
> +++ b/net/ipv6/addrconf.c	2010-12-16 10:30:37.366834482 -0800
> @@ -2669,7 +2669,9 @@ static int addrconf_ifdown(struct net_de
>  
>  	ASSERT_RTNL();
>  
> -	rt6_ifdown(net, dev);
> +	/* Flush routes if device is being removed or it is not loopback */
> +	if (how || !(dev->flags & IFF_LOOPBACK))
> +		rt6_ifdown(net, dev);
>  	neigh_ifdown(&nd_tbl, dev);
>  
>  	idev = __in6_dev_get(dev);

Re: [RFC] ipv6: don't flush routes when setting loopback down

[David Miller]

From: ebiederm@xmission.com (Eric W. Biederman)
Date: Thu, 16 Dec 2010 17:18:13 -0800

> Stephen Hemminger <shemminger@vyatta.com> writes:
> 
>> When loopback device is being brought down, then keep the route table
>> entries because they are special. The entries in the local table for
>> linklocal routes and ::1 address should not be purged.
>>
>> This is a sub optimal solution to the problem and should be replaced
>> by a better fix in future.
>>
>> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> 
> Stephen thanks for this.  This patch looks good to me.  I just tested
> this against 2.6.37-rc6 and my simple tests show it to be working
> without problems.
> 
> Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>

Applied to net-2.6, thanks everyone.

Re: [RFC] ipv6: don't flush routes when setting loopback down

[Jiri Bohac]

Hi,


The commit (29ba5fed1bbd09c2cba890798c8f9eaab251401d) causes
another regression:

Prior to the commit, on a freshly booted system, when I do:
	sysctl net.ipv6.conf.all.disable_ipv6=1
Then any attempt to connect to ::1 will fail immediately with
"Network is unreachable" (e.g. "ping6 ::1" or "telnet ::1 22".

After the commit, doing
	sysctl net.ipv6.conf.all.disable_ipv6=1
makes connection attempts to ::1 wait for a long time before they fail.

This is caused by the local route which is now left configured. 
"ip -6 r l table all" has an additional line in its output:
	local ::1 via :: dev lo  table local  proto none  metric 0  mtu 16436 rtt 40ms rttvar 40ms cwnd 3 advmss 16376 hoplimit 0

With both ::1 and 127.0.0.1 specified for localhost in
/etc/hosts, disabling ipv6 now breaks many applications
connecting to localhost. Deleting the local route manually solves
the problems.

Could this be reverted, please?

I have the feeling that Eric's patch is the safest solution we
have so far:

> 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.
> 
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> ---
> Index: linux-2.6.37-rc5.x86_64/net/ipv6/addrconf.c
> ===================================================================
> --- linux-2.6.37-rc5.x86_64.orig/net/ipv6/addrconf.c
> +++ linux-2.6.37-rc5.x86_64/net/ipv6/addrconf.c
> @@ -2727,6 +2727,7 @@ static int addrconf_ifdown(struct net_de
>  		/* If just doing link down, and address is permanent
>  		   and not link-local, then retain it. */
>  		if (!how &&
> +		    !ipv6_addr_loopback(&ifa->addr) &&
>  		    (ifa->flags&IFA_F_PERMANENT) &&
>  		    !(ipv6_addr_type(&ifa->addr) & IPV6_ADDR_LINKLOCAL)) {
>  			list_move_tail(&ifa->if_list, &keep_list);

-- 
Jiri Bohac <jbohac@suse.cz>
SUSE Labs, SUSE CZ

Re: [RFC] ipv6: don't flush routes when setting loopback down

[Stephen Hemminger]

On Wed, 19 Jan 2011 20:18:23 +0100
Jiri Bohac <jbohac@suse.cz> wrote:

> Hi,
> 
> 
> The commit (29ba5fed1bbd09c2cba890798c8f9eaab251401d) causes
> another regression:
> 
> Prior to the commit, on a freshly booted system, when I do:
> 	sysctl net.ipv6.conf.all.disable_ipv6=1
> Then any attempt to connect to ::1 will fail immediately with
> "Network is unreachable" (e.g. "ping6 ::1" or "telnet ::1 22".
> 
> After the commit, doing
> 	sysctl net.ipv6.conf.all.disable_ipv6=1
> makes connection attempts to ::1 wait for a long time before they fail.
> 
> This is caused by the local route which is now left configured. 
> "ip -6 r l table all" has an additional line in its output:
> 	local ::1 via :: dev lo  table local  proto none  metric 0  mtu 16436 rtt 40ms rttvar 40ms cwnd 3 advmss 16376 hoplimit 0
> 
> With both ::1 and 127.0.0.1 specified for localhost in
> /etc/hosts, disabling ipv6 now breaks many applications
> connecting to localhost. Deleting the local route manually solves
> the problems.
> 
> Could this be reverted, please?
> 
> I have the feeling that Eric's patch is the safest solution we
> have so far:
> 
> > 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.
> > 
> > Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
> > ---
> > Index: linux-2.6.37-rc5.x86_64/net/ipv6/addrconf.c
> > ===================================================================
> > --- linux-2.6.37-rc5.x86_64.orig/net/ipv6/addrconf.c
> > +++ linux-2.6.37-rc5.x86_64/net/ipv6/addrconf.c
> > @@ -2727,6 +2727,7 @@ static int addrconf_ifdown(struct net_de
> >  		/* If just doing link down, and address is permanent
> >  		   and not link-local, then retain it. */
> >  		if (!how &&
> > +		    !ipv6_addr_loopback(&ifa->addr) &&
> >  		    (ifa->flags&IFA_F_PERMANENT) &&
> >  		    !(ipv6_addr_type(&ifa->addr) & IPV6_ADDR_LINKLOCAL)) {
> >  			list_move_tail(&ifa->if_list, &keep_list);
> 

Eric's patch has other regressions, see the discussion.

Re: [RFC] ipv6: don't flush routes when setting loopback down

[Jiri Bohac]

On Wed, Jan 19, 2011 at 11:38:17AM -0800, Stephen Hemminger wrote:
> Jiri Bohac <jbohac@suse.cz> wrote:
> > I have the feeling that Eric's patch is the safest solution we
> > have so far:
> Eric's patch has other regressions, see the discussion.

What regression do you mean? I have read the whole discussion
thoroughly. You only say in one message that deleting ::1 would
propagate to routing daemons. And Eric correctly stated that
people couldn't hit this, because  deleting ::1 would break
things on its own.

Is there a real problem with Eric's fix?

Thanks,

-- 
Jiri Bohac <jbohac@suse.cz>
SUSE Labs, SUSE CZ

Re: [RFC] ipv6: don't flush routes when setting loopback down

[Stephen Hemminger]

On Wed, 19 Jan 2011 20:56:32 +0100
Jiri Bohac <jbohac@suse.cz> wrote:

> On Wed, Jan 19, 2011 at 11:38:17AM -0800, Stephen Hemminger wrote:
> > Jiri Bohac <jbohac@suse.cz> wrote:
> > > I have the feeling that Eric's patch is the safest solution we
> > > have so far:
> > Eric's patch has other regressions, see the discussion.
> 
> What regression do you mean? I have read the whole discussion
> thoroughly. You only say in one message that deleting ::1 would
> propagate to routing daemons. And Eric correctly stated that
> people couldn't hit this, because  deleting ::1 would break
> things on its own.
> 
> Is there a real problem with Eric's fix?
> 
> Thanks,
> 

If address is assigned to loopback interface (other than ::1) then
Eric's fix doesn't work.  It is common to use an additional address
on the lo device when doing routing protocols.

Re: [RFC] ipv6: don't flush routes when setting loopback down

[Eric W. Biederman]

Stephen Hemminger <shemminger@vyatta.com> writes:

> On Wed, 19 Jan 2011 20:56:32 +0100
> Jiri Bohac <jbohac@suse.cz> wrote:
>
>> On Wed, Jan 19, 2011 at 11:38:17AM -0800, Stephen Hemminger wrote:
>> > Jiri Bohac <jbohac@suse.cz> wrote:
>> > > I have the feeling that Eric's patch is the safest solution we
>> > > have so far:
>> > Eric's patch has other regressions, see the discussion.
>> 
>> What regression do you mean? I have read the whole discussion
>> thoroughly. You only say in one message that deleting ::1 would
>> propagate to routing daemons. And Eric correctly stated that
>> people couldn't hit this, because  deleting ::1 would break
>> things on its own.
>> 
>> Is there a real problem with Eric's fix?
>> 
>> Thanks,
>> 
>
> If address is assigned to loopback interface (other than ::1) then
> Eric's fix doesn't work.  It is common to use an additional address
> on the lo device when doing routing protocols.

Sigh.

I just got back to looking through the rest of my failures in 2.6.37 and
despite it looking like it worked when i tested it, your patch doesn't
actually work on my real work load that has broken.

At least your change that confirmed that the root problem is somewhere
in the routing.

Eric

Re: [RFC] ipv6: don't flush routes when setting loopback down

[Stephen Hemminger]

On Sat, 22 Jan 2011 00:17:09 -0800
ebiederm@xmission.com (Eric W. Biederman) wrote:

> Stephen Hemminger <shemminger@vyatta.com> writes:
> 
> > On Wed, 19 Jan 2011 20:56:32 +0100
> > Jiri Bohac <jbohac@suse.cz> wrote:
> >
> >> On Wed, Jan 19, 2011 at 11:38:17AM -0800, Stephen Hemminger wrote:
> >> > Jiri Bohac <jbohac@suse.cz> wrote:
> >> > > I have the feeling that Eric's patch is the safest solution we
> >> > > have so far:
> >> > Eric's patch has other regressions, see the discussion.
> >> 
> >> What regression do you mean? I have read the whole discussion
> >> thoroughly. You only say in one message that deleting ::1 would
> >> propagate to routing daemons. And Eric correctly stated that
> >> people couldn't hit this, because  deleting ::1 would break
> >> things on its own.
> >> 
> >> Is there a real problem with Eric's fix?
> >> 
> >> Thanks,
> >> 
> >
> > If address is assigned to loopback interface (other than ::1) then
> > Eric's fix doesn't work.  It is common to use an additional address
> > on the lo device when doing routing protocols.
> 
> Sigh.
> 
> I just got back to looking through the rest of my failures in 2.6.37 and
> despite it looking like it worked when i tested it, your patch doesn't
> actually work on my real work load that has broken.
> 
> At least your change that confirmed that the root problem is somewhere
> in the routing.
> 
> Eric

The design problem behind all this is that sysctl disable_ipv6 as currently
implemented is passive (just changes a variable). It needs to be implemented
as a more active step that does the same thing as removing the interface from
ipv6.  I will look into it after LCA. 

Re: [RFC] ipv6: don't flush routes when setting loopback down

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Sun, 23 Jan 2011 09:39:40 +1100

> The design problem behind all this is that sysctl disable_ipv6 as currently
> implemented is passive (just changes a variable). It needs to be implemented
> as a more active step that does the same thing as removing the interface from
> ipv6.  I will look into it after LCA. 

All of this stuff worked before your change Stephen.

It doesn't matter how it was implemented before, IT WORKED.

You broke it, and it's still broken.

You keep talking about fixing things other than the changes
you made, but honestly I think we're at the point where you've
been given enough changes and we need to simply revert your
change.

Things like that can't run on and on for months, I don't care
what the reason is.

Re: [RFC] ipv6: don't flush routes when setting loopback down

[Stephen Hemminger]

Having IPv6 remove all addresses when link goes down is fundamentally broken
that is what the original problem being fixed. For users on servers or using
Quagga this matters, how do you plan to fix that?

----- Original Message -----
> From: Stephen Hemminger <shemminger@vyatta.com>
> Date: Sun, 23 Jan 2011 09:39:40 +1100
> 
> > The design problem behind all this is that sysctl disable_ipv6 as
> > currently implemented is passive (just changes a variable). It needs
> > to be implemented
> > as a more active step that does the same thing as removing the
> > interface from
> > ipv6. I will look into it after LCA.
> 
> All of this stuff worked before your change Stephen.
> 
> It doesn't matter how it was implemented before, IT WORKED.
> 
> You broke it, and it's still broken.
> 
> You keep talking about fixing things other than the changes
> you made, but honestly I think we're at the point where you've
> been given enough changes and we need to simply revert your
> change.
> 
> Things like that can't run on and on for months, I don't care
> what the reason is.

Re: [RFC] ipv6: don't flush routes when setting loopback down

[David Miller]

From: Stephen Hemminger <stephen.hemminger@vyatta.com>
Date: Sat, 22 Jan 2011 20:41:12 -0800 (PST)

> Having IPv6 remove all addresses when link goes down is fundamentally broken
> that is what the original problem being fixed. For users on servers or using
> Quagga this matters, how do you plan to fix that?

How about in a way that doesn't break stuff?

And it's been beyond proven that people give more of a crap
about disable_ipv6 than the thing you keep claiming is a big deal.

NOBODY other than you even noticed the issue or made a report about
it.

Yet we have people actively complaining about disable_ipv6 being
broken.

So you lose on two counts.  You can't fix things by breaking other
stuff, and your obscure stuff matters less than things people
actually notice being broken.

Re: [RFC] ipv6: don't flush routes when setting loopback down

[Stephen Hemminger]

On Sat, 22 Jan 2011 21:42:54 -0800 (PST)
David Miller <davem@davemloft.net> wrote:

> From: Stephen Hemminger <stephen.hemminger@vyatta.com>
> Date: Sat, 22 Jan 2011 20:41:12 -0800 (PST)
> 
> > Having IPv6 remove all addresses when link goes down is fundamentally broken
> > that is what the original problem being fixed. For users on servers or using
> > Quagga this matters, how do you plan to fix that?
> 
> How about in a way that doesn't break stuff?
> 
> And it's been beyond proven that people give more of a crap
> about disable_ipv6 than the thing you keep claiming is a big deal.
> 
> NOBODY other than you even noticed the issue or made a report about
> it.
> 
> Yet we have people actively complaining about disable_ipv6 being
> broken.
> 
> So you lose on two counts.  You can't fix things by breaking other
> stuff, and your obscure stuff matters less than things people
> actually notice being broken.

You are probably so upset because I stepped on code you worked hard
on. But the IPv6 semantics should not have been different from IPv4
and the disable_ipv6 flag was a poor API choice as well. Legacy
API's suck, I don't expect perfection but it should be possible
to make a working version that:

Allows disabling IPv6 completely on an interface
AND Has the same address and route semantics for both
IPv4 and IPv6.

Re: [RFC] ipv6: don't flush routes when setting loopback down

[Stephen Hemminger]

On Sun, 23 Jan 2011 19:24:16 +1100
Stephen Hemminger <shemminger@vyatta.com> wrote:

> On Sat, 22 Jan 2011 21:42:54 -0800 (PST)
> David Miller <davem@davemloft.net> wrote:
> 
> > From: Stephen Hemminger <stephen.hemminger@vyatta.com>
> > Date: Sat, 22 Jan 2011 20:41:12 -0800 (PST)
> > 
> > > Having IPv6 remove all addresses when link goes down is fundamentally broken
> > > that is what the original problem being fixed. For users on servers or using
> > > Quagga this matters, how do you plan to fix that?
> > 
> > How about in a way that doesn't break stuff?
> > 
> > And it's been beyond proven that people give more of a crap
> > about disable_ipv6 than the thing you keep claiming is a big deal.
> > 
> > NOBODY other than you even noticed the issue or made a report about
> > it.
> > 
> > Yet we have people actively complaining about disable_ipv6 being
> > broken.
> > 
> > So you lose on two counts.  You can't fix things by breaking other
> > stuff, and your obscure stuff matters less than things people
> > actually notice being broken.
> 
> You are probably so upset because I stepped on code you worked hard
> on. But the IPv6 semantics should not have been different from IPv4
> and the disable_ipv6 flag was a poor API choice as well. Legacy
> API's suck, I don't expect perfection but it should be possible
> to make a working version that:
> 
> Allows disabling IPv6 completely on an interface
> AND Has the same address and route semantics for both
> IPv4 and IPv6.

Also for application sanity, Linux should behave the same as BSD

Re: [stable] [RFC] ipv6: don't flush routes when setting loopback down

[Willy Tarreau]

[ first, is there a reason we have stable@ CCed on this thread ? ]

On Sun, Jan 23, 2011 at 07:26:24PM +1100, Stephen Hemminger wrote:
> > You are probably so upset because I stepped on code you worked hard
> > on. But the IPv6 semantics should not have been different from IPv4
> > and the disable_ipv6 flag was a poor API choice as well. Legacy
> > API's suck, I don't expect perfection but it should be possible
> > to make a working version that:
> > 
> > Allows disabling IPv6 completely on an interface
> > AND Has the same address and route semantics for both
> > IPv4 and IPv6.
> 
> Also for application sanity, Linux should behave the same as BSD

Stephen,

while I agree with all the points you made, David is right in that we
can't use a fix for a bug as a justification for breaking something
that worked for other people. It simply means that everything that was
merged since the first regression was introduced should be reverted
and reworked until a more satisfying solution is found.

Otherwise users lose trust and you have to deal with much more cases
when users report issues.

If the bug is caused by a deep design issue, then maybe a development
branch should be dedicated to it so that the persons affected by it
can track its evolution and report some feedback.

Regards,
Willy

Re: [stable] [RFC] ipv6: don't flush routes when setting loopback down

[Stephen Hemminger]

On Sun, 23 Jan 2011 10:15:32 +0100
Willy Tarreau <w@1wt.eu> wrote:

> [ first, is there a reason we have stable@ CCed on this thread ? ]
> 
> On Sun, Jan 23, 2011 at 07:26:24PM +1100, Stephen Hemminger wrote:
> > > You are probably so upset because I stepped on code you worked hard
> > > on. But the IPv6 semantics should not have been different from IPv4
> > > and the disable_ipv6 flag was a poor API choice as well. Legacy
> > > API's suck, I don't expect perfection but it should be possible
> > > to make a working version that:
> > > 
> > > Allows disabling IPv6 completely on an interface
> > > AND Has the same address and route semantics for both
> > > IPv4 and IPv6.
> > 
> > Also for application sanity, Linux should behave the same as BSD
> 
> Stephen,
> 
> while I agree with all the points you made, David is right in that we
> can't use a fix for a bug as a justification for breaking something
> that worked for other people. It simply means that everything that was
> merged since the first regression was introduced should be reverted
> and reworked until a more satisfying solution is found.
> 
> Otherwise users lose trust and you have to deal with much more cases
> when users report issues.
> 
> If the bug is caused by a deep design issue, then maybe a development
> branch should be dedicated to it so that the persons affected by it

I made my attempt at fixing the issue, others can attack that mud pit.

Re: [stable] [RFC] ipv6: don't flush routes when setting loopback down

[Stephen Hemminger]

I think this fixes the issue with disable_ipv6

--- a/net/ipv6/addrconf.c	2011-01-23 20:30:25.897243002 +1100
+++ b/net/ipv6/addrconf.c	2011-01-23 20:30:41.161243002 +1100
@@ -4197,7 +4197,7 @@ static void dev_disable_change(struct in
 		return;
 
 	if (idev->cnf.disable_ipv6)
-		addrconf_notify(NULL, NETDEV_DOWN, idev->dev);
+		addrconf_notify(NULL, NETDEV_UNREGISTER, idev->dev);
 	else
 		addrconf_notify(NULL, NETDEV_UP, idev->dev);
 }

Re: [RFC] ipv6: don't flush routes when setting loopback down

[Eric W. Biederman]

Stephen Hemminger <shemminger@vyatta.com> writes:

> I think this fixes the issue with disable_ipv6

Somehow I doubt deleting the ipv6 state, and removing the per device
disable_ipv6 flag is going to be backwards compatible.

echo 0 > /proc/sys/net/ipv6/conf/disable_ipv6

Won't work.

What ever other good properties calling NETDEV_UNREGISTER for ipv6
devices may have.

Eric

> --- a/net/ipv6/addrconf.c	2011-01-23 20:30:25.897243002 +1100
> +++ b/net/ipv6/addrconf.c	2011-01-23 20:30:41.161243002 +1100
> @@ -4197,7 +4197,7 @@ static void dev_disable_change(struct in
>  		return;
>  
>  	if (idev->cnf.disable_ipv6)
> -		addrconf_notify(NULL, NETDEV_DOWN, idev->dev);
> +		addrconf_notify(NULL, NETDEV_UNREGISTER, idev->dev);
>  	else
>  		addrconf_notify(NULL, NETDEV_UP, idev->dev);
>  }

Re: [stable] [RFC] ipv6: don't flush routes when setting loopback down

[David Miller]

From: ebiederm@xmission.com (Eric W. Biederman)
Date: Sun, 23 Jan 2011 11:21:29 -0800

> Stephen Hemminger <shemminger@vyatta.com> writes:
> 
>> I think this fixes the issue with disable_ipv6
> 
> Somehow I doubt deleting the ipv6 state, and removing the per device
> disable_ipv6 flag is going to be backwards compatible.
> 
> echo 0 > /proc/sys/net/ipv6/conf/disable_ipv6
> 
> Won't work.
> 
> What ever other good properties calling NETDEV_UNREGISTER for ipv6
> devices may have.

Don't worry Eric, I'm reverting all of the changes that caused this
problem in the first place.

If Stephen wants his change back in, he'll have to resubmit it in
a working state with all the known regressions dealt with.

Re: [RFC] ipv6: don't flush routes when setting loopback down

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Sun, 23 Jan 2011 19:26:24 +1100

> Also for application sanity, Linux should behave the same as BSD

I've heard enough, I'm reverting your changes.  You obviously
don't get what "don't break stuff that was working" means.

Re: [RFC] ipv6: don't flush routes when setting loopback down

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Sun, 23 Jan 2011 19:24:16 +1100

> You are probably so upset because I stepped on code you worked hard
> on.

Frankly, I honestly don't care much at all about ipv6, it's still only
a tiny minuscule fraction of actual usage in this world, even with
ipv4 addresses basically completely depleted right now, and it's also
an over-engineered monster.

> But the IPv6 semantics should not have been different from IPv4
> and the disable_ipv6 flag was a poor API choice as well.

There are no equivalent semantics, because nobody wants to disable
all IPV4 activity and socket protocol operations.  People want it
only for ipv6.

This interface was choosen because this is what users asked for.

They wanted global and per-interface ways to disable IPV6 protocol
activity, so that's what we gave them.

Stephen you don't get it.  The ball is completely in your court
about this, you broke stuff to fix stuff and that's not allowed.

You have also been given months of time to undo the breakage.
Normally I would just immediately revert, so you were given
special allowances because I respect and trust you.

So stop this BS where you say that I'm treating you in an
unfair way.

Re: [PATCH] Fix 2.6.34-rc1 regression in disable_ipv6 support

[Stephen Hemminger]

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.