Message ID | 1386956028-3463-1-git-send-email-yazzep@gmail.com |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
On Sat, Dec 14, 2013 at 02:33:48AM +0900, yazzep@gmail.com wrote: > diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c > index 3c3425e..00c135b 100644 > --- a/net/ipv6/addrconf.c > +++ b/net/ipv6/addrconf.c > @@ -948,18 +948,21 @@ static void ipv6_del_addr(struct inet6_ifaddr *ifp) > if (!onlink) > onlink = -1; > > - spin_lock(&ifa->lock); > - > - lifetime = addrconf_timeout_fixup(ifa->valid_lft, HZ); > - /* > - * Note: Because this address is > - * not permanent, lifetime < > - * LONG_MAX / HZ here. > - */ > - if (time_before(expires, > - ifa->tstamp + lifetime * HZ)) > - expires = ifa->tstamp + lifetime * HZ; > - spin_unlock(&ifa->lock); > + if (ifp->valid_lft != > + INFINITY_LIFE_TIME) { > + spin_lock(&ifa->lock); > + > + lifetime = addrconf_timeout_fixup( > + ifa->valid_lft, HZ); > + /* Note: because this address is > + * not permanent, lifetime < > + * LONG_MAX / HZ here. > + */ > + if (time_before(expires, > + ifa->tstamp + lifetime * HZ)) > + expires = ifa->tstamp + lifetime * HZ; > + spin_unlock(&ifa->lock); > + } Sorry, this does not make sense to me. Below you remove the IFA_F_PERMANENT expression and here you change code depending only on that flag. Is this meant as a shortcut? Please explain, maybe I am missing something. > } > } > } > @@ -2415,7 +2418,6 @@ static int inet6_addr_add(struct net *net, int ifindex, > } else { > expires = 0; > flags = 0; > - ifa_flags |= IFA_F_PERMANENT; I find this very suspicious. IFA_F_PERMANENT may only be removed if valid_lft != inifinity and preferred_lft != infinity if at all (see below). We have to be very careful here with interactions of prefix routes. > timeout = addrconf_timeout_fixup(prefered_lft, HZ); > @@ -3497,8 +3499,12 @@ restart: > ifp->flags |= IFA_F_DEPRECATED; > } > > - if (time_before(ifp->tstamp + ifp->valid_lft * HZ, next)) > - next = ifp->tstamp + ifp->valid_lft * HZ; > + if (ifp->valid_lft != INFINITY_LIFE_TIME) { > + if (time_before(ifp->tstamp + > + ifp->valid_lft * HZ, next)) > + next = ifp->tstamp + > + ifp->valid_lft * HZ; > + } You could reduce one level of indentation if you collapse those two conditions. > > spin_unlock(&ifp->lock); > > @@ -3635,7 +3641,6 @@ static int inet6_addr_modify(struct inet6_ifaddr *ifp, u32 ifa_flags, > } else { > expires = 0; > flags = 0; > - ifa_flags |= IFA_F_PERMANENT; > } Ditto, I do think we cannot remove this flag unconditionally. Actually, I don't think it is the correct way to remove the IFA_F_PERMANENT here at all. We break the interaction with prefix routes and won't remove them at the time of address deletion any more. This is definitely a no-go, sorry. The prefix was added by hand and should get removed immediately if a user removes the address (well actually it is an error we set up a prefix route in the first place, but it is how things are now). IMHO, the changes need to be made in addrconf_verify so that the route does get marked as deprecated. Please check the places where we mark an interface address as deprecated. 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 Sat, Dec 14, 2013 at 10:19:17AM +0100, Hannes Frederic Sowa wrote: > IMHO, the changes need to be made in addrconf_verify so that the route does ^^^^ I actually meant address, sorry. The route must stay valid of course. > get marked as deprecated. Please check the places where we mark an interface > address as deprecated. -- 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
> Ditto, I do think we cannot remove this flag unconditionally. > > Actually, I don't think it is the correct way to remove the IFA_F_PERMANENT > here at all. We break the interaction with prefix routes and won't remove > them > at the time of address deletion any more. This is definitely a no-go, sorry. > > The prefix was added by hand and should get removed immediately if a user > removes the address (well actually it is an error we set up a prefix route > in > the first place, but it is how things are now). > > IMHO, the changes need to be made in addrconf_verify so that the route does > get marked as deprecated. Please check the places where we mark an interface > address as deprecated. > > Thanks, > > Hannes Sorry about not getting back to you sooner. I have been thinking and testing about the correct way of fixing the problem. I understand it's not the correct way to remove the IFA_F_PERMANENT. I agree with you that the changes need to be made in addrconf_verify. and I think the changes also need to be made in inet6_fill_ifaddr so that the "preferred" is set INFINITY_LIFE_TIME unconditionally while ifa->flags is IFA_F_PERMANENT. It means, "preferred" is set to INFINITY_LIFE_TIME when I set preferred_lft a value not zero or infinity, while valid_lft is infinity(0xffffffff) I am going to send a new patch soon. Your kind consideration of this matter would be sincerely appreciated. Yasushi
> > Ditto, I do think we cannot remove this flag unconditionally. > > Actually, I don't think it is the correct way to remove the IFA_F_PERMANENT > here at all. We break the interaction with prefix routes and won't remove them > at the time of address deletion any more. This is definitely a no-go, sorry. > > The prefix was added by hand and should get removed immediately if a user > removes the address (well actually it is an error we set up a prefix route in > the first place, but it is how things are now). > > IMHO, the changes need to be made in addrconf_verify so that the route does > get marked as deprecated. Please check the places where we mark an interface > address as deprecated. > > Thanks, > > Hannes Sorry about not getting back to you sooner. I have been thinking and testing about the correct way of fixing the problem. I understand it's not the correct way to remove the IFA_F_PERMANENT. I agree with you that the changes need to be made in addrconf_verify. and I think changes also need to be made in inet6_fill_ifaddr so that the "preferred" is set INFINITY_LIFE_TIME unconditionally while ifa->flags is IFA_F_PERMANENT. It means, "preferred" is set to INFINITY_LIFE_TIME when I set preferred_lft a value not zero or infinity, while valid_lft is infinity(0xffffffff) I sent a new patch to netdev-ml today. Your kind consideration of this matter would be sincerely appreciated. Yasushi-- 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 3c3425e..00c135b 100644 --- a/net/ipv6/addrconf.c +++ b/net/ipv6/addrconf.c @@ -948,18 +948,21 @@ static void ipv6_del_addr(struct inet6_ifaddr *ifp) if (!onlink) onlink = -1; - spin_lock(&ifa->lock); - - lifetime = addrconf_timeout_fixup(ifa->valid_lft, HZ); - /* - * Note: Because this address is - * not permanent, lifetime < - * LONG_MAX / HZ here. - */ - if (time_before(expires, - ifa->tstamp + lifetime * HZ)) - expires = ifa->tstamp + lifetime * HZ; - spin_unlock(&ifa->lock); + if (ifp->valid_lft != + INFINITY_LIFE_TIME) { + spin_lock(&ifa->lock); + + lifetime = addrconf_timeout_fixup( + ifa->valid_lft, HZ); + /* Note: because this address is + * not permanent, lifetime < + * LONG_MAX / HZ here. + */ + if (time_before(expires, + ifa->tstamp + lifetime * HZ)) + expires = ifa->tstamp + lifetime * HZ; + spin_unlock(&ifa->lock); + } } } } @@ -2415,7 +2418,6 @@ static int inet6_addr_add(struct net *net, int ifindex, } else { expires = 0; flags = 0; - ifa_flags |= IFA_F_PERMANENT; } timeout = addrconf_timeout_fixup(prefered_lft, HZ); @@ -3497,8 +3499,12 @@ restart: ifp->flags |= IFA_F_DEPRECATED; } - if (time_before(ifp->tstamp + ifp->valid_lft * HZ, next)) - next = ifp->tstamp + ifp->valid_lft * HZ; + if (ifp->valid_lft != INFINITY_LIFE_TIME) { + if (time_before(ifp->tstamp + + ifp->valid_lft * HZ, next)) + next = ifp->tstamp + + ifp->valid_lft * HZ; + } spin_unlock(&ifp->lock); @@ -3635,7 +3641,6 @@ static int inet6_addr_modify(struct inet6_ifaddr *ifp, u32 ifa_flags, } else { expires = 0; flags = 0; - ifa_flags |= IFA_F_PERMANENT; } timeout = addrconf_timeout_fixup(prefered_lft, HZ);