Re: [PATCH net-next] rtnetlink: add RTNH_REJECT_MASK

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