[PATCH 0/2] ipv6: fix stuck RA-derived route in container after migration with criu

This patchset fixes the connectivity problem for containers
with RA-derived default route, after they were migrated using criu:
the default routes would lose their "expires" value and become
stuck forever. The corresponding criu issue with the discussion
is at https://github.com/xemul/criu/issues/177

The latter uses "ip -6 route save" to save the routes during
the migration, and "ip -6 route restore" during restore, so the problem
is easily reproducible even without criu.

There are two problems, hence two patches in this patchset:

1) the expiry time for the route is saved in 
RTA_CACHEINFO via rtnl_put_cacheinfo, but the code in rtm_to_fib6_config
expects the RTA_EXPIRES. Rather than cherrypicking in the restore code path
from RTA_CACHEINFO, adding RTA_EXPIRES upon save seemed like
a better option.

2) the restored route, even with the properly restored expires,
does not have the correct RTF_* flags set (RTF_ADDRCONF|RTF_DEFAULT),
preventing the incoming router advertisements from updating it.
During the code review I noticed RTF_ROUTEINFO would be also lost
during save/restore. This can be viewed as an operation symmetric
to that done in f0396f60d7c165018c9b203fb9b89fb224835578.

Tested both net and net-next with the patches.

Andrew Yourtchenko (2):
  ipv6: save route expiry in RTA_EXPIRES if RTF_EXPIRES set
  ipv6: fixup RTF_* flags when restoring RTPROT_RA route from rtnetlink

 net/ipv6/route.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

-- 
2.7.4

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 <redacted>
---
 net/ipv6/route.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 4981755..dc37017 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;
+
 	if (nla_put_u8(skb, RTA_PREF, IPV6_EXTRACT_PREF(rt->rt6i_flags)))
 		goto nla_put_failure;
 

-- 
2.7.4

Fix the flags for RA-derived routes that were saved
via "ip -6 route save" and and subsequently restored via
"ip -6 route restore", allowing the incoming router advertisements
to update them, rather than complain about inability to do so.

Upon the restore of RA-derived saved routes, set the RTF_ADDRCONF
to indicate that the source of the route was originally
a router advertisement, and set the RTF_DEFAULT or RTF_ROUTEINFO
flag depending on prefix length. This can be considered a
sister change of f0396f60d7c165018c9b203fb9b89fb224835578, in
the other direction.

Signed-off-by: Andrew Yourtchenko <redacted>
---
 net/ipv6/route.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index dc37017..4da7237 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c

@@ -2769,6 +2769,16 @@ static int rtm_to_fib6_config(struct sk_buff *skb, struct nlmsghdr *nlh,
 	cfg->fc_protocol = rtm->rtm_protocol;
 	cfg->fc_type = rtm->rtm_type;
 
+	if (rtm->rtm_protocol == RTPROT_RA) {
+	    /* RA-derived route: set flags accordingly. */
+	    cfg->fc_flags |= RTF_ADDRCONF;
+	    if (rtm->rtm_dst_len == 0) {
+		cfg->fc_flags |= RTF_DEFAULT;
+	    } else {
+		cfg->fc_flags |= RTF_ROUTEINFO;
+	    }
+	}
+
 	if (rtm->rtm_type == RTN_UNREACHABLE ||
 	    rtm->rtm_type == RTN_BLACKHOLE ||
 	    rtm->rtm_type == RTN_PROHIBIT ||

-- 
2.7.4

The indentation of both of your patches are incorrect.

You must indent each basic block by a full TAB character.

You must also line up multi-line function calls, such that
the second and subsequent lines start precisely at the first
column after the opennning parenthesis of the function call
itself.  You must use the appropriate number of TAB and
SPC characters necessary to achieve this.

This patchset fixes the connectivity problem for containers
with RA-derived default route, after they were migrated using criu:
the default routes would lose their "expires" value and become
stuck forever. The corresponding criu issue with the discussion
is at https://github.com/xemul/criu/issues/177

The latter uses "ip -6 route save" to save the routes during
the migration, and "ip -6 route restore" during restore, so the problem
is easily reproducible even without criu.

There are two problems, hence two patches in this patchset:

1) the expiry time for the route is saved in
RTA_CACHEINFO via rtnl_put_cacheinfo, but the code in rtm_to_fib6_config
expects the RTA_EXPIRES. Rather than cherrypicking in the restore code path
from RTA_CACHEINFO, adding RTA_EXPIRES upon save seemed like
a better option.

2) the restored route, even with the properly restored expires,
does not have the correct RTF_* flags set (RTF_ADDRCONF|RTF_DEFAULT),
preventing the incoming router advertisements from updating it.
During the code review I noticed RTF_ROUTEINFO would be also lost
during save/restore. This can be viewed as an operation symmetric
to that done in f0396f60d7c165018c9b203fb9b89fb224835578.

