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

From: Ido Schimmel <idosch@idosch.org>
To: Alexander Mikhalitsyn <alexander.mikhalitsyn@virtuozzo.com>
Cc: David Ahern <dsahern@gmail.com>,
	netdev@vger.kernel.org, David Miller <davem@davemloft.net>,
	Stephen Hemminger <stephen@networkplumber.org>,
	Ido Schimmel <idosch@nvidia.com>,
	Jakub Kicinski <kuba@kernel.org>, Roopa Prabhu <roopa@nvidia.com>,
	Andrei Vagin <avagin@gmail.com>,
	Pavel Tikhomirov <ptikhomirov@virtuozzo.com>,
	Alexander Mikhalitsyn <alexander@mihalicyn.com>
Subject: Re: [PATCH net-next] rtnetlink: add RTNH_REJECT_MASK
Date: Tue, 30 Nov 2021 11:28:32 +0200	[thread overview]
Message-ID: <YaXuwEg/hdkwNYEN@shredder> (raw)
In-Reply-To: <20211130113517.35324af97e168a9b0676b751@virtuozzo.com>

On Tue, Nov 30, 2021 at 11:35:17AM +0300, Alexander Mikhalitsyn wrote:
> On Tue, 30 Nov 2021 09:59:25 +0200
> Ido Schimmel <idosch@idosch.org> wrote:
> > 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.