Re: [PATCH net v4 1/8] ipv4/fib_frontend: Rename ip_valid_fib_dump_req, provide non-strict version

[Stefano Brivio <sbrivio@redhat.com>]

On Fri, 14 Jun 2019 21:16:54 -0600
David Ahern <dsahern@gmail.com> wrote:

> On 6/14/19 9:13 PM, Stefano Brivio wrote:
> > On Fri, 14 Jun 2019 20:54:49 -0600
> > David Ahern <dsahern@gmail.com> wrote:
> >   
> >> On 6/14/19 7:32 PM, Stefano Brivio wrote:  
> >>> ip_valid_fib_dump_req() does two things: performs strict checking on
> >>> netlink attributes for dump requests, and sets a dump filter if netlink
> >>> attributes require it.
> >>>
> >>> We might want to just set a filter, without performing strict validation.
> >>>
> >>> Rename it to ip_filter_fib_dump_req(), and add a 'strict' boolean
> >>> argument that must be set if strict validation is requested.
> >>>
> >>> This patch doesn't introduce any functional changes.
> >>>
> >>> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> >>> ---
> >>> v4: New patch
> >>>     
> >>
> >> Can you explain why this patch is needed? The existing function requires
> >> strict mode and is needed to enable any of the kernel side filtering
> >> beyond the RTM_F_CLONED setting in rtm_flags.  
> > 
> > It's mostly to have proper NLM_F_MATCH support. Let's pick an iproute2
> > version without strict checking support (< 5.0), that sets NLM_F_MATCH
> > though. Then we need this check:
> > 
> > 	if (nlh->nlmsg_len < nlmsg_msg_size(sizeof(*rtm)))  
> 
> but that check existed long before any of the strict checking and kernel
> side filtering was added.

Indeed. And now I'm recycling it, even if strict checking is not
requested.

> > and to set filter parameters not just based on flags (i.e. RTM_F_CLONED),
> > but also on table, protocol, etc.  
> 
> and to do that you *must* have strict checking. There is no way to trust
> userspace without that strict flag set because iproute2 for the longest
> time sent the wrong header for almost all dump requests.

So you're implying that:

- we shouldn't support NLM_F_MATCH

- we should keep this broken for iproute2 < 5.0.0?

I guess this might be acceptable, but please state it clearly.

By the way, if really needed, we can do strict checking even if not
requested. But this might add more and more userspace breakage, I guess.

-- 
Stefano