* [PATCH] ipv6: fix incorrect route 'expires' value passed to userspace.
@ 2012-07-16 8:09 Li Wei
2012-07-16 9:56 ` David Miller
2012-07-16 16:41 ` [PATCH] " Stephen Hemminger
0 siblings, 2 replies; 19+ messages in thread
From: Li Wei @ 2012-07-16 8:09 UTC (permalink / raw)
To: David S. Miller; +Cc: netdev, Stephen Hemminger
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(-)
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 21318d1..f92f3d8 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -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);
}
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index becb048..a7fec9d 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -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;
--
1.7.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] ipv6: fix incorrect route 'expires' value passed to userspace.
2012-07-16 8:09 [PATCH] ipv6: fix incorrect route 'expires' value passed to userspace Li Wei
@ 2012-07-16 9:56 ` David Miller
2012-07-17 1:55 ` Li Wei
2012-07-19 2:02 ` [PATCH V2] " Li Wei
2012-07-16 16:41 ` [PATCH] " Stephen Hemminger
1 sibling, 2 replies; 19+ messages in thread
From: David Miller @ 2012-07-16 9:56 UTC (permalink / raw)
To: lw; +Cc: netdev, shemminger
Without a proper signoff, I won't apply your patch.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] ipv6: fix incorrect route 'expires' value passed to userspace.
2012-07-16 8:09 [PATCH] ipv6: fix incorrect route 'expires' value passed to userspace Li Wei
2012-07-16 9:56 ` David Miller
@ 2012-07-16 16:41 ` Stephen Hemminger
2012-07-17 1:53 ` Li Wei
2012-07-17 5:26 ` David Miller
1 sibling, 2 replies; 19+ messages in thread
From: Stephen Hemminger @ 2012-07-16 16:41 UTC (permalink / raw)
To: Li Wei; +Cc: David S. Miller, netdev
On Mon, 16 Jul 2012 16:09:37 +0800
Li Wei <lw@cn.fujitsu.com> wrote:
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index becb048..a7fec9d 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -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?
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] ipv6: fix incorrect route 'expires' value passed to userspace.
2012-07-16 16:41 ` [PATCH] " Stephen Hemminger
@ 2012-07-17 1:53 ` Li Wei
2012-07-17 5:26 ` David Miller
1 sibling, 0 replies; 19+ messages in thread
From: Li Wei @ 2012-07-17 1:53 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: David S. Miller, netdev
于 2012-7-17 0:41, Stephen Hemminger 写道:
> On Mon, 16 Jul 2012 16:09:37 +0800
> Li Wei <lw@cn.fujitsu.com> wrote:
>
>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
>> index becb048..a7fec9d 100644
>> --- a/net/ipv6/route.c
>> +++ b/net/ipv6/route.c
>> @@ -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.
>
>
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] ipv6: fix incorrect route 'expires' value passed to userspace.
2012-07-16 9:56 ` David Miller
@ 2012-07-17 1:55 ` Li Wei
2012-07-19 2:02 ` [PATCH V2] " Li Wei
1 sibling, 0 replies; 19+ messages in thread
From: Li Wei @ 2012-07-17 1:55 UTC (permalink / raw)
To: David Miller; +Cc: netdev, shemminger
于 2012-7-16 17:56, 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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] ipv6: fix incorrect route 'expires' value passed to userspace.
2012-07-16 16:41 ` [PATCH] " Stephen Hemminger
2012-07-17 1:53 ` Li Wei
@ 2012-07-17 5:26 ` David Miller
1 sibling, 0 replies; 19+ messages in thread
From: David Miller @ 2012-07-17 5:26 UTC (permalink / raw)
To: shemminger; +Cc: lw, netdev
From: Stephen Hemminger <shemminger@vyatta.com>
Date: Mon, 16 Jul 2012 09:41:24 -0700
> On Mon, 16 Jul 2012 16:09:37 +0800
> Li Wei <lw@cn.fujitsu.com> wrote:
>
>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
>> index becb048..a7fec9d 100644
>> --- a/net/ipv6/route.c
>> +++ b/net/ipv6/route.c
>> @@ -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.
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH V2] ipv6: fix incorrect route 'expires' value passed to userspace
2012-07-16 9:56 ` David Miller
2012-07-17 1:55 ` Li Wei
@ 2012-07-19 2:02 ` Li Wei
2012-07-19 17:49 ` David Miller
1 sibling, 1 reply; 19+ messages in thread
From: Li Wei @ 2012-07-19 2:02 UTC (permalink / raw)
To: David Miller; +Cc: netdev, shemminger
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 <lw@cn.fujitsu.com>
---
net/core/rtnetlink.c | 3 ++-
net/ipv6/route.c | 8 +++++---
2 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 21318d1..f92f3d8 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -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);
}
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index becb048..78266c3 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -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;
--
1.7.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH V2] ipv6: fix incorrect route 'expires' value passed to userspace
2012-07-19 2:02 ` [PATCH V2] " Li Wei
@ 2012-07-19 17:49 ` David Miller
2012-07-20 1:32 ` Li Wei
2012-07-20 1:42 ` [PATCH V2 resend] " Li Wei
0 siblings, 2 replies; 19+ messages in thread
From: David Miller @ 2012-07-19 17:49 UTC (permalink / raw)
To: lw; +Cc: netdev, shemminger
From: Li Wei <lw@cn.fujitsu.com>
Date: Thu, 19 Jul 2012 10:02:59 +0800
>
> 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 <lw@cn.fujitsu.com>
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.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH V2] ipv6: fix incorrect route 'expires' value passed to userspace
2012-07-19 17:49 ` David Miller
@ 2012-07-20 1:32 ` Li Wei
2012-07-20 1:42 ` [PATCH V2 resend] " Li Wei
1 sibling, 0 replies; 19+ messages in thread
From: Li Wei @ 2012-07-20 1:32 UTC (permalink / raw)
To: David Miller; +Cc: netdev, shemminger
于 2012-7-20 1:49, David Miller 写道:
> From: Li Wei <lw@cn.fujitsu.com>
> Date: Thu, 19 Jul 2012 10:02:59 +0800
>
>>
>> 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 <lw@cn.fujitsu.com>
>
> 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
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH V2 resend] ipv6: fix incorrect route 'expires' value passed to userspace
2012-07-19 17:49 ` David Miller
2012-07-20 1:32 ` Li Wei
@ 2012-07-20 1:42 ` Li Wei
2012-07-20 10:32 ` David Laight
1 sibling, 1 reply; 19+ messages in thread
From: Li Wei @ 2012-07-20 1:42 UTC (permalink / raw)
To: David Miller; +Cc: netdev, shemminger
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 <lw@cn.fujitsu.com>
---
net/core/rtnetlink.c | 3 ++-
net/ipv6/route.c | 7 ++++---
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 21318d1..f92f3d8 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -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);
}
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index becb048..7875255 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -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;
--
1.7.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* RE: [PATCH V2 resend] ipv6: fix incorrect route 'expires' value passed to userspace
2012-07-20 1:42 ` [PATCH V2 resend] " Li Wei
@ 2012-07-20 10:32 ` David Laight
2012-07-20 18:22 ` David Miller
2012-07-23 1:02 ` [PATCH V2 resend] " Li Wei
0 siblings, 2 replies; 19+ messages in thread
From: David Laight @ 2012-07-20 10:32 UTC (permalink / raw)
To: Li Wei, David Miller; +Cc: netdev, shemminger
> - 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;
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH V2 resend] ipv6: fix incorrect route 'expires' value passed to userspace
2012-07-20 10:32 ` David Laight
@ 2012-07-20 18:22 ` David Miller
2012-07-23 1:05 ` Li Wei
2012-07-25 5:25 ` [PATCH V3] " Li Wei
2012-07-23 1:02 ` [PATCH V2 resend] " Li Wei
1 sibling, 2 replies; 19+ messages in thread
From: David Miller @ 2012-07-20 18:22 UTC (permalink / raw)
To: David.Laight; +Cc: lw, netdev, shemminger
From: "David Laight" <David.Laight@ACULAB.COM>
Date: Fri, 20 Jul 2012 11:32:05 +0100
>> - 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;
>
> 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.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH V2 resend] ipv6: fix incorrect route 'expires' value passed to userspace
2012-07-20 10:32 ` David Laight
2012-07-20 18:22 ` David Miller
@ 2012-07-23 1:02 ` Li Wei
1 sibling, 0 replies; 19+ messages in thread
From: Li Wei @ 2012-07-23 1:02 UTC (permalink / raw)
To: David Laight; +Cc: David Miller, netdev, shemminger
On 07/20/2012 06:32 PM, David Laight wrote:
>> - 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;
>
> 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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH V2 resend] ipv6: fix incorrect route 'expires' value passed to userspace
2012-07-20 18:22 ` David Miller
@ 2012-07-23 1:05 ` Li Wei
2012-07-25 5:25 ` [PATCH V3] " Li Wei
1 sibling, 0 replies; 19+ messages in thread
From: Li Wei @ 2012-07-23 1:05 UTC (permalink / raw)
To: David Miller; +Cc: David.Laight, netdev, shemminger
On 07/21/2012 02:22 AM, David Miller wrote:
> From: "David Laight" <David.Laight@ACULAB.COM>
> Date: Fri, 20 Jul 2012 11:32:05 +0100
>
>>> - 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;
>>
>> 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.
I'll redone this patch base on 'net-next'.
Thanks
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH V3] ipv6: fix incorrect route 'expires' value passed to userspace
2012-07-20 18:22 ` David Miller
2012-07-23 1:05 ` Li Wei
@ 2012-07-25 5:25 ` Li Wei
2012-07-25 6:51 ` Eric Dumazet
1 sibling, 1 reply; 19+ messages in thread
From: Li Wei @ 2012-07-25 5:25 UTC (permalink / raw)
To: David Miller; +Cc: David.Laight, netdev, shemminger
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 <lw@cn.fujitsu.com>
---
net/core/rtnetlink.c | 3 ++-
net/ipv6/route.c | 11 ++++++-----
2 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 334b930..2e96396 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -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);
}
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index cf02cb9..6efeb28 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -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;
--
1.7.10.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH V3] ipv6: fix incorrect route 'expires' value passed to userspace
2012-07-25 5:25 ` [PATCH V3] " Li Wei
@ 2012-07-25 6:51 ` Eric Dumazet
2012-07-25 7:33 ` Li Wei
2012-07-30 2:01 ` [PATCH v4] " Li Wei
0 siblings, 2 replies; 19+ messages in thread
From: Eric Dumazet @ 2012-07-25 6:51 UTC (permalink / raw)
To: Li Wei; +Cc: David Miller, David.Laight, netdev, shemminger
On Wed, 2012-07-25 at 13:25 +0800, Li Wei wrote:
> 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 <lw@cn.fujitsu.com>
> ---
> net/core/rtnetlink.c | 3 ++-
> net/ipv6/route.c | 11 ++++++-----
> 2 files changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 334b930..2e96396 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -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);
> }
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index cf02cb9..6efeb28 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -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;
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.
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 334b930..c1c950b 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -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);
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index cf02cb9..8e80fd2 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -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;
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH V3] ipv6: fix incorrect route 'expires' value passed to userspace
2012-07-25 6:51 ` Eric Dumazet
@ 2012-07-25 7:33 ` Li Wei
2012-07-30 2:01 ` [PATCH v4] " Li Wei
1 sibling, 0 replies; 19+ messages in thread
From: Li Wei @ 2012-07-25 7:33 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, David.Laight, netdev, shemminger
On 07/25/2012 02:51 PM, Eric Dumazet wrote:
> On Wed, 2012-07-25 at 13:25 +0800, Li Wei wrote:
>> 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 <lw@cn.fujitsu.com>
>> ---
>> net/core/rtnetlink.c | 3 ++-
>> net/ipv6/route.c | 11 ++++++-----
>> 2 files changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
>> index 334b930..2e96396 100644
>> --- a/net/core/rtnetlink.c
>> +++ b/net/core/rtnetlink.c
>> @@ -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);
>> }
>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
>> index cf02cb9..6efeb28 100644
>> --- a/net/ipv6/route.c
>> +++ b/net/ipv6/route.c
>> @@ -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;
>
> 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.
Ok, I got it.
I tested the following patch, got the correct expires value and not found
any problem.
Thanks Eric :)
>
>
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 334b930..c1c950b 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -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);
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index cf02cb9..8e80fd2 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -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;
>
>
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v4] ipv6: fix incorrect route 'expires' value passed to userspace
2012-07-25 6:51 ` Eric Dumazet
2012-07-25 7:33 ` Li Wei
@ 2012-07-30 2:01 ` Li Wei
2012-07-30 6:20 ` David Miller
1 sibling, 1 reply; 19+ messages in thread
From: Li Wei @ 2012-07-30 2:01 UTC (permalink / raw)
To: davem; +Cc: eric.dumazet, netdev, 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.
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 <lw@cn.fujitsu.com>
---
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(-)
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index bc9e380..5ff949d 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -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);
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index cf02cb9..8e80fd2 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -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;
--
1.7.10.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v4] ipv6: fix incorrect route 'expires' value passed to userspace
2012-07-30 2:01 ` [PATCH v4] " Li Wei
@ 2012-07-30 6:20 ` David Miller
0 siblings, 0 replies; 19+ messages in thread
From: David Miller @ 2012-07-30 6:20 UTC (permalink / raw)
To: lw; +Cc: eric.dumazet, netdev
From: Li Wei <lw@cn.fujitsu.com>
Date: Mon, 30 Jul 2012 10:01:30 +0800
> 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 <lw@cn.fujitsu.com>
Applied.
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2012-07-30 6:20 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-07-16 8:09 [PATCH] ipv6: fix incorrect route 'expires' value passed to userspace Li Wei
2012-07-16 9:56 ` David Miller
2012-07-17 1:55 ` Li Wei
2012-07-19 2:02 ` [PATCH V2] " Li Wei
2012-07-19 17:49 ` David Miller
2012-07-20 1:32 ` Li Wei
2012-07-20 1:42 ` [PATCH V2 resend] " Li Wei
2012-07-20 10:32 ` David Laight
2012-07-20 18:22 ` David Miller
2012-07-23 1:05 ` Li Wei
2012-07-25 5:25 ` [PATCH V3] " Li Wei
2012-07-25 6:51 ` Eric Dumazet
2012-07-25 7:33 ` Li Wei
2012-07-30 2:01 ` [PATCH v4] " Li Wei
2012-07-30 6:20 ` David Miller
2012-07-23 1:02 ` [PATCH V2 resend] " Li Wei
2012-07-16 16:41 ` [PATCH] " Stephen Hemminger
2012-07-17 1:53 ` Li Wei
2012-07-17 5:26 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).