ipv6: fix incorrect route 'expires' value passed to userspace.

PATCH Superseded Li Wei 5 participants 19 messages Jul 16, 2012 – Jul 30, 2012

Subsystems (3)

Networking Network drivers IPv4/IPv6

lore

Share & tools

lore

Li Wei

author

2012-07-16 08:10 UTC #0 lore raw

PATCH 1/1 ipv6: fix incorrect route 'expires' value passed to userspace.

When userspace use RTM_GETROUTE to dump route table, with a already expired route entry, we always got an 'expires' value(2147157) calculated base on INT_MAX.

The reason of this problem is in the following satement:

	rt->dst.expires - jiffies < INT_MAX

gcc promoted the type of both sides of '<' to unsigned long, thus a small negative value would be considered greater than INT_MAX.

This patch fix this by cast the result of subtraction to an 'int' which I think is large enough for the expires.

Also we should do some fix in rtnl_put_cacheinfo() which use jiffies_to_clock_t(which take an unsigned log as parameter) to convert jiffies to clock_t to handle the negative expires.

---
 net/core/rtnetlink.c |    3 ++-
 net/ipv6/route.c     |    2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

net/core/rtnetlink.c +2 -1

index 21318d1..f92f3d8 100644
@@ -641,7 +641,8 @@ int rtnl_put_cacheinfo(struct sk_buff *skb, struct dst_entry *dst, u32 id,
 	};
 
 	if (expires)
-		ci.rta_expires = jiffies_to_clock_t(expires);
+		ci.rta_expires = expires > 0 ? jiffies_to_clock_t(expires)
+			: -jiffies_to_clock_t(-expires);
 
 	return nla_put(skb, RTA_CACHEINFO, sizeof(ci), &ci);
 }

net/ipv6/route.c +1 -1

index becb048..a7fec9d 100644
@@ -2516,7 +2516,7 @@ static int rt6_fill_node(struct net *net,
 		goto nla_put_failure;
 	if (!(rt->rt6i_flags & RTF_EXPIRES))
 		expires = 0;
-	else if (rt->dst.expires - jiffies < INT_MAX)
+	else if ((int)(rt->dst.expires - jiffies) < INT_MAX)
 		expires = rt->dst.expires - jiffies;
 	else
 		expires = INT_MAX;

Signature · 1 line

1.7.1

David Miller

maintainer

2012-07-16 09:56 UTC #1 lore raw

Without a proper signoff, I won't apply your patch.

Li Wei

author

2012-07-17 01:56 UTC #2 lore raw

Quoted 4 lines · David Miller

Without a proper signoff, I won't apply your patch.

Sorry for that, I will append a signoff, do some code refactor and send a V2.

Thanks,
Wei

Li Wei

author

2012-07-19 02:41 UTC #3 lore raw

PATCH v2 1/1 ipv6: fix incorrect route 'expires' value passed to userspace

In reply to David Miller

When userspace use RTM_GETROUTE to dump route table, with an already expired route entry, we always got an 'expires' value(2147157) calculated base on INT_MAX.

The reason of this problem is in the following satement:

	rt->dst.expires - jiffies < INT_MAX

gcc promoted the type of both sides of '<' to unsigned long, thus a small negative value would be considered greater than INT_MAX.

This patch fix this by use the same trick as time_after macro to avoid the 'unsigned long' type promotion and deal with jiffies wrapping.

Also we should do some fix in rtnl_put_cacheinfo() which use jiffies_to_clock_t(which take an unsigned long as parameter) to convert jiffies to clock_t to handle the negative expires.

Signed-off-by Li Wei <[email protected]>

---
 net/core/rtnetlink.c |    3 ++-
 net/ipv6/route.c     |    8 +++++---
 2 files changed, 7 insertions(+), 4 deletions(-)

net/core/rtnetlink.c +2 -1

index 21318d1..f92f3d8 100644
@@ -641,7 +641,8 @@ int rtnl_put_cacheinfo(struct sk_buff *skb, struct dst_entry *dst, u32 id,
 	};
 
 	if (expires)
-		ci.rta_expires = jiffies_to_clock_t(expires);
+		ci.rta_expires = expires > 0 ? jiffies_to_clock_t(expires)
+			: -jiffies_to_clock_t(-expires);
 
 	return nla_put(skb, RTA_CACHEINFO, sizeof(ci), &ci);
 }

net/ipv6/route.c +4 -3

