Re: [net PATCH] net: route dump netlink NLM_F_MULTI flag missing

[Stefano Brivio <sbrivio@redhat.com>]

On Fri, 23 Aug 2019 17:11:38 -0700
John Fastabend <john.fastabend@gmail.com> wrote:

> An excerpt from netlink(7) man page,
> 
>   In multipart messages (multiple nlmsghdr headers with associated payload
>   in one byte stream) the first and all following headers have the
>   NLM_F_MULTI flag set, except for the last  header  which  has the type
>   NLMSG_DONE.
> 
> but, after (ee28906) there is a missing NLM_F_MULTI flag in the middle of a
> FIB dump.

In your case (see below), it can be zero or more, depending on how many
exception routes you have.

> The result is user space applications following above man page
> excerpt may get confused and may stop parsing msg believing something went
> wrong.

Worse yet, also RFC 3459 says:

	[...] For multipart
	messages, the first and all following headers have the NLM_F_MULTI
	Netlink header flag set, except for the last header which has the
	Netlink header type NLMSG_DONE.

But iproute2 doesn't check for this, so the selftests I added didn't
notice. Thanks for fixing this!

> In the golang netlink lib [0] the library logic stops parsing believing the
> message is not a multipart message. Found this running Cilium[1] against
> net-next while adding a feature to auto-detect routes. I noticed with
> multiple route tables we no longer could detect the default routes on net
> tree kernels because the library logic was not returning them.

However, note that if strict netlink checking is requested (I think the
library should be updated), and RTM_F_CLONED is not set (which should
be the case if you are just looking for "regular" routes), you won't
hit this.

> Fix this by handling the fib_dump_info_fnhe() case the same way the
> fib_dump_info() handles it by passing the flags argument through the
> call chain and adding a flags argument to rt_fill_info().
> 
> Tested with Cilium stack and auto-detection of routes works again. Also
> annotated libs to dump netlink msgs and inspected NLM_F_MULTI and
> NLMSG_DONE flags look correct after this.
> 
> Note: In inet_rtm_getroute() pass rt_fill_info() '0' for flags the same
> as is done for fib_dump_info() so this looks correct to me.

Yes, that's correct, because if the buffer is too small for a single
route dumped by a single rt_fill_info() call, we'll just fail, so that
will never be a multipart message.

> [0] https://github.com/vishvananda/netlink/
> [1] https://github.com/cilium/
> 
> Fixes: ee28906fd7a14 ("ipv4: Dump route exceptions if requested")
> Signed-off-by: John Fastabend <john.fastabend@gmail.com>

Reviewed-by: Stefano Brivio <sbrivio@redhat.com>

-- 
Stefano