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

[Hannes Frederic Sowa <hannes@stressinduktion.org>]

On Sun, Dec 29, 2013 at 04:47:40PM +0900, yazzep@gmail.com wrote:

Subject: Re: [PATCH] ipv6 addrconf:fix preferred lifetime state-changing behavior while valid_lft is infinity

Please a space between addrconf: and fix.

> 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 | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index cd2d7d0..796d52a 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -3479,7 +3479,8 @@ restart:
>  					 &inet6_addr_lst[i], addr_lst) {
>  			unsigned long age;
>  
> -			if (ifp->flags & IFA_F_PERMANENT)
> +			if ((ifp->flags & IFA_F_PERMANENT) &&
> +			    (ifp->prefered_lft == INFINITY_LIFE_TIME))
>  				continue;

As in the first mail, a comment would be nice how permanent addresses where
prefered_lft != INFINITY_LIFE_TIME could come from.

Maybe we could also add this to the header file where IFA_F_PERMANENT is
declared. There was recently confusion about that with another patch.

>  			spin_lock(&ifp->lock);
> @@ -3504,8 +3505,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;
> +				}

Please combine the two if-tests here (with &&).

>  
>  				spin_unlock(&ifp->lock);
>  
> @@ -3804,7 +3809,8 @@ static int inet6_fill_ifaddr(struct sk_buff *skb, struct inet6_ifaddr *ifa,
>  	put_ifaddrmsg(nlh, ifa->prefix_len, ifa->flags, rt_scope(ifa->scope),
>  		      ifa->idev->dev->ifindex);
>  
> -	if (!(ifa->flags&IFA_F_PERMANENT)) {
> +	if (!((ifa->flags&IFA_F_PERMANENT) &&
> +	      (ifa->prefered_lft == INFINITY_LIFE_TIME))) {
>  		preferred = ifa->prefered_lft;
>  		valid = ifa->valid_lft;
>  		if (preferred != INFINITY_LIFE_TIME) {

Otherwise the patch looks good and my testing showed no problems.

Thank you!