index becb048..78266c3 100644
@@ -2516,10 +2516,12 @@ static int rt6_fill_node(struct net *net,
 		goto nla_put_failure;
 	if (!(rt->rt6i_flags & RTF_EXPIRES))
 		expires = 0;
-	else if (rt->dst.expires - jiffies < INT_MAX)
-		expires = rt->dst.expires - jiffies;
+	else if ((long)rt->dst.expires - (long)jiffies > INT_MIN
+			&& (long)rt->dst.expires - (long)jiffies < INT_MAX)
+		expires = (long)rt->dst.expires - (long)jiffies;
 	else
-		expires = INT_MAX;
+		expires = time_is_after_jiffies(rt->dst.expires) ? INT_MAX : INT_MIN;
 
 	peer = rt->rt6i_peer;
 	ts = tsage = 0;

Signature · 1 line

1.7.1

David Miller

maintainer

2012-07-19 17:49 UTC #4 lore raw

Re: [PATCH V2] ipv6: fix incorrect route 'expires' value passed to userspace

From: Li Wei <[email protected]>
Date: Thu, 19 Jul 2012 10:02:59 +0800

Quoted 14 lines

When userspace use RTM_GETROUTE to dump route table, with an already expired route entry, we always got an 'expires' value(2147157) calculated base on INT_MAX.
The reason of this problem is in the following satement:
	rt->dst.expires - jiffies < INT_MAX
gcc promoted the type of both sides of '<' to unsigned long, thus a small negative value would be considered greater than INT_MAX.
This patch fix this by use the same trick as time_after macro to avoid the 'unsigned long' type promotion and deal with jiffies wrapping.

Quoted 5 lines

Also we should do some fix in rtnl_put_cacheinfo() which use jiffies_to_clock_t(which take an unsigned long as parameter) to convert jiffies to clock_t to handle the negative expires.
Signed-off-by Li Wei <[email protected]>

Your patch is corrupted by your email client and therefore will not apply cleanly.

I think this isn't the first time your patch submissions have had this problem, and if so then you should do the necessary work to prevent problem with more certainty in the future as such this makes a lot of extra work for other people.

Li Wei

author

2012-07-20 01:33 UTC #5 lore raw

Re: [PATCH V2] ipv6: fix incorrect route 'expires' value passed to userspace

Quoted 23 lines · David Miller

From: Li Wei <[email protected]>
Date: Thu, 19 Jul 2012 10:02:59 +0800
Quoted 14 lines
When userspace use RTM_GETROUTE to dump route table, with an already expired route entry, we always got an 'expires' value(2147157) calculated base on INT_MAX.
The reason of this problem is in the following satement:
	rt->dst.expires - jiffies < INT_MAX
gcc promoted the type of both sides of '<' to unsigned long, thus a small negative value would be considered greater than INT_MAX.
This patch fix this by use the same trick as time_after macro to avoid the 'unsigned long' type promotion and deal with jiffies wrapping.
Quoted 5 lines
Also we should do some fix in rtnl_put_cacheinfo() which use jiffies_to_clock_t(which take an unsigned long as parameter) to convert jiffies to clock_t to handle the negative expires.
Signed-off-by Li Wei <[email protected]>

Quoted 8 lines

Your patch is corrupted by your email client and therefore will not apply cleanly.
I think this isn't the first time your patch submissions have had this problem, and if so then you should do the necessary work to prevent problem with more certainty in the future as such this makes a lot of extra work for other people.

Really sorry for that, I'll resend this patch and before that sending myself a copy to confirm the mail client works properly.

Thanks

Li Wei

author

2012-07-20 01:43 UTC #6 lore raw

PATCH RESEND v2 1/1 ipv6: fix incorrect route 'expires' value passed to userspace

In reply to David Miller

When userspace use RTM_GETROUTE to dump route table, with an already expired route entry, we always got an 'expires' value(2147157) calculated base on INT_MAX.

The reason of this problem is in the following satement:

	rt->dst.expires - jiffies < INT_MAX

gcc promoted the type of both sides of '<' to unsigned long, thus a small negative value would be considered greater than INT_MAX.

This patch fix this by use the same trick as time_after macro to avoid the 'unsigned long' type promotion and deal with jiffies wrapping.

Also we should do some fix in rtnl_put_cacheinfo() which use jiffies_to_clock_t(which take an unsigned long as parameter) to convert jiffies to clock_t to handle the negative expires.

Signed-off-by Li Wei <[email protected]>

---
 net/core/rtnetlink.c |    3 ++-
 net/ipv6/route.c     |    7 ++++---
 2 files changed, 6 insertions(+), 4 deletions(-)

net/core/rtnetlink.c +2 -1

index 21318d1..f92f3d8 100644
@@ -641,7 +641,8 @@ int rtnl_put_cacheinfo(struct sk_buff *skb, struct dst_entry *dst, u32 id,
 	};
 
 	if (expires)
-		ci.rta_expires = jiffies_to_clock_t(expires);
+		ci.rta_expires = expires > 0 ? jiffies_to_clock_t(expires)
+			: -jiffies_to_clock_t(-expires);
 
 	return nla_put(skb, RTA_CACHEINFO, sizeof(ci), &ci);
 }

