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 Sat, 15 Jun 2019 05:27:05 +0200
Stefano Brivio <sbrivio@redhat.com> wrote:

> 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.

Maybe I have a simpler alternative, that doesn't allow filters without
strict checking (your concern above) and fixes the issue for most
iproute2 versions (except for 'ip -6 route cache flush' from 5.0.0 to
current, unpatched version). I would also like to avoid introducing
this bug:

- 'ip route list cache table main' currently returns nothing (bug)

- 'ip route list cache table main' with v1-v3 would return all cached
  routes (new bug)

and retain this feature from v4:

- if neither NLM_F_MATCH nor other filters are set, dump all cached and
  uncached routes. There's no way to get cached and uncached ones with
  a single request, otherwise. This would also fit RFC 3549.

We could do this:

- strict checking enabled (iproute2 >= 5.0.0):
  - in inet{,6}_dump_fib(): if NLM_F_MATCH is set, set
    filter->filter_set in any case

  - in fn_trie_dump_leaf() and rt6_dump_route(): use filter->filter_set
    to decide if we want to filter depending on RTM_F_CLONED being
    set/unset. If other filters (rt_type, dev, protocol) are not set,
    they are still wildcards (existing implementation)

- no strict checking (iproute2 < 5.0.0):
  - we can't filter consistently, so apply no filters at all: dump all
    the routes (filter->filter_set not set), cached and uncached. That
    means more netlink messages, but no spam as iproute2 filters them
    anyway, and list/flush cache commands work again.

I would drop 1/8, turn 2/8 and 6/8 into a straightforward:

 	if (cb->strict_check) {
 		err = ip_valid_fib_dump_req(net, nlh, &filter, cb);
 		if (err < 0)
 			return err;
+		if (nlh->nlmsg_flags & NLM_F_MATCH)
+			filter.filter_set = 1;
 	} else if (nlmsg_len(nlh) >= sizeof(struct rtmsg)) {
 		struct rtmsg *rtm = nlmsg_data(nlh);

and other patches remain the same.

What do you think?

-- 
Stefano