IPv6: Fix temporary address support

PATCH In review Benoit Boissinot 3 participants 25 messages Mar 23, 2008 – Apr 2, 2008

Subsystems (3)

Networking Network drivers IPv4/IPv6

lore

Share & tools

lore

Benoit Boissinot

author

2008-03-23 20:46 UTC #0 lore raw

PATCH 1/1 IPv6: Fix temporary address support

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 ?

Benoit Boissinot

author

2008-03-23 20:46 UTC #1 lore raw

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

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

Quoted 1 line

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 <[email protected]>

net/ipv6/addrconf.c +16 -0

diff -r 5e50b19fbb75 -r 268e602e1908 net/ipv6/addrconf.c
@@ -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() */

YOSHIFUJI Hideaki / 吉藤英明

2008-03-27 18:38 UTC #2 lore raw

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

In article <268e602e19080797193e.1206305171@pirzuine> (at Sun, 23 Mar 2008 21:46:11 +0100), Benoit Boissinot <[email protected]> says:

Quoted 14 lines

IPv6: do not create temporary adresses with too short preferred lifetime
Quoted 1 line
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 <[email protected]>
diff -r 5e50b19fbb75 -r 268e602e1908 net/ipv6/addrconf.c

net/ipv6/addrconf.c line 779

Quoted 4 lines

net/ipv6/addrconf.c +1 -0

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

net/ipv6/addrconf.c line 841

Quoted 11 lines

net/ipv6/addrconf.c +3 -0

 	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.

Quoted 6 lines

net/ipv6/addrconf.c +5 -0

 	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.

net/ipv6/addrconf.c line 858

Quoted 10 lines

net/ipv6/addrconf.c +7 -0

+	 */
+	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

Benoit Boissinot

author

2008-03-23 20:46 UTC #3 lore raw

PATCH 1/1 IPv6: only update the lifetime of the relevant temporary address

In reply to 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 <[email protected]>

net/ipv6/addrconf.c +4 -0

diff -r dd125a7f8696 -r 5e50b19fbb75 net/ipv6/addrconf.c
@@ -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)

YOSHIFUJI Hideaki / 吉藤英明

2008-03-27 18:30 UTC #4 lore raw

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

In article <5e50b19fbb757f080905.1206305170@pirzuine> (at Sun, 23 Mar 2008 21:46:10 +0100), Benoit Boissinot <[email protected]> says:

Quoted 8 lines

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 <[email protected]>

I generally agree.

Quoted 4 lines

diff -r dd125a7f8696 -r 5e50b19fbb75 net/ipv6/addrconf.c

net/ipv6/addrconf.c line 1841

Quoted 10 lines

net/ipv6/addrconf.c +4 -0

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

Benoit Boissinot

author

2008-03-23 20:46 UTC #5 lore raw

PATCH 1/1 IPv6: do not wrap around when the lifetime has expired

In reply to 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 <[email protected]>

net/ipv6/addrconf.c +11 -4

diff -r c6c17f6335a1 -r dd125a7f8696 net/ipv6/addrconf.c
@@ -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;

YOSHIFUJI Hideaki / 吉藤英明

NAK

2008-03-27 18:24 UTC #6 lore raw

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

In article <dd125a7f86968371046d.1206305169@pirzuine> (at Sun, 23 Mar 2008 21:46:09 +0100), Benoit Boissinot <[email protected]> says:

Quoted 6 lines

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 <[email protected]>

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

Benoit Boissinot

author

2008-03-27 19:38 UTC #7 lore raw

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

Quoted 7 lines · YOSHIFUJI Hideaki / 吉藤英明

In article <dd125a7f86968371046d.1206305169@pirzuine> (at Sun, 23 Mar 2008 21:46:09 +0100), Benoit Boissinot <[email protected]> says:
Quoted 5 lines
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.

Quoted 7 lines

Quoted 1 line
Signed-off-by Benoit Boissinot <[email protected]>
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

Signature · 1 line

:wq

YOSHIFUJI Hideaki / 吉藤英明

2008-03-28 03:04 UTC #8 lore raw

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

In article <[email protected]> (at Thu, 27 Mar 2008 20:38:54 +0100), Benoit Boissinot <[email protected]> says:

Quoted 16 lines · YOSHIFUJI Hideaki / 吉藤英明

Quoted 7 lines
In article <dd125a7f86968371046d.1206305169@pirzuine> (at Sun, 23 Mar 2008 21:46:09 +0100), Benoit Boissinot <[email protected]> says:
Quoted 5 lines
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.
Quoted 7 lines
Quoted 1 line
Signed-off-by Benoit Boissinot <[email protected]>
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).

Quoted 6 lines

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

