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

[Ido Schimmel <idosch@idosch.org>]

On Tue, Nov 30, 2021 at 12:53:52PM +0300, Alexander Mikhalitsyn wrote:
> On Tue, 30 Nov 2021 11:28:32 +0200
> Ido Schimmel <idosch@idosch.org> wrote:
> 
> > 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.
> 
> 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.

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?