[RFC PATCH iproute2] ip route: save: exclude rtnh_flags which can't be set

During "ip route save" we preserve all rtnh_flags,
even those that can't be set directly by the userspace.
This looks like a bug because a user can't restore
route dump which was generated by "ip route save" back.
This also prevents CRIU from correct restore of the
containers with some route configurations inside.

Reproducer:
$ ip link add type veth
$ ip addr add 10.0.0.1/24 dev veth0
$ ip link set veth0 up
$ ip route add default via 10.0.0.1
$ ip route save > route_dump
$ ip route restore < route_dump
Error: Invalid rtm_flags - can not contain DEAD or LINKDOWN.

Let's just omit non-settable rtnh_flags from the dump image.

According to the check in the fib_create_info() kernel
function it looks like we can't restore back only
RTNH_F_DEAD and RTNH_F_LINKDOWN flags. But according to the
ip route command manual user may set only RTNH_F_PERVASIVE
and RTNH_F_ONLINK flags. Does this mean that all rest flags
such as RTNH_F_OFFLOAD, RTNH_F_TRAP, and so on should be also
filtered out on the kernel side as RTNH_F_DEAD and RTNH_F_LINKDOWN?

I've checked that at the moment kernel doesn't prevent the setting
of RTNH_F_OFFLOAD and RTNH_F_TRAP from the userspace side.
Is this correct? If not then I am ready to prepare corresponding
patches for the kernel.

See also
[RFC PATCH net-next] rtnetlink: add RTNH_F_REJECT_MASK

Cc: David Miller <davem@davemloft.net>
Cc: David Ahern <redacted>
Cc: Stephen Hemminger <stephen@networkplumber.org>
Cc: Ido Schimmel <idosch@nvidia.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Andrei Vagin <redacted>
Cc: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
Cc: Alexander Mikhalitsyn <redacted>
Signed-off-by: Alexander Mikhalitsyn <redacted>
---
 include/uapi/linux/rtnetlink.h | 3 +++
 ip/iproute.c                   | 6 ++++++
 2 files changed, 9 insertions(+)

diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
index e01efa28..3ce9ba3c 100644
--- a/include/uapi/linux/rtnetlink.h
+++ b/include/uapi/linux/rtnetlink.h

@@ -417,6 +417,9 @@ struct rtnexthop {
 #define RTNH_COMPARE_MASK	(RTNH_F_DEAD | RTNH_F_LINKDOWN | \
 				 RTNH_F_OFFLOAD | RTNH_F_TRAP)
 
+/* these flags can't be set by the userspace */
+#define RTNH_F_REJECT_MASK	(RTNH_F_DEAD | RTNH_F_LINKDOWN)
+
 /* Macros to handle hexthops */
 
 #define RTNH_ALIGNTO	4

diff --git a/ip/iproute.c b/ip/iproute.c
index 1447a5f7..88faadeb 100644
--- a/ip/iproute.c
+++ b/ip/iproute.c

@@ -1632,6 +1632,12 @@ static int save_route(struct nlmsghdr *n, void *arg)
 	if (!filter_nlmsg(n, tb, host_len))
 		return 0;
 
