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.
> 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 21 Sep 2019 22:06:17 -0000
> @@ -189,14 +189,13 @@ recv_hello(struct iface *iface, struct i
> nbr->dr.s_addr = hello.d_rtr;
> nbr->bdr.s_addr = hello.bd_rtr;
> nbr->priority = hello.rtr_priority;
> - /* XXX neighbor address shouldn't be stored on virtual links */
> - nbr->addr.s_addr = src.s_addr;
> + nbr_update_addr(nbr->peerid, 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;
> + nbr_update_addr(nbr->peerid, 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 21 Sep 2019 15:28:43 -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;
> @@ -347,6 +348,23 @@ nbr_del(struct nbr *nbr)
> LIST_REMOVE(nbr, hash);
>
> free(nbr);
> +}
> +
> +int
Since the callers of nbr_update_addr() don't use the return value make
this a void function.
> +nbr_update_addr(u_int32_t peerid, struct in_addr addr) {
> +
Fix bad placement of {
> + struct nbr *nbr = NULL;
> +
> + nbr = nbr_find_peerid(peerid);
> + if (nbr == NULL)
> + return (1);
Why passing peerid to this function to look up the neighbor again when the
functions are called like this?
nbr_update_addr(nbr->peerid, src);
Why not just pass struct nbr *nbr? Would also remove a possible error case.
> +
> + /* XXX neighbor address shouldn't be stored on virtual links */
> + nbr->addr.s_addr = addr.s_addr;
> + ospfe_imsg_compose_rde(IMSG_NEIGHBOR_ADDR, peerid, 0, &addr,
> + sizeof(addr));
Another option is to just add this ospfe_imsg_compose_rde() to the two
places where the addr is updated.
> +
> + 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.
> +
> 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) {
...
}
> 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: ospfe.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/ospfd/ospfe.h,v
> retrieving revision 1.46
> diff -u -p -r1.46 ospfe.h
> --- ospfe.h 25 Oct 2014 03:23:49 -0000 1.46
> +++ ospfe.h 21 Sep 2019 15:27:08 -0000
> @@ -204,6 +204,7 @@ void lsa_cache_put(struct lsa_ref *, s
> void nbr_init(u_int32_t);
> struct nbr *nbr_new(u_int32_t, struct iface *, int);
> void nbr_del(struct nbr *);
> +int nbr_update_addr(u_int32_t, struct in_addr);
>
> struct nbr *nbr_find_id(struct iface *, u_int32_t);
> struct nbr *nbr_find_peerid(u_int32_t);
> 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");
>
Apart for the comments about nbr_update_addr() this diff is fine by me.
--
:wq Claudio
