[PATCH 0 of 5] IPv6: Fix temporary address support

[PATCH 0 of 5] IPv6: Fix temporary address support

[Benoit Boissinot]

This is the set of patches I needed to make IPv6 temporary addresses
work in my setup.

First my ISP advertizes 2 prefixes, this was not tested with the current code.
Next one of the prefix has a preferred lifetime of 0, the current code
didn't follow the rfc (no temporary addresses should be created in this case).
Finally the other patch results from my test using a small tmp_preferred_lifetime
and the issues I encountered in this case.

The second patch is probably the most important of the serie and worth applying
to stable, without it I can't use temporary addresses.

Please review and consider applying.

Benoit

PS: I noticed that MAX_DESYNC_FACTOR was used to calculated the preferred
    lifetime, instead of a random number between 0 and MAX_DESYNC_FACTOR. Is
    this on purpose, or should a write a patch ?

[PATCH 1 of 5] IPv6: do not wrap around when the lifetime has expired

[Benoit Boissinot]

IPv6: do not wrap around when the lifetime has expired

Instead of reporting overly large lifetimes to userspace,
report a lifetime of 0 when a lifetime has expired.

Signed-off-by: Benoit Boissinot <benoit.boissinot@ens-lyon.org>

diff -r c6c17f6335a1 -r dd125a7f8696 net/ipv6/addrconf.c
--- a/net/ipv6/addrconf.c	Sat Mar 22 14:33:51 2008 +0100
+++ b/net/ipv6/addrconf.c	Sat Mar 22 00:50:21 2008 +0100
@@ -3230,10 +3230,17 @@
 		preferred = ifa->prefered_lft;
 		valid = ifa->valid_lft;
 		if (preferred != INFINITY_LIFE_TIME) {
-			long tval = (jiffies - ifa->tstamp)/HZ;
-			preferred -= tval;
-			if (valid != INFINITY_LIFE_TIME)
-				valid -= tval;
+			unsigned long tval = (jiffies - ifa->tstamp)/HZ;
+			if (tval > preferred)
+				preferred = 0;
+			else
+				preferred -= tval;
+			if (valid != INFINITY_LIFE_TIME) {
+				if (tval > valid)
+					valid = 0;
+				else
+					valid -= tval;
+			}
 		}
 	} else {
 		preferred = INFINITY_LIFE_TIME;

Re: [PATCH 1 of 5] IPv6: do not wrap around when the lifetime has expired

[YOSHIFUJI Hideaki / 吉藤英明]

In article <dd125a7f86968371046d.1206305169@pirzuine> (at Sun, 23 Mar 2008 21:46:09 +0100), Benoit Boissinot <benoit.boissinot@ens-lyon.org> says:

> IPv6: do not wrap around when the lifetime has expired
> 
> Instead of reporting overly large lifetimes to userspace,
> report a lifetime of 0 when a lifetime has expired.
> 
> Signed-off-by: Benoit Boissinot <benoit.boissinot@ens-lyon.org>

NAK.

(signed) lifetime < 0 means it has expired, but 0 does not mean
that the lifetime has expired, but it is being expired
(within 1 second).

--yoshfuji

Re: [PATCH 1 of 5] IPv6: do not wrap around when the lifetime has expired

[Benoit Boissinot]

On Fri, Mar 28, 2008 at 03:25:39AM +0900, YOSHIFUJI Hideaki / 吉藤英明 wrote:
> In article <dd125a7f86968371046d.1206305169@pirzuine> (at Sun, 23 Mar 2008 21:46:09 +0100), Benoit Boissinot <benoit.boissinot@ens-lyon.org> says:
> 
> > IPv6: do not wrap around when the lifetime has expired
> > 
> > Instead of reporting overly large lifetimes to userspace,
> > report a lifetime of 0 when a lifetime has expired.
> > 
> > Signed-off-by: Benoit Boissinot <benoit.boissinot@ens-lyon.org>
> 
> NAK.
> 
> (signed) lifetime < 0 means it has expired, but 0 does not mean
> that the lifetime has expired, but it is being expired
> (within 1 second).

It makes sense, so is the output of ip addr correct ?

inet6 2a01:5d8:58a0:ebfc:b5fb:88a3:27a5:ce96/64 scope global secondary deprecated dynamic
   valid_lft 84064sec preferred_lft 4294964960sec

Can iproute2 be "fixed" ? or is it the expected output ?

thanks,

Benoit

-- 
:wq

Re: [PATCH 1 of 5] IPv6: do not wrap around when the lifetime has expired

[YOSHIFUJI Hideaki / 吉藤英明]

In article <20080327193854.GC8574@ens-lyon.fr> (at Thu, 27 Mar 2008 20:38:54 +0100), Benoit Boissinot <benoit.boissinot@ens-lyon.org> says:

> On Fri, Mar 28, 2008 at 03:25:39AM +0900, YOSHIFUJI Hideaki / 吉藤英明 wrote:
> > In article <dd125a7f86968371046d.1206305169@pirzuine> (at Sun, 23 Mar 2008 21:46:09 +0100), Benoit Boissinot <benoit.boissinot@ens-lyon.org> says:
> > 
> > > IPv6: do not wrap around when the lifetime has expired
> > > 
> > > Instead of reporting overly large lifetimes to userspace,
> > > report a lifetime of 0 when a lifetime has expired.
> > > 
> > > Signed-off-by: Benoit Boissinot <benoit.boissinot@ens-lyon.org>
> > 
> > NAK.
> > 
> > (signed) lifetime < 0 means it has expired, but 0 does not mean
> > that the lifetime has expired, but it is being expired
> > (within 1 second).
> 
> It makes sense, so is the output of ip addr correct ?
> 
> inet6 2a01:5d8:58a0:ebfc:b5fb:88a3:27a5:ce96/64 scope global secondary deprecated dynamic
>    valid_lft 84064sec preferred_lft 4294964960sec
> 
> Can iproute2 be "fixed" ? or is it the expected output ?

Please fix iproute2.

--yoshfuji

Re: [PATCH 1 of 5] IPv6: do not wrap around when the lifetime has expired

[Benoit Boissinot]

Ok, I think I missed something.

On Fri, Mar 28, 2008 at 12:06:22PM +0900, YOSHIFUJI Hideaki / 吉藤英明 wrote:
> In article <20080327193854.GC8574@ens-lyon.fr> (at Thu, 27 Mar 2008 20:38:54 +0100), Benoit Boissinot <benoit.boissinot@ens-lyon.org> says:
> 
> > On Fri, Mar 28, 2008 at 03:25:39AM +0900, YOSHIFUJI Hideaki / 吉藤英明 wrote:
> > > In article <dd125a7f86968371046d.1206305169@pirzuine> (at Sun, 23 Mar 2008 21:46:09 +0100), Benoit Boissinot <benoit.boissinot@ens-lyon.org> says:
> > > 
> > > > IPv6: do not wrap around when the lifetime has expired
> > > > 
> > > > Instead of reporting overly large lifetimes to userspace,
> > > > report a lifetime of 0 when a lifetime has expired.
> > > > 
> > > > Signed-off-by: Benoit Boissinot <benoit.boissinot@ens-lyon.org>
> > > 
> > > NAK.
> > > 
> > > (signed) lifetime < 0 means it has expired, but 0 does not mean
> > > that the lifetime has expired, but it is being expired
> > > (within 1 second).

I re-checked the code, and while it is true that some path takes care
not to use a too big lifetime (one that is > 0x7fffffffUL/HZ), for
example:
- inet6_addr_{add,modify}
there are other places where there are no checks, eg:
- addrconf_prefix_rcv, ipv6_create_tempaddr

Furthermore, at least for the case of the preferred lifetime, a lifetime
of 0 received from RA means deprecated (== expired ?).
And due to the rounding, if age = (now - ift->tsamp)/HZ
then (age == lifetime) being true means that the lifetime has fully expired right ?

In any case, the current code is buggy, because if preferred == 1 and tval == 2,
then we will send INFINITY_LIFETIME to userspace.

Finally, cstamp and tstamp are sent via netlink, it could be useful to
solve this issue from userspace, but it's useless as it is since the
tstamp sent is relative to the lifetime stored in the kernel, not
relative to the lifetime we send (we should send "jiffies" for it to be
useful).

regards,

Benoit
-- 
:wq

[PATCH 2 of 5] IPv6: only update the lifetime of the relevant temporary address

[Benoit Boissinot]

IPv6: only update the lifetime of the relevant temporary address

When receiving a prefix information from a routeur, only update the lifetimes
of the temporary address associated with that prefix.
Otherwise if one deprecated prefix is advertized, all your temporary addresses
will become deprecated.

Signed-off-by: Benoit Boissinot <benoit.boissinot@ens-lyon.org>

diff -r dd125a7f8696 -r 5e50b19fbb75 net/ipv6/addrconf.c
--- a/net/ipv6/addrconf.c	Sat Mar 22 00:50:21 2008 +0100
+++ b/net/ipv6/addrconf.c	Sat Mar 22 00:41:39 2008 +0100
@@ -1832,6 +1832,10 @@
 				 * when processing a Prefix Information Option.
 				 */
 				spin_lock(&ift->lock);
+				if (ifp != ift->ifpub) {
+					spin_unlock(&ift->lock);
+					continue;
+				}
 				flags = ift->flags;
 				if (ift->valid_lft > valid_lft &&
 				    ift->valid_lft - valid_lft > (jiffies - ift->tstamp) / HZ)

Re: [PATCH 2 of 5] IPv6: only update the lifetime of the relevant temporary address

[YOSHIFUJI Hideaki / 吉藤英明]

In article <5e50b19fbb757f080905.1206305170@pirzuine> (at Sun, 23 Mar 2008 21:46:10 +0100), Benoit Boissinot <benoit.boissinot@ens-lyon.org> says:

> IPv6: only update the lifetime of the relevant temporary address
> 
> When receiving a prefix information from a routeur, only update the lifetimes
> of the temporary address associated with that prefix.
> Otherwise if one deprecated prefix is advertized, all your temporary addresses
> will become deprecated.
> 
> Signed-off-by: Benoit Boissinot <benoit.boissinot@ens-lyon.org>

I generally agree.

> diff -r dd125a7f8696 -r 5e50b19fbb75 net/ipv6/addrconf.c
> --- a/net/ipv6/addrconf.c	Sat Mar 22 00:50:21 2008 +0100
> +++ b/net/ipv6/addrconf.c	Sat Mar 22 00:41:39 2008 +0100
> @@ -1832,6 +1832,10 @@
>  				 * when processing a Prefix Information Option.
>  				 */
>  				spin_lock(&ift->lock);
> +				if (ifp != ift->ifpub) {
> +					spin_unlock(&ift->lock);
> +					continue;
> +				}
>  				flags = ift->flags;
>  				if (ift->valid_lft > valid_lft &&
>  				    ift->valid_lft - valid_lft > (jiffies - ift->tstamp) / HZ)

I think you can compare ift->ifpub outside the lock here.

--yoshfuji

[PATCH 3 of 5] IPv6: do not create temporary adresses with too short preferred lifetime

[Benoit Boissinot]

IPv6: do not create temporary adresses with too short preferred lifetime

>From RFC341:
A temporary address is created only if this calculated Preferred
Lifetime is greater than REGEN_ADVANCE time units.  In particular,
an implementation must not create a temporary address with a zero
Preferred Lifetime.

Signed-off-by: Benoit Boissinot <benoit.boissinot@ens-lyon.org>

diff -r 5e50b19fbb75 -r 268e602e1908 net/ipv6/addrconf.c
--- a/net/ipv6/addrconf.c	Sat Mar 22 00:41:39 2008 +0100
+++ b/net/ipv6/addrconf.c	Sat Mar 22 00:39:16 2008 +0100
@@ -776,6 +776,7 @@
 	struct inet6_dev *idev = ifp->idev;
 	struct in6_addr addr, *tmpaddr;
 	unsigned long tmp_prefered_lft, tmp_valid_lft, tmp_cstamp, tmp_tstamp;
+	unsigned int regen_advance;
 	int tmp_plen;
 	int ret = 0;
 	int max_addresses;
@@ -834,9 +835,24 @@
 	max_addresses = idev->cnf.max_addresses;
 	tmp_cstamp = ifp->cstamp;
 	tmp_tstamp = ifp->tstamp;
+	regen_advance = idev->cnf.regen_max_retry *
+	                idev->cnf.dad_transmits *
+	                idev->nd_parms->retrans_time / HZ;
 	spin_unlock_bh(&ifp->lock);
 
 	write_unlock(&idev->lock);
+
+	/* A temporary address is created only if this calculated Preferred
+	 * Lifetime is greater than REGEN_ADVANCE time units.  In particular,
+	 * an implementation must not create a temporary address with a zero
+	 * Preferred Lifetime.
+	 */
+	if (tmp_prefered_lft <= regen_advance) {
+		in6_ifa_put(ifp);
+		in6_dev_put(idev);
+		ret = -1;
+		goto out;
+	}
 
 	addr_flags = IFA_F_TEMPORARY;
 	/* set in addrconf_prefix_rcv() */

Re: [PATCH 3 of 5] IPv6: do not create temporary adresses with too short preferred lifetime

[YOSHIFUJI Hideaki / 吉藤英明]

In article <268e602e19080797193e.1206305171@pirzuine> (at Sun, 23 Mar 2008 21:46:11 +0100), Benoit Boissinot <benoit.boissinot@ens-lyon.org> says:

> IPv6: do not create temporary adresses with too short preferred lifetime
> 
> >From RFC341:
> A temporary address is created only if this calculated Preferred
> Lifetime is greater than REGEN_ADVANCE time units.  In particular,
> an implementation must not create a temporary address with a zero
> Preferred Lifetime.
> 
> Signed-off-by: Benoit Boissinot <benoit.boissinot@ens-lyon.org>
> 
> diff -r 5e50b19fbb75 -r 268e602e1908 net/ipv6/addrconf.c
> --- a/net/ipv6/addrconf.c	Sat Mar 22 00:41:39 2008 +0100
> +++ b/net/ipv6/addrconf.c	Sat Mar 22 00:39:16 2008 +0100
> @@ -776,6 +776,7 @@
>  	struct inet6_dev *idev = ifp->idev;
>  	struct in6_addr addr, *tmpaddr;
>  	unsigned long tmp_prefered_lft, tmp_valid_lft, tmp_cstamp, tmp_tstamp;
> +	unsigned int regen_advance;

unsigned long

>  	int tmp_plen;
>  	int ret = 0;
>  	int max_addresses;
> @@ -834,9 +835,24 @@
>  	max_addresses = idev->cnf.max_addresses;
>  	tmp_cstamp = ifp->cstamp;
>  	tmp_tstamp = ifp->tstamp;
> +	regen_advance = idev->cnf.regen_max_retry *
> +	                idev->cnf.dad_transmits *
> +	                idev->nd_parms->retrans_time / HZ;
>  	spin_unlock_bh(&ifp->lock);

regen_advance can be outside of the lock of ifp.

>  	write_unlock(&idev->lock);
> +
> +	/* A temporary address is created only if this calculated Preferred
> +	 * Lifetime is greater than REGEN_ADVANCE time units.  In particular,
> +	 * an implementation must not create a temporary address with a zero
> +	 * Preferred Lifetime.
> +	 */
> +	if (tmp_prefered_lft <= regen_advance) {
> +		in6_ifa_put(ifp);
> +		in6_dev_put(idev);
> +		ret = -1;
> +		goto out;
> +	}
>  
>  	addr_flags = IFA_F_TEMPORARY;
>  	/* set in addrconf_prefix_rcv() */

Otherwise, I agree.

--yoshfuji

[PATCH 4 of 5] IPv6: fix lifetime calculation on temporary address creation

[Benoit Boissinot]

IPv6: fix lifetime calculation on temporary address creation

The lifetime calculation was buggy since it copied the tstamp
from the associated public address.
If (now - ifp->prefered_lft)/HZ (ie the elapsed time since the
timestamp was set in the public address) was greater than
temp_prefered_lft, you would always get deprecated addresses.

This patch corrects the lifetime calculation by setting the tstamp
to "now" and calculating the remaining time from the public address.

Signed-off-by: Benoit Boissinot <benoit.boissinot@ens-lyon.org>

diff -r 268e602e1908 -r ec551b4a5bb2 net/ipv6/addrconf.c
--- a/net/ipv6/addrconf.c	Sat Mar 22 00:39:16 2008 +0100
+++ b/net/ipv6/addrconf.c	Sat Mar 22 00:38:40 2008 +0100
@@ -775,8 +775,8 @@
 {
 	struct inet6_dev *idev = ifp->idev;
 	struct in6_addr addr, *tmpaddr;
-	unsigned long tmp_prefered_lft, tmp_valid_lft, tmp_cstamp, tmp_tstamp;
-	unsigned int regen_advance;
+	unsigned int tmp_prefered_lft, tmp_valid_lft, elapsed, regen_advance;
+	unsigned long now;
 	int tmp_plen;
 	int ret = 0;
 	int max_addresses;
@@ -825,16 +825,22 @@
 		goto out;
 	}
 	memcpy(&addr.s6_addr[8], idev->rndid, 8);
-	tmp_valid_lft = min_t(__u32,
-			      ifp->valid_lft,
-			      idev->cnf.temp_valid_lft);
-	tmp_prefered_lft = min_t(__u32,
-				 ifp->prefered_lft,
-				 idev->cnf.temp_prefered_lft - desync_factor / HZ);
+	now = jiffies;
+	elapsed = (now - ifp->tstamp) / HZ;
+	if (elapsed >= ifp->valid_lft)
+		tmp_valid_lft = 0;
+	else
+		tmp_valid_lft = min_t(__u32,
+				      ifp->valid_lft - elapsed,
+				      idev->cnf.temp_valid_lft);
+	if (elapsed >= ifp->prefered_lft)
+		tmp_prefered_lft = 0;
+	else
+		tmp_prefered_lft = min_t(__u32,
+					 ifp->prefered_lft - elapsed,
+					 idev->cnf.temp_prefered_lft - desync_factor / HZ);
 	tmp_plen = ifp->prefix_len;
 	max_addresses = idev->cnf.max_addresses;
-	tmp_cstamp = ifp->cstamp;
-	tmp_tstamp = ifp->tstamp;
 	regen_advance = idev->cnf.regen_max_retry *
 	                idev->cnf.dad_transmits *
 	                idev->nd_parms->retrans_time / HZ;
@@ -878,8 +884,6 @@
 	ift->ifpub = ifp;
 	ift->valid_lft = tmp_valid_lft;
 	ift->prefered_lft = tmp_prefered_lft;
-	ift->cstamp = tmp_cstamp;
-	ift->tstamp = tmp_tstamp;
 	spin_unlock_bh(&ift->lock);
 
 	addrconf_dad_start(ift, 0);

Re: [PATCH 4 of 5] IPv6: fix lifetime calculation on temporary address creation

[YOSHIFUJI Hideaki / 吉藤英明]

In article <ec551b4a5bb25cda00ff.1206305172@pirzuine> (at Sun, 23 Mar 2008 21:46:12 +0100), Benoit Boissinot <benoit.boissinot@ens-lyon.org> says:

> IPv6: fix lifetime calculation on temporary address creation
> 
> The lifetime calculation was buggy since it copied the tstamp
> from the associated public address.
> If (now - ifp->prefered_lft)/HZ (ie the elapsed time since the
> timestamp was set in the public address) was greater than
> temp_prefered_lft, you would always get deprecated addresses.
> 
> This patch corrects the lifetime calculation by setting the tstamp
> to "now" and calculating the remaining time from the public address.
> 
> Signed-off-by: Benoit Boissinot <benoit.boissinot@ens-lyon.org>
:
>  	memcpy(&addr.s6_addr[8], idev->rndid, 8);
> -	tmp_valid_lft = min_t(__u32,
> -			      ifp->valid_lft,
> -			      idev->cnf.temp_valid_lft);
> -	tmp_prefered_lft = min_t(__u32,
> -				 ifp->prefered_lft,
> -				 idev->cnf.temp_prefered_lft - desync_factor / HZ);
> +	now = jiffies;
> +	elapsed = (now - ifp->tstamp) / HZ;
> +	if (elapsed >= ifp->valid_lft)
> +		tmp_valid_lft = 0;
> +	else
> +		tmp_valid_lft = min_t(__u32,
> +				      ifp->valid_lft - elapsed,
> +				      idev->cnf.temp_valid_lft);
> +	if (elapsed >= ifp->prefered_lft)
> +		tmp_prefered_lft = 0;
> +	else
> +		tmp_prefered_lft = min_t(__u32,
> +					 ifp->prefered_lft - elapsed,
> +					 idev->cnf.temp_prefered_lft - desync_factor / HZ);

Basically I agree, but it is possible to expire the temporary
address AFTER public address, which is not good.  Please fix this.

--yoshfuji

Re: [PATCH 4 of 5] IPv6: fix lifetime calculation on temporary address creation

[Benoit Boissinot]

On Fri, Mar 28, 2008 at 12:04:01PM +0900, YOSHIFUJI Hideaki / 吉藤英明 wrote:
> In article <ec551b4a5bb25cda00ff.1206305172@pirzuine> (at Sun, 23 Mar 2008 21:46:12 +0100), Benoit Boissinot <benoit.boissinot@ens-lyon.org> says:
> 
> > IPv6: fix lifetime calculation on temporary address creation
[snip]
> > +	now = jiffies;
> > +	elapsed = (now - ifp->tstamp) / HZ;
> > +	if (elapsed >= ifp->valid_lft)
> > +		tmp_valid_lft = 0;
> > +	else
> > +		tmp_valid_lft = min_t(__u32,
> > +				      ifp->valid_lft - elapsed,
> > +				      idev->cnf.temp_valid_lft);
> > +	if (elapsed >= ifp->prefered_lft)
> > +		tmp_prefered_lft = 0;
> > +	else
> > +		tmp_prefered_lft = min_t(__u32,
> > +					 ifp->prefered_lft - elapsed,
> > +					 idev->cnf.temp_prefered_lft - desync_factor / HZ);
> 
> Basically I agree, but it is possible to expire the temporary
> address AFTER public address, which is not good.  Please fix this.

do you mean because of the rounding of 'elapsed' ? otherwise I don't see
what the problem is, sorry.

regards,

Benoit

-- 
:wq

Re: [PATCH 4 of 5] IPv6: fix lifetime calculation on temporary address creation

[YOSHIFUJI Hideaki / 吉藤英明]

In article <20080401215657.GP475@pirzuine> (at Tue, 1 Apr 2008 23:56:57 +0200), Benoit Boissinot <benoit.boissinot@ens-lyon.org> says:

> On Fri, Mar 28, 2008 at 12:04:01PM +0900, YOSHIFUJI Hideaki / 吉藤英明 wrote:
> > In article <ec551b4a5bb25cda00ff.1206305172@pirzuine> (at Sun, 23 Mar 2008 21:46:12 +0100), Benoit Boissinot <benoit.boissinot@ens-lyon.org> says:
> > 
> > > IPv6: fix lifetime calculation on temporary address creation
> [snip]
> > > +	now = jiffies;
> > > +	elapsed = (now - ifp->tstamp) / HZ;
> > > +	if (elapsed >= ifp->valid_lft)
> > > +		tmp_valid_lft = 0;
> > > +	else
> > > +		tmp_valid_lft = min_t(__u32,
> > > +				      ifp->valid_lft - elapsed,
> > > +				      idev->cnf.temp_valid_lft);
> > > +	if (elapsed >= ifp->prefered_lft)
> > > +		tmp_prefered_lft = 0;
> > > +	else
> > > +		tmp_prefered_lft = min_t(__u32,
> > > +					 ifp->prefered_lft - elapsed,
> > > +					 idev->cnf.temp_prefered_lft - desync_factor / HZ);
> > 
> > Basically I agree, but it is possible to expire the temporary
> > address AFTER public address, which is not good.  Please fix this.
> 
> do you mean because of the rounding of 'elapsed' ? otherwise I don't see
> what the problem is, sorry.

Right.  Maybe we could substruct "now" by adj = (now - ifp->tstamp) % HZ;
   now = jiffies
   elapsed = (now - ifp->tstamp) / HZ;
   now -= (now - ifp->tstamp) % HZ;

--yoshfuji

Re: [PATCH 4 of 5] IPv6: fix lifetime calculation on temporary address creation

[Benoit Boissinot]

On Wed, Apr 02, 2008 at 08:41:14AM +0900, YOSHIFUJI Hideaki / 吉藤英明 wrote:
> In article <20080401215657.GP475@pirzuine> (at Tue, 1 Apr 2008 23:56:57 +0200), Benoit Boissinot <benoit.boissinot@ens-lyon.org> says:
> 
> > On Fri, Mar 28, 2008 at 12:04:01PM +0900, YOSHIFUJI Hideaki / 吉藤英明 wrote:
> > > In article <ec551b4a5bb25cda00ff.1206305172@pirzuine> (at Sun, 23 Mar 2008 21:46:12 +0100), Benoit Boissinot <benoit.boissinot@ens-lyon.org> says:
> > > 
> > > > IPv6: fix lifetime calculation on temporary address creation
> > [snip]
> > > > +	now = jiffies;
> > > > +	elapsed = (now - ifp->tstamp) / HZ;
> > > > +	if (elapsed >= ifp->valid_lft)
> > > > +		tmp_valid_lft = 0;
> > > > +	else
> > > > +		tmp_valid_lft = min_t(__u32,
> > > > +				      ifp->valid_lft - elapsed,
> > > > +				      idev->cnf.temp_valid_lft);
> > > > +	if (elapsed >= ifp->prefered_lft)
> > > > +		tmp_prefered_lft = 0;
> > > > +	else
> > > > +		tmp_prefered_lft = min_t(__u32,
> > > > +					 ifp->prefered_lft - elapsed,
> > > > +					 idev->cnf.temp_prefered_lft - desync_factor / HZ);
> > > 
> > > Basically I agree, but it is possible to expire the temporary
> > > address AFTER public address, which is not good.  Please fix this.
> > 
> > do you mean because of the rounding of 'elapsed' ? otherwise I don't see
> > what the problem is, sorry.
> 
> Right.  Maybe we could substruct "now" by adj = (now - ifp->tstamp) % HZ;
>    now = jiffies
>    elapsed = (now - ifp->tstamp) / HZ;
>    now -= (now - ifp->tstamp) % HZ;

Why not just simply round it above:
elapsed = (now - ifp->tstamp + HZ - 1) / HZ; /* round it up */
and the rest stays the same.

Or do we care about having a lifetime a little bit (<1s) shorter ?

updated patch below:

IPv6: fix lifetime calculation on temporary address creation

The lifetime calculation was buggy since it copied the tstamp
from the associated public address.
If (now - ifp->prefered_lft)/HZ (ie the elapsed time since the
timestamp was set in the public address) was greater than
temp_prefered_lft, you would always get deprecated addresses.

This patch corrects the lifetime calculation by setting the tstamp
to "now" and calculating the remaining time from the public address.

Signed-off-by: Benoit Boissinot <benoit.boissinot@ens-lyon.org>

diff -r 00affc24c178 net/ipv6/addrconf.c
--- a/net/ipv6/addrconf.c	Sat Mar 22 00:39:16 2008 +0100
+++ b/net/ipv6/addrconf.c	Wed Apr 02 02:14:13 2008 +0200
@@ -775,8 +775,7 @@
 {
 	struct inet6_dev *idev = ifp->idev;
 	struct in6_addr addr, *tmpaddr;
-	unsigned long tmp_prefered_lft, tmp_valid_lft, tmp_cstamp, tmp_tstamp;
-	unsigned long regen_advance;
+	unsigned long tmp_prefered_lft, tmp_valid_lft, elapsed, regen_advance;
 	int tmp_plen;
 	int ret = 0;
 	int max_addresses;
@@ -825,16 +824,21 @@
 		goto out;
 	}
 	memcpy(&addr.s6_addr[8], idev->rndid, 8);
-	tmp_valid_lft = min_t(__u32,
-			      ifp->valid_lft,
-			      idev->cnf.temp_valid_lft);
-	tmp_prefered_lft = min_t(__u32,
-				 ifp->prefered_lft,
-				 idev->cnf.temp_prefered_lft - desync_factor / HZ);
+	elapsed = (jiffies - ifp->tstamp + HZ - 1) / HZ; /* round above */
+	if (elapsed >= ifp->valid_lft)
+		tmp_valid_lft = 0;
+	else
+		tmp_valid_lft = min_t(__u32,
+				      ifp->valid_lft - elapsed,
+				      idev->cnf.temp_valid_lft);
+	if (elapsed >= ifp->prefered_lft)
+		tmp_prefered_lft = 0;
+	else
+		tmp_prefered_lft = min_t(__u32,
+					 ifp->prefered_lft - elapsed,
+					 idev->cnf.temp_prefered_lft - desync_factor / HZ);
 	tmp_plen = ifp->prefix_len;
 	max_addresses = idev->cnf.max_addresses;
-	tmp_cstamp = ifp->cstamp;
-	tmp_tstamp = ifp->tstamp;
 	spin_unlock_bh(&ifp->lock);
 
 	regen_advance = idev->cnf.regen_max_retry *
@@ -878,8 +882,6 @@
 	ift->ifpub = ifp;
 	ift->valid_lft = tmp_valid_lft;
 	ift->prefered_lft = tmp_prefered_lft;
-	ift->cstamp = tmp_cstamp;
-	ift->tstamp = tmp_tstamp;
 	spin_unlock_bh(&ift->lock);
 
 	addrconf_dad_start(ift, 0);

-- 
:wq

Re: [PATCH 4 of 5] IPv6: fix lifetime calculation on temporary address creation

[YOSHIFUJI Hideaki / 吉藤英明]

In article <20080402001706.GD32592@pirzuine> (at Wed, 2 Apr 2008 02:17:06 +0200), Benoit Boissinot <benoit.boissinot@ens-lyon.org> says:

> On Wed, Apr 02, 2008 at 08:41:14AM +0900, YOSHIFUJI Hideaki / 吉藤英明 wrote:
> > In article <20080401215657.GP475@pirzuine> (at Tue, 1 Apr 2008 23:56:57 +0200), Benoit Boissinot <benoit.boissinot@ens-lyon.org> says:
> > 
> > > On Fri, Mar 28, 2008 at 12:04:01PM +0900, YOSHIFUJI Hideaki / 吉藤英明 wrote:
> > > > In article <ec551b4a5bb25cda00ff.1206305172@pirzuine> (at Sun, 23 Mar 2008 21:46:12 +0100), Benoit Boissinot <benoit.boissinot@ens-lyon.org> says:
> > > > 
> > > > > IPv6: fix lifetime calculation on temporary address creation
> > > [snip]
> > > > > +	now = jiffies;
> > > > > +	elapsed = (now - ifp->tstamp) / HZ;
> > > > > +	if (elapsed >= ifp->valid_lft)
> > > > > +		tmp_valid_lft = 0;
> > > > > +	else
> > > > > +		tmp_valid_lft = min_t(__u32,
> > > > > +				      ifp->valid_lft - elapsed,
> > > > > +				      idev->cnf.temp_valid_lft);
> > > > > +	if (elapsed >= ifp->prefered_lft)
> > > > > +		tmp_prefered_lft = 0;
> > > > > +	else
> > > > > +		tmp_prefered_lft = min_t(__u32,
> > > > > +					 ifp->prefered_lft - elapsed,
> > > > > +					 idev->cnf.temp_prefered_lft - desync_factor / HZ);
> > > > 
> > > > Basically I agree, but it is possible to expire the temporary
> > > > address AFTER public address, which is not good.  Please fix this.
> > > 
> > > do you mean because of the rounding of 'elapsed' ? otherwise I don't see
> > > what the problem is, sorry.
> > 
> > Right.  Maybe we could substruct "now" by adj = (now - ifp->tstamp) % HZ;
> >    now = jiffies
> >    elapsed = (now - ifp->tstamp) / HZ;
> >    now -= (now - ifp->tstamp) % HZ;
> 
> Why not just simply round it above:
> elapsed = (now - ifp->tstamp + HZ - 1) / HZ; /* round it up */
> and the rest stays the same.
> 
> Or do we care about having a lifetime a little bit (<1s) shorter ?

Yes, I do.

--yoshfuji

Re: [PATCH 4 of 5] IPv6: fix lifetime calculation on temporary address creation

[Pekka Savola]

On Wed, 2 Apr 2008, YOSHIFUJI Hideaki / ???? wrote:
>> Why not just simply round it above:
>> elapsed = (now - ifp->tstamp + HZ - 1) / HZ; /* round it up */
>> and the rest stays the same.
>>
>> Or do we care about having a lifetime a little bit (<1s) shorter ?
>
> Yes, I do.

Did I understand correctly that this issue is about the 0..1second 
interval being (for simplicity sake) being a bit shorter than exactly 
1 second?

Is there a good reason why this would be a problem?

I observe that the granularity (and I one could argue the accuracy) of 
Valid/Preferred Lifetime information is one second, so I'm not sure if 
the user should expect accuracy that's much finer than that (e.g. 0.1 
seconds).  Typically Valid/Preferred lifetimes are configured to be in 
the order of hours or even days, so what happens (with 0.1-0.01 second 
accuracy) during the last second doesn't seem very interesting from 
the practical point of view.