+	/*
+	 * Exclude flags which can't be set directly
+	 * by the userspace from the rtmsg dump.
+	 */
+	r->rtm_flags &= ~RTNH_F_REJECT_MASK;
+
 	ret = write(STDOUT_FILENO, n, n->nlmsg_len);
 	if ((ret > 0) && (ret != n->nlmsg_len)) {
 		fprintf(stderr, "Short write while saving nlmsg\n");

-- 
2.31.1

Introduce RTNH_F_REJECT_MASK mask which contains
all rtnh_flags which can't be set by the userspace
directly.

This mask will be used in the iproute utility
to exclude rtnh_flags which can't be restored
from "ip route save" image.

This patch doesn't change kernel behavior, but
it looks like we need to prohibit setting
RTNH_F_OFFLOAD, RTNH_F_TRAP flags too.
Am I right?

Please, take a look on
[RFC PATCH iproute2] ip route: save: exclude rtnh_flags which can't be set

Cc: David Miller <davem@davemloft.net>
Cc: David Ahern <redacted>
Cc: Stephen Hemminger <stephen@networkplumber.org>
Cc: Ido Schimmel <idosch@nvidia.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Andrei Vagin <redacted>
Cc: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
Cc: Alexander Mikhalitsyn <redacted>
Signed-off-by: Alexander Mikhalitsyn <redacted>
---
 include/uapi/linux/rtnetlink.h | 3 +++
 net/ipv4/fib_semantics.c       | 4 ++--
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
index 5888492a5257..c15e591e5d25 100644
--- a/include/uapi/linux/rtnetlink.h
+++ b/include/uapi/linux/rtnetlink.h

@@ -417,6 +417,9 @@ struct rtnexthop {
 #define RTNH_COMPARE_MASK	(RTNH_F_DEAD | RTNH_F_LINKDOWN | \
 				 RTNH_F_OFFLOAD | RTNH_F_TRAP)
 
+/* these flags can't be set by the userspace */
+#define RTNH_F_REJECT_MASK	(RTNH_F_DEAD | RTNH_F_LINKDOWN)
+
 /* Macros to handle hexthops */
 
 #define RTNH_ALIGNTO	4

diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c
index 4c0c33e4710d..7a383c54fe46 100644
--- a/net/ipv4/fib_semantics.c
+++ b/net/ipv4/fib_semantics.c

@@ -685,7 +685,7 @@ static int fib_get_nhs(struct fib_info *fi, struct rtnexthop *rtnh,
 			return -EINVAL;
 		}
 
-		if (rtnh->rtnh_flags & (RTNH_F_DEAD | RTNH_F_LINKDOWN)) {
+		if (rtnh->rtnh_flags & RTNH_F_REJECT_MASK) {
 			NL_SET_ERR_MSG(extack,
 				       "Invalid flags for nexthop - can not contain DEAD or LINKDOWN");
 			return -EINVAL;

@@ -1363,7 +1363,7 @@ struct fib_info *fib_create_info(struct fib_config *cfg,
 		goto err_inval;
 	}
 
-	if (cfg->fc_flags & (RTNH_F_DEAD | RTNH_F_LINKDOWN)) {
+	if (cfg->fc_flags & RTNH_F_REJECT_MASK) {
 		NL_SET_ERR_MSG(extack,
 			       "Invalid rtm_flags - can not contain DEAD or LINKDOWN");
 		goto err_inval;

-- 
2.31.1

On Thu, 11 Nov 2021 19:02:40 +0300 Alexander Mikhalitsyn wrote:

You should probably drop the _F_ since RTNH_COMPARE_MASK above 
does not have it.

Dear Jakub,

Thanks for your attention to the patch. Sure, I will do it.

Please, let me know, what do you think about RTNH_F_OFFLOAD,
RTNH_F_TRAP flags? Don't we need to prohibit it too?

Alex

On Thu, Nov 11, 2021 at 8:48 PM Jakub Kicinski [off-list ref] wrote:

On Thu, 11 Nov 2021 19:02:40 +0300 Alexander Mikhalitsyn wrote:

quoted

diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
index 5888492a5257..c15e591e5d25 100644
--- a/include/uapi/linux/rtnetlink.h
+++ b/include/uapi/linux/rtnetlink.h

@@ -417,6 +417,9 @@ struct rtnexthop {
 #define RTNH_COMPARE_MASK    (RTNH_F_DEAD | RTNH_F_LINKDOWN | \
                               RTNH_F_OFFLOAD | RTNH_F_TRAP)

+/* these flags can't be set by the userspace */
+#define RTNH_F_REJECT_MASK   (RTNH_F_DEAD | RTNH_F_LINKDOWN)

You should probably drop the _F_ since RTNH_COMPARE_MASK above
does not have it.

On Thu, 11 Nov 2021 20:51:20 +0300 Alexander Mikhalitsyn wrote:

Thanks for your attention to the patch. Sure, I will do it.

Please, let me know, what do you think about RTNH_F_OFFLOAD,
RTNH_F_TRAP flags? Don't we need to prohibit it too?

Looks like an omission indeed but I'll let Dave and Ido comment.

Reminder: please don't top post on the ML.

On Thu, Nov 11, 2021 at 8:56 PM Jakub Kicinski [off-list ref] wrote:

On Thu, 11 Nov 2021 20:51:20 +0300 Alexander Mikhalitsyn wrote:

quoted

Thanks for your attention to the patch. Sure, I will do it.

Please, let me know, what do you think about RTNH_F_OFFLOAD,
RTNH_F_TRAP flags? Don't we need to prohibit it too?

Looks like an omission indeed but I'll let Dave and Ido comment.

Sure.

Reminder: please don't top post on the ML.

Oh, yep. I'm sorry.

On 11/11/21 9:02 AM, Alexander Mikhalitsyn wrote:

Userspace can not set any of the flags in RTNH_COMPARE_MASK.

On Thu, Nov 11, 2021 at 10:13 PM David Ahern [off-list ref] wrote:

On 11/11/21 9:02 AM, Alexander Mikhalitsyn wrote:

quoted

diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
index 5888492a5257..c15e591e5d25 100644
--- a/include/uapi/linux/rtnetlink.h
+++ b/include/uapi/linux/rtnetlink.h

@@ -417,6 +417,9 @@ struct rtnexthop {
 #define RTNH_COMPARE_MASK    (RTNH_F_DEAD | RTNH_F_LINKDOWN | \
                               RTNH_F_OFFLOAD | RTNH_F_TRAP)

+/* these flags can't be set by the userspace */
+#define RTNH_F_REJECT_MASK   (RTNH_F_DEAD | RTNH_F_LINKDOWN)
+
 /* Macros to handle hexthops */

Userspace can not set any of the flags in RTNH_COMPARE_MASK.

Hi David,

thanks! So, I have to prepare a patch which fixes current checks for rtnh_flags
against RTNH_COMPARE_MASK. So, there is no need to introduce a separate
RTNH_F_REJECT_MASK.
Am I right?

Regards,
Alex

[ cc roopa]

On 11/11/21 12:23 PM, Alexander Mikhalitsyn wrote:

On Thu, Nov 11, 2021 at 10:13 PM David Ahern [off-list ref] wrote:

quoted

On 11/11/21 9:02 AM, Alexander Mikhalitsyn wrote:

quoted

diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
index 5888492a5257..c15e591e5d25 100644
--- a/include/uapi/linux/rtnetlink.h
+++ b/include/uapi/linux/rtnetlink.h

@@ -417,6 +417,9 @@ struct rtnexthop {
 #define RTNH_COMPARE_MASK    (RTNH_F_DEAD | RTNH_F_LINKDOWN | \
                               RTNH_F_OFFLOAD | RTNH_F_TRAP)

+/* these flags can't be set by the userspace */
+#define RTNH_F_REJECT_MASK   (RTNH_F_DEAD | RTNH_F_LINKDOWN)
+
 /* Macros to handle hexthops */

Userspace can not set any of the flags in RTNH_COMPARE_MASK.

Hi David,

thanks! So, I have to prepare a patch which fixes current checks for rtnh_flags
against RTNH_COMPARE_MASK. So, there is no need to introduce a separate
RTNH_F_REJECT_MASK.
Am I right?

Added Roopa to double check if Cumulus relies on this for their switchd.

If that answer is no, then there is no need for a new mask.

On 11/11/21 2:19 PM, David Ahern wrote:

[ cc roopa]

On 11/11/21 12:23 PM, Alexander Mikhalitsyn wrote:

quoted

On Thu, Nov 11, 2021 at 10:13 PM David Ahern [off-list ref] wrote:

quoted

On 11/11/21 9:02 AM, Alexander Mikhalitsyn wrote:

quoted

diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
index 5888492a5257..c15e591e5d25 100644
--- a/include/uapi/linux/rtnetlink.h
+++ b/include/uapi/linux/rtnetlink.h

@@ -417,6 +417,9 @@ struct rtnexthop {
  #define RTNH_COMPARE_MASK    (RTNH_F_DEAD | RTNH_F_LINKDOWN | \
                                RTNH_F_OFFLOAD | RTNH_F_TRAP)

+/* these flags can't be set by the userspace */
+#define RTNH_F_REJECT_MASK   (RTNH_F_DEAD | RTNH_F_LINKDOWN)
+
  /* Macros to handle hexthops */

Userspace can not set any of the flags in RTNH_COMPARE_MASK.

Hi David,

thanks! So, I have to prepare a patch which fixes current checks for rtnh_flags
against RTNH_COMPARE_MASK. So, there is no need to introduce a separate
RTNH_F_REJECT_MASK.
Am I right?

Added Roopa to double check if Cumulus relies on this for their switchd.

If that answer is no, then there is no need for a new mask.

yes, these flags are already exposed to userspace and we do use it.

We have also considered optimizations where routing daemons set OFFLOAD 
and drivers clear it when offload fails.

I wont be surprised if other open network os distributions are also 
using it.


Thanks for the headsup David.

On 11/11/21 6:02 PM, Roopa Prabhu wrote:

On 11/11/21 2:19 PM, David Ahern wrote:

quoted

[ cc roopa]

On 11/11/21 12:23 PM, Alexander Mikhalitsyn wrote:

quoted

On Thu, Nov 11, 2021 at 10:13 PM David Ahern [off-list ref] wrote:

quoted

On 11/11/21 9:02 AM, Alexander Mikhalitsyn wrote:

quoted

diff --git a/include/uapi/linux/rtnetlink.h

b/include/uapi/linux/rtnetlink.h
index 5888492a5257..c15e591e5d25 100644

--- a/include/uapi/linux/rtnetlink.h
+++ b/include/uapi/linux/rtnetlink.h

@@ -417,6 +417,9 @@ struct rtnexthop {

  #define RTNH_COMPARE_MASK    (RTNH_F_DEAD | RTNH_F_LINKDOWN | \
                                RTNH_F_OFFLOAD | RTNH_F_TRAP)

+/* these flags can't be set by the userspace */
+#define RTNH_F_REJECT_MASK   (RTNH_F_DEAD | RTNH_F_LINKDOWN)
+
  /* Macros to handle hexthops */

Userspace can not set any of the flags in RTNH_COMPARE_MASK.

Hi David,

thanks! So, I have to prepare a patch which fixes current checks for
rtnh_flags
against RTNH_COMPARE_MASK. So, there is no need to introduce a separate
RTNH_F_REJECT_MASK.
Am I right?

Added Roopa to double check if Cumulus relies on this for their switchd.

If that answer is no, then there is no need for a new mask.

yes, these flags are already exposed to userspace and we do use it.

We have also considered optimizations where routing daemons set OFFLOAD
and drivers clear it when offload fails.

I wont be surprised if other open network os distributions are also
using it.


Thanks for the headsup David.

Thanks, Roopa. So then the separate mask is needed.

During "ip route save" we preserve all rtnh_flags,
even those that can't be set directly by the userspace.
This looks like a bug because a user can't restore
route dump which was generated by "ip route save" back.
This also prevents CRIU from correct restore of the
containers with some route configurations inside.

Reproducer:
$ ip link add type veth
$ ip addr add 10.0.0.1/24 dev veth0
$ ip link set veth0 up
$ ip route add default via 10.0.0.1
$ ip route save > route_dump
$ ip route restore < route_dump
Error: Invalid rtm_flags - can not contain DEAD or LINKDOWN.

Let's just omit non-settable rtnh_flags from the dump image.

According to the check in the fib_create_info() kernel
function it looks like we can't restore back only
RTNH_F_DEAD and RTNH_F_LINKDOWN flags, so RTNH_REJECT_MASK
contains this flags for now.

See also linux kernel patch:
[PATCH net-next] rtnetlink: add RTNH_REJECT_MASK

Cc: David Miller <davem@davemloft.net>
Cc: David Ahern <redacted>
Cc: Stephen Hemminger <stephen@networkplumber.org>
Cc: Ido Schimmel <idosch@nvidia.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Roopa Prabhu <redacted>
Cc: Andrei Vagin <redacted>
Cc: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
Cc: Alexander Mikhalitsyn <redacted>
Signed-off-by: Alexander Mikhalitsyn <redacted>
---
 include/uapi/linux/rtnetlink.h | 3 +++
 ip/iproute.c                   | 6 ++++++
 2 files changed, 9 insertions(+)

diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
index e01efa28..6de83c62 100644
--- a/include/uapi/linux/rtnetlink.h
+++ b/include/uapi/linux/rtnetlink.h

@@ -417,6 +417,9 @@ struct rtnexthop {
 #define RTNH_COMPARE_MASK	(RTNH_F_DEAD | RTNH_F_LINKDOWN | \
 				 RTNH_F_OFFLOAD | RTNH_F_TRAP)
 
+/* these flags can't be set by the userspace */
+#define RTNH_REJECT_MASK	(RTNH_F_DEAD | RTNH_F_LINKDOWN)
+
 /* Macros to handle hexthops */
 
 #define RTNH_ALIGNTO	4

diff --git a/ip/iproute.c b/ip/iproute.c
index 1447a5f7..a72b652b 100644
--- a/ip/iproute.c
+++ b/ip/iproute.c

@@ -1632,6 +1632,12 @@ static int save_route(struct nlmsghdr *n, void *arg)
 	if (!filter_nlmsg(n, tb, host_len))
 		return 0;
 
+	/*
+	 * Exclude flags which can't be set directly
+	 * by the userspace from the rtmsg dump.
+	 */
+	r->rtm_flags &= ~RTNH_REJECT_MASK;
+
 	ret = write(STDOUT_FILENO, n, n->nlmsg_len);
 	if ((ret > 0) && (ret != n->nlmsg_len)) {
 		fprintf(stderr, "Short write while saving nlmsg\n");

-- 
2.31.1

Introduce RTNH_REJECT_MASK mask which contains
all rtnh_flags which can't be set by the userspace
directly.

This mask will be used in the iproute utility
to exclude rtnh_flags which can't be restored
from "ip route save" image.

This patch doesn't change kernel behavior at all.

Please, take a look on
[PATCH iproute2] ip route: save: exclude rtnh_flags which can't be set

Cc: David Miller <davem@davemloft.net>
Cc: David Ahern <redacted>
Cc: Stephen Hemminger <stephen@networkplumber.org>
Cc: Ido Schimmel <idosch@nvidia.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Roopa Prabhu <redacted>
Cc: Andrei Vagin <redacted>
Cc: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
Cc: Alexander Mikhalitsyn <redacted>
Signed-off-by: Alexander Mikhalitsyn <redacted>
---
 include/uapi/linux/rtnetlink.h | 3 +++
 net/ipv4/fib_semantics.c       | 4 ++--
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
index 5888492a5257..9c065e2fdef9 100644
--- a/include/uapi/linux/rtnetlink.h
+++ b/include/uapi/linux/rtnetlink.h

@@ -417,6 +417,9 @@ struct rtnexthop {
 #define RTNH_COMPARE_MASK	(RTNH_F_DEAD | RTNH_F_LINKDOWN | \
 				 RTNH_F_OFFLOAD | RTNH_F_TRAP)
 
+/* these flags can't be set by the userspace */
+#define RTNH_REJECT_MASK	(RTNH_F_DEAD | RTNH_F_LINKDOWN)
+
 /* Macros to handle hexthops */
 
 #define RTNH_ALIGNTO	4

diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c
index 4c0c33e4710d..805f5e05b56d 100644
--- a/net/ipv4/fib_semantics.c
+++ b/net/ipv4/fib_semantics.c

@@ -685,7 +685,7 @@ static int fib_get_nhs(struct fib_info *fi, struct rtnexthop *rtnh,
 			return -EINVAL;
 		}
 
-		if (rtnh->rtnh_flags & (RTNH_F_DEAD | RTNH_F_LINKDOWN)) {
+		if (rtnh->rtnh_flags & RTNH_REJECT_MASK) {
 			NL_SET_ERR_MSG(extack,
 				       "Invalid flags for nexthop - can not contain DEAD or LINKDOWN");
 			return -EINVAL;

@@ -1363,7 +1363,7 @@ struct fib_info *fib_create_info(struct fib_config *cfg,
 		goto err_inval;
 	}
 
-	if (cfg->fc_flags & (RTNH_F_DEAD | RTNH_F_LINKDOWN)) {
+	if (cfg->fc_flags & RTNH_REJECT_MASK) {
 		NL_SET_ERR_MSG(extack,
 			       "Invalid rtm_flags - can not contain DEAD or LINKDOWN");
 		goto err_inval;

-- 
2.31.1

On Fri, Nov 26, 2021 at 04:43:10PM +0300, Alexander Mikhalitsyn wrote:

+	/*
+	 * Exclude flags which can't be set directly
+	 * by the userspace from the rtmsg dump.
+	 */

I believe iproute2 is following netdev style comments [1] so this should
be:

/* Exclude flags which can't be set directly
 * by the userspace from the rtmsg dump.
 */

At least it's consistent with other comments in the file.

[1] https://www.kernel.org/doc/html/v5.12/networking/netdev-FAQ.html#is-the-comment-style-convention-different-for-the-networking-content

+	r->rtm_flags &= ~RTNH_REJECT_MASK;
+
 	ret = write(STDOUT_FILENO, n, n->nlmsg_len);
 	if ((ret > 0) && (ret != n->nlmsg_len)) {
 		fprintf(stderr, "Short write while saving nlmsg\n");
-- 
2.31.1

On Fri, Nov 26, 2021 at 04:43:11PM +0300, Alexander Mikhalitsyn wrote:

Instead of a deny list as in the legacy nexthop code, the new nexthop
code has an allow list (from rtm_to_nh_config()):

	if (nhm->nh_flags & ~NEXTHOP_VALID_USER_FLAGS) {
		NL_SET_ERR_MSG(extack, "Invalid nexthop flags in ancillary header");
		goto out;
	}

Where:

#define NEXTHOP_VALID_USER_FLAGS RTNH_F_ONLINK

So while the legacy nexthop code allows setting flags such as
RTNH_F_OFFLOAD, the new nexthop code denies them. I don't have a use
case for setting these flags from user space so I don't care if we allow
or deny them, but I believe the legacy and new nexthop code should be
consistent.

WDYT? Should we allow these flags in the new nexthop code as well or
keep denying them?

 		goto err_inval;
-- 
2.31.1

On 11/28/21 7:01 AM, Ido Schimmel wrote:

On Fri, Nov 26, 2021 at 04:43:11PM +0300, Alexander Mikhalitsyn wrote:

quoted

diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
index 5888492a5257..9c065e2fdef9 100644
--- a/include/uapi/linux/rtnetlink.h
+++ b/include/uapi/linux/rtnetlink.h

@@ -417,6 +417,9 @@ struct rtnexthop {
 #define RTNH_COMPARE_MASK	(RTNH_F_DEAD | RTNH_F_LINKDOWN | \
 				 RTNH_F_OFFLOAD | RTNH_F_TRAP)
 
+/* these flags can't be set by the userspace */
+#define RTNH_REJECT_MASK	(RTNH_F_DEAD | RTNH_F_LINKDOWN)
+
 /* Macros to handle hexthops */
 
 #define RTNH_ALIGNTO	4

diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c
index 4c0c33e4710d..805f5e05b56d 100644
--- a/net/ipv4/fib_semantics.c
+++ b/net/ipv4/fib_semantics.c

@@ -685,7 +685,7 @@ static int fib_get_nhs(struct fib_info *fi, struct rtnexthop *rtnh,
 			return -EINVAL;
 		}
 
-		if (rtnh->rtnh_flags & (RTNH_F_DEAD | RTNH_F_LINKDOWN)) {
+		if (rtnh->rtnh_flags & RTNH_REJECT_MASK) {
 			NL_SET_ERR_MSG(extack,
 				       "Invalid flags for nexthop - can not contain DEAD or LINKDOWN");
 			return -EINVAL;

@@ -1363,7 +1363,7 @@ struct fib_info *fib_create_info(struct fib_config *cfg,
 		goto err_inval;
 	}
 
-	if (cfg->fc_flags & (RTNH_F_DEAD | RTNH_F_LINKDOWN)) {
+	if (cfg->fc_flags & RTNH_REJECT_MASK) {
 		NL_SET_ERR_MSG(extack,
 			       "Invalid rtm_flags - can not contain DEAD or LINKDOWN");

Instead of a deny list as in the legacy nexthop code, the new nexthop
code has an allow list (from rtm_to_nh_config()):

	if (nhm->nh_flags & ~NEXTHOP_VALID_USER_FLAGS) {
		NL_SET_ERR_MSG(extack, "Invalid nexthop flags in ancillary header");
		goto out;
	}

Where:

#define NEXTHOP_VALID_USER_FLAGS RTNH_F_ONLINK

So while the legacy nexthop code allows setting flags such as
RTNH_F_OFFLOAD, the new nexthop code denies them. I don't have a use
case for setting these flags from user space so I don't care if we allow
or deny them, but I believe the legacy and new nexthop code should be
consistent.

WDYT? Should we allow these flags in the new nexthop code as well or
keep denying them?

quoted

 		goto err_inval;

I like the positive naming - RTNH_VALID_USER_FLAGS.

nexthop API should allow the OFFLOAD flag to be consistent; separate
change though.

On Sun, Nov 28, 2021 at 05:19:38PM -0700, David Ahern wrote:

On 11/28/21 7:01 AM, Ido Schimmel wrote:

quoted

On Fri, Nov 26, 2021 at 04:43:11PM +0300, Alexander Mikhalitsyn wrote:

quoted

diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
index 5888492a5257..9c065e2fdef9 100644
--- a/include/uapi/linux/rtnetlink.h
+++ b/include/uapi/linux/rtnetlink.h

@@ -417,6 +417,9 @@ struct rtnexthop {
 #define RTNH_COMPARE_MASK	(RTNH_F_DEAD | RTNH_F_LINKDOWN | \
 				 RTNH_F_OFFLOAD | RTNH_F_TRAP)
 
+/* these flags can't be set by the userspace */
+#define RTNH_REJECT_MASK	(RTNH_F_DEAD | RTNH_F_LINKDOWN)
+
 /* Macros to handle hexthops */
 
 #define RTNH_ALIGNTO	4

diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c
index 4c0c33e4710d..805f5e05b56d 100644
--- a/net/ipv4/fib_semantics.c
+++ b/net/ipv4/fib_semantics.c

@@ -685,7 +685,7 @@ static int fib_get_nhs(struct fib_info *fi, struct rtnexthop *rtnh,
 			return -EINVAL;
 		}
 
-		if (rtnh->rtnh_flags & (RTNH_F_DEAD | RTNH_F_LINKDOWN)) {
+		if (rtnh->rtnh_flags & RTNH_REJECT_MASK) {
 			NL_SET_ERR_MSG(extack,
 				       "Invalid flags for nexthop - can not contain DEAD or LINKDOWN");
 			return -EINVAL;

@@ -1363,7 +1363,7 @@ struct fib_info *fib_create_info(struct fib_config *cfg,
 		goto err_inval;
 	}
 
-	if (cfg->fc_flags & (RTNH_F_DEAD | RTNH_F_LINKDOWN)) {
+	if (cfg->fc_flags & RTNH_REJECT_MASK) {
 		NL_SET_ERR_MSG(extack,
 			       "Invalid rtm_flags - can not contain DEAD or LINKDOWN");

Instead of a deny list as in the legacy nexthop code, the new nexthop
code has an allow list (from rtm_to_nh_config()):

	if (nhm->nh_flags & ~NEXTHOP_VALID_USER_FLAGS) {
		NL_SET_ERR_MSG(extack, "Invalid nexthop flags in ancillary header");
		goto out;
	}

Where:

#define NEXTHOP_VALID_USER_FLAGS RTNH_F_ONLINK

So while the legacy nexthop code allows setting flags such as
RTNH_F_OFFLOAD, the new nexthop code denies them. I don't have a use
case for setting these flags from user space so I don't care if we allow
or deny them, but I believe the legacy and new nexthop code should be
consistent.

WDYT? Should we allow these flags in the new nexthop code as well or
keep denying them?

quoted

 		goto err_inval;

I like the positive naming - RTNH_VALID_USER_FLAGS.

I don't think we can move the legacy code to the same allow list as the
new nexthop code without potentially breaking user space. The legacy
code allows for much more flags to be set in the ancillary header than
the new nexthop code.

Looking at the patch again, what is the motivation to expose
RTNH_REJECT_MASK to user space? iproute2 already knows that it only
makes sense to set RTNH_F_ONLINK. Can't we just do:

diff --git a/ip/iproute.c b/ip/iproute.c
index 1447a5f78f49..0e6dad2b67e5 100644
--- a/ip/iproute.c
+++ b/ip/iproute.c

@@ -1632,6 +1632,8 @@ static int save_route(struct nlmsghdr *n, void *arg)
        if (!filter_nlmsg(n, tb, host_len))
                return 0;
 
+       r->rtm_flags &= ~RTNH_F_ONLINK;
+
        ret = write(STDOUT_FILENO, n, n->nlmsg_len);
        if ((ret > 0) && (ret != n->nlmsg_len)) {
                fprintf(stderr, "Short write while saving nlmsg\n");

nexthop API should allow the OFFLOAD flag to be consistent; separate
change though.

On Sun, 28 Nov 2021 16:01:27 +0200
Ido Schimmel [off-list ref] wrote:

On Fri, Nov 26, 2021 at 04:43:11PM +0300, Alexander Mikhalitsyn wrote:

quoted

diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
index 5888492a5257..9c065e2fdef9 100644
--- a/include/uapi/linux/rtnetlink.h
+++ b/include/uapi/linux/rtnetlink.h

@@ -417,6 +417,9 @@ struct rtnexthop {
 #define RTNH_COMPARE_MASK	(RTNH_F_DEAD | RTNH_F_LINKDOWN | \
 				 RTNH_F_OFFLOAD | RTNH_F_TRAP)
 
+/* these flags can't be set by the userspace */
+#define RTNH_REJECT_MASK	(RTNH_F_DEAD | RTNH_F_LINKDOWN)
+
 /* Macros to handle hexthops */
 
 #define RTNH_ALIGNTO	4

diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c
index 4c0c33e4710d..805f5e05b56d 100644
--- a/net/ipv4/fib_semantics.c
+++ b/net/ipv4/fib_semantics.c

@@ -685,7 +685,7 @@ static int fib_get_nhs(struct fib_info *fi, struct rtnexthop *rtnh,
 			return -EINVAL;
 		}
 
-		if (rtnh->rtnh_flags & (RTNH_F_DEAD | RTNH_F_LINKDOWN)) {
+		if (rtnh->rtnh_flags & RTNH_REJECT_MASK) {
 			NL_SET_ERR_MSG(extack,
 				       "Invalid flags for nexthop - can not contain DEAD or LINKDOWN");
 			return -EINVAL;

@@ -1363,7 +1363,7 @@ struct fib_info *fib_create_info(struct fib_config *cfg,
 		goto err_inval;
 	}
 
-	if (cfg->fc_flags & (RTNH_F_DEAD | RTNH_F_LINKDOWN)) {
+	if (cfg->fc_flags & RTNH_REJECT_MASK) {
 		NL_SET_ERR_MSG(extack,
 			       "Invalid rtm_flags - can not contain DEAD or LINKDOWN");

Instead of a deny list as in the legacy nexthop code, the new nexthop
code has an allow list (from rtm_to_nh_config()):

	if (nhm->nh_flags & ~NEXTHOP_VALID_USER_FLAGS) {
		NL_SET_ERR_MSG(extack, "Invalid nexthop flags in ancillary header");
		goto out;
	}

Where:

#define NEXTHOP_VALID_USER_FLAGS RTNH_F_ONLINK

So while the legacy nexthop code allows setting flags such as
RTNH_F_OFFLOAD, the new nexthop code denies them. I don't have a use
case for setting these flags from user space so I don't care if we allow
or deny them, but I believe the legacy and new nexthop code should be
consistent.

Dear Ido,

thanks for your attention to the patches and our checkpoint/restore problem.

Yep, I've read nexthop code too and notices some inconsistencies, but
unfortunately I'm newbie here and my first goal is to fix thing and not break
something, that's why my patch is so trivial and not invasive :)

We have some discussion about these flags here:
https://lore.kernel.org/netdev/d7c2d8fa-052e-b941-2ef1-830c1ba655c1@gmail.com/#r

I've noticed, that current iproute2 code not allows us to set RTNH_F_OFFLOAD and
RTNH_F_TRAP directly. And asked If we should prohibit setting these flags from
the userspace. But huge thanks to Roopa and David here - it turned out that some
userspace code usings these flags and sets it.

So, let's decide which flags we should allow to set from the userspace side
and which not. I'm ready to prepare all needed changes for both the kernel and
iproute2 side. ;)

WDYT? Should we allow these flags in the new nexthop code as well or
keep denying them?

IMHO, we should try to be consistent between the new nexthop code and the lagacy one.

Regards,
Alex

quoted

 		goto err_inval;
-- 
2.31.1

On Tue, 30 Nov 2021 09:59:25 +0200
Ido Schimmel [off-list ref] wrote:

On Sun, Nov 28, 2021 at 05:19:38PM -0700, David Ahern wrote:

quoted

On 11/28/21 7:01 AM, Ido Schimmel wrote:

quoted

On Fri, Nov 26, 2021 at 04:43:11PM +0300, Alexander Mikhalitsyn wrote:

quoted

diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
index 5888492a5257..9c065e2fdef9 100644
--- a/include/uapi/linux/rtnetlink.h
+++ b/include/uapi/linux/rtnetlink.h

@@ -417,6 +417,9 @@ struct rtnexthop {
 #define RTNH_COMPARE_MASK	(RTNH_F_DEAD | RTNH_F_LINKDOWN | \
 				 RTNH_F_OFFLOAD | RTNH_F_TRAP)
 
+/* these flags can't be set by the userspace */
+#define RTNH_REJECT_MASK	(RTNH_F_DEAD | RTNH_F_LINKDOWN)
+
 /* Macros to handle hexthops */
 
 #define RTNH_ALIGNTO	4

diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c
index 4c0c33e4710d..805f5e05b56d 100644
--- a/net/ipv4/fib_semantics.c
+++ b/net/ipv4/fib_semantics.c

@@ -685,7 +685,7 @@ static int fib_get_nhs(struct fib_info *fi, struct rtnexthop *rtnh,
 			return -EINVAL;
 		}
 
-		if (rtnh->rtnh_flags & (RTNH_F_DEAD | RTNH_F_LINKDOWN)) {
+		if (rtnh->rtnh_flags & RTNH_REJECT_MASK) {
 			NL_SET_ERR_MSG(extack,
 				       "Invalid flags for nexthop - can not contain DEAD or LINKDOWN");
 			return -EINVAL;

@@ -1363,7 +1363,7 @@ struct fib_info *fib_create_info(struct fib_config *cfg,
 		goto err_inval;
 	}
 
-	if (cfg->fc_flags & (RTNH_F_DEAD | RTNH_F_LINKDOWN)) {
+	if (cfg->fc_flags & RTNH_REJECT_MASK) {
 		NL_SET_ERR_MSG(extack,
 			       "Invalid rtm_flags - can not contain DEAD or LINKDOWN");

Instead of a deny list as in the legacy nexthop code, the new nexthop
code has an allow list (from rtm_to_nh_config()):

	if (nhm->nh_flags & ~NEXTHOP_VALID_USER_FLAGS) {
		NL_SET_ERR_MSG(extack, "Invalid nexthop flags in ancillary header");
		goto out;
	}

Where:

#define NEXTHOP_VALID_USER_FLAGS RTNH_F_ONLINK

So while the legacy nexthop code allows setting flags such as
RTNH_F_OFFLOAD, the new nexthop code denies them. I don't have a use
case for setting these flags from user space so I don't care if we allow
or deny them, but I believe the legacy and new nexthop code should be
consistent.

WDYT? Should we allow these flags in the new nexthop code as well or
keep denying them?

quoted

 		goto err_inval;

I like the positive naming - RTNH_VALID_USER_FLAGS.

I don't think we can move the legacy code to the same allow list as the
new nexthop code without potentially breaking user space. The legacy
code allows for much more flags to be set in the ancillary header than
the new nexthop code.

Hello, Ido

agreed, let's keep this side unchanged

Looking at the patch again, what is the motivation to expose
RTNH_REJECT_MASK to user space? iproute2 already knows that it only
makes sense to set RTNH_F_ONLINK. Can't we just do:

Sorry, but that's not fully clear for me, why we should exclude RTNH_F_ONLINK?
I thought that we should exclude RTNH_F_DEAD and RTNH_F_LINKDOWN just because
kernel doesn't allow to set these flags.

I'd also thought about another approach - "offload" this flags filtering
problems to the kernel side for better iproute dump images compatibility.

Now we dump all routes using netlink message like this
	struct {
		struct nlmsghdr nlh;
		struct rtmsg rtm;
		char buf[128];
	} req = {
		.nlh.nlmsg_len = NLMSG_LENGTH(sizeof(struct rtmsg)),
		.nlh.nlmsg_type = RTM_GETROUTE,
		.nlh.nlmsg_flags = NLM_F_DUMP | NLM_F_REQUEST,
...
	};

But we can introduce some "special" flag like NLM_F_FILTERED_DUMP (or something like that)
	} req = {
		.nlh.nlmsg_len = NLMSG_LENGTH(sizeof(struct rtmsg)),
		.nlh.nlmsg_type = RTM_GETROUTE,
		.nlh.nlmsg_flags = NLM_F_FILTERED_DUMP | NLM_F_REQUEST,
...
	};

The idea here is that the kernel nows better which flags should be omitted from the dump
(<=> which flags is prohibited to set directly from the userspace side).

But that change is more "global". WDYT about this?

I'm ready to implement any of the approaches with your kind advice.

Alex

On Tue, Nov 30, 2021 at 11:35:17AM +0300, Alexander Mikhalitsyn wrote:

On Tue, 30 Nov 2021 09:59:25 +0200
Ido Schimmel [off-list ref] wrote:

quoted

Looking at the patch again, what is the motivation to expose
RTNH_REJECT_MASK to user space? iproute2 already knows that it only
makes sense to set RTNH_F_ONLINK. Can't we just do:

Sorry, but that's not fully clear for me, why we should exclude RTNH_F_ONLINK?
I thought that we should exclude RTNH_F_DEAD and RTNH_F_LINKDOWN just because
kernel doesn't allow to set these flags.

I don't think we should exclude RTNH_F_ONLINK. I'm saying that it is the
only flag that it makes sense to send to the kernel in the ancillary
header of RTM_NEWROUTE messages. The rest of the RNTH_F_* flags are
either not used by the kernel or are only meant to be sent from the
kernel to user space. Due to omission, they are mistakenly allowed.

Therefore, I think that the only necessary patch is an iproute2 patch
that makes sure that during save/restore you are clearing all the
RTNH_F_* flags but RTNH_F_ONLINK.

BTW, looking at save_route() in iproute2, I think the patch only clears
these flags from the ancillary header, but not from 'struct rtnexthop'
that is nested in RTA_MULTIPATH for multipath routes. See this blog post
for depiction of the message:
http://codecave.cc/multipath-routing-in-linux-part-1.html

I'd also thought about another approach - "offload" this flags filtering
problems to the kernel side for better iproute dump images compatibility.

Now we dump all routes using netlink message like this
	struct {
		struct nlmsghdr nlh;
		struct rtmsg rtm;
		char buf[128];
	} req = {
		.nlh.nlmsg_len = NLMSG_LENGTH(sizeof(struct rtmsg)),
		.nlh.nlmsg_type = RTM_GETROUTE,
		.nlh.nlmsg_flags = NLM_F_DUMP | NLM_F_REQUEST,
...
	};

But we can introduce some "special" flag like NLM_F_FILTERED_DUMP (or something like that)
	} req = {
		.nlh.nlmsg_len = NLMSG_LENGTH(sizeof(struct rtmsg)),
		.nlh.nlmsg_type = RTM_GETROUTE,
		.nlh.nlmsg_flags = NLM_F_FILTERED_DUMP | NLM_F_REQUEST,
...
	};

The idea here is that the kernel nows better which flags should be omitted from the dump
(<=> which flags is prohibited to set directly from the userspace side).

But that change is more "global". WDYT about this?

I'm ready to implement any of the approaches with your kind advice.

Having the kernel filter RO flags upon RTM_GETROUTE with a new special
flag / attribute would be easiest to implement in iproute2 (especially
if my comment about RTA_MULTIPATH is correct), but it's a quite invasive
change that requires new uAPI.

Personally, I think that if something can be done in user space, then I
would do it in user space instead of adding new uAPI.

On Tue, 30 Nov 2021 11:28:32 +0200
Ido Schimmel [off-list ref] wrote:

On Tue, Nov 30, 2021 at 11:35:17AM +0300, Alexander Mikhalitsyn wrote:

quoted

On Tue, 30 Nov 2021 09:59:25 +0200
Ido Schimmel [off-list ref] wrote:

quoted

Looking at the patch again, what is the motivation to expose
RTNH_REJECT_MASK to user space? iproute2 already knows that it only
makes sense to set RTNH_F_ONLINK. Can't we just do:

Sorry, but that's not fully clear for me, why we should exclude RTNH_F_ONLINK?
I thought that we should exclude RTNH_F_DEAD and RTNH_F_LINKDOWN just because
kernel doesn't allow to set these flags.

I don't think we should exclude RTNH_F_ONLINK. I'm saying that it is the
only flag that it makes sense to send to the kernel in the ancillary
header of RTM_NEWROUTE messages. The rest of the RNTH_F_* flags are
either not used by the kernel or are only meant to be sent from the
kernel to user space. Due to omission, they are mistakenly allowed.

Ah, okay, so, the patch should be like

diff --git a/ip/iproute.c b/ip/iproute.c
index 1447a5f78f49..0e6dad2b67e5 100644
--- a/ip/iproute.c
+++ b/ip/iproute.c

@@ -1632,6 +1632,8 @@ static int save_route(struct nlmsghdr *n, void *arg)
        if (!filter_nlmsg(n, tb, host_len))
                return 0;
 
+       r->rtm_flags &= RTNH_F_ONLINK;
+
        ret = write(STDOUT_FILENO, n, n->nlmsg_len);
        if ((ret > 0) && (ret != n->nlmsg_len)) {
                fprintf(stderr, "Short write while saving nlmsg\n");

to filter out all flags *except* RTNH_F_ONLINK.

But what about discussion from
https://lore.kernel.org/netdev/ff405eae-21d9-35f4-1397-b6f9a29a57ff@nvidia.com/

As far as I understand Roopa, we have to save at least RTNH_F_OFFLOAD flag too,
for instance, if user uses Cumulus and want to dump/restore routes.

I'm sorry if I misunderstood something.

Therefore, I think that the only necessary patch is an iproute2 patch
that makes sure that during save/restore you are clearing all the
RTNH_F_* flags but RTNH_F_ONLINK.

BTW, looking at save_route() in iproute2, I think the patch only clears
these flags from the ancillary header, but not from 'struct rtnexthop'
that is nested in RTA_MULTIPATH for multipath routes. See this blog post
for depiction of the message:
http://codecave.cc/multipath-routing-in-linux-part-1.html

Sure, I will handle these nested structures too.

quoted

I'd also thought about another approach - "offload" this flags filtering
problems to the kernel side for better iproute dump images compatibility.

Now we dump all routes using netlink message like this
	struct {
		struct nlmsghdr nlh;
		struct rtmsg rtm;
		char buf[128];
	} req = {
		.nlh.nlmsg_len = NLMSG_LENGTH(sizeof(struct rtmsg)),
		.nlh.nlmsg_type = RTM_GETROUTE,
		.nlh.nlmsg_flags = NLM_F_DUMP | NLM_F_REQUEST,
...
	};

But we can introduce some "special" flag like NLM_F_FILTERED_DUMP (or something like that)
	} req = {
		.nlh.nlmsg_len = NLMSG_LENGTH(sizeof(struct rtmsg)),
		.nlh.nlmsg_type = RTM_GETROUTE,
		.nlh.nlmsg_flags = NLM_F_FILTERED_DUMP | NLM_F_REQUEST,
...
	};

The idea here is that the kernel nows better which flags should be omitted from the dump
(<=> which flags is prohibited to set directly from the userspace side).

But that change is more "global". WDYT about this?

I'm ready to implement any of the approaches with your kind advice.

Having the kernel filter RO flags upon RTM_GETROUTE with a new special
flag / attribute would be easiest to implement in iproute2 (especially
if my comment about RTA_MULTIPATH is correct), but it's a quite invasive
change that requires new uAPI.

Personally, I think that if something can be done in user space, then I
would do it in user space instead of adding new uAPI.

agreed

On Tue, Nov 30, 2021 at 12:53:52PM +0300, Alexander Mikhalitsyn wrote:

Yes

But what about discussion from
https://lore.kernel.org/netdev/ff405eae-21d9-35f4-1397-b6f9a29a57ff@nvidia.com/

As far as I understand Roopa, we have to save at least RTNH_F_OFFLOAD flag too,
for instance, if user uses Cumulus and want to dump/restore routes.

I'm sorry if I misunderstood something.

Roopa, do you see a problem with the above patch?

On 11/30/21 3:28 AM, Ido Schimmel wrote:

quoted

diff --git a/ip/iproute.c b/ip/iproute.c
index 1447a5f78f49..0e6dad2b67e5 100644
--- a/ip/iproute.c
+++ b/ip/iproute.c

@@ -1632,6 +1632,8 @@ static int save_route(struct nlmsghdr *n, void *arg)
        if (!filter_nlmsg(n, tb, host_len))
                return 0;
 
+       r->rtm_flags &= RTNH_F_ONLINK;
+
        ret = write(STDOUT_FILENO, n, n->nlmsg_len);
        if ((ret > 0) && (ret != n->nlmsg_len)) {
                fprintf(stderr, "Short write while saving nlmsg\n");

to filter out all flags *except* RTNH_F_ONLINK.

Yes

quoted

But what about discussion from
https://lore.kernel.org/netdev/ff405eae-21d9-35f4-1397-b6f9a29a57ff@nvidia.com/

As far as I understand Roopa, we have to save at least RTNH_F_OFFLOAD flag too,
for instance, if user uses Cumulus and want to dump/restore routes.

I'm sorry if I misunderstood something.

Roopa, do you see a problem with the above patch?

The offload flag can be set from userspace but seems to me that should
only be done by the process that talks to hardware. Using iproute2 to
dump routes and then restore them should not set that flag.