diff mbox

[1/1] ipv6 addrconf:fix preferred lifetime state-changing behavior while valid_lft is infinity

Message ID 1386956028-3463-1-git-send-email-yazzep@gmail.com
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

yazzep@gmail.com Dec. 13, 2013, 5:33 p.m. UTC 
From: Yasushi Asano <yasushi.asano@jp.fujitsu.com>

Fixed a problem with setting the lifetime of an IPv6
address. When setting preferred_lft to a value not zero or
infinity, while valid_lft is infinity(0xffffffff) preferred
lifetime is set to forever and does not update. Therefore
preferred lifetime never becomes deprecated. valid lifetime
and preferred lifetime should be set independently, even if
valid lifetime is infinity, preferred lifetime must expire
correctly (meaning it must eventually become deprecated)


Signed-off-by: Yasushi Asano <yasushi.asano@jp.fujitsu.com>
---
 net/ipv6/addrconf.c | 37 +++++++++++++++++++++----------------
 1 file changed, 21 insertions(+), 16 deletions(-)

--
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

Comments

Hannes Frederic Sowa Dec. 14, 2013, 9:19 a.m. UTC | #1 
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
Hannes Frederic Sowa Dec. 14, 2013, 9:22 a.m. UTC | #2 
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
Asano, Yasushi Dec. 29, 2013, 5:57 a.m. UTC | #3 
> 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
yazzep@gmail.com Dec. 29, 2013, 12:04 p.m. UTC | #4 
> 
> 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 mbox

Patch

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);