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?

> 
> > 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;
> > +
> >             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);
> >  
> >     return (error);
> > @@ -1472,7 +1507,7 @@ rtm_miss(int type, struct rt_addrinfo *rtinfo, int 
> > flags, uint8_t prio,
> >     rtm->rtm_tableid = tableid;
> >     rtm->rtm_addrs = rtinfo->rti_addrs;
> >     rtm->rtm_index = ifidx;
> > -   route_input(m, NULL, sa ? sa->sa_family : AF_UNSPEC);
> > +   route_input(m, NULL, NULL, sa ? sa->sa_family : AF_UNSPEC);
> >  }
> >  
> >  /*
> > @@ -1497,7 +1532,7 @@ rtm_ifchg(struct ifnet *ifp)
> >     ifm->ifm_xflags = ifp->if_xflags;
> >     if_getdata(ifp, &ifm->ifm_data);
> >     ifm->ifm_addrs = 0;
> > -   route_input(m, NULL, AF_UNSPEC);
> > +   route_input(m, NULL, NULL, AF_UNSPEC);
> >  }
> >  
> >  /*
> > @@ -1533,7 +1568,7 @@ rtm_addr(struct rtentry *rt, int cmd, struct ifaddr 
> > *ifa)
> >     ifam->ifam_addrs = info.rti_addrs;
> >     ifam->ifam_tableid = ifp->if_rdomain;
> >  
> > -   route_input(m, NULL,
> > +   route_input(m, NULL, NULL,
> >         ifa->ifa_addr ? ifa->ifa_addr->sa_family : AF_UNSPEC);
> >  }
> >  
> > @@ -1556,7 +1591,7 @@ rtm_ifannounce(struct ifnet *ifp, int what)
> >     ifan->ifan_index = ifp->if_index;
> >     strlcpy(ifan->ifan_name, ifp->if_xname, sizeof(ifan->ifan_name));
> >     ifan->ifan_what = what;
> > -   route_input(m, NULL, AF_UNSPEC);
> > +   route_input(m, NULL, NULL, AF_UNSPEC);
> >  }
> >  
> >  #ifdef BFD
> > @@ -1587,7 +1622,7 @@ rtm_bfd(struct bfd_config *bfd)
> >     bfd2sa(bfd->bc_rt, &sa_bfd);
> >     memcpy(&bfdm->bm_sa, &sa_bfd, sizeof(sa_bfd));
> >  
> > -   route_input(m, NULL, info.rti_info[RTAX_DST]->sa_family);
> > +   route_input(m, NULL, NULL, info.rti_info[RTAX_DST]->sa_family);
> >  }
> >  #endif /* BFD */
> >  
> > 
> 

Reply via email to