| Message ID | 52CD0331.8040204@cn.fujitsu.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed, Jan 08, 2014 at 03:50:09PM +0800, Gao feng wrote: > On 01/03/2014 02:53 PM, Hannes Frederic Sowa wrote: > > On Thu, Jan 02, 2014 at 05:33:15PM +0800, chenweilong wrote: > >> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c > >> index 62d1799..d2f8c0a 100644 > >> --- a/net/ipv6/addrconf.c > >> +++ b/net/ipv6/addrconf.c > >> @@ -2422,8 +2422,9 @@ static void init_loopback(struct net_device *dev) > >> if (sp_ifa->flags & (IFA_F_DADFAILED | IFA_F_TENTATIVE)) > >> continue; > >> > >> - if (sp_ifa->rt) > >> - continue; > >> + if (sp_ifa->rt && sp_ifa->rt->dst.dev == dev) { > >> + ip6_del_rt(sp_ifa->rt); > >> + } > >> > >> sp_rt = addrconf_dst_alloc(idev, &sp_ifa->addr, 0); > >> > > > > Maybe this change would not be that bad after all, as those ifa attached dsts > > are already dead and queued up for gc and should not get inserted back. > > I like this idea, maybe the below patch is better. we only need to delete this > route when it has been added to garbage list. > > diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c > index 1a341f7..4dca886 100644 > --- a/net/ipv6/addrconf.c > +++ b/net/ipv6/addrconf.c > @@ -2610,8 +2610,16 @@ static void init_loopback(struct net_device *dev) > if (sp_ifa->flags & (IFA_F_DADFAILED | IFA_F_TENTATIVE)) > continue; > > - if (sp_ifa->rt) > - continue; > + if (sp_ifa->rt) { > + /* This dst has been added to garbage list when > + * lo device down, delete this obsolete dst and > + * reallocate new router for ifa. */ > + if (sp_ifa->rt->dst.obsolete > 0) { > + ip6_del_rt(sp_ifa->rt); > + sp_ifa->rt = NULL; > + } else > + continue; > + } > > sp_rt = addrconf_dst_alloc(idev, &sp_ifa->addr, false); It looks like it can work but I don't know if we should just fix this the clean way (see below). > > I'll try to just disable routes without removing them at all when we set an > > interface to down at the weekend. > > > > How do you decide which route should be disabled? use rt6_flags? I don't know > if your way will cause miscarriage. What I did so far is that I added a new function next to rt6_ifdown that only gets called if interface gets shutdown but not unregistered (from addrconf_ifdown). fib6_clean_all then iterates over the whole routing table with a new predicate function which checks in the same way like fib6_ifdown, if it is a matching route (the interfaces match up) and if so, toggles a new "DEAD" flag in rt6i_flags. When bringing up the interface I distinguish between up and register and just clear this death flag from the routes on bringing it up. fib lookup code then does not honour those routes. I had no time to test this thoroughly at the weekend and still have some code paths were I am unsure. Do you see any problems with this so far? We could then delete the special cases on loopback interface init. Thanks, Hannes -- 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 01/08/2014 04:05 PM, Hannes Frederic Sowa wrote: > On Wed, Jan 08, 2014 at 03:50:09PM +0800, Gao feng wrote: >> On 01/03/2014 02:53 PM, Hannes Frederic Sowa wrote: >>> On Thu, Jan 02, 2014 at 05:33:15PM +0800, chenweilong wrote: >>>> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c >>>> index 62d1799..d2f8c0a 100644 >>>> --- a/net/ipv6/addrconf.c >>>> +++ b/net/ipv6/addrconf.c >>>> @@ -2422,8 +2422,9 @@ static void init_loopback(struct net_device *dev) >>>> if (sp_ifa->flags & (IFA_F_DADFAILED | IFA_F_TENTATIVE)) >>>> continue; >>>> >>>> - if (sp_ifa->rt) >>>> - continue; >>>> + if (sp_ifa->rt && sp_ifa->rt->dst.dev == dev) { >>>> + ip6_del_rt(sp_ifa->rt); >>>> + } >>>> >>>> sp_rt = addrconf_dst_alloc(idev, &sp_ifa->addr, 0); >>>> >>> >>> Maybe this change would not be that bad after all, as those ifa attached dsts >>> are already dead and queued up for gc and should not get inserted back. >> >> I like this idea, maybe the below patch is better. we only need to delete this >> route when it has been added to garbage list. >> >> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c >> index 1a341f7..4dca886 100644 >> --- a/net/ipv6/addrconf.c >> +++ b/net/ipv6/addrconf.c >> @@ -2610,8 +2610,16 @@ static void init_loopback(struct net_device *dev) >> if (sp_ifa->flags & (IFA_F_DADFAILED | IFA_F_TENTATIVE)) >> continue; >> >> - if (sp_ifa->rt) >> - continue; >> + if (sp_ifa->rt) { >> + /* This dst has been added to garbage list when >> + * lo device down, delete this obsolete dst and >> + * reallocate new router for ifa. */ >> + if (sp_ifa->rt->dst.obsolete > 0) { >> + ip6_del_rt(sp_ifa->rt); >> + sp_ifa->rt = NULL; >> + } else >> + continue; >> + } >> >> sp_rt = addrconf_dst_alloc(idev, &sp_ifa->addr, false); > > It looks like it can work but I don't know if we should just fix this the > clean way (see below). > >>> I'll try to just disable routes without removing them at all when we set an >>> interface to down at the weekend. >>> >> >> How do you decide which route should be disabled? use rt6_flags? I don't know >> if your way will cause miscarriage. > > What I did so far is that I added a new function next to rt6_ifdown that > only gets called if interface gets shutdown but not unregistered (from > addrconf_ifdown). > rt6_ifdown has alreay put this device related routes to the garbage list. > fib6_clean_all then iterates over the whole routing table with a new predicate > function which checks in the same way like fib6_ifdown, if it is a matching route > (the interfaces match up) and if so, toggles a new "DEAD" flag in rt6i_flags. > > When bringing up the interface I distinguish between up and register and just > clear this death flag from the routes on bringing it up. > > fib lookup code then does not honour those routes. > > I had no time to test this thoroughly at the weekend and still have some code > paths were I am unsure. Do you see any problems with this so far? We could > then delete the special cases on loopback interface init. So you add a special case in the general network interface down logic. I think this is more complex... Thanks! -- 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 08, 2014 at 04:42:46PM +0800, Gao feng wrote: > On 01/08/2014 04:05 PM, Hannes Frederic Sowa wrote: > > On Wed, Jan 08, 2014 at 03:50:09PM +0800, Gao feng wrote: > >> On 01/03/2014 02:53 PM, Hannes Frederic Sowa wrote: > >>> On Thu, Jan 02, 2014 at 05:33:15PM +0800, chenweilong wrote: > >>>> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c > >>>> index 62d1799..d2f8c0a 100644 > >>>> --- a/net/ipv6/addrconf.c > >>>> +++ b/net/ipv6/addrconf.c > >>>> @@ -2422,8 +2422,9 @@ static void init_loopback(struct net_device *dev) > >>>> if (sp_ifa->flags & (IFA_F_DADFAILED | IFA_F_TENTATIVE)) > >>>> continue; > >>>> > >>>> - if (sp_ifa->rt) > >>>> - continue; > >>>> + if (sp_ifa->rt && sp_ifa->rt->dst.dev == dev) { > >>>> + ip6_del_rt(sp_ifa->rt); > >>>> + } > >>>> > >>>> sp_rt = addrconf_dst_alloc(idev, &sp_ifa->addr, 0); > >>>> > >>> > >>> Maybe this change would not be that bad after all, as those ifa attached dsts > >>> are already dead and queued up for gc and should not get inserted back. > >> > >> I like this idea, maybe the below patch is better. we only need to delete this > >> route when it has been added to garbage list. > >> > >> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c > >> index 1a341f7..4dca886 100644 > >> --- a/net/ipv6/addrconf.c > >> +++ b/net/ipv6/addrconf.c > >> @@ -2610,8 +2610,16 @@ static void init_loopback(struct net_device *dev) > >> if (sp_ifa->flags & (IFA_F_DADFAILED | IFA_F_TENTATIVE)) > >> continue; > >> > >> - if (sp_ifa->rt) > >> - continue; > >> + if (sp_ifa->rt) { > >> + /* This dst has been added to garbage list when > >> + * lo device down, delete this obsolete dst and > >> + * reallocate new router for ifa. */ > >> + if (sp_ifa->rt->dst.obsolete > 0) { > >> + ip6_del_rt(sp_ifa->rt); > >> + sp_ifa->rt = NULL; > >> + } else > >> + continue; > >> + } > >> > >> sp_rt = addrconf_dst_alloc(idev, &sp_ifa->addr, false); > > > > It looks like it can work but I don't know if we should just fix this the > > clean way (see below). > > > >>> I'll try to just disable routes without removing them at all when we set an > >>> interface to down at the weekend. > >>> > >> > >> How do you decide which route should be disabled? use rt6_flags? I don't know > >> if your way will cause miscarriage. > > > > What I did so far is that I added a new function next to rt6_ifdown that > > only gets called if interface gets shutdown but not unregistered (from > > addrconf_ifdown). > > > > rt6_ifdown has alreay put this device related routes to the garbage list. > > > fib6_clean_all then iterates over the whole routing table with a new predicate > > function which checks in the same way like fib6_ifdown, if it is a matching route > > (the interfaces match up) and if so, toggles a new "DEAD" flag in rt6i_flags. > > > > When bringing up the interface I distinguish between up and register and just > > clear this death flag from the routes on bringing it up. > > > > fib lookup code then does not honour those routes. > > > > I had no time to test this thoroughly at the weekend and still have some code > > paths were I am unsure. Do you see any problems with this so far? We could > > then delete the special cases on loopback interface init. > > So you add a special case in the general network interface down logic. I think this > is more complex... The problem is, that we only have a reference to ifp->rt, the loopback RTF_LOCAL route. Currently we silently remove all other routes this interface has. Sure, they come back soon with RAs and such, but this is not the way this should work. Greetings, Hannes -- 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 2014/1/8 16:55, Hannes Frederic Sowa wrote: > On Wed, Jan 08, 2014 at 04:42:46PM +0800, Gao feng wrote: >> On 01/08/2014 04:05 PM, Hannes Frederic Sowa wrote: >>> On Wed, Jan 08, 2014 at 03:50:09PM +0800, Gao feng wrote: >>>> On 01/03/2014 02:53 PM, Hannes Frederic Sowa wrote: >>>>> On Thu, Jan 02, 2014 at 05:33:15PM +0800, chenweilong wrote: >>>>>> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c >>>>>> index 62d1799..d2f8c0a 100644 >>>>>> --- a/net/ipv6/addrconf.c >>>>>> +++ b/net/ipv6/addrconf.c >>>>>> @@ -2422,8 +2422,9 @@ static void init_loopback(struct net_device *dev) >>>>>> if (sp_ifa->flags & (IFA_F_DADFAILED | IFA_F_TENTATIVE)) >>>>>> continue; >>>>>> >>>>>> - if (sp_ifa->rt) >>>>>> - continue; >>>>>> + if (sp_ifa->rt && sp_ifa->rt->dst.dev == dev) { >>>>>> + ip6_del_rt(sp_ifa->rt); >>>>>> + } >>>>>> >>>>>> sp_rt = addrconf_dst_alloc(idev, &sp_ifa->addr, 0); >>>>>> >>>>> >>>>> Maybe this change would not be that bad after all, as those ifa attached dsts >>>>> are already dead and queued up for gc and should not get inserted back. >>>> >>>> I like this idea, maybe the below patch is better. we only need to delete this >>>> route when it has been added to garbage list. >>>> >>>> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c >>>> index 1a341f7..4dca886 100644 >>>> --- a/net/ipv6/addrconf.c >>>> +++ b/net/ipv6/addrconf.c >>>> @@ -2610,8 +2610,16 @@ static void init_loopback(struct net_device *dev) >>>> if (sp_ifa->flags & (IFA_F_DADFAILED | IFA_F_TENTATIVE)) >>>> continue; >>>> >>>> - if (sp_ifa->rt) >>>> - continue; >>>> + if (sp_ifa->rt) { >>>> + /* This dst has been added to garbage list when >>>> + * lo device down, delete this obsolete dst and >>>> + * reallocate new router for ifa. */ >>>> + if (sp_ifa->rt->dst.obsolete > 0) { >>>> + ip6_del_rt(sp_ifa->rt); >>>> + sp_ifa->rt = NULL; >>>> + } else >>>> + continue; >>>> + } >>>> >>>> sp_rt = addrconf_dst_alloc(idev, &sp_ifa->addr, false); >>> >>> It looks like it can work but I don't know if we should just fix this the >>> clean way (see below). >>> >>>>> I'll try to just disable routes without removing them at all when we set an >>>>> interface to down at the weekend. >>>>> >>>> >>>> How do you decide which route should be disabled? use rt6_flags? I don't know >>>> if your way will cause miscarriage. >>> >>> What I did so far is that I added a new function next to rt6_ifdown that >>> only gets called if interface gets shutdown but not unregistered (from >>> addrconf_ifdown). >>> >> >> rt6_ifdown has alreay put this device related routes to the garbage list. >> >>> fib6_clean_all then iterates over the whole routing table with a new predicate >>> function which checks in the same way like fib6_ifdown, if it is a matching route >>> (the interfaces match up) and if so, toggles a new "DEAD" flag in rt6i_flags. >>> >>> When bringing up the interface I distinguish between up and register and just >>> clear this death flag from the routes on bringing it up. >>> >>> fib lookup code then does not honour those routes. >>> >>> I had no time to test this thoroughly at the weekend and still have some code >>> paths were I am unsure. Do you see any problems with this so far? We could >>> then delete the special cases on loopback interface init. >> >> So you add a special case in the general network interface down logic. I think this >> is more complex... > > The problem is, that we only have a reference to ifp->rt, the loopback > RTF_LOCAL route. Currently we silently remove all other routes this interface > has. Sure, they come back soon with RAs and such, but this is not the way this > should work. > > Greetings, > > Hannes > > > . > Hi, It's quite a long time, How's your patch going on? -- 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 Fri, Jan 17, 2014 at 10:02:17AM +0800, chenweilong wrote:
> It's quite a long time, How's your patch going on?
I apologize, it has gotten a bit off my radar lately, but I have the branch
still around and will resurrect it asap. I think it is a problem which really
should be solved.
Thanks,
Hannes
--
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 1a341f7..4dca886 100644 --- a/net/ipv6/addrconf.c +++ b/net/ipv6/addrconf.c @@ -2610,8 +2610,16 @@ static void init_loopback(struct net_device *dev) if (sp_ifa->flags & (IFA_F_DADFAILED | IFA_F_TENTATIVE)) continue; - if (sp_ifa->rt) - continue; + if (sp_ifa->rt) { + /* This dst has been added to garbage list when + * lo device down, delete this obsolete dst and + * reallocate new router for ifa. */ + if (sp_ifa->rt->dst.obsolete > 0) { + ip6_del_rt(sp_ifa->rt); + sp_ifa->rt = NULL; + } else + continue; + } sp_rt = addrconf_dst_alloc(idev, &sp_ifa->addr, false);