| Message ID | 20101216132812.2d7fd885@nehalam |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
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 -- 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 <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); -- 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
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. -- 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
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);
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. -- 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
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,
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. -- 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 <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 -- 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
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. -- 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
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. -- 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
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. -- 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
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. -- 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
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. -- 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
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 -- 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
[ 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 -- 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
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. -- 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
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. -- 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
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. -- 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
--- 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);
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. -- 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