net/ipv6/route.c +4 -3

index becb048..7875255 100644
@@ -2516,10 +2516,11 @@ static int rt6_fill_node(struct net *net,
 		goto nla_put_failure;
 	if (!(rt->rt6i_flags & RTF_EXPIRES))
 		expires = 0;
-	else if (rt->dst.expires - jiffies < INT_MAX)
-		expires = rt->dst.expires - jiffies;
+	else if ((long)rt->dst.expires - (long)jiffies > INT_MIN
+			&& (long)rt->dst.expires - (long)jiffies < INT_MAX)
+		expires = (long)rt->dst.expires - (long)jiffies;
 	else
-		expires = INT_MAX;
+		expires = time_is_after_jiffies(rt->dst.expires) ? INT_MAX : INT_MIN;
 
 	peer = rt->rt6i_peer;
 	ts = tsage = 0;

Signature · 1 line

1.7.1

David Laight

2012-07-20 10:41 UTC #7 lore raw

RE: [PATCH V2 resend] ipv6: fix incorrect route 'expires' value passed to userspace

net/ipv6/route.c static int rt6_fill_node(struct net *net,

Quoted 4 lines

net/ipv6/route.c +2 -2

-	else if (rt->dst.expires - jiffies < INT_MAX)
-		expires = rt->dst.expires - jiffies;
+	else if ((long)rt->dst.expires - (long)jiffies > INT_MIN
+			&& (long)rt->dst.expires - (long)jiffies <

INT_MAX)

net/ipv6/route.c static int rt6_fill_node(struct net *net,

Quoted 4 lines

net/ipv6/route.c +2 -1

+		expires = (long)rt->dst.expires - (long)jiffies;
 	else
-		expires = INT_MAX;
+		expires = time_is_after_jiffies(rt->dst.expires) ?

INT_MAX : INT_MIN;

I can't help feeling there is a better way to do this. Maybe:

	long expires = rt->dst.expires - jiffies;
	if (expires != (int)expires)
		expires = expires > 0 ? INT_MAX : INT_MIN;

Although maybe -INT_MAX instead of INT_MIN.

David

David Miller

maintainer

2012-07-20 18:22 UTC #8 lore raw

Re: [PATCH V2 resend] ipv6: fix incorrect route 'expires' value passed to userspace

From: "David Laight" <[email protected]>
Date: Fri, 20 Jul 2012 11:32:05 +0100

Quoted 11 lines

net/ipv6/route.c static int rt6_fill_node(struct net *net,
Quoted 4 lines
net/ipv6/route.c +2 -2
-	else if (rt->dst.expires - jiffies < INT_MAX)
-		expires = rt->dst.expires - jiffies;
+	else if ((long)rt->dst.expires - (long)jiffies > INT_MIN
+			&& (long)rt->dst.expires - (long)jiffies <
INT_MAX)
net/ipv6/route.c static int rt6_fill_node(struct net *net,
Quoted 4 lines
net/ipv6/route.c +2 -1
+		expires = (long)rt->dst.expires - (long)jiffies;
 	else
-		expires = INT_MAX;
+		expires = time_is_after_jiffies(rt->dst.expires) ?
INT_MAX : INT_MIN;

Quoted 6 lines

I can't help feeling there is a better way to do this. Maybe:
	long expires = rt->dst.expires - jiffies;
	if (expires != (int)expires)
		expires = expires > 0 ? INT_MAX : INT_MIN;
Although maybe -INT_MAX instead of INT_MIN.

This patch also does not apply at all to net-next, so needs to be redone regardless.

Li Wei

author

2012-07-23 01:05 UTC #9 lore raw

Re: [PATCH V2 resend] ipv6: fix incorrect route 'expires' value passed to userspace

Quoted 21 lines · David Miller

From: "David Laight" <[email protected]>
Date: Fri, 20 Jul 2012 11:32:05 +0100
Quoted 11 lines
net/ipv6/route.c static int rt6_fill_node(struct net *net,
Quoted 4 lines
net/ipv6/route.c +2 -2
-	else if (rt->dst.expires - jiffies < INT_MAX)
-		expires = rt->dst.expires - jiffies;
+	else if ((long)rt->dst.expires - (long)jiffies > INT_MIN
+			&& (long)rt->dst.expires - (long)jiffies <
INT_MAX)
net/ipv6/route.c static int rt6_fill_node(struct net *net,
Quoted 4 lines
net/ipv6/route.c +2 -1
+		expires = (long)rt->dst.expires - (long)jiffies;
 	else
-		expires = INT_MAX;
+		expires = time_is_after_jiffies(rt->dst.expires) ?
INT_MAX : INT_MIN;
Quoted 6 lines
I can't help feeling there is a better way to do this. Maybe:
	long expires = rt->dst.expires - jiffies;
	if (expires != (int)expires)
		expires = expires > 0 ? INT_MAX : INT_MIN;
Although maybe -INT_MAX instead of INT_MIN.

Quoted 2 lines

This patch also does not apply at all to net-next, so needs to be redone regardless.

I'll redone this patch base on 'net-next'.

Thanks

Quoted 2 lines

Li Wei

author

2012-07-25 05:26 UTC #10 lore raw

PATCH v3 1/1 ipv6: fix incorrect route 'expires' value passed to userspace

In reply to David Miller

When userspace use RTM_GETROUTE to dump route table, with an already expired route entry, we always got an 'expires' value(2147157) calculated base on INT_MAX.

The reason of this problem is in the following satement:

	rt->dst.expires - jiffies < INT_MAX

gcc promoted the type of both sides of '<' to unsigned long, thus a small negative value would be considered greater than INT_MAX.

This patch fix this by use the same trick as time_after macro to avoid the 'unsigned long' type promotion and deal with jiffies wrapping.

Also we should do some fix in rtnl_put_cacheinfo() which use jiffies_to_clock_t(which take an unsigned long as parameter) to convert jiffies to clock_t to handle the negative expires.

With the help of David Laight, we can make the code a little clean.

Signed-off-by Li Wei <[email protected]>

---
 net/core/rtnetlink.c |    3 ++-
 net/ipv6/route.c     |   11 ++++++-----
 2 files changed, 8 insertions(+), 6 deletions(-)

net/core/rtnetlink.c +2 -1

index 334b930..2e96396 100644
@@ -626,7 +626,8 @@ int rtnl_put_cacheinfo(struct sk_buff *skb, struct dst_entry *dst, u32 id,
 	};
 
 	if (expires)
-		ci.rta_expires = jiffies_to_clock_t(expires);
+		ci.rta_expires = expires > 0 ? jiffies_to_clock_t(expires)
+			: -jiffies_to_clock_t(-expires);
 
 	return nla_put(skb, RTA_CACHEINFO, sizeof(ci), &ci);
 }

net/ipv6/route.c +6 -5

index cf02cb9..6efeb28 100644
@@ -2480,12 +2480,13 @@ static int rt6_fill_node(struct net *net,
 		goto nla_put_failure;
 	if (nla_put_u32(skb, RTA_PRIORITY, rt->rt6i_metric))
 		goto nla_put_failure;
-	if (!(rt->rt6i_flags & RTF_EXPIRES))
+	if (!(rt->rt6i_flags & RTF_EXPIRES)) {
 		expires = 0;
-	else if (rt->dst.expires - jiffies < INT_MAX)
-		expires = rt->dst.expires - jiffies;
-	else
-		expires = INT_MAX;
+	} else {
+		expires = (long)rt->dst.expires - (long)jiffies;
+		if (expires != (int)expires)
+			expires = expires > 0 ? INT_MAX : INT_MIN;
+	}
 
 	if (rtnl_put_cacheinfo(skb, &rt->dst, 0, expires, rt->dst.error) < 0)
 		goto nla_put_failure;

Signature · 1 line

1.7.10.1

Eric Dumazet

2012-07-25 06:52 UTC #11 lore raw

Re: [PATCH V3] ipv6: fix incorrect route 'expires' value passed to userspace

Quoted 52 lines · Li Wei

When userspace use RTM_GETROUTE to dump route table, with an already expired route entry, we always got an 'expires' value(2147157) calculated base on INT_MAX.
The reason of this problem is in the following satement:
	rt->dst.expires - jiffies < INT_MAX
gcc promoted the type of both sides of '<' to unsigned long, thus a small negative value would be considered greater than INT_MAX.
This patch fix this by use the same trick as time_after macro to avoid the 'unsigned long' type promotion and deal with jiffies wrapping.
Also we should do some fix in rtnl_put_cacheinfo() which use jiffies_to_clock_t(which take an unsigned long as parameter) to convert jiffies to clock_t to handle the negative expires.
With the help of David Laight, we can make the code a little clean.
Signed-off-by Li Wei <[email protected]>
---
 net/core/rtnetlink.c |    3 ++-
 net/ipv6/route.c     |   11 ++++++-----
 2 files changed, 8 insertions(+), 6 deletions(-)
net/core/rtnetlink.c +2 -1
index 334b930..2e96396 100644
@@ -626,7 +626,8 @@ int rtnl_put_cacheinfo(struct sk_buff *skb, struct dst_entry *dst, u32 id,
 	};
 
 	if (expires)
-		ci.rta_expires = jiffies_to_clock_t(expires);
+		ci.rta_expires = expires > 0 ? jiffies_to_clock_t(expires)
+			: -jiffies_to_clock_t(-expires);
 
 	return nla_put(skb, RTA_CACHEINFO, sizeof(ci), &ci);
 }
net/ipv6/route.c +1 -3
index cf02cb9..6efeb28 100644
@@ -2480,12 +2480,13 @@ static int rt6_fill_node(struct net *net,
 		goto nla_put_failure;
 	if (nla_put_u32(skb, RTA_PRIORITY, rt->rt6i_metric))
 		goto nla_put_failure;
-	if (!(rt->rt6i_flags & RTF_EXPIRES))
+	if (!(rt->rt6i_flags & RTF_EXPIRES)) {
 		expires = 0;
-	else if (rt->dst.expires - jiffies < INT_MAX)
-		expires = rt->dst.expires - jiffies;

net/ipv6/route.c static int rt6_fill_node(struct net *net, · line 2492

Quoted 10 lines

net/ipv6/route.c +5 -2

-	else
-		expires = INT_MAX;
+	} else {
+		expires = (long)rt->dst.expires - (long)jiffies;
+		if (expires != (int)expires)
+			expires = expires > 0 ? INT_MAX : INT_MIN;
+	}
 
 	if (rtnl_put_cacheinfo(skb, &rt->dst, 0, expires, rt->dst.error) < 0)
 		goto nla_put_failure;

All this sounds not very clean.

rtnl_put_cacheinfo( ... long expires ... )

Any out of bound checks should be done in rtnl_put_cacheinfo(), _after_ conversion to clock_t.

net/core/rtnetlink.c +6 -2

index 334b930..c1c950b 100644
@@ -625,9 +625,13 @@ int rtnl_put_cacheinfo(struct sk_buff *skb, struct dst_entry *dst, u32 id,
 		.rta_id =  id,
 	};
 
-	if (expires)
-		ci.rta_expires = jiffies_to_clock_t(expires);
+	if (expires) {
+		unsigned long clock;
 
+		clock = jiffies_to_clock_t(abs(expires));
+		clock = min_t(unsigned long, clock, INT_MAX);
+		ci.rta_expires = (expires > 0) ? clock : -clock;
+	}
 	return nla_put(skb, RTA_CACHEINFO, sizeof(ci), &ci);
 }
 EXPORT_SYMBOL_GPL(rtnl_put_cacheinfo);

net/ipv6/route.c +2 -6

index cf02cb9..8e80fd2 100644
@@ -2480,12 +2480,8 @@ static int rt6_fill_node(struct net *net,
 		goto nla_put_failure;
 	if (nla_put_u32(skb, RTA_PRIORITY, rt->rt6i_metric))
 		goto nla_put_failure;
-	if (!(rt->rt6i_flags & RTF_EXPIRES))
-		expires = 0;
-	else if (rt->dst.expires - jiffies < INT_MAX)
-		expires = rt->dst.expires - jiffies;
-	else
-		expires = INT_MAX;
+
+	expires = (rt->rt6i_flags & RTF_EXPIRES) ? rt->dst.expires - jiffies : 0;
 
 	if (rtnl_put_cacheinfo(skb, &rt->dst, 0, expires, rt->dst.error) < 0)
 		goto nla_put_failure;

Li Wei

author

2012-07-25 07:34 UTC #12 lore raw

Re: [PATCH V3] ipv6: fix incorrect route 'expires' value passed to userspace

Quoted 66 lines · Eric Dumazet

Quoted 52 lines · Li Wei
When userspace use RTM_GETROUTE to dump route table, with an already expired route entry, we always got an 'expires' value(2147157) calculated base on INT_MAX.
The reason of this problem is in the following satement:
	rt->dst.expires - jiffies < INT_MAX
gcc promoted the type of both sides of '<' to unsigned long, thus a small negative value would be considered greater than INT_MAX.
This patch fix this by use the same trick as time_after macro to avoid the 'unsigned long' type promotion and deal with jiffies wrapping.
Also we should do some fix in rtnl_put_cacheinfo() which use jiffies_to_clock_t(which take an unsigned long as parameter) to convert jiffies to clock_t to handle the negative expires.
With the help of David Laight, we can make the code a little clean.
Signed-off-by Li Wei <[email protected]>
---
 net/core/rtnetlink.c |    3 ++-
 net/ipv6/route.c     |   11 ++++++-----
 2 files changed, 8 insertions(+), 6 deletions(-)
net/core/rtnetlink.c +2 -1
index 334b930..2e96396 100644
@@ -626,7 +626,8 @@ int rtnl_put_cacheinfo(struct sk_buff *skb, struct dst_entry *dst, u32 id,
 	};
 
 	if (expires)
-		ci.rta_expires = jiffies_to_clock_t(expires);
+		ci.rta_expires = expires > 0 ? jiffies_to_clock_t(expires)
+			: -jiffies_to_clock_t(-expires);
 
 	return nla_put(skb, RTA_CACHEINFO, sizeof(ci), &ci);
 }
net/ipv6/route.c +1 -3
index cf02cb9..6efeb28 100644
@@ -2480,12 +2480,13 @@ static int rt6_fill_node(struct net *net,
 		goto nla_put_failure;
 	if (nla_put_u32(skb, RTA_PRIORITY, rt->rt6i_metric))
 		goto nla_put_failure;
-	if (!(rt->rt6i_flags & RTF_EXPIRES))
+	if (!(rt->rt6i_flags & RTF_EXPIRES)) {
 		expires = 0;
-	else if (rt->dst.expires - jiffies < INT_MAX)
-		expires = rt->dst.expires - jiffies;
net/ipv6/route.c static int rt6_fill_node(struct net *net, · line 2492
Quoted 10 lines
net/ipv6/route.c +5 -2
-	else
-		expires = INT_MAX;
+	} else {
+		expires = (long)rt->dst.expires - (long)jiffies;
+		if (expires != (int)expires)
+			expires = expires > 0 ? INT_MAX : INT_MIN;
+	}
 
 	if (rtnl_put_cacheinfo(skb, &rt->dst, 0, expires, rt->dst.error) < 0)
 		goto nla_put_failure;
All this sounds not very clean.

Quoted 4 lines

rtnl_put_cacheinfo( ... long expires ... )
Any out of bound checks should be done in rtnl_put_cacheinfo(), _after_ conversion to clock_t.

Ok, I got it.

I tested the following patch, got the correct expires value and not found any problem.

Thanks Eric :)

Quoted 45 lines

net/core/rtnetlink.c +6 -2

index 334b930..c1c950b 100644
@@ -625,9 +625,13 @@ int rtnl_put_cacheinfo(struct sk_buff *skb, struct dst_entry *dst, u32 id,
 		.rta_id =  id,
 	};
 
-	if (expires)
-		ci.rta_expires = jiffies_to_clock_t(expires);
+	if (expires) {
+		unsigned long clock;
 
+		clock = jiffies_to_clock_t(abs(expires));
+		clock = min_t(unsigned long, clock, INT_MAX);
+		ci.rta_expires = (expires > 0) ? clock : -clock;
+	}
 	return nla_put(skb, RTA_CACHEINFO, sizeof(ci), &ci);
 }
 EXPORT_SYMBOL_GPL(rtnl_put_cacheinfo);

net/ipv6/route.c +2 -6

index cf02cb9..8e80fd2 100644
@@ -2480,12 +2480,8 @@ static int rt6_fill_node(struct net *net,
 		goto nla_put_failure;
 	if (nla_put_u32(skb, RTA_PRIORITY, rt->rt6i_metric))
 		goto nla_put_failure;
-	if (!(rt->rt6i_flags & RTF_EXPIRES))
-		expires = 0;
-	else if (rt->dst.expires - jiffies < INT_MAX)
-		expires = rt->dst.expires - jiffies;
-	else
-		expires = INT_MAX;
+
+	expires = (rt->rt6i_flags & RTF_EXPIRES) ? rt->dst.expires - jiffies : 0;
 
 	if (rtnl_put_cacheinfo(skb, &rt->dst, 0, expires, rt->dst.error) < 0)
 		goto nla_put_failure;

Li Wei

author

2012-07-30 02:03 UTC #13 lore raw

PATCH v4 1/1 ipv6: fix incorrect route 'expires' value passed to userspace

In reply to Eric Dumazet

When userspace use RTM_GETROUTE to dump route table, with an already expired route entry, we always got an 'expires' value(2147157) calculated base on INT_MAX.

The reason of this problem is in the following satement:

	rt->dst.expires - jiffies < INT_MAX

gcc promoted the type of both sides of '<' to unsigned long, thus a small negative value would be considered greater than INT_MAX.

With the help of Eric Dumazet, do the out of bound checks in rtnl_put_cacheinfo(), _after_ conversion to clock_t.

Signed-off-by Li Wei <[email protected]>

---
In fact, all the code was reconstructed by Eric, I just put the commit log and resent it to the maillist, thanks Eric!

 net/core/rtnetlink.c |    8 ++++++--
 net/ipv6/route.c     |    8 ++------
 2 files changed, 8 insertions(+), 8 deletions(-)

net/core/rtnetlink.c +6 -2

index bc9e380..5ff949d 100644
@@ -625,9 +625,13 @@ int rtnl_put_cacheinfo(struct sk_buff *skb, struct dst_entry *dst, u32 id,
 		.rta_id =  id,
 	};
 
-	if (expires)
-		ci.rta_expires = jiffies_to_clock_t(expires);
+	if (expires) {
+		unsigned long clock;
 
+		clock = jiffies_to_clock_t(abs(expires));
+		clock = min_t(unsigned long, clock, INT_MAX);
+		ci.rta_expires = (expires > 0) ? clock : -clock;
+	}
 	return nla_put(skb, RTA_CACHEINFO, sizeof(ci), &ci);
 }
 EXPORT_SYMBOL_GPL(rtnl_put_cacheinfo);

net/ipv6/route.c +2 -6

index cf02cb9..8e80fd2 100644
@@ -2480,12 +2480,8 @@ static int rt6_fill_node(struct net *net,
 		goto nla_put_failure;
 	if (nla_put_u32(skb, RTA_PRIORITY, rt->rt6i_metric))
 		goto nla_put_failure;
-	if (!(rt->rt6i_flags & RTF_EXPIRES))
-		expires = 0;
-	else if (rt->dst.expires - jiffies < INT_MAX)
-		expires = rt->dst.expires - jiffies;
-	else
-		expires = INT_MAX;
+
+	expires = (rt->rt6i_flags & RTF_EXPIRES) ? rt->dst.expires - jiffies : 0;
 
 	if (rtnl_put_cacheinfo(skb, &rt->dst, 0, expires, rt->dst.error) < 0)
 		goto nla_put_failure;

Signature · 1 line

1.7.10.1

David Miller

maintainer Applied

2012-07-30 06:20 UTC #14 lore raw

Re: [PATCH v4] ipv6: fix incorrect route 'expires' value passed to userspace

From: Li Wei <[email protected]>
Date: Mon, 30 Jul 2012 10:01:30 +0800

Quoted 5 lines

When userspace use RTM_GETROUTE to dump route table, with an already expired route entry, we always got an 'expires' value(2147157) calculated base on INT_MAX.
The reason of this problem is in the following satement:

Quoted 8 lines

	rt->dst.expires - jiffies < INT_MAX
gcc promoted the type of both sides of '<' to unsigned long, thus a small negative value would be considered greater than INT_MAX.
With the help of Eric Dumazet, do the out of bound checks in rtnl_put_cacheinfo(), _after_ conversion to clock_t.
Signed-off-by Li Wei <[email protected]>

Applied.

Li Wei

author

2012-07-23 01:03 UTC #15 lore raw

Re: [PATCH V2 resend] ipv6: fix incorrect route 'expires' value passed to userspace

In reply to David Laight

Quoted 11 lines · David Laight

net/ipv6/route.c static int rt6_fill_node(struct net *net,
Quoted 4 lines
net/ipv6/route.c +2 -2
-	else if (rt->dst.expires - jiffies < INT_MAX)
-		expires = rt->dst.expires - jiffies;
+	else if ((long)rt->dst.expires - (long)jiffies > INT_MIN
+			&& (long)rt->dst.expires - (long)jiffies <
INT_MAX)
net/ipv6/route.c static int rt6_fill_node(struct net *net,
Quoted 4 lines
net/ipv6/route.c +2 -1
+		expires = (long)rt->dst.expires - (long)jiffies;
 	else
-		expires = INT_MAX;
+		expires = time_is_after_jiffies(rt->dst.expires) ?
INT_MAX : INT_MIN;

Quoted 9 lines

I can't help feeling there is a better way to do this. Maybe:
	long expires = rt->dst.expires - jiffies;
	if (expires != (int)expires)
		expires = expires > 0 ? INT_MAX : INT_MIN;
Although maybe -INT_MAX instead of INT_MIN.
David

Thanks David, your code looks much cleaner and can archieve the same function except we should use

	long expires = (long)rt->dst.expires - (long)jiffies;

to avoid the wrapping of jiffies.

Thanks,
Wei

Stephen Hemminger

2012-07-16 16:41 UTC #16 lore raw

In reply to Li Wei

net/ipv6/route.c static int rt6_fill_node(struct net *net, · line 2522

Quoted 13 lines · On Mon, 16 Jul 2012 16:09:37 +0800 Li Wei <[email protected]> wrote:

net/ipv6/route.c +1 -1

index becb048..a7fec9d 100644
@@ -2516,7 +2516,7 @@ static int rt6_fill_node(struct net *net,
 		goto nla_put_failure;
 	if (!(rt->rt6i_flags & RTF_EXPIRES))
 		expires = 0;
-	else if (rt->dst.expires - jiffies < INT_MAX)
+	else if ((int)(rt->dst.expires - jiffies) < INT_MAX)
 		expires = rt->dst.expires - jiffies;
 	else
 		expires = INT_MAX;

Why not use time_is_after_jiffies() macro?

Li Wei

author

2012-07-17 01:54 UTC #17 lore raw

Quoted 18 lines · Stephen Hemminger

net/ipv6/route.c static int rt6_fill_node(struct net *net, · line 2522
Quoted 13 lines · On Mon, 16 Jul 2012 16:09:37 +0800 Li Wei <[email protected]> wrote:
net/ipv6/route.c +1 -1
index becb048..a7fec9d 100644
@@ -2516,7 +2516,7 @@ static int rt6_fill_node(struct net *net,
 		goto nla_put_failure;
 	if (!(rt->rt6i_flags & RTF_EXPIRES))
 		expires = 0;
-	else if (rt->dst.expires - jiffies < INT_MAX)
+	else if ((int)(rt->dst.expires - jiffies) < INT_MAX)
 		expires = rt->dst.expires - jiffies;
 	else
 		expires = INT_MAX;
Why not use time_is_after_jiffies() macro?

time_is_after_jiffies() return a bool but we need "how much time before/after jiffies" here.

Quoted 2 lines

However, I also think these code seems a little ugly, because we need to store the result of two "unsigned long"'s subtraction into an integer. Maybe we should distinguish expires before and after jiffies to proper process the overflows.

Thanks,
Wei

David Miller

maintainer

2012-07-17 05:26 UTC #18 lore raw

In reply to Stephen Hemminger

From: Stephen Hemminger <[email protected]>
Date: Mon, 16 Jul 2012 09:41:24 -0700

Quoted 18 lines

net/ipv6/route.c static int rt6_fill_node(struct net *net, · line 2522
Quoted 13 lines · On Mon, 16 Jul 2012 16:09:37 +0800 Li Wei <[email protected]> wrote:
net/ipv6/route.c +1 -1
index becb048..a7fec9d 100644
@@ -2516,7 +2516,7 @@ static int rt6_fill_node(struct net *net,
 		goto nla_put_failure;
 	if (!(rt->rt6i_flags & RTF_EXPIRES))
 		expires = 0;
-	else if (rt->dst.expires - jiffies < INT_MAX)
+	else if ((int)(rt->dst.expires - jiffies) < INT_MAX)
 		expires = rt->dst.expires - jiffies;
 	else
 		expires = INT_MAX;
Why not use time_is_after_jiffies() macro?

This test has a wider window of acceptance.