| Message ID | 4D00F58A.2050307@hp.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
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
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
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
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
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
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
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
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.
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
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. */