* [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 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
* [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 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
* 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] 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 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
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).