Benoit Boissinot

author

2008-04-01 22:44 UTC #9 lore raw

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

Ok, I think I missed something.

Quoted 17 lines · YOSHIFUJI Hideaki / 吉藤英明

In article <[email protected]> (at Thu, 27 Mar 2008 20:38:54 +0100), Benoit Boissinot <[email protected]> says:
Quoted 15 lines · YOSHIFUJI Hideaki / 吉藤英明
Quoted 14 lines
In article <dd125a7f86968371046d.1206305169@pirzuine> (at Sun, 23 Mar 2008 21:46:09 +0100), Benoit Boissinot <[email protected]> says:
Quoted 6 lines
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 <[email protected]>
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

Signature · 1 line

:wq

Benoit Boissinot

author

2008-03-23 20:46 UTC #10 lore raw

PATCH 1/1 IPv6: fix lifetime calculation on temporary address creation

In reply to 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 <[email protected]>

net/ipv6/addrconf.c +16 ~~-12~~

diff -r 268e602e1908 -r ec551b4a5bb2 net/ipv6/addrconf.c
@@ -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);

YOSHIFUJI Hideaki / 吉藤英明

2008-03-28 03:02 UTC #11 lore raw

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

In article <ec551b4a5bb25cda00ff.1206305172@pirzuine> (at Sun, 23 Mar 2008 21:46:12 +0100), Benoit Boissinot <[email protected]> says:

Quoted 8 lines

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.

Quoted 4 lines

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 <[email protected]>

Quoted 11 lines

net/ipv6/addrconf.c +4 -6

 	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;

net/ipv6/addrconf.c line 841

Quoted 10 lines

net/ipv6/addrconf.c +10 -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

Benoit Boissinot

author

2008-04-01 21:57 UTC #12 lore raw

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

Quoted 3 lines · YOSHIFUJI Hideaki / 吉藤英明

In article <ec551b4a5bb25cda00ff.1206305172@pirzuine> (at Sun, 23 Mar 2008 21:46:12 +0100), Benoit Boissinot <[email protected]> says:
Quoted 1 line
IPv6: fix lifetime calculation on temporary address creation

[snip]

Quoted 9 lines

net/ipv6/addrconf.c +9 -0

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

Quoted 8 lines

net/ipv6/addrconf.c line 841
Quoted 5 lines
net/ipv6/addrconf.c +5 -0
+		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

Signature · 1 line

:wq

YOSHIFUJI Hideaki / 吉藤英明

2008-04-01 23:39 UTC #13 lore raw

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

In article <20080401215657.GP475@pirzuine> (at Tue, 1 Apr 2008 23:56:57 +0200), Benoit Boissinot <[email protected]> says:

Quoted 23 lines · YOSHIFUJI Hideaki / 吉藤英明

Quoted 3 lines
In article <ec551b4a5bb25cda00ff.1206305172@pirzuine> (at Sun, 23 Mar 2008 21:46:12 +0100), Benoit Boissinot <[email protected]> says:
Quoted 1 line
IPv6: fix lifetime calculation on temporary address creation
[snip]
Quoted 9 lines
Quoted 9 lines
net/ipv6/addrconf.c +9 -0
+	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)
Quoted 8 lines
net/ipv6/addrconf.c line 841
Quoted 5 lines
net/ipv6/addrconf.c +5 -0
+		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.

Quoted 2 lines

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

Benoit Boissinot

author

2008-04-02 00:17 UTC #14 lore raw

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

Quoted 25 lines · YOSHIFUJI Hideaki / 吉藤英明

In article <20080401215657.GP475@pirzuine> (at Tue, 1 Apr 2008 23:56:57 +0200), Benoit Boissinot <[email protected]> says:
Quoted 23 lines · YOSHIFUJI Hideaki / 吉藤英明
Quoted 3 lines
In article <ec551b4a5bb25cda00ff.1206305172@pirzuine> (at Sun, 23 Mar 2008 21:46:12 +0100), Benoit Boissinot <[email protected]> says:
Quoted 1 line
IPv6: fix lifetime calculation on temporary address creation
[snip]
Quoted 17 lines
Quoted 4 lines
(quoted diff) +4 -0
+	now = jiffies;
+	elapsed = (now - ifp->tstamp) / HZ;
+	if (elapsed >= ifp->valid_lft)
+		tmp_valid_lft = 0;
Review
Quoted 10 lines
(quoted diff) +10 -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.

Quoted 7 lines

Quoted 2 lines
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

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 <[email protected]>

net/ipv6/addrconf.c +14 ~~-12~~

diff -r 00affc24c178 net/ipv6/addrconf.c
@@ -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);

Signature · 1 line

