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 Mon, 17 Jun 2019 07:38:54 -0600
David Ahern <dsahern@gmail.com> wrote:

> On 6/16/19 2:04 PM, Stefano Brivio wrote:
> > 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?
> >   
> 
> With strict checking (5.0 and forward):
> - RTM_F_CLONED NOT set means dump only FIB entries
> - RTM_F_CLONED set means dump only exceptions

Okay. Should we really ignore the RFC and NLM_F_MATCH though? If we add
field(s) to the filter, it comes almost for free, something like:

	if (nlh->nlmsg_flags & NLM_F_MATCH)
		filter->dump_exceptions = rtm->rtm_flags & RTM_F_CLONED;

instead of:

	filter->dump_exceptions = rtm->rtm_flags & RTM_F_CLONED;

> Without strict checking (old iproute2 on any kernel):
> - dump all, userspace has to sort
> 
> Kernel side this can be handled with new field, dump_exceptions, in the
> filter that defaults to true and then is reset in the strict path if the
> flag is not set.

I guess we need to add two fields, we'll need a 'dump_routes' too.

Otherwise, the dump functions can't distinguish between the three cases
('no strict checking', 'strict checking and RTM_F_CLONED', 'strict
checking and no RTM_F_CLONED'). How would you do this with a single
additional field?

-- 
Stefano