netdev - Re: [PATCH v2 1/2] ipv6: save route expiry in RTA_EXPIRES if RTF

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <alpine.DEB.2.20.1608221334210.31827@ayourtch-u1604b2>
Date:   Mon, 22 Aug 2016 13:41:41 +0200 (CEST)
From:   Andrew Yourtchenko <ayourtch@...il.com>
To:     Eric Dumazet <eric.dumazet@...il.com>
cc:     netdev@...r.kernel.org
Subject: Re: [PATCH v2 1/2] ipv6: save route expiry in RTA_EXPIRES if
 RTF_EXPIRES set

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 <ayourtch@...il.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 <avagin@...tuozzo.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