:wq

YOSHIFUJI Hideaki / 吉藤英明

2008-04-02 00:23 UTC #15 lore raw

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

In article <20080402001706.GD32592@pirzuine> (at Wed, 2 Apr 2008 02:17:06 +0200), Benoit Boissinot <[email protected]> says:

Quoted 34 lines · YOSHIFUJI Hideaki / 吉藤英明

Quoted 25 lines
In article <20080401215657.GP475@pirzuine> (at Tue, 1 Apr 2008 23:56:57 +0200), Benoit Boissinot <[email protected]> says:
Quoted 23 lines · YOSHIFUJI Hideaki / 吉藤英明
Quoted 3 lines
In article <ec551b4a5bb25cda00ff.1206305172@pirzuine> (at Sun, 23 Mar 2008 21:46:12 +0100), Benoit Boissinot <[email protected]> says:
Quoted 1 line
IPv6: fix lifetime calculation on temporary address creation
[snip]
Quoted 17 lines
Quoted 4 lines
net/ipv6/addrconf.c +4 -0
+	now = jiffies;
+	elapsed = (now - ifp->tstamp) / HZ;
+	if (elapsed >= ifp->valid_lft)
+		tmp_valid_lft = 0;
net/ipv6/addrconf.c line 839
Quoted 10 lines
net/ipv6/addrconf.c +10 -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.
Quoted 7 lines
Quoted 2 lines
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;

Quoted 5 lines

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

Pekka Savola

2008-04-02 05:57 UTC #16 lore raw

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

Quoted 7 lines · YOSHIFUJI Hideaki / ????

Quoted 5 lines
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.

Signature · 3 lines

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

Benoit Boissinot

author

2008-03-23 20:46 UTC #17 lore raw

PATCH 1/1 IPv6: temporary address: update the timer for tentative addresses

In reply to 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 <[email protected]>

net/ipv6/addrconf.c +3 -3

diff -r ec551b4a5bb2 -r 4765830d9f38 net/ipv6/addrconf.c
@@ -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;

YOSHIFUJI Hideaki / 吉藤英明

2008-03-28 03:24 UTC #18 lore raw

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

In article <4765830d9f3848ef14e6.1206305173@pirzuine> (at Sun, 23 Mar 2008 21:46:13 +0100), Benoit Boissinot <[email protected]> says:

Quoted 5 lines

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

Benoit Boissinot

author

2008-04-01 21:53 UTC #19 lore raw

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

Quoted 11 lines · YOSHIFUJI Hideaki / 吉藤英明

In article <4765830d9f3848ef14e6.1206305173@pirzuine> (at Sun, 23 Mar 2008 21:46:13 +0100), Benoit Boissinot <[email protected]> says:
Quoted 5 lines
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.

Signature · 1 line

:wq

Benoit Boissinot

author

2008-03-27 17:59 UTC #20 lore raw

In reply to Benoit Boissinot

Quoted 6 lines · Benoit Boissinot

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

Signature · 1 line

:wq

YOSHIFUJI Hideaki / 吉藤英明

2008-03-27 18:11 UTC #21 lore raw

In article <[email protected]> (at Thu, 27 Mar 2008 18:59:06 +0100), Benoit Boissinot <[email protected]> says:

Quoted 6 lines · Benoit Boissinot

Quoted 5 lines
This is the set of patches I needed to make IPv6 temporary addresses work in my setup.
[snip]

Quoted 8 lines

Quoted 1 line
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

Benoit Boissinot

author

2008-03-28 00:32 UTC #22 lore raw

Quoted 12 lines · YOSHIFUJI Hideaki / 吉藤英明

In article <[email protected]> (at Thu, 27 Mar 2008 18:59:06 +0100), Benoit Boissinot <[email protected]> says:
Quoted 10 lines · Benoit Boissinot
Quoted 6 lines
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]

Quoted 7 lines

Quoted 4 lines
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

Signature · 1 line

:wq

YOSHIFUJI Hideaki / 吉藤英明

2008-03-28 03:14 UTC #23 lore raw

In article <[email protected]> (at Fri, 28 Mar 2008 01:32:21 +0100), Benoit Boissinot <[email protected]> says:

Quoted 1 line

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

Benoit Boissinot

author

2008-04-01 21:55 UTC #24 lore raw

Quoted 5 lines · YOSHIFUJI Hideaki / 吉藤英明

In article <[email protected]> (at Fri, 28 Mar 2008 01:32:21 +0100), Benoit Boissinot <[email protected]> says:
Quoted 1 line
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).

Quoted 1 line

and, max_desync_factor is not effective.

what do you mean by that ?

regards,

Benoit

Signature · 1 line

:wq