Tested both net and net-next with the patches.

Changes since v1 [1]:
 * Fixed the indentation in both patches as per David Miller's review comments:
    - basic block indented always by a TAB
    - multiline function call second line to start at the first column
      after the opening parenthesis of the function call, using
      appropriate number of TABs (and SPC only at the end, if needed).

[1] v1: http://marc.info/?l=linux-netdev&m=147135599322286&w=2

Andrew Yourtchenko (2):
  ipv6: save route expiry in RTA_EXPIRES if RTF_EXPIRES set
  ipv6: fixup RTF_* flags when restoring RTPROT_RA route from rtnetlink

 net/ipv6/route.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

-- 
2.7.4

Fix the flags for RA-derived routes that were saved
via "ip -6 route save" and and subsequently restored via
"ip -6 route restore", allowing the incoming router advertisements
to update them, rather than complain about inability to do so.

Upon the restore of RA-derived saved routes, set the RTF_ADDRCONF
to indicate that the source of the route was originally
a router advertisement, and set the RTF_DEFAULT or RTF_ROUTEINFO
flag depending on prefix length. This can be considered a
sister change of f0396f60d7c165018c9b203fb9b89fb224835578, in
the other direction.

Signed-off-by: Andrew Yourtchenko <redacted>
---
Changes since v1 [1]:
 * fixed the indentation of the basic blocks to be always a full TAB
   as per David Miller's review

[1] v1: http://marc.info/?l=linux-netdev&m=147135599322285&w=2

 net/ipv6/route.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index f5b987d..60d95cd 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c

@@ -2769,6 +2769,16 @@ static int rtm_to_fib6_config(struct sk_buff *skb, struct nlmsghdr *nlh,
 	cfg->fc_protocol = rtm->rtm_protocol;
 	cfg->fc_type = rtm->rtm_type;
 
+	if (rtm->rtm_protocol == RTPROT_RA) {
+		/* RA-derived route: set flags accordingly. */
+		cfg->fc_flags |= RTF_ADDRCONF;
+		if (rtm->rtm_dst_len == 0) {
+			cfg->fc_flags |= RTF_DEFAULT;
+		} else {
+			cfg->fc_flags |= RTF_ROUTEINFO;
+		}
+	}
+
 	if (rtm->rtm_type == RTN_UNREACHABLE ||
 	    rtm->rtm_type == RTN_BLACKHOLE ||
 	    rtm->rtm_type == RTN_PROHIBIT ||

-- 
2.7.4

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 <redacted>
---
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;
+
 	if (nla_put_u8(skb, RTA_PREF, IPV6_EXTRACT_PREF(rt->rt6i_flags)))
 		goto nla_put_failure;
 

-- 
2.7.4

Hello.

On 08/19/2016 07:41 PM, Andrew Yourtchenko wrote:

Fix the flags for RA-derived routes that were saved
via "ip -6 route save" and and subsequently restored via
"ip -6 route restore", allowing the incoming router advertisements
to update them, rather than complain about inability to do so.

Upon the restore of RA-derived saved routes, set the RTF_ADDRCONF
to indicate that the source of the route was originally
a router advertisement, and set the RTF_DEFAULT or RTF_ROUTEINFO
flag depending on prefix length. This can be considered a
sister change of f0396f60d7c165018c9b203fb9b89fb224835578, in

   It's enough to specify 12 digits but you also need to specify the commit summary enclosed in ("").

   {} not needed here and above.

[...]

MBR, Sergei

On Fri, 2016-08-19 at 18:41 +0200, Andrew Yourtchenko wrote:

Why sending (kernel -> user) the value in second units, while the other
direction (user -> kernel) expects hz ?

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);

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;
+               }
+       }
+

