On Tue, Nov 08, 2016 at 03:36:22PM +0100, Martin Pieuchot wrote: > On 04/11/16(Fri) 10:45, Claudio Jeker wrote: > > On Wed, Nov 02, 2016 at 05:44:14PM +0100, Martin Pieuchot wrote: > > > [..] > > > Diff below should fix that by automagically creating a loopback > > > interface when a new routing domain is created. That mean loX will > > > now correspond to routing domain X. > > > > I like this a lot. I think having a loopback interface per rdomain is kind > > of required but it was something I never finished. > > If we could assign 127.0.0.1 and ::1 to those interfaces automatically > > that would be even better (but that can be done as a second step). > > Adding ::1 is trivial, it's a one line change. > > To add 127.0.0.1 properly it's another story as currently netstart(8) > sets it.
I would love to kill this part out of netstart(8). 127.0.0.1 should always exists. > > > Not that if you are currently using a lo2 and a rdomain 2 this diff > > > will break your rdomain setup. > > > > Should we split loopbacks into lo(4) and e.g. loop(4) one is for loopback > > interfaces from userland and the other is only there for the 127.0.0.1 and > > ::1 loopbacks? At least that we the conflict would be solved since we then > > have different namespaces. Again this is something we can do on top of > > this. > > I'm not sure to understand the benefit. What's the use case for loop(4)? 2 name spaces, so that I don't have a conflict if I use lo1 for my loopback IPs and then later on create rdomain 1. > > > I'll write the manual page for rtable_loindex(9) if you're ok with > > > the approach. > > > > I think this approach is fine. The only thing I dislike is how you cram > > ifindex and rdomain id into the rtable 2 rdomain mapping. It feels dirty > > to me to do it that way instead of using a little struct holding the two > > values as independent data. > > It is dirty but using a small struct is not possible without revamping the > mp-safeness of rtable_get(9) which is, IMHO, not worth the effort. > Hmmm. That sucks. It makes the code hard to read and understand but I understand the reason and so maybe I need to swallow this bitter pill. > Here's an updated diff with man page, I'm asking for oks now that > semarie confirmed the m_pullup(9) regression isn't part of my diff. A few things inline. > Index: sys/kern/init_main.c > =================================================================== > RCS file: /cvs/src/sys/kern/init_main.c,v > retrieving revision 1.262 > diff -u -p -r1.262 init_main.c > --- sys/kern/init_main.c 7 Nov 2016 00:26:32 -0000 1.262 > +++ sys/kern/init_main.c 8 Nov 2016 14:22:12 -0000 > @@ -389,6 +389,9 @@ main(void *framep) > msginit(); > #endif > > + /* Create default routing table before attaching lo0. */ > + rtable_init(); > + > /* Attach pseudo-devices. */ > for (pdev = pdevinit; pdev->pdev_attach != NULL; pdev++) > if (pdev->pdev_count > 0) > @@ -398,8 +401,6 @@ main(void *framep) > crypto_init(); > swcr_init(); > #endif /* CRYPTO */ > - > - rtable_init(); > > /* > * Initialize protocols. Block reception of incoming packets > Index: sys/net/if.c > =================================================================== > RCS file: /cvs/src/sys/net/if.c,v > retrieving revision 1.458 > diff -u -p -r1.458 if.c > --- sys/net/if.c 8 Nov 2016 10:47:10 -0000 1.458 > +++ sys/net/if.c 8 Nov 2016 14:22:12 -0000 > @@ -258,7 +258,6 @@ struct srp_gc if_ifp_gc = SRP_GC_INITIAL > struct srp_gc if_map_gc = SRP_GC_INITIALIZER(if_map_dtor, NULL); > > struct ifnet_head ifnet = TAILQ_HEAD_INITIALIZER(ifnet); > -unsigned int lo0ifidx; > > void > if_idxmap_init(unsigned int limit) > @@ -1350,12 +1349,7 @@ p2p_rtrequest(struct ifnet *ifp, int req > > KASSERT(ifa == rt->rt_ifa); > > - /* > - * XXX Since lo0 is in the default rdomain we should not > - * (ab)use it for any route related to an interface of a > - * different rdomain. > - */ > - lo0ifp = if_get(lo0ifidx); > + lo0ifp = if_get(rtable_loindex(ifp->if_rdomain)); > KASSERT(lo0ifp != NULL); > TAILQ_FOREACH(lo0ifa, &lo0ifp->if_addrlist, ifa_list) { > if (lo0ifa->ifa_addr->sa_family == > @@ -1438,7 +1432,7 @@ if_up(struct ifnet *ifp) > > #ifdef INET6 > /* Userland expects the kernel to set ::1 on lo0. */ > - if (ifp->if_index == lo0ifidx) > + if (ifp->if_index == rtable_loindex(0)) > in6_ifattach(ifp); > #endif > > @@ -1605,14 +1599,31 @@ if_setrdomain(struct ifnet *ifp, int rdo > if (rdomain < 0 || rdomain > RT_TABLEID_MAX) > return (EINVAL); > > - /* make sure that the routing table exists */ > + /* > + * Create the routing table if it does not exist, including its > + * loopback interface with unit == rdomain. > + */ > if (!rtable_exists(rdomain)) { > + struct ifnet *loifp; > + char loifname[IFNAMSIZ]; > + > + snprintf(loifname, sizeof(loifname), "lo%d", rdomain); > + if ((error = if_clone_create(loifname, 0))) > + return (error); > + > + if ((loifp = ifunit(loifname)) == NULL) > + return (ENXIO); > + > s = splsoftnet(); > if ((error = rtable_add(rdomain)) == 0) > - rtable_l2set(rdomain, rdomain); > + rtable_l2set(rdomain, rdomain, loifp->if_index); > splx(s); > - if (error) > + if (error) { > + if_clone_destroy(loifname); > return (error); > + } > + > + loifp->if_rdomain = rdomain; > } > > /* make sure that the routing table is a real rdomain */ > Index: sys/net/if_loop.c > =================================================================== > RCS file: /cvs/src/sys/net/if_loop.c,v > retrieving revision 1.76 > diff -u -p -r1.76 if_loop.c > --- sys/net/if_loop.c 13 Apr 2016 11:41:15 -0000 1.76 > +++ sys/net/if_loop.c 8 Nov 2016 14:22:12 -0000 > @@ -121,6 +121,7 @@ > #include <net/if_var.h> > #include <net/if_types.h> > #include <net/netisr.h> > +#include <net/rtable.h> > #include <net/route.h> > > #include <netinet/in.h> > @@ -182,7 +183,7 @@ loop_clone_create(struct if_clone *ifc, > if (unit == 0) { > if_attachhead(ifp); > if_addgroup(ifp, ifc->ifc_name); > - lo0ifidx = ifp->if_index; > + rtable_l2set(0, 0, ifp->if_index); > } else > if_attach(ifp); > if_alloc_sadl(ifp); > @@ -195,7 +196,7 @@ loop_clone_create(struct if_clone *ifc, > int > loop_clone_destroy(struct ifnet *ifp) > { > - if (ifp->if_index == lo0ifidx) > + if (ifp->if_index == rtable_loindex(ifp->if_rdomain)) > return (EPERM); > > if_detach(ifp); > Index: sys/net/if_var.h > =================================================================== > RCS file: /cvs/src/sys/net/if_var.h,v > retrieving revision 1.76 > diff -u -p -r1.76 if_var.h > --- sys/net/if_var.h 8 Nov 2016 10:46:05 -0000 1.76 > +++ sys/net/if_var.h 8 Nov 2016 14:22:12 -0000 > @@ -291,7 +291,6 @@ int niq_enlist(struct niqueue *, struct > sysctl_mq((_n), (_l), (_op), (_olp), (_np), (_nl), &(_niq)->ni_q) > > extern struct ifnet_head ifnet; > -extern unsigned int lo0ifidx; > extern struct taskq *softnettq; > > void if_start(struct ifnet *); > Index: sys/net/route.c > =================================================================== > RCS file: /cvs/src/sys/net/route.c,v > retrieving revision 1.334 > diff -u -p -r1.334 route.c > --- sys/net/route.c 8 Nov 2016 10:39:32 -0000 1.334 > +++ sys/net/route.c 8 Nov 2016 14:22:12 -0000 > @@ -197,8 +197,6 @@ route_init(void) > while (rt_hashjitter == 0) > rt_hashjitter = arc4random(); > > - if (rtable_add(0) != 0) > - panic("route_init rtable_add"); > #ifdef BFD > bfdinit(); > #endif > Index: sys/net/rtable.c > =================================================================== > RCS file: /cvs/src/sys/net/rtable.c,v > retrieving revision 1.52 > diff -u -p -r1.52 rtable.c > --- sys/net/rtable.c 7 Sep 2016 09:36:49 -0000 1.52 > +++ sys/net/rtable.c 8 Nov 2016 14:22:12 -0000 > @@ -1,7 +1,7 @@ > /* $OpenBSD: rtable.c,v 1.52 2016/09/07 09:36:49 mpi Exp $ */ > > /* > - * Copyright (c) 2014-2015 Martin Pieuchot > + * Copyright (c) 2014-2016 Martin Pieuchot > * > * Permission to use, copy, modify, and distribute this software for any > * purpose with or without fee is hereby granted, provided that the above > @@ -41,7 +41,7 @@ > * afmap rtmap/dommp > * ----------- --------- ----- > * | 0 |--------> | 0 | 0 | ... | 0 | Array mapping rtableid > (=index) > - * ----------- --------- ----- to rdomain (=value). > + * ----------- --------- ----- to rdomain/loopback (=value). > * | AF_INET |. > * ----------- `. .---------. .---------. > * ... `----> | rtable0 | ... | rtableN | Array of pointers for > @@ -59,10 +59,20 @@ struct rtmap { > void **tbl; > }; > > -/* Array of rtableid -> rdomain mapping. */ > +/* > + * Array of rtableid -> rdomain mapping. > + * > + * Only used for the first index as describbed above. > + */ > struct dommp { > unsigned int limit; > - unsigned int *dom; > + /* > + * Array to get the routing domain and loopback interface related to > + * a routing table. Format: > + * > + * 8 unused bits | 16 bits for loopback index | 8 bits for rdomain > + */ > + unsigned int *value; > }; > > unsigned int rtmap_limit = 0; > @@ -146,6 +156,8 @@ rtable_init(void) > unsigned int keylen = 0; > int i; > > + KASSERT(sizeof(struct rtmap) == sizeof(struct dommp)); > + > /* We use index 0 for the rtable/rdomain map. */ > af2idx_max = 1; > memset(af2idx, 0, sizeof(af2idx)); > @@ -173,6 +185,9 @@ rtable_init(void) > M_WAITOK|M_ZERO); > > rtmap_init(); > + > + if (rtable_add(0) != 0) > + panic("unable to create default routing table"); > } > > int > @@ -221,7 +236,7 @@ rtable_add(unsigned int id) > > /* Use main rtable/rdomain by default. */ > dmm = srp_get_locked(&afmap[0]); > - dmm->dom[id] = 0; > + dmm->value[id] = 0; > > return (0); > } > @@ -272,24 +287,42 @@ rtable_l2(unsigned int rtableid) > > dmm = srp_enter(&sr, &afmap[0]); > if (rtableid < dmm->limit) > - rdomain = dmm->dom[rtableid]; > + rdomain = (dmm->value[rtableid] & RT_TABLEID_MASK); > srp_leave(&sr); > > return (rdomain); > } > > +unsigned int > +rtable_loindex(unsigned int rtableid) > +{ > + struct dommp *dmm; > + unsigned int loifidx = 0; > + struct srp_ref sr; > + > + dmm = srp_enter(&sr, &afmap[0]); > + if (rtableid < dmm->limit) > + loifidx = (dmm->value[rtableid] >> RT_TABLEID_BITS); > + srp_leave(&sr); > + > + return (loifidx); > +} > + > void > -rtable_l2set(unsigned int rtableid, unsigned int rdomain) > +rtable_l2set(unsigned int rtableid, unsigned int rdomain, unsigned int > loifidx) > { > struct dommp *dmm; > + unsigned int value; > > KERNEL_ASSERT_LOCKED(); > > if (!rtable_exists(rtableid) || !rtable_exists(rdomain)) > return; > > + value = (rdomain & RT_TABLEID_MASK) + (loifidx << RT_TABLEID_BITS); I would use a | here instead of the +. Seems cleaner to me. > + > dmm = srp_get_locked(&afmap[0]); > - dmm->dom[rtableid] = rdomain; > + dmm->value[rtableid] = value; > } > > #ifndef ART > Index: sys/net/rtable.h > =================================================================== > RCS file: /cvs/src/sys/net/rtable.h,v > retrieving revision 1.16 > diff -u -p -r1.16 rtable.h > --- sys/net/rtable.h 7 Sep 2016 09:36:49 -0000 1.16 > +++ sys/net/rtable.h 8 Nov 2016 14:22:12 -0000 > @@ -54,7 +54,8 @@ void rtable_init(void); > int rtable_exists(unsigned int); > int rtable_add(unsigned int); > unsigned int rtable_l2(unsigned int); > -void rtable_l2set(unsigned int, unsigned int); > +unsigned int rtable_loindex(unsigned int); > +void rtable_l2set(unsigned int, unsigned int, unsigned int); > > struct rtentry *rtable_lookup(unsigned int, struct sockaddr *, > struct sockaddr *, struct sockaddr *, uint8_t); > Index: sys/netinet/ip_output.c > =================================================================== > RCS file: /cvs/src/sys/netinet/ip_output.c,v > retrieving revision 1.327 > diff -u -p -r1.327 ip_output.c > --- sys/netinet/ip_output.c 4 Sep 2016 17:18:56 -0000 1.327 > +++ sys/netinet/ip_output.c 8 Nov 2016 14:22:12 -0000 > @@ -211,7 +211,7 @@ reroute: > > ia = ifatoia(ro->ro_rt->rt_ifa); > if (ISSET(ro->ro_rt->rt_flags, RTF_LOCAL)) > - ifp = if_get(lo0ifidx); > + ifp = if_get(rtable_loindex(m->m_pkthdr.ph_rtableid)); > else > ifp = if_get(ro->ro_rt->rt_ifidx); > if (ifp == NULL) { > Index: sys/netinet6/ip6_input.c > =================================================================== > RCS file: /cvs/src/sys/netinet6/ip6_input.c,v > retrieving revision 1.168 > diff -u -p -r1.168 ip6_input.c > --- sys/netinet6/ip6_input.c 24 Aug 2016 09:41:12 -0000 1.168 > +++ sys/netinet6/ip6_input.c 8 Nov 2016 14:22:12 -0000 > @@ -211,7 +211,10 @@ ip6_input(struct mbuf *m) > } else { > if (m->m_next) { > if (m->m_flags & M_LOOP) { > - ip6stat.ip6s_m2m[lo0ifidx]++; /*XXX*/ > + int ifidx; > + > + ifidx = rtable_loindex(m->m_pkthdr.ph_rtableid); > + ip6stat.ip6s_m2m[ifidx]++; This is not save because ip6s_m2m has only 32 elements. So it will overflow for rdomains > 32 (or in some other cases). A possible option is: if (m->m_next) { int ifidx = ifp->if_index; if (m->m_flags & M_LOOP) ifidx = rtable_loindex(m->m_pkthdr.ph_rtableid); if (ifidx < nitems(ip6stat.ip6s_m2m)) ip6stat.ip6s_m2m[ifidx]++; else ip6stat.ip6s_m2m[0]++; } else ip6stat.ip6s_m1++; But not sure if we should just drop the per interface list (probably a different diff). > } else if (ifp->if_index < nitems(ip6stat.ip6s_m2m)) > ip6stat.ip6s_m2m[ifp->if_index]++; > else > Index: sys/netinet6/ip6_output.c > =================================================================== > RCS file: /cvs/src/sys/netinet6/ip6_output.c,v > retrieving revision 1.216 > diff -u -p -r1.216 ip6_output.c > --- sys/netinet6/ip6_output.c 19 Sep 2016 18:09:09 -0000 1.216 > +++ sys/netinet6/ip6_output.c 8 Nov 2016 14:22:12 -0000 > @@ -460,7 +460,7 @@ reroute: > goto bad; > } > if (ISSET(rt->rt_flags, RTF_LOCAL)) > - ifp = if_get(lo0ifidx); > + ifp = if_get(rtable_loindex(m->m_pkthdr.ph_rtableid)); > else > ifp = if_get(rt->rt_ifidx); > } else { > Index: sys/sys/socket.h > =================================================================== > RCS file: /cvs/src/sys/sys/socket.h,v > retrieving revision 1.92 > diff -u -p -r1.92 socket.h > --- sys/sys/socket.h 28 Sep 2016 18:50:20 -0000 1.92 > +++ sys/sys/socket.h 8 Nov 2016 14:22:12 -0000 > @@ -143,7 +143,9 @@ struct splice { > /* > * Maximum number of alternate routing tables > */ > -#define RT_TABLEID_MAX 255 > +#define RT_TABLEID_MAX 255 > +#define RT_TABLEID_BITS 8 > +#define RT_TABLEID_MASK 0xff > > #endif /* __BSD_VISIBLE */ > > Index: share/man/man9/rtable_add.9 > =================================================================== > RCS file: /cvs/src/share/man/man9/rtable_add.9,v > retrieving revision 1.7 > diff -u -p -r1.7 rtable_add.9 > --- share/man/man9/rtable_add.9 11 Dec 2015 16:08:30 -0000 1.7 > +++ share/man/man9/rtable_add.9 8 Nov 2016 14:28:25 -0000 > @@ -21,6 +21,7 @@ > .Sh NAME > .Nm rtable_add , > .Nm rtable_exists , > +.Nm rtable_loindex , > .Nm rtable_l2 , > .Nm rtable_l2set > .Nd routing tables and routing domains interface > @@ -31,6 +32,8 @@ > .Ft int > .Fn rtable_exists "unsigned int id" > .Ft unsigned int > +.Fn rtable_loindex "unsigned int id" > +.Ft unsigned int > .Fn rtable_l2 "unsigned int id" > .Ft void > .Fn rtable_l2set "unsigned int id" "unsigned int rdomain" > @@ -53,6 +56,11 @@ if routing table with ID > exists, > .Fa 0 > otherwise. > +.It Fn rtable_loindex "unsigned int id" > +Return the default > +.Xr lo 4 > +interface index for the routing table with ID of > +.Fa id . > .It Fn rtable_l2 "unsigned int id" > Get the routing domain of routing table with ID of > .Fa id . > @@ -65,6 +73,7 @@ under the routing domain with ID of > .Sh CONTEXT > .Fn rtable_add , > .Fn rtable_exists , > +.Fn rtable_loindex , > .Fn rtable_l2 , > and > .Fn task_l2set > @@ -82,5 +91,6 @@ already exists. > Memory could not be allocated to extend the list of routing domains. > .El > .Sh SEE ALSO > +.Xr lo 4 , > .Xr route 4 , > .Xr route 8 > -- :wq Claudio