On Wed, Nov 04, 2015 at 12:33:23PM +0100, Martin Pieuchot wrote: > On 12/08/15(Wed) 17:03, Martin Pieuchot wrote: > > I'm currently working on the routing table interface to make is safe > > to use by multiple CPUs at the same time. The diff below is a big > > step in this direction and I'd really appreciate if people could test > > it with their usual network setup and report back. > > Below is an updated version of the diff now that bluhm@ fixed the > potential loop with RTF_GATEWAY routes. > > Note that regression tests will fail because we no longer call > rtalloc(9) inside rt_setgate(). In other words we do not cache > a next-hop route before using it. > > Ok?
OK bluhm@ > > Index: net/route.c > =================================================================== > RCS file: /cvs/src/sys/net/route.c,v > retrieving revision 1.268 > diff -u -p -r1.268 route.c > --- net/route.c 4 Nov 2015 10:13:55 -0000 1.268 > +++ net/route.c 4 Nov 2015 11:15:00 -0000 > @@ -151,6 +151,7 @@ void rt_timer_init(void); > int rtflushclone1(struct rtentry *, void *, u_int); > void rtflushclone(unsigned int, struct rtentry *); > int rt_if_remove_rtdelete(struct rtentry *, void *, u_int); > +struct rtentry *rt_match(struct sockaddr *, int, unsigned int); > > struct ifaddr *ifa_ifwithroute(int, struct sockaddr *, struct sockaddr > *, > u_int); > @@ -194,6 +195,12 @@ rtisvalid(struct rtentry *rt) > if (rt == NULL) > return (0); > > +#ifdef DIAGNOSTIC > + if (ISSET(rt->rt_flags, RTF_GATEWAY) && (rt->rt_gwroute != NULL) && > + ISSET(rt->rt_gwroute->rt_flags, RTF_GATEWAY)) > + panic("next hop must be directly reachable"); > +#endif > + > if ((rt->rt_flags & RTF_UP) == 0) > return (0); > > @@ -201,11 +208,27 @@ rtisvalid(struct rtentry *rt) > if (rt->rt_ifa == NULL || rt->rt_ifa->ifa_ifp == NULL) > return (0); > > + if (ISSET(rt->rt_flags, RTF_GATEWAY) && !rtisvalid(rt->rt_gwroute)) > + return (0); > + > return (1); > } > > +/* > + * Do the actual lookup for rtalloc(9), do not use directly! > + * > + * Return the best matching entry for the destination ``dst''. > + * > + * "RT_RESOLVE" means that a corresponding L2 entry should > + * be added to the routing table and resolved (via ARP or > + * NDP), if it does not exist. > + * > + * "RT_REPORT" indicates that a message should be sent to > + * userland if no matching route has been found or if an > + * error occured while adding a L2 entry. > + */ > struct rtentry * > -rtalloc(struct sockaddr *dst, int flags, unsigned int tableid) > +rt_match(struct sockaddr *dst, int flags, unsigned int tableid) > { > struct rtentry *rt0, *rt = NULL; > struct rt_addrinfo info; > @@ -336,6 +359,76 @@ rtalloc_mpath(struct sockaddr *dst, uint > } > #endif /* SMALL_KERNEL */ > > +/* > + * Look in the routing table for the best matching entry for > + * ``dst''. > + * > + * If a route with a gateway is found and its next hop is no > + * longer valid, try to cache it. > + */ > +struct rtentry * > +rtalloc(struct sockaddr *dst, int flags, unsigned int rtableid) > +{ > + struct rtentry *rt, *nhrt; > + > + rt = rt_match(dst, flags, rtableid); > + > + /* No match or route to host? We're done. */ > + if (rt == NULL || !ISSET(rt->rt_flags, RTF_GATEWAY)) > + return (rt); > + > + /* Nothing to do if the next hop is valid. */ > + if (rtisvalid(rt->rt_gwroute)) > + return (rt); > + > + rtfree(rt->rt_gwroute); > + rt->rt_gwroute = NULL; > + > + /* > + * If we cannot find a valid next hop, return the route > + * with a gateway. > + * > + * XXX Some dragons hiding in the tree certainly depends on > + * this behavior. But it is safe since rt_checkgate() wont > + * allow us to us this route later on. > + */ > + nhrt = rt_match(rt->rt_gateway, flags | RT_RESOLVE, rtableid); > + if (nhrt == NULL) > + return (rt); > + > + /* > + * Next hop must be reachable, this also prevents rtentry > + * loops for example when rt->rt_gwroute points to rt. > + */ > + if (ISSET(nhrt->rt_flags, RTF_CLONING|RTF_GATEWAY)) { > + rtfree(nhrt); > + return (rt); > + } > + > + /* > + * Next hop entry must be UP and on the same interface. > + */ > + if (!ISSET(nhrt->rt_flags, RTF_UP) || nhrt->rt_ifidx != rt->rt_ifidx) { > + rtfree(nhrt); > + return (rt); > + } > + > + /* > + * If the MTU of next hop is 0, this will reset the MTU of the > + * route to run PMTUD again from scratch. > + */ > + if (!ISSET(rt->rt_locks, RTV_MTU) && (rt->rt_mtu > nhrt->rt_mtu)) > + rt->rt_mtu = nhrt->rt_mtu; > + > + /* > + * Do not return the cached next-hop route, rt_checkgate() will > + * do the magic for us. > + */ > + rt->rt_gwroute = nhrt; > + > + return (rt); > +} > + > void > rtref(struct rtentry *rt) > { > @@ -499,7 +592,7 @@ create: > rt->rt_flags |= RTF_MODIFIED; > flags |= RTF_MODIFIED; > stat = &rtstat.rts_newgateway; > - rt_setgate(rt, gateway, rdomain); > + rt_setgate(rt, gateway); > } > } else > error = EHOSTUNREACH; > @@ -931,8 +1024,7 @@ rtrequest(int req, struct rt_addrinfo *i > * the routing table because the radix MPATH code use > * it to (re)order routes. > */ > - if ((error = rt_setgate(rt, info->rti_info[RTAX_GATEWAY], > - tableid))) { > + if ((error = rt_setgate(rt, info->rti_info[RTAX_GATEWAY]))) { > free(ndst, M_RTABLE, dlen); > pool_put(&rtentry_pool, rt); > return (error); > @@ -985,7 +1077,7 @@ rtrequest(int req, struct rt_addrinfo *i > } > > int > -rt_setgate(struct rtentry *rt, struct sockaddr *gate, unsigned int tableid) > +rt_setgate(struct rtentry *rt, struct sockaddr *gate) > { > int glen = ROUNDUP(gate->sa_len); > struct sockaddr *sa; > @@ -1003,22 +1095,7 @@ rt_setgate(struct rtentry *rt, struct so > rtfree(rt->rt_gwroute); > rt->rt_gwroute = NULL; > } > - if (rt->rt_flags & RTF_GATEWAY) { > - /* XXX is this actually valid to cross tables here? */ > - rt->rt_gwroute = rtalloc(gate, RT_REPORT|RT_RESOLVE, tableid); > - /* > - * If we switched gateways, grab the MTU from the new > - * gateway route if the current MTU is 0 or greater > - * than the MTU of gateway. > - * Note that, if the MTU of gateway is 0, we will reset the > - * MTU of the route to run PMTUD again from scratch. XXX > - */ > - if (rt->rt_gwroute && !(rt->rt_rmx.rmx_locks & RTV_MTU) && > - rt->rt_rmx.rmx_mtu && > - rt->rt_rmx.rmx_mtu > rt->rt_gwroute->rt_rmx.rmx_mtu) { > - rt->rt_rmx.rmx_mtu = rt->rt_gwroute->rt_rmx.rmx_mtu; > - } > - } > + > return (0); > } > > @@ -1033,26 +1110,8 @@ rt_checkgate(struct ifnet *ifp, struct r > rt0 = rt; > > if (rt->rt_flags & RTF_GATEWAY) { > - if (rt->rt_gwroute && !(rt->rt_gwroute->rt_flags & RTF_UP)) { > - rtfree(rt->rt_gwroute); > - rt->rt_gwroute = NULL; > - } > - if (rt->rt_gwroute == NULL) { > - rt->rt_gwroute = rtalloc(rt->rt_gateway, > - RT_REPORT|RT_RESOLVE, rtableid); > - if (rt->rt_gwroute == NULL) > - return (EHOSTUNREACH); > - } > - /* > - * Next hop must be reachable, this also prevents rtentry > - * loops, for example when rt->rt_gwroute points to rt. > - */ > - if (((rt->rt_gwroute->rt_flags & (RTF_UP|RTF_GATEWAY)) != > - RTF_UP) || (rt->rt_gwroute->rt_ifidx != ifp->if_index)) { > - rtfree(rt->rt_gwroute); > - rt->rt_gwroute = NULL; > + if (rt->rt_gwroute == NULL) > return (EHOSTUNREACH); > - } > rt = rt->rt_gwroute; > } > > Index: net/route.h > =================================================================== > RCS file: /cvs/src/sys/net/route.h,v > retrieving revision 1.118 > diff -u -p -r1.118 route.h > --- net/route.h 30 Oct 2015 09:39:42 -0000 1.118 > +++ net/route.h 4 Nov 2015 11:00:15 -0000 > @@ -119,6 +119,8 @@ struct rtentry { > }; > #define rt_use rt_rmx.rmx_pksent > #define rt_expire rt_rmx.rmx_expire > +#define rt_locks rt_rmx.rmx_locks > +#define rt_mtu rt_rmx.rmx_mtu > > #define RTF_UP 0x1 /* route usable */ > #define RTF_GATEWAY 0x2 /* destination is a gateway */ > @@ -358,7 +360,7 @@ void rt_maskedcopy(struct sockaddr *, > void rt_sendmsg(struct rtentry *, int, u_int); > void rt_sendaddrmsg(struct rtentry *, int); > void rt_missmsg(int, struct rt_addrinfo *, int, u_int, int, u_int); > -int rt_setgate(struct rtentry *, struct sockaddr *, unsigned int); > +int rt_setgate(struct rtentry *, struct sockaddr *); > int rt_checkgate(struct ifnet *, struct rtentry *, struct sockaddr *, > unsigned int, struct rtentry **); > void rt_setmetrics(u_long, struct rt_metrics *, struct rt_kmetrics *); > Index: net/rtsock.c > =================================================================== > RCS file: /cvs/src/sys/net/rtsock.c,v > retrieving revision 1.181 > diff -u -p -r1.181 rtsock.c > --- net/rtsock.c 2 Nov 2015 14:40:09 -0000 1.181 > +++ net/rtsock.c 4 Nov 2015 11:01:17 -0000 > @@ -745,9 +745,8 @@ report: > goto flush; > ifa = info.rti_ifa; > } > - if (info.rti_info[RTAX_GATEWAY] != NULL && > - (error = rt_setgate(rt, info.rti_info[RTAX_GATEWAY], > - tableid))) > + if (info.rti_info[RTAX_GATEWAY] != NULL && (error = > + rt_setgate(rt, info.rti_info[RTAX_GATEWAY]))) > goto flush; > if (ifa) { > if (rt->rt_ifa != ifa) { > Index: net/if_spppsubr.c > =================================================================== > RCS file: /cvs/src/sys/net/if_spppsubr.c,v > retrieving revision 1.144 > diff -u -p -r1.144 if_spppsubr.c > --- net/if_spppsubr.c 2 Nov 2015 11:19:30 -0000 1.144 > +++ net/if_spppsubr.c 4 Nov 2015 11:03:24 -0000 > @@ -4244,10 +4244,9 @@ sppp_update_gw_walker(struct rtentry *rt > if (rt->rt_ifidx == ifp->if_index) { > if (rt->rt_ifa->ifa_dstaddr->sa_family != > rt->rt_gateway->sa_family || > - (rt->rt_flags & RTF_GATEWAY) == 0) > + !ISSET(rt->rt_flags, RTF_GATEWAY)) > return (0); /* do not modify non-gateway routes */ > - bcopy(rt->rt_ifa->ifa_dstaddr, rt->rt_gateway, > - rt->rt_ifa->ifa_dstaddr->sa_len); > + rt_setgate(rt, rt->rt_ifa->ifa_dstaddr); > } > return (0); > }