On Sun, Nov 17, 2019 at 12:44:44PM +0100, Remi Locherer wrote: > On Sat, Nov 16, 2019 at 06:58:35AM +0100, Claudio Jeker wrote: > > On Fri, Nov 15, 2019 at 06:06:42PM +0100, Remi Locherer wrote: > > > On Mon, Nov 04, 2019 at 02:01:57PM +0200, Kapetanakis Giannis wrote: > > > > On 25/10/2019 13:57, Remi Locherer wrote: > > > > > Hi tech@, > > > > > > > > > > earlier this year I sent a diff that allowed to change an interface > > > > > from broadcast to point-to-point. > > > > > > > > > > https://marc.info/?l=openbsd-tech&m=156132923203704&w=2 > > > > > > > > > > It turned out that this was not sufficient. It made the adjacency > > > > > come up in p2p mode (no selection of DR or BDR) but didn't set a valid > > > > > next hop for routes learned over this p2p link. Actually the next hop > > > > > was > > > > > 0.0.0.0 which was never installed into the routing table. > > > > > > > > > > This is because for P2P interfaces the neighbor address is not taken > > > > > from > > > > > the received hello but from the "destination" parameter configured on > > > > > the > > > > > interface. Since this is not set on a broadcast interface the address > > > > > is > > > > > 0.0.0.0. > > > > > > > > > > My new diff changes this. Now also for P2P links the IP address of the > > > > > neighbor is taken from the hello packets (src address). This on it's > > > > > own > > > > > would make it simpler to interfere with the routing from remote. One > > > > > could > > > > > send unicast ospf hello messages and potentially disrupt the routing > > > > > setup. > > > > > I believe I mitigated this with an additional check I committed in > > > > > August: > > > > > only hello messages sent to the multicast address are now processed. > > > > > > > > > > The config looks like this: > > > > > > > > > > area 0.0.0.0 { > > > > > interface em0 { > > > > > type p2p > > > > > } > > > > > } > > > > > > > > > > It would be nice to get test reports for this new feature (check the > > > > > fib > > > > > and routing table!) and also test reports with real p2p2 interfaces > > > > > (gif > > > > > or gre). > > > > > > > > > > Of course OKs are also welcome. ;-) > > > > > > > > > > Remi > > > > > > > > > > > > Hi, > > > > > > > > From first test seems to work :) > > > > > > > > looking forward test it for IPv6 as well > > > > > > > > thanks > > > > > > > > Giannis > > > > > > > > > Anyone willing to OK this? > > > > See inline. > > > > [...] > > > > > Another option is to just add this ospfe_imsg_compose_rde() to the two > > places where the addr is updated. > > Right, that is actually simpler. > > > > > > + > > > + return (0); > > > } > > > > > > struct nbr * > > > Index: ospfd.c > > > =================================================================== > > > RCS file: /cvs/src/usr.sbin/ospfd/ospfd.c,v > > > retrieving revision 1.108 > > > diff -u -p -r1.108 ospfd.c > > > --- ospfd.c 16 May 2019 05:49:22 -0000 1.108 > > > +++ ospfd.c 23 Jun 2019 21:06:44 -0000 > > > @@ -911,6 +911,22 @@ merge_interfaces(struct area *a, struct > > > if_fsm(i, IF_EVT_UP); > > > } > > > > > > + if (i->p2p != xi->p2p) { > > > + /* re-add interface to enable or disable DR election */ > > > + if (ospfd_process == PROC_OSPF_ENGINE) > > > + if_fsm(i, IF_EVT_DOWN); > > > + else if (ospfd_process == PROC_RDE_ENGINE) > > > + rde_nbr_iface_del(i); > > > + LIST_REMOVE(i, entry); > > > + if_del(i); > > > + LIST_REMOVE(xi, entry); > > > + LIST_INSERT_HEAD(&a->iface_list, xi, entry); > > > + xi->area = a; > > > + if (ospfd_process == PROC_OSPF_ENGINE) > > > + xi->state = IF_STA_NEW; > > > + continue; > > > + } > > > > This is one big hammer. Would be nice to be a bit softer, also should this > > code be before > > log_debug("merge_interfaces: proc %d area %s merging " > > "interface %s", ospfd_process, inet_ntoa(a->id), i->name); > > > > Since it re-adds an interface. If so it should also have its own > > log_debug(). At least I think it makes little sense to merge most of the > > interface to just remove it and re-add it. > > I somehow added this with my first attempt and didn't re-evaluate. It turns > out that a restart is actually sufficient. > > > > > > + > > > strlcpy(i->dependon, xi->dependon, > > > sizeof(i->dependon)); > > > i->depend_ok = xi->depend_ok; > > > > Unrelated but I feel this code should be moved up before > > if (i->passive != xi->passive) { > > ... > > } > > Yes, I'll send a separate diff for that later. > > OK for the new diff? > > > > Index: hello.c > =================================================================== > RCS file: /cvs/src/usr.sbin/ospfd/hello.c,v > retrieving revision 1.24 > diff -u -p -r1.24 hello.c > --- hello.c 12 Aug 2019 20:21:58 -0000 1.24 > +++ hello.c 16 Nov 2019 14:52:10 -0000 > @@ -191,12 +191,16 @@ recv_hello(struct iface *iface, struct i > nbr->priority = hello.rtr_priority; > /* XXX neighbor address shouldn't be stored on virtual links */ > nbr->addr.s_addr = src.s_addr; > + ospfe_imsg_compose_rde(IMSG_NEIGHBOR_ADDR, nbr->peerid, 0, > + &src, sizeof(src)); > } > > if (nbr->addr.s_addr != src.s_addr) { > log_warnx("%s: neighbor ID %s changed its IP address", > __func__, inet_ntoa(nbr->id)); > nbr->addr.s_addr = src.s_addr; > + ospfe_imsg_compose_rde(IMSG_NEIGHBOR_ADDR, nbr->peerid, 0, > + &src, sizeof(src)); > } > > nbr->options = hello.opts; > Index: lsupdate.c > =================================================================== > RCS file: /cvs/src/usr.sbin/ospfd/lsupdate.c,v > retrieving revision 1.46 > diff -u -p -r1.46 lsupdate.c > --- lsupdate.c 15 Jul 2019 18:26:39 -0000 1.46 > +++ lsupdate.c 15 Aug 2019 21:10:13 -0000 > @@ -470,7 +470,7 @@ ls_retrans_timer(int fd, short event, vo > /* ls_retrans_list_free retriggers the timer */ > return; > } else if (nbr->iface->type == IF_TYPE_POINTOPOINT) > - memcpy(&addr, &nbr->iface->dst, sizeof(addr)); > + memcpy(&addr, &nbr->addr, sizeof(addr)); > else > inet_aton(AllDRouters, &addr); > } else > Index: neighbor.c > =================================================================== > RCS file: /cvs/src/usr.sbin/ospfd/neighbor.c,v > retrieving revision 1.48 > diff -u -p -r1.48 neighbor.c > --- neighbor.c 9 Feb 2018 02:14:03 -0000 1.48 > +++ neighbor.c 16 Nov 2019 14:53:52 -0000 > @@ -312,6 +312,7 @@ nbr_new(u_int32_t nbr_id, struct iface * > bzero(&rn, sizeof(rn)); > rn.id.s_addr = nbr->id.s_addr; > rn.area_id.s_addr = nbr->iface->area->id.s_addr; > + rn.addr.s_addr = nbr->addr.s_addr; > rn.ifindex = nbr->iface->ifindex; > rn.state = nbr->state; > rn.self = self; > Index: ospfd.c > =================================================================== > RCS file: /cvs/src/usr.sbin/ospfd/ospfd.c,v > retrieving revision 1.108 > diff -u -p -r1.108 ospfd.c > --- ospfd.c 16 May 2019 05:49:22 -0000 1.108 > +++ ospfd.c 16 Nov 2019 21:36:45 -0000 > @@ -911,6 +911,15 @@ merge_interfaces(struct area *a, struct > if_fsm(i, IF_EVT_UP); > } > > + if (i->p2p != xi->p2p) { > + /* restart interface to enable or disable DR election */ > + if (ospfd_process == PROC_OSPF_ENGINE) > + if_fsm(i, IF_EVT_DOWN); > + i->p2p = xi->p2p; > + if (ospfd_process == PROC_OSPF_ENGINE) > + if_fsm(i, IF_EVT_UP); > + } > + > strlcpy(i->dependon, xi->dependon, > sizeof(i->dependon)); > i->depend_ok = xi->depend_ok; > Index: ospfd.conf.5 > =================================================================== > RCS file: /cvs/src/usr.sbin/ospfd/ospfd.conf.5,v > retrieving revision 1.57 > diff -u -p -r1.57 ospfd.conf.5 > --- ospfd.conf.5 10 Jun 2019 06:07:15 -0000 1.57 > +++ ospfd.conf.5 23 Jun 2019 22:10:32 -0000 > @@ -419,6 +419,9 @@ Router. > .It Ic transmit-delay Ar seconds > Set the transmit delay. > The default value is 1; valid range is 1\-3600 seconds. > +.It Ic type p2p > +Set the interface type to point to point. > +This disables the election of a DR and BDR for the given interface. > .El > .Sh FILES > .Bl -tag -width "/etc/ospfd.conf" -compact > Index: ospfd.h > =================================================================== > RCS file: /cvs/src/usr.sbin/ospfd/ospfd.h,v > retrieving revision 1.104 > diff -u -p -r1.104 ospfd.h > --- ospfd.h 16 May 2019 05:49:22 -0000 1.104 > +++ ospfd.h 21 Sep 2019 10:25:32 -0000 > @@ -107,6 +107,7 @@ enum imsg_type { > IMSG_IFINFO, > IMSG_NEIGHBOR_UP, > IMSG_NEIGHBOR_DOWN, > + IMSG_NEIGHBOR_ADDR, > IMSG_NEIGHBOR_CHANGE, > IMSG_NEIGHBOR_CAPA, > IMSG_NETWORK_ADD, > @@ -363,6 +364,7 @@ struct iface { > u_int8_t linkstate; > u_int8_t priority; > u_int8_t passive; > + u_int8_t p2p; > }; > > struct ifaddrchange { > Index: parse.y > =================================================================== > RCS file: /cvs/src/usr.sbin/ospfd/parse.y,v > retrieving revision 1.98 > diff -u -p -r1.98 parse.y > --- parse.y 7 Jun 2019 04:57:45 -0000 1.98 > +++ parse.y 24 Oct 2019 20:09:11 -0000 > @@ -129,7 +129,7 @@ typedef struct { > %token AREA INTERFACE ROUTERID FIBPRIORITY FIBUPDATE REDISTRIBUTE > RTLABEL > %token RDOMAIN RFC1583COMPAT STUB ROUTER SPFDELAY SPFHOLDTIME EXTTAG > %token AUTHKEY AUTHTYPE AUTHMD AUTHMDKEYID > -%token METRIC PASSIVE > +%token METRIC P2P PASSIVE > %token HELLOINTERVAL FASTHELLOINTERVAL TRANSMITDELAY > %token RETRANSMITINTERVAL ROUTERDEADTIME ROUTERPRIORITY > %token SET TYPE > @@ -743,6 +743,10 @@ interfaceopts_l : interfaceopts_l interf > ; > > interfaceoptsl : PASSIVE { iface->passive = 1; } > + | TYPE P2P { > + iface->p2p = 1; > + iface->type = IF_TYPE_POINTOPOINT; > + } > | DEMOTE STRING { > if (strlcpy(iface->demote_group, $2, > sizeof(iface->demote_group)) >= > @@ -833,6 +837,7 @@ lookup(char *s) > {"msec", MSEC}, > {"no", NO}, > {"on", ON}, > + {"p2p", P2P}, > {"passive", PASSIVE}, > {"rdomain", RDOMAIN}, > {"redistribute", REDISTRIBUTE}, > Index: printconf.c > =================================================================== > RCS file: /cvs/src/usr.sbin/ospfd/printconf.c,v > retrieving revision 1.20 > diff -u -p -r1.20 printconf.c > --- printconf.c 28 Dec 2018 19:25:10 -0000 1.20 > +++ printconf.c 23 Jun 2019 22:05:55 -0000 > @@ -149,6 +149,9 @@ print_iface(struct iface *iface) > printf("\t\trouter-priority %d\n", iface->priority); > printf("\t\ttransmit-delay %d\n", iface->transmit_delay); > > + if (iface->p2p) > + printf("\t\ttype p2p\n"); > + > printf("\t\tauth-type %s\n", if_auth_name(iface->auth_type)); > switch (iface->auth_type) { > case AUTH_TYPE_NONE: > Index: rde.c > =================================================================== > RCS file: /cvs/src/usr.sbin/ospfd/rde.c,v > retrieving revision 1.109 > diff -u -p -r1.109 rde.c > --- rde.c 5 Feb 2018 12:11:28 -0000 1.109 > +++ rde.c 21 Sep 2019 12:17:09 -0000 > @@ -257,6 +257,7 @@ rde_dispatch_imsg(int fd, short event, v > struct timespec tp; > struct lsa *lsa; > struct area *area; > + struct in_addr addr; > struct vertex *v; > char *buf; > ssize_t n; > @@ -300,6 +301,17 @@ rde_dispatch_imsg(int fd, short event, v > break; > case IMSG_NEIGHBOR_DOWN: > rde_nbr_del(rde_nbr_find(imsg.hdr.peerid)); > + break; > + case IMSG_NEIGHBOR_ADDR: > + if (imsg.hdr.len - IMSG_HEADER_SIZE != sizeof(addr)) > + fatalx("invalid size of OE request"); > + memcpy(&addr, imsg.data, sizeof(addr)); > + > + nbr = rde_nbr_find(imsg.hdr.peerid); > + if (nbr == NULL) > + break; > + > + nbr->addr.s_addr = addr.s_addr; > break; > case IMSG_NEIGHBOR_CHANGE: > if (imsg.hdr.len - IMSG_HEADER_SIZE != sizeof(state)) > Index: rde.h > =================================================================== > RCS file: /cvs/src/usr.sbin/ospfd/rde.h,v > retrieving revision 1.39 > diff -u -p -r1.39 rde.h > --- rde.h 14 Mar 2015 02:22:09 -0000 1.39 > +++ rde.h 19 Sep 2019 13:46:43 -0000 > @@ -66,6 +66,7 @@ struct rde_nbr { > LIST_ENTRY(rde_nbr) entry, hash; > struct in_addr id; > struct in_addr area_id; > + struct in_addr addr; > TAILQ_HEAD(, rde_req_entry) req_list; > struct area *area; > struct iface *iface; > Index: rde_spf.c > =================================================================== > RCS file: /cvs/src/usr.sbin/ospfd/rde_spf.c,v > retrieving revision 1.77 > diff -u -p -r1.77 rde_spf.c > --- rde_spf.c 4 Apr 2019 19:57:08 -0000 1.77 > +++ rde_spf.c 29 Sep 2019 21:08:49 -0000 > @@ -373,6 +373,7 @@ calc_nexthop(struct vertex *dst, struct > { > struct v_nexthop *vn; > struct iface *iface; > + struct rde_nbr *nbr; > int i; > > /* case 1 */ > @@ -382,10 +383,14 @@ calc_nexthop(struct vertex *dst, struct > if (rtr_link->type != LINK_TYPE_POINTTOPOINT) > fatalx("inconsistent SPF tree"); > LIST_FOREACH(iface, &area->iface_list, entry) { > - if (rtr_link->data == iface->addr.s_addr) { > - vertex_nexthop_add(dst, parent, > - iface->dst.s_addr); > - return; > + if (rtr_link->data != iface->addr.s_addr) > + continue; > + LIST_FOREACH(nbr, &area->nbr_list, entry) { > + if (nbr->ifindex == iface->ifindex) { > + vertex_nexthop_add(dst, parent, > + nbr->addr.s_addr); > + return; > + } > } > } > fatalx("no interface found for interface"); >
Yes, this is OK claudio@ -- :wq Claudio
