On Fri, 19 Aug 2016, Eric Dumazet wrote:
> On Fri, 2016-08-19 at 18:41 +0200, Andrew Yourtchenko wrote:
> > This allows "ip -6 route save" to save the expiry for the routes
> > that have it, such that it can be correctly restored later by
> > "ip -6 route restore".
> >
> > If a route has RTF_EXPIRES set, generate RTA_EXPIRES value which
> > will be used to restore the flag and expiry value by already
> > existing code in rtm_to_fib6_config.
> >
> > The expiry was already being saved as part of RTA_CACHEINFO
> > in rtnl_put_cacheinfo(), but adding code to generate RTF_EXPIRES upon save
> > looked more appropriate than redundant cherrypicking from
> > RTA_CACHEINFO upon restore.
> >
> > Signed-off-by: Andrew Yourtchenko <ayour...@gmail.com>
> > ---
> > Changes since v1 [1]:
> > * fixed the indentation in a multiline function call
> > as per David Miller's review
> >
> > [1] v1: http://marc.info/?l=linux-netdev&m=147135597422280&w=2
> >
> > net/ipv6/route.c | 8 ++++++++
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> > index 4981755..f5b987d 100644
> > --- a/net/ipv6/route.c
> > +++ b/net/ipv6/route.c
> > @@ -3244,6 +3244,14 @@ static int rt6_fill_node(struct net *net,
> > if (rtnl_put_cacheinfo(skb, &rt->dst, 0, expires, rt->dst.error) < 0)
> > goto nla_put_failure;
> >
> > + /* Can't rely on expires == 0. It is zero if no expires flag,
> > + * or if the timing is precisely at expiry. So, recheck the flag.
> > + */
> > + if (rt->rt6i_flags & RTF_EXPIRES)
> > + if (nla_put_u32(skb, RTA_EXPIRES,
> > + expires > 0 ? expires / HZ : 0))
> > + goto nla_put_failure;
> > +
>
> Why sending (kernel -> user) the value in second units, while the other
> direction (user -> kernel) expects hz ?
No, the userland does expect seconds, see below:
>
> iproute2$ git grep -n RTA_EXPIRES
> include/linux/rtnetlink.h:389: RTA_EXPIRES,
> ip/iproute.c:930: addattr32(&req.n, sizeof(req),
> RTA_EXPIRES, expires*hz);
>
Fixed by commit eecc006952d6f3992b632974d0f04f995d2a176e
Author: Andrew Vagin <ava...@virtuozzo.com>
Date: Wed Jun 29 02:27:14 2016 +0300
ip route: timeout for routes has to be set in seconds
Currently a timeout is multiplied by HZ in user-space and
then it multiplied by HZ in kernel-space.
> kernel patch : (32bc201e1974976b7d3fea9a9b17bb7392ca6394)
>
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
>
> @@ -2809,6 +2810,15 @@ static int rtm_to_fib6_config(struct sk_buff *skb,
> struct nlmsghdr *nlh,
> if (tb[RTA_ENCAP_TYPE])
> cfg->fc_encap_type = nla_get_u16(tb[RTA_ENCAP_TYPE]);
>
> + if (tb[RTA_EXPIRES]) {
> + unsigned long timeout =
> addrconf_timeout_fixup(nla_get_u32(tb[RTA_EXPIRES]), HZ);
> +
> + if (addrconf_finite_timeout(timeout)) {
> + cfg->fc_expires = jiffies_to_clock_t(timeout * HZ);
> + cfg->fc_flags |= RTF_EXPIRES;
> + }
> + }
> +
>
Which is exactly the bug this snippet would trigger together with the
older git grep that you have enclosed - the resulting value from user
space assigned to fc_expires would have been:
jiffies_to_clock_t(timeout * HZ * hz).
So, the userland needs value in seconds. I suppose a good idea might be to
change my comment within the commit into something like:
/* Can't rely on expires == 0. It is zero if no expires flag,
* or if the timing is precisely at expiry. So, recheck the flag.
* The division by HZ is to obtain the value in seconds, which
* is needed by the userland, see commit
* eecc006952d6f3992b632974d0f04f995d2a176e
* "ip route: timeout for routes has to be set in seconds"
*/
What do you think ?
--a