Hello, 

Thanks for the review and feedback, one small clarification below.

On Fri, 19 Aug 2016, Sergei Shtylyov wrote:

Hello.

On 08/19/2016 07:41 PM, Andrew Yourtchenko wrote:

quoted

Fix the flags for RA-derived routes that were saved
via "ip -6 route save" and and subsequently restored via
"ip -6 route restore", allowing the incoming router advertisements
to update them, rather than complain about inability to do so.

Upon the restore of RA-derived saved routes, set the RTF_ADDRCONF
to indicate that the source of the route was originally
a router advertisement, and set the RTF_DEFAULT or RTF_ROUTEINFO
flag depending on prefix length. This can be considered a
sister change of f0396f60d7c165018c9b203fb9b89fb224835578, in

   It's enough to specify 12 digits but you also need to specify the commit
summary enclosed in ("").

Is just the first line of "ipv6: fix RTPROT_RA markup of RA routes 
w/nexthops" enough, or do I need to include the additional two 
paragraphs that follow ? (and can I keep the full hash rather than 
truncate down to 12 digits?)

quoted

the other direction.

Signed-off-by: Andrew Yourtchenko <redacted>
---
Changes since v1 [1]:
 * fixed the indentation of the basic blocks to be always a full TAB
   as per David Miller's review

[1] v1: http://marc.info/?l=linux-netdev&m=147135599322285&w=2

 net/ipv6/route.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index f5b987d..60d95cd 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c

@@ -2769,6 +2769,16 @@ static int rtm_to_fib6_config(struct sk_buff *skb,

struct nlmsghdr *nlh,
 	cfg->fc_protocol = rtm->rtm_protocol;
 	cfg->fc_type = rtm->rtm_type;

+	if (rtm->rtm_protocol == RTPROT_RA) {
+		/* RA-derived route: set flags accordingly. */
+		cfg->fc_flags |= RTF_ADDRCONF;
+		if (rtm->rtm_dst_len == 0) {
+			cfg->fc_flags |= RTF_DEFAULT;
+		} else {
+			cfg->fc_flags |= RTF_ROUTEINFO;
+		}

   {} not needed here and above.

Will fix in v3, thanks!

--a

[...]

MBR, Sergei

Hello.

On 8/22/2016 2:04 PM, Andrew Yourtchenko wrote:

quoted

quoted

Fix the flags for RA-derived routes that were saved
via "ip -6 route save" and and subsequently restored via
"ip -6 route restore", allowing the incoming router advertisements
to update them, rather than complain about inability to do so.

Upon the restore of RA-derived saved routes, set the RTF_ADDRCONF
to indicate that the source of the route was originally
a router advertisement, and set the RTF_DEFAULT or RTF_ROUTEINFO
flag depending on prefix length. This can be considered a
sister change of f0396f60d7c165018c9b203fb9b89fb224835578, in

   It's enough to specify 12 digits but you also need to specify the commit
summary enclosed in ("").

Is just the first line of "ipv6: fix RTPROT_RA markup of RA routes
w/nexthops" enough, or do I need to include the additional two

   Summary means the patch subject (and the first line of the commit log).

paragraphs that follow ? (and can I keep the full hash rather than
truncate down to 12 digits?)

   Yes.

quoted

quoted

the other direction.

Signed-off-by: Andrew Yourtchenko <redacted>

[...]

MBR, Sergei

On Fri, 19 Aug 2016, Eric Dumazet wrote:

On Fri, 2016-08-19 at 18:41 +0200, Andrew Yourtchenko wrote:

quoted

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 <redacted>
---
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 [off-list ref]
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.

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