-- 
Pekka Savola                 "You each name yourselves king, yet the
Netcore Oy                    kingdom bleeds."
Systems. Networks. Security. -- George R.R. Martin: A Clash of Kings

[PATCH 5 of 5] IPv6: temporary address: update the timer for tentative addresses

[Benoit Boissinot]

IPv6: temporary address: update the timer for tentative addresses

Even for tentative addresses, we should update the timer,
otherwise if the preferred lifetime is too short we will
miss the opportunity to regenerate the adress.

Signed-off-by: Benoit Boissinot <benoit.boissinot@ens-lyon.org>

diff -r ec551b4a5bb2 -r 4765830d9f38 net/ipv6/addrconf.c
--- a/net/ipv6/addrconf.c	Sat Mar 22 00:38:40 2008 +0100
+++ b/net/ipv6/addrconf.c	Sun Mar 23 21:33:12 2008 +0100
@@ -3003,9 +3003,9 @@
 					goto restart;
 				}
 #ifdef CONFIG_IPV6_PRIVACY
-			} else if ((ifp->flags&IFA_F_TEMPORARY) &&
-				   !(ifp->flags&IFA_F_TENTATIVE)) {
-				if (age >= ifp->prefered_lft - regen_advance) {
+			} else if (ifp->flags&IFA_F_TEMPORARY) {
+				if (!(ifp->flags&IFA_F_TENTATIVE) &&
+				    age >= ifp->prefered_lft - regen_advance) {
 					struct inet6_ifaddr *ifpub = ifp->ifpub;
 					if (time_before(ifp->tstamp + ifp->prefered_lft * HZ, next))
 						next = ifp->tstamp + ifp->prefered_lft * HZ;

Re: [PATCH 5 of 5] IPv6: temporary address: update the timer for tentative addresses

[YOSHIFUJI Hideaki / 吉藤英明]

In article <4765830d9f3848ef14e6.1206305173@pirzuine> (at Sun, 23 Mar 2008 21:46:13 +0100), Benoit Boissinot <benoit.boissinot@ens-lyon.org> says:

> IPv6: temporary address: update the timer for tentative addresses
> 
> Even for tentative addresses, we should update the timer,
> otherwise if the preferred lifetime is too short we will
> miss the opportunity to regenerate the adress.

Well, but it this ever happens, it implies that the lifetime
is too short, doesn't it?
We don't want such temporary address too much.

--yoshfuji

Re: [PATCH 5 of 5] IPv6: temporary address: update the timer for tentative addresses

[Benoit Boissinot]

On Fri, Mar 28, 2008 at 12:25:58PM +0900, YOSHIFUJI Hideaki / 吉藤英明 wrote:
> In article <4765830d9f3848ef14e6.1206305173@pirzuine> (at Sun, 23 Mar 2008 21:46:13 +0100), Benoit Boissinot <benoit.boissinot@ens-lyon.org> says:
> 
> > IPv6: temporary address: update the timer for tentative addresses
> > 
> > Even for tentative addresses, we should update the timer,
> > otherwise if the preferred lifetime is too short we will
> > miss the opportunity to regenerate the adress.
> 
> Well, but it this ever happens, it implies that the lifetime
> is too short, doesn't it?
> We don't want such temporary address too much.

I do agree. In this case, short means less than ADDR_CHECK_FREQUENCY
(2 minutes), and it might be too aggressive.
In fact, I don't really know what are the consequences of running
with a very short regen for temporary addresses (let's say for example,
1 minutes preferred lifetime, and 5 minutes valid lifetime).

But it was really useful for me for testing it, and it fixes a bug
(anyway if you use too short lifetimes you'll hit the max addresses
limit really fast).

regards,

Benoit

ps: I'll resubmit the patches to which you already agreed soon.

-- 
:wq

Re: [PATCH 0 of 5] IPv6: Fix temporary address support

[Benoit Boissinot]

On Sun, Mar 23, 2008 at 09:46:08PM +0100, Benoit Boissinot wrote:
> This is the set of patches I needed to make IPv6 temporary addresses
> work in my setup.
> 
> [snip]
>
> Please review and consider applying.

[sorry for being too impatient]

Do you think there's a problem with those patches ? Or is it just a
problem of reviewer time/availability ?

I'd really like not to miss the merge window for them.

(the first one probably is only remotely IPv6 related, you could
probably review/apply it separately)

thanks,

Benoit

-- 
:wq

Re: [PATCH 0 of 5] IPv6: Fix temporary address support

[YOSHIFUJI Hideaki / 吉藤英明]

In article <20080327175839.GB8574@ens-lyon.fr> (at Thu, 27 Mar 2008 18:59:06 +0100), Benoit Boissinot <benoit.boissinot@ens-lyon.org> says:

> On Sun, Mar 23, 2008 at 09:46:08PM +0100, Benoit Boissinot wrote:
> > This is the set of patches I needed to make IPv6 temporary addresses
> > work in my setup.
> > 
> > [snip]
> >
> > Please review and consider applying.
> 
> [sorry for being too impatient]
> 
> Do you think there's a problem with those patches ? Or is it just a
> problem of reviewer time/availability ?
> 
> I'd really like not to miss the merge window for them.

We have no merge window for bug fixes.
I'll take a look at it, anyway.

--yoshfuji

Re: [PATCH 0 of 5] IPv6: Fix temporary address support

[Benoit Boissinot]

On Fri, Mar 28, 2008 at 03:12:40AM +0900, YOSHIFUJI Hideaki / 吉藤英明 wrote:
> In article <20080327175839.GB8574@ens-lyon.fr> (at Thu, 27 Mar 2008 18:59:06 +0100), Benoit Boissinot <benoit.boissinot@ens-lyon.org> says:
> 
> > On Sun, Mar 23, 2008 at 09:46:08PM +0100, Benoit Boissinot wrote:
> > > This is the set of patches I needed to make IPv6 temporary addresses
> > > work in my setup.
> > > 
> > > [snip]
> > >
> > > Please review and consider applying.
> > 
> > [sorry for being too impatient]
> > 
> > Do you think there's a problem with those patches ? Or is it just a
> > problem of reviewer time/availability ?
> > 
> > I'd really like not to miss the merge window for them.
> 
> We have no merge window for bug fixes.
> I'll take a look at it, anyway.

Thank you! I've fixed the patches for the comments you've sent.

Once someone finds the time to review the last two patches, I'll resend
them.

Do you have an opinion about my PS (DESYNC_FACTOR not being random) ?

regards,

Benoit

-- 
:wq

Re: [PATCH 0 of 5] IPv6: Fix temporary address support

[YOSHIFUJI Hideaki / 吉藤英明]

In article <20080328003221.GE8574@ens-lyon.fr> (at Fri, 28 Mar 2008 01:32:21 +0100), Benoit Boissinot <benoit.boissinot@ens-lyon.org> says:

> Do you have an opinion about my PS (DESYNC_FACTOR not being random) ?

Oh, that's a bug ... and, max_desync_factor is not effective.

--yoshfuji

Re: [PATCH 0 of 5] IPv6: Fix temporary address support

[Benoit Boissinot]

On Fri, Mar 28, 2008 at 12:15:40PM +0900, YOSHIFUJI Hideaki / 吉藤英明 wrote:
> In article <20080328003221.GE8574@ens-lyon.fr> (at Fri, 28 Mar 2008 01:32:21 +0100), Benoit Boissinot <benoit.boissinot@ens-lyon.org> says:
> 
> > Do you have an opinion about my PS (DESYNC_FACTOR not being random) ?
> 
> Oh, that's a bug ...

Ok. What is the best way to generate a random number ? get_random_bytes
and modulo ? (the distribution won't be ok, but I guess it's not really
a problem for this case).

> and, max_desync_factor is not effective.

what do you mean by that ?

regards,

Benoit
-- 
:wq