On 19/02/18(Mon) 14:00, Sebastian Benoit wrote:
> Martin Pieuchot(m...@openbsd.org) on 2018.02.12 15:23:45 +0100:
> > On 12/02/18(Mon) 12:02, Sebastian Benoit wrote:
> > > routefilter currently filters the default route,
> > > if it's priority is higher than the filter prio.
> >
> > Then why not change the priority of the default route?
> >
> > > This might not be a good idea - for example you might want to
> > > redistribute a default route into ospf, for that you need it in the
> > > routing table but it is configured with a low priority (high value) as
> > > a route of last resort or maybe even as a reject/blackhole.
> >
> > Are you using multiple default routes? I don't understand why priority
> > matter, a default route will always be the last resort.
>
> Because we cannot guarantee that the default route has prio 8. For example
> bgpd might inject the default route with prio 48, and you want to
> redistribute it into ospf. Thats just how the interaction between our
> routing daemons works.
>
> I admit that this is a bit of a hack, because we kind of also want filters
> for any prefix (because you could have "redistribute 8.8.8.0/24" in your
> config) and route labels later on. I havent tackled that yet, so we only
> filter on prio and the default route special case and turn of the prio
> filter when we find a configuration that needs to see all routes.
>
> We are trying to optimize the ammount of data going over the route socket
> here. Shoveling information through it thats not needed is just useless
> work for the cpus.
>
> > > Treat the default route as a special case and always pass it.
> >
> > I'm not fan of adding more complexity to route_input().
>
> I understand that, but where else on the kernel side would you put filters?
> If i continue with this, i'm happy to make the code cleaner.
>
> > This function
> > does not deal with address nor mask, it is just delivering already built
> > messages. Now if this is unavoidable it would be nice if isdefault()
> > could be reused in if_group_routechange(), rt_ifa_addlocal() and
> > rt_ifa_dellocal().
>
> I saw that, but did not want to mix it into this diff. Will send one when
> this gets in.
>
> so ok?
I understand, I have a comment below but sure, I'm ok with the idea :)
> > > diff --git sys/net/rtsock.c sys/net/rtsock.c
> > > index 70497d29a93..2e94fdbc0b1 100644
> > > --- sys/net/rtsock.c
> > > +++ sys/net/rtsock.c
> > > @@ -110,7 +110,9 @@ int route_output(struct mbuf *, struct socket *,
> > > struct sockaddr *,
> > > int route_ctloutput(int, struct socket *, int, int, struct mbuf *);
> > > int route_usrreq(struct socket *, int, struct mbuf *, struct mbuf *,
> > > struct mbuf *, struct proc *);
> > > -void route_input(struct mbuf *m0, struct socket *, sa_family_t);
> > > +int route_is_default(struct sockaddr *, struct sockaddr *);
> > > +void route_input(struct mbuf *m0, struct socket *, struct
> > > rt_addrinfo *,
> > > + sa_family_t);
> > > int route_arp_conflict(struct rtentry *, struct rt_addrinfo *);
> > > int route_cleargateway(struct rtentry *, void *, unsigned int);
> > > void route_senddesync(void *);
> > > @@ -403,8 +405,33 @@ route_senddesync(void *data)
> > > timeout_add(&rop->timeout, ROUTE_DESYNC_RESEND_TIMEOUT);
> > > }
> > >
> > > +int
> > > +route_is_default(struct sockaddr *dst, struct sockaddr *mask)
> > > +{
> > > + /* see if.c if_group_routechange() */
> > > + switch (dst->sa_family) {
> > > + case AF_INET:
> > > + if (satosin(dst)->sin_addr.s_addr == INADDR_ANY &&
> > > + mask && (mask->sa_len == 0 ||
> > > + satosin(mask)->sin_addr.s_addr == INADDR_ANY))
> > > + return (1);
> > > + break;
> > > +#ifdef INET6
> > > + case AF_INET6:
> > > + if (IN6_ARE_ADDR_EQUAL(&(satosin6(dst))->sin6_addr,
> > > + &in6addr_any) && mask && (mask->sa_len == 0 ||
> > > + IN6_ARE_ADDR_EQUAL(&(satosin6(mask))->sin6_addr,
> > > + &in6addr_any)))
> > > + return (1);
> > > + break;
> > > +#endif
> > > + }
> > > + return (0);
> > > +}
> > > +
> > > void
> > > -route_input(struct mbuf *m0, struct socket *so, sa_family_t sa_family)
> > > +route_input(struct mbuf *m0, struct socket *so, struct rt_addrinfo *info,
> > > + sa_family_t sa_family)
> > > {
> > > struct routecb *rop;
> > > struct rawcb *rp;
> > > @@ -447,8 +474,16 @@ route_input(struct mbuf *m0, struct socket *so,
> > > sa_family_t sa_family)
> > > if (rtm->rtm_type != RTM_DESYNC && rop->msgfilter != 0 &&
> > > !(rop->msgfilter & (1 << rtm->rtm_type)))
> > > continue;
> > > - if (rop->priority != 0 && rop->priority < rtm->rtm_priority)
> > > - continue;
> > > + /* priority filter */
> > > + if (rop->priority != 0)
> > > + if ((info == NULL &&
> > > + rop->priority < rtm->rtm_priority) ||
> > > + (info != NULL &&
> > > + !route_is_default(info->rti_info[RTAX_DST],
> > > + info->rti_info[RTAX_NETMASK]) &&
> > > + rop->priority < rtm->rtm_priority))
> > > + continue;
I'd suggest to rewrite this like:
if (filter && rop->priority != 0 &&
rop->priority < rtm->rtm_priority)
continue;
And have the following signature for route_input():
route_input(struct mbuf *, struct socket *, sa_family_t, int filter);
So you'll call route_is_default() only once per route_input() instead of
once per socket(). And you don't need to pass a 'struct rt_addrinfo *'
to route_input() ;)
> > > switch (rtm->rtm_type) {
> > > case RTM_IFANNOUNCE:
> > > case RTM_DESYNC:
> > > @@ -767,7 +802,7 @@ fail:
> > > free(rtm, M_RTABLE, len);
> > > }
> > > if (m)
> > > - route_input(m, so, info.rti_info[RTAX_DST] ?
> > > + route_input(m, so, &info, info.rti_info[RTAX_DST] ?
> > > info.rti_info[RTAX_DST]->sa_family : AF_UNSPEC);
This will become:
af = info.rti_info[RTAX_DST] ? ...->sa_family : AF_UNSPEC;
filter = !route_is_default(...);
route_input(m, so, af, filter);
The resulting code is ready to be extend with arbitrary filers ;)
