Re: ospfd more ibuf cleanup
On Mon, Jul 03, 2023 at 11:28:44AM +0200, Claudio Jeker wrote: > Similar to the relayd diff use ibuf_data instead of ibuf->buf. ok tb
ospfd more ibuf cleanup
Similar to the relayd diff use ibuf_data instead of ibuf->buf. -- :wq Claudio Index: auth.c === RCS file: /cvs/src/usr.sbin/ospfd/auth.c,v retrieving revision 1.21 diff -u -p -r1.21 auth.c --- auth.c 20 Jun 2023 15:19:55 - 1.21 +++ auth.c 30 Jun 2023 08:56:56 - @@ -154,13 +154,13 @@ auth_gen(struct ibuf *buf, struct iface switch (iface->auth_type) { case AUTH_NONE: - chksum = in_cksum(buf->buf, ibuf_size(buf)); + chksum = in_cksum(ibuf_data(buf), ibuf_size(buf)); if (ibuf_set(buf, offsetof(struct ospf_hdr, chksum), &chksum, sizeof(chksum)) == -1) fatalx("auth_gen: ibuf_set failed"); break; case AUTH_SIMPLE: - chksum = in_cksum(buf->buf, ibuf_size(buf)); + chksum = in_cksum(ibuf_data(buf), ibuf_size(buf)); if (ibuf_set(buf, offsetof(struct ospf_hdr, chksum), &chksum, sizeof(chksum)) == -1) fatalx("auth_gen: ibuf_set failed"); @@ -193,7 +193,7 @@ auth_gen(struct ibuf *buf, struct iface /* calculate MD5 digest */ MD5Init(&hash); - MD5Update(&hash, buf->buf, ibuf_size(buf)); + MD5Update(&hash, ibuf_data(buf), ibuf_size(buf)); MD5Update(&hash, digest, MD5_DIGEST_LENGTH); MD5Final(digest, &hash); Index: ospfe.c === RCS file: /cvs/src/usr.sbin/ospfd/ospfe.c,v retrieving revision 1.112 diff -u -p -r1.112 ospfe.c --- ospfe.c 20 Jun 2023 15:19:55 - 1.112 +++ ospfe.c 30 Jun 2023 08:55:44 - @@ -1099,13 +1099,13 @@ orig_rtr_lsa(struct area *area) if (ibuf_set(buf, 0, &lsa_hdr, sizeof(lsa_hdr)) == -1) fatal("orig_rtr_lsa: ibuf_set failed"); - chksum = iso_cksum(buf->buf, ibuf_size(buf), LS_CKSUM_OFFSET); + chksum = iso_cksum(ibuf_data(buf), ibuf_size(buf), LS_CKSUM_OFFSET); if (ibuf_set_n16(buf, LS_CKSUM_OFFSET, chksum) == -1) fatal("orig_rtr_lsa: ibuf_set_n16 failed"); if (self && num_links) imsg_compose_event(iev_rde, IMSG_LS_UPD, self->peerid, 0, - -1, buf->buf, ibuf_size(buf)); + -1, ibuf_data(buf), ibuf_size(buf)); else log_warnx("orig_rtr_lsa: empty area %s", inet_ntoa(area->id)); @@ -1165,12 +1165,12 @@ orig_net_lsa(struct iface *iface) if (ibuf_set(buf, 0, &lsa_hdr, sizeof(lsa_hdr)) == -1) fatal("orig_net_lsa: ibuf_set failed"); - chksum = iso_cksum(buf->buf, ibuf_size(buf), LS_CKSUM_OFFSET); + chksum = iso_cksum(ibuf_data(buf), ibuf_size(buf), LS_CKSUM_OFFSET); if (ibuf_set_n16(buf, LS_CKSUM_OFFSET, chksum) == -1) fatal("orig_net_lsa: ibuf_set_n16 failed"); imsg_compose_event(iev_rde, IMSG_LS_UPD, iface->self->peerid, 0, - -1, buf->buf, ibuf_size(buf)); + -1, ibuf_data(buf), ibuf_size(buf)); ibuf_free(buf); } Index: packet.c === RCS file: /cvs/src/usr.sbin/ospfd/packet.c,v retrieving revision 1.36 diff -u -p -r1.36 packet.c --- packet.c3 Nov 2021 21:40:03 - 1.36 +++ packet.c7 Nov 2021 11:14:35 - @@ -85,7 +85,7 @@ send_packet(struct iface *iface, struct bzero(&msg, sizeof(msg)); iov[0].iov_base = &ip_hdr; iov[0].iov_len = sizeof(ip_hdr); - iov[1].iov_base = buf->buf; + iov[1].iov_base = ibuf_data(buf); iov[1].iov_len = ibuf_size(buf); msg.msg_name = dst; msg.msg_namelen = sizeof(*dst);
Re: ospfd use new ibuf functions
On Tue, Jun 20, 2023 at 06:29:59PM +0200, Claudio Jeker wrote: > On Tue, Jun 20, 2023 at 02:47:41PM +0200, Claudio Jeker wrote: > > This diff updates ospfd to use the new ibuf API. > > > > It mainly removes the use of ibuf_seek() and replaces these calls with > > ibuf_set(). > > > > Regress still passes with this diff in. > > Here the same diff for ospf6d. ok Two minor comments below. Feel free to ignore them. > -- > :wq Claudio > > Index: database.c > === > RCS file: /cvs/src/usr.sbin/ospf6d/database.c,v > retrieving revision 1.22 > diff -u -p -r1.22 database.c > --- database.c8 Mar 2023 04:43:14 - 1.22 > +++ database.c16 Jun 2023 10:27:04 - > @@ -51,7 +51,7 @@ send_db_description(struct nbr *nbr) > goto fail; > > /* reserve space for database description header */ > - if (ibuf_reserve(buf, sizeof(dd_hdr)) == NULL) > + if (ibuf_add_zero(buf, sizeof(dd_hdr)) == -1) > goto fail; > > switch (nbr->state) { > @@ -134,8 +134,9 @@ send_db_description(struct nbr *nbr) > dd_hdr.bits = bits; > dd_hdr.dd_seq_num = htonl(nbr->dd_seq_num); > > - memcpy(ibuf_seek(buf, sizeof(struct ospf_hdr), sizeof(dd_hdr)), > - &dd_hdr, sizeof(dd_hdr)); > + if (ibuf_set(buf, sizeof(struct ospf_hdr), &dd_hdr, > + sizeof(dd_hdr)) == -1) > + goto fail; > > /* calculate checksum */ > if (upd_ospf_hdr(buf, nbr->iface)) > Index: lsupdate.c > === > RCS file: /cvs/src/usr.sbin/ospf6d/lsupdate.c,v > retrieving revision 1.22 > diff -u -p -r1.22 lsupdate.c > --- lsupdate.c8 Mar 2023 04:43:14 - 1.22 > +++ lsupdate.c16 Jun 2023 10:27:15 - > @@ -177,7 +177,7 @@ prepare_ls_update(struct iface *iface, i > goto fail; > > /* reserve space for number of lsa field */ > - if (ibuf_reserve(buf, sizeof(u_int32_t)) == NULL) > + if (ibuf_add_zero(buf, sizeof(u_int32_t)) == -1) > goto fail; > > return (buf); > @@ -208,8 +208,10 @@ add_ls_update(struct ibuf *buf, struct i > age = ntohs(age); > if ((age += older + iface->transmit_delay) >= MAX_AGE) > age = MAX_AGE; > - age = htons(age); > - memcpy(ibuf_seek(buf, ageoff, sizeof(age)), &age, sizeof(age)); > + if (ibuf_set_n16(buf, ageoff, age) == -1) { > + log_warn("add_ls_update"); This error is weid but it's like the others nearby. > + return (0); > + } > > return (1); > } > @@ -218,9 +220,8 @@ int > send_ls_update(struct ibuf *buf, struct iface *iface, struct in6_addr addr, > u_int32_t nlsa) > { > - nlsa = htonl(nlsa); > - memcpy(ibuf_seek(buf, sizeof(struct ospf_hdr), sizeof(nlsa)), > - &nlsa, sizeof(nlsa)); > + if (ibuf_set_n32(buf, sizeof(struct ospf_hdr), nlsa) == -1) > + goto fail; > /* calculate checksum */ > if (upd_ospf_hdr(buf, iface)) > goto fail; > Index: ospfe.c > === > RCS file: /cvs/src/usr.sbin/ospf6d/ospfe.c,v > retrieving revision 1.68 > diff -u -p -r1.68 ospfe.c > --- ospfe.c 8 Mar 2023 04:43:14 - 1.68 > +++ ospfe.c 20 Jun 2023 16:25:52 - > @@ -780,11 +780,11 @@ orig_rtr_lsa(struct area *area) > fatal("orig_rtr_lsa"); > > /* reserve space for LSA header and LSA Router header */ > - if (ibuf_reserve(buf, sizeof(lsa_hdr)) == NULL) > - fatal("orig_rtr_lsa: ibuf_reserve failed"); > + if (ibuf_add_zero(buf, sizeof(lsa_hdr)) == -1) > + fatal("orig_rtr_lsa: ibuf_add_zero failed"); > > - if (ibuf_reserve(buf, sizeof(lsa_rtr)) == NULL) > - fatal("orig_rtr_lsa: ibuf_reserve failed"); > + if (ibuf_add_zero(buf, sizeof(lsa_rtr)) == -1) > + fatal("orig_rtr_lsa: ibuf_add_zero failed"); These two could be combined like the others below, but I'm fine with keeping it as close to mechanical as possible. > > /* links */ > LIST_FOREACH(iface, &area->iface_list, entry) { > @@ -944,8 +944,8 @@ orig_rtr_lsa(struct area *area) > LSA_24_SETLO(lsa_rtr.opts, area_ospf_options(area)); > LSA_24_SETHI(lsa_rtr.opts, flags); > lsa_rtr.opts = htonl(lsa_rtr.opts); > - memcpy(ibuf_seek(buf, sizeof(lsa_hdr), sizeof(lsa_rtr)), > - &lsa_rtr, sizeof(lsa_rtr)); > +
Re: ospfd use new ibuf functions
On Tue, Jun 20, 2023 at 02:47:41PM +0200, Claudio Jeker wrote: > This diff updates ospfd to use the new ibuf API. > > It mainly removes the use of ibuf_seek() and replaces these calls with > ibuf_set(). > > Regress still passes with this diff in. Here the same diff for ospf6d. -- :wq Claudio Index: database.c === RCS file: /cvs/src/usr.sbin/ospf6d/database.c,v retrieving revision 1.22 diff -u -p -r1.22 database.c --- database.c 8 Mar 2023 04:43:14 - 1.22 +++ database.c 16 Jun 2023 10:27:04 - @@ -51,7 +51,7 @@ send_db_description(struct nbr *nbr) goto fail; /* reserve space for database description header */ - if (ibuf_reserve(buf, sizeof(dd_hdr)) == NULL) + if (ibuf_add_zero(buf, sizeof(dd_hdr)) == -1) goto fail; switch (nbr->state) { @@ -134,8 +134,9 @@ send_db_description(struct nbr *nbr) dd_hdr.bits = bits; dd_hdr.dd_seq_num = htonl(nbr->dd_seq_num); - memcpy(ibuf_seek(buf, sizeof(struct ospf_hdr), sizeof(dd_hdr)), - &dd_hdr, sizeof(dd_hdr)); + if (ibuf_set(buf, sizeof(struct ospf_hdr), &dd_hdr, + sizeof(dd_hdr)) == -1) + goto fail; /* calculate checksum */ if (upd_ospf_hdr(buf, nbr->iface)) Index: lsupdate.c === RCS file: /cvs/src/usr.sbin/ospf6d/lsupdate.c,v retrieving revision 1.22 diff -u -p -r1.22 lsupdate.c --- lsupdate.c 8 Mar 2023 04:43:14 - 1.22 +++ lsupdate.c 16 Jun 2023 10:27:15 - @@ -177,7 +177,7 @@ prepare_ls_update(struct iface *iface, i goto fail; /* reserve space for number of lsa field */ - if (ibuf_reserve(buf, sizeof(u_int32_t)) == NULL) + if (ibuf_add_zero(buf, sizeof(u_int32_t)) == -1) goto fail; return (buf); @@ -208,8 +208,10 @@ add_ls_update(struct ibuf *buf, struct i age = ntohs(age); if ((age += older + iface->transmit_delay) >= MAX_AGE) age = MAX_AGE; - age = htons(age); - memcpy(ibuf_seek(buf, ageoff, sizeof(age)), &age, sizeof(age)); + if (ibuf_set_n16(buf, ageoff, age) == -1) { + log_warn("add_ls_update"); + return (0); + } return (1); } @@ -218,9 +220,8 @@ int send_ls_update(struct ibuf *buf, struct iface *iface, struct in6_addr addr, u_int32_t nlsa) { - nlsa = htonl(nlsa); - memcpy(ibuf_seek(buf, sizeof(struct ospf_hdr), sizeof(nlsa)), - &nlsa, sizeof(nlsa)); + if (ibuf_set_n32(buf, sizeof(struct ospf_hdr), nlsa) == -1) + goto fail; /* calculate checksum */ if (upd_ospf_hdr(buf, iface)) goto fail; Index: ospfe.c === RCS file: /cvs/src/usr.sbin/ospf6d/ospfe.c,v retrieving revision 1.68 diff -u -p -r1.68 ospfe.c --- ospfe.c 8 Mar 2023 04:43:14 - 1.68 +++ ospfe.c 20 Jun 2023 16:25:52 - @@ -780,11 +780,11 @@ orig_rtr_lsa(struct area *area) fatal("orig_rtr_lsa"); /* reserve space for LSA header and LSA Router header */ - if (ibuf_reserve(buf, sizeof(lsa_hdr)) == NULL) - fatal("orig_rtr_lsa: ibuf_reserve failed"); + if (ibuf_add_zero(buf, sizeof(lsa_hdr)) == -1) + fatal("orig_rtr_lsa: ibuf_add_zero failed"); - if (ibuf_reserve(buf, sizeof(lsa_rtr)) == NULL) - fatal("orig_rtr_lsa: ibuf_reserve failed"); + if (ibuf_add_zero(buf, sizeof(lsa_rtr)) == -1) + fatal("orig_rtr_lsa: ibuf_add_zero failed"); /* links */ LIST_FOREACH(iface, &area->iface_list, entry) { @@ -944,8 +944,8 @@ orig_rtr_lsa(struct area *area) LSA_24_SETLO(lsa_rtr.opts, area_ospf_options(area)); LSA_24_SETHI(lsa_rtr.opts, flags); lsa_rtr.opts = htonl(lsa_rtr.opts); - memcpy(ibuf_seek(buf, sizeof(lsa_hdr), sizeof(lsa_rtr)), - &lsa_rtr, sizeof(lsa_rtr)); + if (ibuf_set(buf, sizeof(lsa_hdr), &lsa_rtr, sizeof(lsa_rtr)) == -1) + fatal("orig_rtr_lsa: ibuf_set failed"); /* LSA header */ lsa_hdr.age = htons(DEFAULT_AGE); @@ -956,11 +956,12 @@ orig_rtr_lsa(struct area *area) lsa_hdr.seq_num = htonl(INIT_SEQ_NUM); lsa_hdr.len = htons(buf->wpos); lsa_hdr.ls_chksum = 0; /* updated later */ - memcpy(ibuf_seek(buf, 0, sizeof(lsa_hdr)), &lsa_hdr, sizeof(lsa_hdr)); + if (ibuf_set(buf, 0, &lsa_hdr, sizeof(lsa_hdr)) == -1) + fatal("orig_rtr_lsa: ibuf_set failed"); - chksum = htons(iso_cksum(buf->buf, buf->wpos, LS_CKSUM_OFFSET)); - memcpy(ibuf_seek(buf, LS_CKSUM_OFFSET, sizeo
Re: ospfd use new ibuf functions
On Tue, Jun 20, 2023 at 03:46:23PM +0200, Theo Buehler wrote: > On Tue, Jun 20, 2023 at 02:47:41PM +0200, Claudio Jeker wrote: > > This diff updates ospfd to use the new ibuf API. > > > > It mainly removes the use of ibuf_seek() and replaces these calls with > > ibuf_set(). > > > > Regress still passes with this diff in. > > There's a function vs fatal mismatch in orig_rtr_lsa > > > + if (ibuf_set_n16(buf, LS_CKSUM_OFFSET, chksum) == -1) > > + fatal("orig_rtr_lsa: ibuf_set failed"); > > not sure if that's deliberate. Similarly in orig_net_lsa. It is not deliberate. I will fix them before commit. > ok -- :wq Claudio
Re: ospfd use new ibuf functions
On Tue, Jun 20, 2023 at 02:47:41PM +0200, Claudio Jeker wrote: > This diff updates ospfd to use the new ibuf API. > > It mainly removes the use of ibuf_seek() and replaces these calls with > ibuf_set(). > > Regress still passes with this diff in. There's a function vs fatal mismatch in orig_rtr_lsa > + if (ibuf_set_n16(buf, LS_CKSUM_OFFSET, chksum) == -1) > + fatal("orig_rtr_lsa: ibuf_set failed"); not sure if that's deliberate. Similarly in orig_net_lsa. ok
ospfd use new ibuf functions
This diff updates ospfd to use the new ibuf API. It mainly removes the use of ibuf_seek() and replaces these calls with ibuf_set(). Regress still passes with this diff in. -- :wq Claudio Index: auth.c === RCS file: /cvs/src/usr.sbin/ospfd/auth.c,v retrieving revision 1.20 diff -u -p -r1.20 auth.c --- auth.c 5 May 2015 01:26:37 - 1.20 +++ auth.c 16 Jun 2023 10:25:48 - @@ -141,35 +141,44 @@ auth_gen(struct ibuf *buf, struct iface { MD5_CTX hash; u_int8_t digest[MD5_DIGEST_LENGTH]; - struct ospf_hdr *ospf_hdr; + struct crypt crypt; struct auth_md *md; - - if ((ospf_hdr = ibuf_seek(buf, 0, sizeof(*ospf_hdr))) == NULL) - fatalx("auth_gen: buf_seek failed"); + u_int16_tchksum; /* update length */ if (ibuf_size(buf) > USHRT_MAX) fatalx("auth_gen: resulting ospf packet too big"); - ospf_hdr->len = htons(ibuf_size(buf)); - /* clear auth_key field */ - bzero(ospf_hdr->auth_key.simple, sizeof(ospf_hdr->auth_key.simple)); + if (ibuf_set_n16(buf, offsetof(struct ospf_hdr, len), + ibuf_size(buf)) == -1) + fatalx("auth_gen: ibuf_set_n16 failed"); switch (iface->auth_type) { case AUTH_NONE: - ospf_hdr->chksum = in_cksum(buf->buf, ibuf_size(buf)); + chksum = in_cksum(buf->buf, ibuf_size(buf)); + if (ibuf_set(buf, offsetof(struct ospf_hdr, chksum), + &chksum, sizeof(chksum)) == -1) + fatalx("auth_gen: ibuf_set failed"); break; case AUTH_SIMPLE: - ospf_hdr->chksum = in_cksum(buf->buf, ibuf_size(buf)); + chksum = in_cksum(buf->buf, ibuf_size(buf)); + if (ibuf_set(buf, offsetof(struct ospf_hdr, chksum), + &chksum, sizeof(chksum)) == -1) + fatalx("auth_gen: ibuf_set failed"); - strncpy(ospf_hdr->auth_key.simple, iface->auth_key, - sizeof(ospf_hdr->auth_key.simple)); + if (ibuf_set(buf, offsetof(struct ospf_hdr, auth_key), + iface->auth_key, strlen(iface->auth_key)) == -1) + fatalx("auth_gen: ibuf_set failed"); break; case AUTH_CRYPT: - ospf_hdr->chksum = 0; - ospf_hdr->auth_key.crypt.keyid = iface->auth_keyid; - ospf_hdr->auth_key.crypt.seq_num = htonl(iface->crypt_seq_num); - ospf_hdr->auth_key.crypt.len = MD5_DIGEST_LENGTH; + bzero(&crypt, sizeof(crypt)); + crypt.keyid = iface->auth_keyid; + crypt.seq_num = htonl(iface->crypt_seq_num); + crypt.len = MD5_DIGEST_LENGTH; iface->crypt_seq_num++; + + if (ibuf_set(buf, offsetof(struct ospf_hdr, auth_key), + &crypt, sizeof(crypt)) == -1) + fatalx("auth_gen: ibuf_set failed"); /* insert plaintext key */ if ((md = md_list_find(&iface->auth_md_list, Index: database.c === RCS file: /cvs/src/usr.sbin/ospfd/database.c,v retrieving revision 1.36 diff -u -p -r1.36 database.c --- database.c 8 Mar 2023 04:43:14 - 1.36 +++ database.c 16 Jun 2023 10:26:00 - @@ -53,7 +53,7 @@ send_db_description(struct nbr *nbr) goto fail; /* reserve space for database description header */ - if (ibuf_reserve(buf, sizeof(dd_hdr)) == NULL) + if (ibuf_add_zero(buf, sizeof(dd_hdr)) == -1) goto fail; switch (nbr->state) { @@ -140,8 +140,9 @@ send_db_description(struct nbr *nbr) dd_hdr.bits = bits; dd_hdr.dd_seq_num = htonl(nbr->dd_seq_num); - memcpy(ibuf_seek(buf, sizeof(struct ospf_hdr), sizeof(dd_hdr)), - &dd_hdr, sizeof(dd_hdr)); + if (ibuf_set(buf, sizeof(struct ospf_hdr), &dd_hdr, + sizeof(dd_hdr)) == -1) + goto fail; /* update authentication and calculate checksum */ if (auth_gen(buf, nbr->iface)) Index: lsupdate.c === RCS file: /cvs/src/usr.sbin/ospfd/lsupdate.c,v retrieving revision 1.51 diff -u -p -r1.51 lsupdate.c --- lsupdate.c 8 Mar 2023 04:43:14 - 1.51 +++ lsupdate.c 16 Jun 2023 10:26:17 - @@ -158,7 +158,7 @@ prepare_ls_update(struct iface *iface) goto fail; /* reserve space for number of lsa field */ - if (ibuf_reserve(buf, sizeof(u_int32_t)) == NULL) + if (ibuf_add_zero(buf, sizeof(u_int3
Re: ospfd/ospf6d, interfaces in log messages
Remi Locherer(remi.loche...@relo.ch) on 2021.11.03 22:23:44 +0100: > On Tue, Nov 02, 2021 at 05:27:11PM +, Stuart Henderson wrote: > > I've recently started seeing a number of flaps with ospfd/ospf6d > > with invalid seq nums / "seq num mismatch, bad flags" logged. > > Not quite sure what's going yet as they must be occurring on > > various local switched segments on one nic and also on ethernet > > wan circuits direct to router on a separate pcie nic, anyway > > it's made it clear that very few of the log messages relating > > to neighbours identify which interface is involved. > > > > I don't know if it makes sense to commit or not, but there's a > > diff below adding the interface wherever the neighbour ID is logged > > if anyone's interested (same changes to both ospfd and ospf6d). > > > > > > Nov 2 11:29:30 ospfd[78532]: recv_db_description: neighbor ID > > xx.2: invalid seq num, mine 20d22487 his 20d22485 > > Nov 2 11:29:30 ospf6d[89545]: recv_db_description: neighbor ID > > xx.2: invalid seq num, mine 4cabc5c1 his 4cabc5c0 > > Nov 2 11:29:34 ospf6d[89545]: recv_db_description: neighbor ID > > xx.1: invalid seq num, mine 98360a5 his 98360a4 > > Nov 2 11:29:34 ospfd[78532]: recv_db_description: neighbor ID > > xx.1: invalid seq num, mine f708c646 his f708c645 > > Nov 2 11:29:38 ospfd[78532]: recv_db_description: neighbor ID > > xx.11: invalid seq num, mine e4068bcc his e4068bcb > > Nov 2 11:30:06 ospf6d[89545]: recv_db_description: neighbor ID > > xx.3: seq num mismatch, bad flags > > Nov 2 11:30:14 ospf6d[89545]: recv_db_description: neighbor ID > > xxxxxx.1: invalid seq num, mine 98360ae his 98360ad > > Nov 2 11:30:14 ospfd[78532]: recv_db_description: neighbor ID > > xx.1: invalid seq num, mine f708c64f his f708c64e > > Nov 2 11:30:22 ospfd[78532]: recv_db_description: neighbor ID > > xx.2: invalid seq num, mine 20d22493 his 20d22490 > > Nov 2 11:30:22 ospfd[78532]: recv_db_description: neighbor ID > > xx.2: invalid seq num, mine 20d22493 his 20d22492 > > Nov 2 11:30:39 ospfd[78532]: recv_db_description: neighbor ID > > xx.2: invalid seq num, mine 20d2249c his 20d2249b > > Nov 2 11:30:59 ospf6d[89545]: recv_db_description: neighbor ID > > xx.11: seq num mismatch, bad flags > > Nov 2 11:30:59 ospfd[78532]: recv_db_description: neighbor ID > > xx.11: seq num mismatch, bad flags > > Nov 2 11:31:09 ospfd[78532]: recv_db_description: neighbor ID > > xx.1: invalid seq num, mine f708c65c his f708c65b > > > > I think this addition makes sense. Over which link a neighbor is connected > can only be looked up via ospfctl. It's valuable having this info in the > logs when analysing past events. I thought i had okayed this yesterday already ;) Makes complete sense. > Diff reads fine, applies and compiles. same. > OK remi ok benno > > > > > Index: ospf6d/database.c > > === > > RCS file: /cvs/src/usr.sbin/ospf6d/database.c,v > > retrieving revision 1.20 > > diff -u -p -r1.20 database.c > > --- ospf6d/database.c 15 Jul 2020 14:47:41 - 1.20 > > +++ ospf6d/database.c 2 Nov 2021 17:11:38 - > > @@ -60,9 +60,9 @@ send_db_description(struct nbr *nbr) > > case NBR_STA_INIT: > > case NBR_STA_2_WAY: > > case NBR_STA_SNAP: > > - log_debug("send_db_description: neighbor ID %s: " > > + log_debug("send_db_description: neighbor ID %s (%s): " > > "cannot send packet in state %s", inet_ntoa(nbr->id), > > - nbr_state_name(nbr->state)); > > + nbr->iface->name, nbr_state_name(nbr->state)); > > goto fail; > > case NBR_STA_XSTRT: > > bits |= OSPF_DBD_MS | OSPF_DBD_M | OSPF_DBD_I; > > @@ -160,8 +160,8 @@ recv_db_description(struct nbr *nbr, cha > > int dupe = 0; > > > > if (len < sizeof(dd_hdr)) { > > - log_warnx("recv_db_description: neighbor ID %s: " > > - "bad packet size", inet_ntoa(nbr->id)); > > + log_warnx("recv_db_description: neighbor ID %s (%s): " > > + "bad packet size", inet_ntoa(nbr->id), nbr->iface->name); > > return; > > } > > memcpy(&dd_hdr, buf, sizeof(dd_hdr));
Re: ospfd/ospf6d, interfaces in log messages
On Tue, Nov 02, 2021 at 05:27:11PM +, Stuart Henderson wrote: > I've recently started seeing a number of flaps with ospfd/ospf6d > with invalid seq nums / "seq num mismatch, bad flags" logged. > Not quite sure what's going yet as they must be occurring on > various local switched segments on one nic and also on ethernet > wan circuits direct to router on a separate pcie nic, anyway > it's made it clear that very few of the log messages relating > to neighbours identify which interface is involved. > > I don't know if it makes sense to commit or not, but there's a > diff below adding the interface wherever the neighbour ID is logged > if anyone's interested (same changes to both ospfd and ospf6d). > > > Nov 2 11:29:30 ospfd[78532]: recv_db_description: neighbor ID xx.2: > invalid seq num, mine 20d22487 his 20d22485 > Nov 2 11:29:30 ospf6d[89545]: recv_db_description: neighbor ID > xx.2: invalid seq num, mine 4cabc5c1 his 4cabc5c0 > Nov 2 11:29:34 ospf6d[89545]: recv_db_description: neighbor ID > xx.1: invalid seq num, mine 98360a5 his 98360a4 > Nov 2 11:29:34 ospfd[78532]: recv_db_description: neighbor ID xx.1: > invalid seq num, mine f708c646 his f708c645 > Nov 2 11:29:38 ospfd[78532]: recv_db_description: neighbor ID > xx.11: invalid seq num, mine e4068bcc his e4068bcb > Nov 2 11:30:06 ospf6d[89545]: recv_db_description: neighbor ID > xx.3: seq num mismatch, bad flags > Nov 2 11:30:14 ospf6d[89545]: recv_db_description: neighbor ID > xx.1: invalid seq num, mine 98360ae his 98360ad > Nov 2 11:30:14 ospfd[78532]: recv_db_description: neighbor ID xx.1: > invalid seq num, mine f708c64f his f708c64e > Nov 2 11:30:22 ospfd[78532]: recv_db_description: neighbor ID xx.2: > invalid seq num, mine 20d22493 his 20d22490 > Nov 2 11:30:22 ospfd[78532]: recv_db_description: neighbor ID xx.2: > invalid seq num, mine 20d22493 his 20d22492 > Nov 2 11:30:39 ospfd[78532]: recv_db_description: neighbor ID xx.2: > invalid seq num, mine 20d2249c his 20d2249b > Nov 2 11:30:59 ospf6d[89545]: recv_db_description: neighbor ID > xx.11: seq num mismatch, bad flags > Nov 2 11:30:59 ospfd[78532]: recv_db_description: neighbor ID > xx.11: seq num mismatch, bad flags > Nov 2 11:31:09 ospfd[78532]: recv_db_description: neighbor ID xx.1: > invalid seq num, mine f708c65c his f708c65b > I think this addition makes sense. Over which link a neighbor is connected can only be looked up via ospfctl. It's valuable having this info in the logs when analysing past events. Diff reads fine, applies and compiles. OK remi > > Index: ospf6d/database.c > === > RCS file: /cvs/src/usr.sbin/ospf6d/database.c,v > retrieving revision 1.20 > diff -u -p -r1.20 database.c > --- ospf6d/database.c 15 Jul 2020 14:47:41 - 1.20 > +++ ospf6d/database.c 2 Nov 2021 17:11:38 - > @@ -60,9 +60,9 @@ send_db_description(struct nbr *nbr) > case NBR_STA_INIT: > case NBR_STA_2_WAY: > case NBR_STA_SNAP: > - log_debug("send_db_description: neighbor ID %s: " > + log_debug("send_db_description: neighbor ID %s (%s): " > "cannot send packet in state %s", inet_ntoa(nbr->id), > - nbr_state_name(nbr->state)); > + nbr->iface->name, nbr_state_name(nbr->state)); > goto fail; > case NBR_STA_XSTRT: > bits |= OSPF_DBD_MS | OSPF_DBD_M | OSPF_DBD_I; > @@ -160,8 +160,8 @@ recv_db_description(struct nbr *nbr, cha > int dupe = 0; > > if (len < sizeof(dd_hdr)) { > - log_warnx("recv_db_description: neighbor ID %s: " > - "bad packet size", inet_ntoa(nbr->id)); > + log_warnx("recv_db_description: neighbor ID %s (%s): " > + "bad packet size", inet_ntoa(nbr->id), nbr->iface->name); > return; > } > memcpy(&dd_hdr, buf, sizeof(dd_hdr)); > @@ -170,9 +170,10 @@ recv_db_description(struct nbr *nbr, cha > > /* db description packet sanity checks */ > if (ntohs(dd_hdr.iface_mtu) > nbr->iface->mtu) { > - log_warnx("recv_db_description: neighbor ID %s: " > + log_warnx("recv_db_description: neighbor ID %s (%s): " > "invalid MTU %d expected %d", inet_ntoa(nbr->id), > - ntohs(dd_hdr.iface_mtu), nbr->iface->mtu); > +
ospfd/ospf6d, interfaces in log messages
I've recently started seeing a number of flaps with ospfd/ospf6d with invalid seq nums / "seq num mismatch, bad flags" logged. Not quite sure what's going yet as they must be occurring on various local switched segments on one nic and also on ethernet wan circuits direct to router on a separate pcie nic, anyway it's made it clear that very few of the log messages relating to neighbours identify which interface is involved. I don't know if it makes sense to commit or not, but there's a diff below adding the interface wherever the neighbour ID is logged if anyone's interested (same changes to both ospfd and ospf6d). Nov 2 11:29:30 ospfd[78532]: recv_db_description: neighbor ID xx.2: invalid seq num, mine 20d22487 his 20d22485 Nov 2 11:29:30 ospf6d[89545]: recv_db_description: neighbor ID xx.2: invalid seq num, mine 4cabc5c1 his 4cabc5c0 Nov 2 11:29:34 ospf6d[89545]: recv_db_description: neighbor ID xx.1: invalid seq num, mine 98360a5 his 98360a4 Nov 2 11:29:34 ospfd[78532]: recv_db_description: neighbor ID xx.1: invalid seq num, mine f708c646 his f708c645 Nov 2 11:29:38 ospfd[78532]: recv_db_description: neighbor ID xx.11: invalid seq num, mine e4068bcc his e4068bcb Nov 2 11:30:06 ospf6d[89545]: recv_db_description: neighbor ID xx.3: seq num mismatch, bad flags Nov 2 11:30:14 ospf6d[89545]: recv_db_description: neighbor ID xx.1: invalid seq num, mine 98360ae his 98360ad Nov 2 11:30:14 ospfd[78532]: recv_db_description: neighbor ID xx.1: invalid seq num, mine f708c64f his f708c64e Nov 2 11:30:22 ospfd[78532]: recv_db_description: neighbor ID xx.2: invalid seq num, mine 20d22493 his 20d22490 Nov 2 11:30:22 ospfd[78532]: recv_db_description: neighbor ID xx.2: invalid seq num, mine 20d22493 his 20d22492 Nov 2 11:30:39 ospfd[78532]: recv_db_description: neighbor ID xx.2: invalid seq num, mine 20d2249c his 20d2249b Nov 2 11:30:59 ospf6d[89545]: recv_db_description: neighbor ID xx.11: seq num mismatch, bad flags Nov 2 11:30:59 ospfd[78532]: recv_db_description: neighbor ID xx.11: seq num mismatch, bad flags Nov 2 11:31:09 ospfd[78532]: recv_db_description: neighbor ID xx.1: invalid seq num, mine f708c65c his f708c65b Index: ospf6d/database.c === RCS file: /cvs/src/usr.sbin/ospf6d/database.c,v retrieving revision 1.20 diff -u -p -r1.20 database.c --- ospf6d/database.c 15 Jul 2020 14:47:41 - 1.20 +++ ospf6d/database.c 2 Nov 2021 17:11:38 - @@ -60,9 +60,9 @@ send_db_description(struct nbr *nbr) case NBR_STA_INIT: case NBR_STA_2_WAY: case NBR_STA_SNAP: - log_debug("send_db_description: neighbor ID %s: " + log_debug("send_db_description: neighbor ID %s (%s): " "cannot send packet in state %s", inet_ntoa(nbr->id), - nbr_state_name(nbr->state)); + nbr->iface->name, nbr_state_name(nbr->state)); goto fail; case NBR_STA_XSTRT: bits |= OSPF_DBD_MS | OSPF_DBD_M | OSPF_DBD_I; @@ -160,8 +160,8 @@ recv_db_description(struct nbr *nbr, cha int dupe = 0; if (len < sizeof(dd_hdr)) { - log_warnx("recv_db_description: neighbor ID %s: " - "bad packet size", inet_ntoa(nbr->id)); + log_warnx("recv_db_description: neighbor ID %s (%s): " + "bad packet size", inet_ntoa(nbr->id), nbr->iface->name); return; } memcpy(&dd_hdr, buf, sizeof(dd_hdr)); @@ -170,9 +170,10 @@ recv_db_description(struct nbr *nbr, cha /* db description packet sanity checks */ if (ntohs(dd_hdr.iface_mtu) > nbr->iface->mtu) { - log_warnx("recv_db_description: neighbor ID %s: " + log_warnx("recv_db_description: neighbor ID %s (%s): " "invalid MTU %d expected %d", inet_ntoa(nbr->id), - ntohs(dd_hdr.iface_mtu), nbr->iface->mtu); + nbr->iface->name, ntohs(dd_hdr.iface_mtu), + nbr->iface->mtu); return; } @@ -180,8 +181,9 @@ recv_db_description(struct nbr *nbr, cha nbr->last_rx_bits == dd_hdr.bits && ntohl(dd_hdr.dd_seq_num) == nbr->dd_seq_num - nbr->dd_master ? 1 : 0) { - log_debug("recv_db_description: dupe from neighbor ID %s", - inet_ntoa(nbr->id)); + log_debug("recv_db_description: dupe from " + "neighbor ID %s (%s)", inet_ntoa(nbr->id), + nbr->
Re: ospfd seq out of order in ls_upd floods
On 05/06/2021 21:31, Stuart Henderson wrote: Sometimes I see authentication errors from ospfd, mainly (though possibly not entirely always) on a 30 minute cycle, e.g. these log entries 2021-06-03T05:30:04.952Z ospfd[31748]: spf_calc: area 0.0.0.0 calculated 2021-06-03T05:51:43.785Z ospfd[76044]: auth_validate: decreasing seq num, interface vlan760 2021-06-03T05:51:43.785Z ospfd[76044]: recv_packet: authentication error, interface vlan760 2021-06-03T05:56:03.248Z ospfd[76044]: auth_validate: decreasing seq num, interface vlan760 2021-06-03T05:56:03.248Z ospfd[76044]: recv_packet: authentication error, interface vlan760 2021-06-03T05:59:58.978Z ospfd[31748]: spf_calc: area 0.0.0.0 calculated snip... Has anyone else noticed something like this, or have any suspicions about code in this area that might be problematic? snip... Don't know if it's relevant, but I almost always see authentication errors upon starting/restarting the daemon. One log entry though and not continuous. G
ospfd seq out of order in ls_upd floods
Sometimes I see authentication errors from ospfd, mainly (though possibly not entirely always) on a 30 minute cycle, e.g. these log entries 2021-06-03T05:30:04.952Z ospfd[31748]: spf_calc: area 0.0.0.0 calculated 2021-06-03T05:51:43.785Z ospfd[76044]: auth_validate: decreasing seq num, interface vlan760 2021-06-03T05:51:43.785Z ospfd[76044]: recv_packet: authentication error, interface vlan760 2021-06-03T05:56:03.248Z ospfd[76044]: auth_validate: decreasing seq num, interface vlan760 2021-06-03T05:56:03.248Z ospfd[76044]: recv_packet: authentication error, interface vlan760 2021-06-03T05:59:58.978Z ospfd[31748]: spf_calc: area 0.0.0.0 calculated 2021-06-03T08:21:43.851Z ospfd[76044]: auth_validate: decreasing seq num, interface vlan760 2021-06-03T08:21:43.851Z ospfd[76044]: recv_packet: authentication error, interface vlan760 2021-06-03T08:26:03.434Z ospfd[76044]: auth_validate: decreasing seq num, interface vlan760 2021-06-03T08:26:03.434Z ospfd[76044]: recv_packet: authentication error, interface vlan760 2021-06-03T08:29:59.087Z ospfd[31748]: spf_calc: area 0.0.0.0 calculated 2021-06-03T08:30:04.097Z ospfd[31748]: spf_calc: area 0.0.0.0 calculated 2021-06-03T08:51:43.858Z ospfd[76044]: auth_validate: decreasing seq num, interface vlan760 2021-06-03T08:51:43.858Z ospfd[76044]: recv_packet: authentication error, interface vlan760 2021-06-03T08:56:03.470Z ospfd[76044]: auth_validate: decreasing seq num, interface vlan760 2021-06-03T08:56:03.470Z ospfd[76044]: recv_packet: authentication error, interface vlan760 2021-06-03T09:00:01.033Z ospfd[31748]: spf_calc: area 0.0.0.0 calculated 2021-06-03T09:00:06.043Z ospfd[31748]: spf_calc: area 0.0.0.0 calculated 2021-06-03T09:21:43.876Z ospfd[76044]: auth_validate: decreasing seq num, interface vlan760 2021-06-03T09:21:43.876Z ospfd[76044]: recv_packet: authentication error, interface vlan760 2021-06-03T09:26:03.518Z ospfd[76044]: auth_validate: decreasing seq num, interface vlan760 2021-06-03T09:26:03.518Z ospfd[76044]: recv_packet: authentication error, interface vlan760 2021-06-03T09:30:00.060Z ospfd[31748]: spf_calc: area 0.0.0.0 calculated 2021-06-03T09:30:05.070Z ospfd[31748]: spf_calc: area 0.0.0.0 calculated 2021-06-03T09:51:43.893Z ospfd[76044]: auth_validate: decreasing seq num, interface vlan760 2021-06-03T09:51:43.893Z ospfd[76044]: recv_packet: authentication error, interface vlan760 2021-06-03T09:56:03.555Z ospfd[76044]: auth_validate: decreasing seq num, interface vlan760 2021-06-03T09:56:03.555Z ospfd[76044]: recv_packet: authentication error, interface vlan760 2021-06-03T09:59:59.086Z ospfd[31748]: spf_calc: area 0.0.0.0 calculated 2021-06-03T11:00:05.088Z ospfd[31748]: spf_calc: area 0.0.0.0 calculated 2021-06-03T11:21:43.932Z ospfd[76044]: auth_validate: decreasing seq num, interface vlan760 2021-06-03T11:21:43.932Z ospfd[76044]: recv_packet: authentication error, interface vlan760 2021-06-03T11:26:03.675Z ospfd[76044]: auth_validate: decreasing seq num, interface vlan760 2021-06-03T11:26:03.675Z ospfd[76044]: recv_packet: authentication error, interface vlan760 The cyclical nature suggests it's in the ls_upd floods and looking at packets confirms that, note the ls_upd where 8715337 and 8715336 are sent out of order: 11:26:02.246716 xxx.xx.xxx.28 > 224.0.0.5: OSPFv2-hello 52: rtrid yyy.yy.yyy.13 backbone auth MD5 key-id 1 seq 8715334 off 0 len 16 E mask 255.255.255.248 int 1 pri 1 dead 4 dr xxx.xx.xxx.28 bdr xxx.xx.xxx.25 nbrs yyy.yy.yyy.11 yyy.yy.yyy.8 [tos 0xc0] [ttl 1] (id 38681, len 88) 11:26:03.249345 xxx.xx.xxx.28 > 224.0.0.5: OSPFv2-hello 52: rtrid yyy.yy.yyy.13 backbone auth MD5 key-id 1 seq 8715335 off 0 len 16 E mask 255.255.255.248 int 1 pri 1 dead 4 dr xxx.xx.xxx.28 bdr xxx.xx.xxx.25 nbrs yyy.yy.yyy.11 yyy.yy.yyy.8 [tos 0xc0] [ttl 1] (id 38060, len 88) 11:26:03.584768 xxx.xx.xxx.28 > 224.0.0.5: OSPFv2-ls_upd 628: rtrid yyy.yy.yyy.13 backbone auth MD5 key-id 1 seq 8715337 off 0 len 16 { S 864D age 9 ase 10.2.62.0 asbr yyy.yy.yyy.9 mask 255.255.255.0 type 1 tos 0 metric 100 } { S 864D age 9 ase 10.2.66.0 asbr yyy.yy.yyy.9 mask 255.255.255.0 type 1 tos 0 metric 100 } { E S 80013EB3 age 9 rtr yyy.yy.yyy.9 E { net 185.91.102.64 mask 255.255.255.248 tos 0 metric 10 } { net 10.88.15.0 mask 255.255.255.224 tos 0 metric 10 } { net xxx.xx.xxx.64 mask 255.255.255.255 tos 0 metric 10 } { net yyy.yy.yyy.72 mask 255.255.255.248 tos 0 metric 10 } { net zzz.zzz.44.0 mask 255.255.255.192 tos 0 metric 10 } { net zzz.zzz.43.96 mask 255.255.255.240 tos 0 metric 10 } { net zzz.zzz.43.64 mask 255.255.255.224 tos 0 metric 10 } { net zzz.zzz.43.128 mask 255.255.255.240 tos 0 metric 10 } { net zzz.zzz.43.32 mask 255.255.255.224 tos 0 metric 10 } { net zzz.zzz.41.176 mask 255.255.255.248 tos 0 metric 10 } { net zzz.zzz.41.160 mask 255.255.255.240 tos 0 metric 10 } { net zzz.zzz.41.144 mask 255.255.255.240 tos 0 metric 10 } { net zzz.zzz.41.32 mask 255.255
ospfd -fno-common fixes
This is my try at cleaning up commons in ospfd. I made one big combined diff but will probably split up a few things into own commits. E.g. the lsupdate.c and lsreq.c ones. I had to cleanup the control.c code a bit since this was a bit of a mess. While in bgpd I was able to remove the global bgpd_process variable this is not really possible in ospfd. Too much code depends on it and unrolling it just gets messy. -- :wq Claudio Index: control.c === RCS file: /cvs/src/usr.sbin/ospfd/control.c,v retrieving revision 1.46 diff -u -p -r1.46 control.c --- control.c 16 Sep 2020 20:50:10 - 1.46 +++ control.c 18 Jan 2021 10:11:05 - @@ -32,12 +32,20 @@ #include "log.h" #include "control.h" +TAILQ_HEAD(ctl_conns, ctl_conn)ctl_conns = TAILQ_HEAD_INITIALIZER(ctl_conns); + #defineCONTROL_BACKLOG 5 struct ctl_conn*control_connbyfd(int); struct ctl_conn*control_connbypid(pid_t); voidcontrol_close(int); +struct { + struct eventev; + struct eventevt; + int fd; +} control_state; + int control_check(char *path) { @@ -108,8 +116,9 @@ control_init(char *path) } int -control_listen(void) +control_listen(int fd) { + control_state.fd = fd; if (listen(control_state.fd, CONTROL_BACKLOG) == -1) { log_warn("control_listen: listen"); Index: control.h === RCS file: /cvs/src/usr.sbin/ospfd/control.h,v retrieving revision 1.8 diff -u -p -r1.8 control.h --- control.h 16 Sep 2020 20:50:10 - 1.8 +++ control.h 18 Jan 2021 10:11:18 - @@ -23,12 +23,6 @@ #include #include -struct { - struct eventev; - struct eventevt; - int fd; -} control_state; - struct ctl_conn { TAILQ_ENTRY(ctl_conn) entry; struct imsgev iev; @@ -36,7 +30,7 @@ struct ctl_conn { intcontrol_check(char *); intcontrol_init(char *); -intcontrol_listen(void); +intcontrol_listen(int); void control_accept(int, short, void *); void control_dispatch_imsg(int, short, void *); intcontrol_imsg_relay(struct imsg *); Index: lsreq.c === RCS file: /cvs/src/usr.sbin/ospfd/lsreq.c,v retrieving revision 1.21 diff -u -p -r1.21 lsreq.c --- lsreq.c 15 Jul 2019 18:26:39 - 1.21 +++ lsreq.c 18 Jan 2021 10:48:04 - @@ -27,8 +27,6 @@ #include "log.h" #include "ospfe.h" -extern struct imsgev *iev_rde; - /* link state request packet handling */ int send_ls_req(struct nbr *nbr) @@ -107,8 +105,7 @@ recv_ls_req(struct nbr *nbr, char *buf, case NBR_STA_XCHNG: case NBR_STA_LOAD: case NBR_STA_FULL: - imsg_compose_event(iev_rde, IMSG_LS_REQ, nbr->peerid, - 0, -1, buf, len); + ospfe_imsg_compose_rde(IMSG_LS_REQ, nbr->peerid, 0, buf, len); break; default: fatalx("recv_ls_req: unknown neighbor state"); Index: lsupdate.c === RCS file: /cvs/src/usr.sbin/ospfd/lsupdate.c,v retrieving revision 1.48 diff -u -p -r1.48 lsupdate.c --- lsupdate.c 6 May 2020 14:40:54 - 1.48 +++ lsupdate.c 18 Jan 2021 10:48:10 - @@ -32,9 +32,6 @@ #include "ospfe.h" #include "rde.h" -extern struct ospfd_conf *oeconf; -extern struct imsgev *iev_rde; - struct ibuf *prepare_ls_update(struct iface *); intadd_ls_update(struct ibuf *, struct iface *, void *, u_int16_t, u_int16_t); @@ -276,8 +273,8 @@ recv_ls_update(struct nbr *nbr, char *bu "neighbor ID %s", inet_ntoa(nbr->id)); return; } - imsg_compose_event(iev_rde, IMSG_LS_UPD, nbr->peerid, 0, - -1, buf, ntohs(lsa.len)); + ospfe_imsg_compose_rde(IMSG_LS_UPD, nbr->peerid, 0, + buf, ntohs(lsa.len)); buf += ntohs(lsa.len); len -= ntohs(lsa.len); } Index: ospfd.c === RCS file: /cvs/src/usr.sbin/ospfd/ospfd.c,v retrieving revision 1.114 diff -u -p -r1.114 ospfd.c --- ospfd.c 16 Sep 2020 20:50:10 - 1.114 +++ ospfd.c 18 Jan 2021 10:50:40 - @@ -64,9 +64,10 @@ int pipe_parent2ospfe[2]; intpipe_parent2rde[2]; intpipe_ospfe2rde[2]; +enum ospfd_process ospfd_process; struct ospfd_conf *ospfd_conf = NULL; -struct imsgev *iev_ospfe; -struct imsgev *iev_rde; +static struct imsgev *iev_ospfe; +static struct imsge
ospfd: use ROUTE_FLAGFILTER
ospfd is our first target for using ROUTE_FLAGFILTER to reduce pressure on the route socket, so here's the diff we've been running for a couple of weeks now (minus the fix for RTM_DELETE flags, notably). ok? Index: kroute.c === RCS file: /cvs/src/usr.sbin/ospfd/kroute.c,v retrieving revision 1.113 diff -u -p -r1.113 kroute.c --- kroute.c9 Nov 2019 15:54:19 - 1.113 +++ kroute.c27 Jul 2020 03:45:41 - @@ -133,6 +133,7 @@ kr_init(int fs, u_int rdomain, int redis int opt = 0, rcvbuf, default_rcvbuf; socklen_t optlen; int filter_prio = fib_prio; + int filter_flags = RTF_LLINFO | RTF_BROADCAST; kr_state.fib_sync = fs; kr_state.rdomain = rdomain; @@ -158,6 +159,11 @@ kr_init(int fs, u_int rdomain, int redis if (setsockopt(kr_state.fd, AF_ROUTE, ROUTE_PRIOFILTER, &filter_prio, sizeof(filter_prio)) == -1) { log_warn("%s: setsockopt AF_ROUTE ROUTE_PRIOFILTER", __func__); + /* not fatal */ + } + if (setsockopt(kr_state.fd, AF_ROUTE, ROUTE_FLAGFILTER, &filter_flags, + sizeof(filter_flags)) == -1) { + log_warn("%s: setsockopt AF_ROUTE ROUTE_FLAGFILTER", __func__); /* not fatal */ }
Re: Prometheus core metrics for bgpd and ospfd approach ideas
Thanks for the comments, some good things to think about. Out of interest, can anyone think of some good examples of daemons which call ctl commands, just wanting to review the patterns and approach, and what is the best, best practice examples today. On Mon, 18 May 2020, 16:46 Theo de Raadt, wrote: > Claudio Jeker wrote: > > > Last note, please do not try to directly talk to the daemons always pass > > via the *ctl program. The API used between for example bgpd and bgpctl > > is not public and also not stable. It requires that both tools are in > > sync. > > There is an additional reason for doing this. Having to do fork+exec is > a form of privsep, especially if pledge/unveil are used carefully. >
Re: Prometheus core metrics for bgpd and ospfd approach ideas
Claudio Jeker wrote: > Last note, please do not try to directly talk to the daemons always pass > via the *ctl program. The API used between for example bgpd and bgpctl > is not public and also not stable. It requires that both tools are in > sync. There is an additional reason for doing this. Having to do fork+exec is a form of privsep, especially if pledge/unveil are used carefully.
Re: Prometheus core metrics for bgpd and ospfd approach ideas
On Mon, May 18, 2020 at 04:16:05PM +0100, Stuart Henderson wrote: > On 2020/05/18 15:31, Richard Chivers wrote: > > Hi, > > > > We could do with exposing certain metrics from bgpd, ospfd and pf. > > > > I was considering a couple of approaches and really was just > > interested in what would make most sense in general. > > > > Has anyone else considered this at all? > > > > Would this be useful to anyone else? > > It would be useful. > > I think the method which fits best with the rest of the system would > be to export these via agentx -> snmpd, where possible using standard > MIBs e.g. for BGP that would be BGP4-MIB/CISCO-BGP4-MIB. > > IIRC it has been looked at a bit, but I'm not sure of the current state > of any of it. > > Data can be picked up from SNMP directly by some existing tools, > for Prometheus it could be sent via snmp_exporter. I'm totally torn by this. agentx support sounds great but then you realize how complex and cumbersome it is to implement those mibs inside the daemons (even if abstracted away there are various snmp specific getters to be implemented which looks like a lot of code). bgpctl show terse and show neighbor terse was created to do simple monitoring scripts. Also part of the reason for JSON support was to provide a machine readable output for monitoring systems. I would not mind to actually add special commands similar to terse for this. Prometheus is a bit of a special snow-flake since it formats the metric pages in its own special format. So you would need to write a translation wrapper. Last note, please do not try to directly talk to the daemons always pass via the *ctl program. The API used between for example bgpd and bgpctl is not public and also not stable. It requires that both tools are in sync. -- :wq Claudio
Re: Prometheus core metrics for bgpd and ospfd approach ideas
On 2020/05/18 15:31, Richard Chivers wrote: > Hi, > > We could do with exposing certain metrics from bgpd, ospfd and pf. > > I was considering a couple of approaches and really was just > interested in what would make most sense in general. > > Has anyone else considered this at all? > > Would this be useful to anyone else? It would be useful. I think the method which fits best with the rest of the system would be to export these via agentx -> snmpd, where possible using standard MIBs e.g. for BGP that would be BGP4-MIB/CISCO-BGP4-MIB. IIRC it has been looked at a bit, but I'm not sure of the current state of any of it. Data can be picked up from SNMP directly by some existing tools, for Prometheus it could be sent via snmp_exporter.
Prometheus core metrics for bgpd and ospfd approach ideas
Hi, We could do with exposing certain metrics from bgpd, ospfd and pf. I was considering a couple of approaches and really was just interested in what would make most sense in general. Has anyone else considered this at all? Would this be useful to anyone else? Also just to say I am not necessarily suggesting this being in the core OpenBSd project, but happy to do that too potentially if it makes sense, just looking for some design ideas and experience, I am sure with each of the approaches suggested their may be things I just can't know at this stage, with my relative inexperience Option A) Create a daemon called ospf_metrics or similar which talks directly to ospfd and fetches and sames metrics to a txt file or similar (which the prometheus exporter i believe supports as standard) This could keep some data in memory such as rib counts or similar so it only occasionally has to query full lists etc. Option B) Create a daemon called ospf_metrics or similar which talks directly to ospfctl and fetches and saves metrics to a txt file or similar (which the prometheus exporter i believe supports as standard) This could keep some data in memory such as rib counts or similar so it only occasionally has to query full lists etc. Option C) Extend ospfctl so that it can output metrics a new flag, this could then be added to a cron or similar. I could use the new output capabilities in bgpctl and ospfctl to introduce a prometheus output (just worried this is too flavor of the month for a long term inclusion in ospfctl/bgpctl), and potentially the current cli options don't really necessarily match one to one with metrics you would want to emit. Option D) One of the above options but a new daemon called metrics or similar which provides metrics for various parts of openbsd. (similar I guess to snmpd) Option E) Extend the node exporter to do this natively in some way, haven't looked too hard at that yet, guessing could be a golang exe which talks to ospfctl or similar. Option F) A suggestion someone else has which is probably better and simpler... :) Cheers Richard
that ospfd fix for ospf6d
Can someone give this diff for ospf6d a try? This fixes the same issue that I just committed for ospfd: revision 1.48 date: 2020/05/06 14:40:54; author: claudio; state: Exp; lines: +5 -5; commitid: 1nh8JCAv0Kmqd1jV; Do not use the pointer returned by ibuf_reserve() after calling another ibuf function. After the call the internal buffer may have moved by realloc() and so the pointer is invalid. Instead use ibuf_size() to get the current offset in the buffer and use ibuf_seek() later on to write back the updated lsa age into the buffer at the right spot. This fixes an issue seen by Richard Chivers on routers with many passive interfaces. OK stsp@ deraadt@ Thanks -- :wq Claudio Index: lsupdate.c === RCS file: /cvs/src/usr.sbin/ospf6d/lsupdate.c,v retrieving revision 1.16 diff -u -p -r1.16 lsupdate.c --- lsupdate.c 4 May 2020 14:36:51 - 1.16 +++ lsupdate.c 6 May 2020 14:33:40 - @@ -194,13 +194,13 @@ int add_ls_update(struct ibuf *buf, struct iface *iface, void *data, u_int16_t len, u_int16_t older) { - void*lsage; - u_int16_tage; + size_t ageoff; + u_int16_t age; if (buf->wpos + len >= buf->max) return (0); - lsage = ibuf_reserve(buf, 0); + ageoff = ibuf_size(buf); if (ibuf_add(buf, data, len)) { log_warn("add_ls_update"); return (0); @@ -212,7 +212,7 @@ add_ls_update(struct ibuf *buf, struct i if ((age += older + iface->transmit_delay) >= MAX_AGE) age = MAX_AGE; age = htons(age); - memcpy(lsage, &age, sizeof(age)); + memcpy(ibuf_seek(buf, ageoff, sizeof(age)), &age, sizeof(age)); return (1); }
Re: ospf6d: bring ospf6d closer to ospfd
On Sat, Mar 28, 2020 at 05:00:11PM +0100, Remi Locherer wrote: > On Sat, Mar 21, 2020 at 05:25:45PM +0100, Denis Fondras wrote: > > Biggest chunk is rework of rde_asext_get()/rde_asext_put(). > > Also change get_net_link() and get_rtr_link() to work like ospfd couterpart. > > Reads good to me and I didn't spot any issues running tests with it. > Thank you Remi. > One question: why "if 0" the "Dump SPF tree to log"? > Doh! It is not (yet) time to '#if 0' this part. This is from an unpublished diff that changes how if_find() works. Thus printing the SPF tree needs to be rewritten. > > > > Index: rde.c > > === > > RCS file: /cvs/src/usr.sbin/ospf6d/rde.c,v > > retrieving revision 1.84 > > diff -u -p -r1.84 rde.c > > --- rde.c 17 Feb 2020 08:12:22 - 1.84 > > +++ rde.c 21 Mar 2020 16:04:47 - > > @@ -59,8 +59,9 @@ intrde_req_list_exists(struct rde_nbr > > voidrde_req_list_del(struct rde_nbr *, struct lsa_hdr *); > > voidrde_req_list_free(struct rde_nbr *); > > > > -struct lsa *rde_asext_get(struct kroute *); > > -struct lsa *rde_asext_put(struct kroute *); > > +struct iface *rde_asext_lookup(struct in6_addr, int); > > +voidrde_asext_get(struct kroute *); > > +voidrde_asext_put(struct kroute *); > > > > int comp_asext(struct lsa *, struct lsa *); > > struct lsa *orig_asext_lsa(struct kroute *, u_int16_t); > > @@ -217,6 +218,7 @@ __dead void > > rde_shutdown(void) > > { > > struct area *a; > > + struct vertex *v, *nv; > > > > /* close pipes */ > > msgbuf_clear(&iev_ospfe->ibuf.w); > > @@ -232,6 +234,10 @@ rde_shutdown(void) > > LIST_REMOVE(a, entry); > > area_del(a); > > } > > + for (v = RB_MIN(lsa_tree, &asext_tree); v != NULL; v = nv) { > > + nv = RB_NEXT(lsa_tree, &asext_tree, v); > > + vertex_free(v); > > + } > > rde_nbr_free(); > > > > free(iev_ospfe); > > @@ -643,8 +649,6 @@ rde_dispatch_parent(int fd, short event, > > struct kroutekr; > > struct imsgev *iev = bula; > > struct imsgbuf *ibuf = &iev->ibuf; > > - struct lsa *lsa; > > - struct vertex *v; > > ssize_t n; > > int shut = 0, link_ok, prev_link_ok, orig_lsa; > > unsigned int ifindex; > > @@ -676,13 +680,7 @@ rde_dispatch_parent(int fd, short event, > > break; > > } > > memcpy(&kr, imsg.data, sizeof(kr)); > > - > > - if ((lsa = rde_asext_get(&kr)) != NULL) { > > - v = lsa_find(NULL, lsa->hdr.type, > > - lsa->hdr.ls_id, lsa->hdr.adv_rtr); > > - > > - lsa_merge(nbrself, lsa, v); > > - } > > + rde_asext_get(&kr); > > break; > > case IMSG_NETWORK_DEL: > > if (imsg.hdr.len != IMSG_HEADER_SIZE + sizeof(kr)) { > > @@ -691,20 +689,7 @@ rde_dispatch_parent(int fd, short event, > > break; > > } > > memcpy(&kr, imsg.data, sizeof(kr)); > > - > > - if ((lsa = rde_asext_put(&kr)) != NULL) { > > - v = lsa_find(NULL, lsa->hdr.type, > > - lsa->hdr.ls_id, lsa->hdr.adv_rtr); > > - > > - /* > > -* if v == NULL no LSA is in the table and > > -* nothing has to be done. > > -*/ > > - if (v) > > - lsa_merge(nbrself, lsa, v); > > - else > > - free(lsa); > > - } > > + rde_asext_put(&kr); > > break; > > case IMSG_IFINFO: > > if (imsg.hdr.len != IMSG_HEADER_SIZE + > > @@ -1202,48 +1187,77 @@ rde_req_list_free(struct rde_nbr *nbr) > > /* > > * as-external LSA handling > > */ > > -struct lsa * > > -r
Re: ospf6d: bring ospf6d closer to ospfd
On Sat, Mar 21, 2020 at 05:25:45PM +0100, Denis Fondras wrote: > Biggest chunk is rework of rde_asext_get()/rde_asext_put(). > Also change get_net_link() and get_rtr_link() to work like ospfd couterpart. Reads good to me and I didn't spot any issues running tests with it. One question: why "if 0" the "Dump SPF tree to log"? > > Index: rde.c > === > RCS file: /cvs/src/usr.sbin/ospf6d/rde.c,v > retrieving revision 1.84 > diff -u -p -r1.84 rde.c > --- rde.c 17 Feb 2020 08:12:22 - 1.84 > +++ rde.c 21 Mar 2020 16:04:47 - > @@ -59,8 +59,9 @@ int rde_req_list_exists(struct rde_nbr > void rde_req_list_del(struct rde_nbr *, struct lsa_hdr *); > void rde_req_list_free(struct rde_nbr *); > > -struct lsa *rde_asext_get(struct kroute *); > -struct lsa *rde_asext_put(struct kroute *); > +struct iface *rde_asext_lookup(struct in6_addr, int); > +void rde_asext_get(struct kroute *); > +void rde_asext_put(struct kroute *); > > int comp_asext(struct lsa *, struct lsa *); > struct lsa *orig_asext_lsa(struct kroute *, u_int16_t); > @@ -217,6 +218,7 @@ __dead void > rde_shutdown(void) > { > struct area *a; > + struct vertex *v, *nv; > > /* close pipes */ > msgbuf_clear(&iev_ospfe->ibuf.w); > @@ -232,6 +234,10 @@ rde_shutdown(void) > LIST_REMOVE(a, entry); > area_del(a); > } > + for (v = RB_MIN(lsa_tree, &asext_tree); v != NULL; v = nv) { > + nv = RB_NEXT(lsa_tree, &asext_tree, v); > + vertex_free(v); > + } > rde_nbr_free(); > > free(iev_ospfe); > @@ -643,8 +649,6 @@ rde_dispatch_parent(int fd, short event, > struct kroutekr; > struct imsgev *iev = bula; > struct imsgbuf *ibuf = &iev->ibuf; > - struct lsa *lsa; > - struct vertex *v; > ssize_t n; > int shut = 0, link_ok, prev_link_ok, orig_lsa; > unsigned int ifindex; > @@ -676,13 +680,7 @@ rde_dispatch_parent(int fd, short event, > break; > } > memcpy(&kr, imsg.data, sizeof(kr)); > - > - if ((lsa = rde_asext_get(&kr)) != NULL) { > - v = lsa_find(NULL, lsa->hdr.type, > - lsa->hdr.ls_id, lsa->hdr.adv_rtr); > - > - lsa_merge(nbrself, lsa, v); > - } > + rde_asext_get(&kr); > break; > case IMSG_NETWORK_DEL: > if (imsg.hdr.len != IMSG_HEADER_SIZE + sizeof(kr)) { > @@ -691,20 +689,7 @@ rde_dispatch_parent(int fd, short event, > break; > } > memcpy(&kr, imsg.data, sizeof(kr)); > - > - if ((lsa = rde_asext_put(&kr)) != NULL) { > - v = lsa_find(NULL, lsa->hdr.type, > - lsa->hdr.ls_id, lsa->hdr.adv_rtr); > - > - /* > - * if v == NULL no LSA is in the table and > - * nothing has to be done. > - */ > - if (v) > - lsa_merge(nbrself, lsa, v); > - else > - free(lsa); > - } > + rde_asext_put(&kr); > break; > case IMSG_IFINFO: > if (imsg.hdr.len != IMSG_HEADER_SIZE + > @@ -1202,48 +1187,77 @@ rde_req_list_free(struct rde_nbr *nbr) > /* > * as-external LSA handling > */ > -struct lsa * > -rde_asext_get(struct kroute *kr) > +struct iface * > +rde_asext_lookup(struct in6_addr prefix, int plen) > { > + > struct area *area; > struct iface*iface; > struct iface_addr *ia; > - struct in6_addr addr; > - > - LIST_FOREACH(area, &rdeconf->area_list, entry) > - LIST_FOREACH(iface, &area->iface_list, entry) > + struct in6_addr ina, inb; > + > + LIST_FOREACH(area, &rdeconf->area_list, entry) { > + LIST_FOREACH(iface, &area->iface_list, entry) { > TAILQ_FOREACH(
ospf6d: bring ospf6d closer to ospfd
Biggest chunk is rework of rde_asext_get()/rde_asext_put(). Also change get_net_link() and get_rtr_link() to work like ospfd couterpart. Index: rde.c === RCS file: /cvs/src/usr.sbin/ospf6d/rde.c,v retrieving revision 1.84 diff -u -p -r1.84 rde.c --- rde.c 17 Feb 2020 08:12:22 - 1.84 +++ rde.c 21 Mar 2020 16:04:47 - @@ -59,8 +59,9 @@ intrde_req_list_exists(struct rde_nbr voidrde_req_list_del(struct rde_nbr *, struct lsa_hdr *); voidrde_req_list_free(struct rde_nbr *); -struct lsa *rde_asext_get(struct kroute *); -struct lsa *rde_asext_put(struct kroute *); +struct iface *rde_asext_lookup(struct in6_addr, int); +voidrde_asext_get(struct kroute *); +voidrde_asext_put(struct kroute *); int comp_asext(struct lsa *, struct lsa *); struct lsa *orig_asext_lsa(struct kroute *, u_int16_t); @@ -217,6 +218,7 @@ __dead void rde_shutdown(void) { struct area *a; + struct vertex *v, *nv; /* close pipes */ msgbuf_clear(&iev_ospfe->ibuf.w); @@ -232,6 +234,10 @@ rde_shutdown(void) LIST_REMOVE(a, entry); area_del(a); } + for (v = RB_MIN(lsa_tree, &asext_tree); v != NULL; v = nv) { + nv = RB_NEXT(lsa_tree, &asext_tree, v); + vertex_free(v); + } rde_nbr_free(); free(iev_ospfe); @@ -643,8 +649,6 @@ rde_dispatch_parent(int fd, short event, struct kroutekr; struct imsgev *iev = bula; struct imsgbuf *ibuf = &iev->ibuf; - struct lsa *lsa; - struct vertex *v; ssize_t n; int shut = 0, link_ok, prev_link_ok, orig_lsa; unsigned int ifindex; @@ -676,13 +680,7 @@ rde_dispatch_parent(int fd, short event, break; } memcpy(&kr, imsg.data, sizeof(kr)); - - if ((lsa = rde_asext_get(&kr)) != NULL) { - v = lsa_find(NULL, lsa->hdr.type, - lsa->hdr.ls_id, lsa->hdr.adv_rtr); - - lsa_merge(nbrself, lsa, v); - } + rde_asext_get(&kr); break; case IMSG_NETWORK_DEL: if (imsg.hdr.len != IMSG_HEADER_SIZE + sizeof(kr)) { @@ -691,20 +689,7 @@ rde_dispatch_parent(int fd, short event, break; } memcpy(&kr, imsg.data, sizeof(kr)); - - if ((lsa = rde_asext_put(&kr)) != NULL) { - v = lsa_find(NULL, lsa->hdr.type, - lsa->hdr.ls_id, lsa->hdr.adv_rtr); - - /* -* if v == NULL no LSA is in the table and -* nothing has to be done. -*/ - if (v) - lsa_merge(nbrself, lsa, v); - else - free(lsa); - } + rde_asext_put(&kr); break; case IMSG_IFINFO: if (imsg.hdr.len != IMSG_HEADER_SIZE + @@ -1202,48 +1187,77 @@ rde_req_list_free(struct rde_nbr *nbr) /* * as-external LSA handling */ -struct lsa * -rde_asext_get(struct kroute *kr) +struct iface * +rde_asext_lookup(struct in6_addr prefix, int plen) { + struct area *area; struct iface*iface; struct iface_addr *ia; - struct in6_addr addr; - - LIST_FOREACH(area, &rdeconf->area_list, entry) - LIST_FOREACH(iface, &area->iface_list, entry) + struct in6_addr ina, inb; + + LIST_FOREACH(area, &rdeconf->area_list, entry) { + LIST_FOREACH(iface, &area->iface_list, entry) { TAILQ_FOREACH(ia, &iface->ifa_list, entry) { if (IN6_IS_ADDR_LINKLOCAL(&ia->addr)) continue; - inet6applymask(&addr, &ia->addr, - kr->prefixlen); - if (!memcmp(&addr, &kr->prefix, - sizeof(addr)) && kr->prefixlen == - ia->prefixlen) { - /* already announced as Prefix LS
Re: ospf6d: sync hello.c with ospfd
On Thu, Jan 02, 2020 at 05:17:02PM +0100, Denis Fondras wrote: > Sync with ospfd's hello.c ok remi@ > > Index: hello.c > === > RCS file: /cvs/src/usr.sbin/ospf6d/hello.c,v > retrieving revision 1.21 > diff -u -p -r1.21 hello.c > --- hello.c 23 Dec 2019 11:25:41 - 1.21 > +++ hello.c 2 Jan 2020 16:11:19 - > @@ -41,8 +41,6 @@ send_hello(struct iface *iface) > struct hello_hdr hello; > struct nbr *nbr; > struct ibuf *buf; > - int ret; > - u_int32_topts; > > switch (iface->type) { > case IF_TYPE_POINTOPOINT: > @@ -72,10 +70,8 @@ send_hello(struct iface *iface) > /* hello header */ > hello.iface_id = htonl(iface->ifindex); > LSA_24_SETHI(hello.opts, iface->priority); > - opts = area_ospf_options(iface->area); > - LSA_24_SETLO(hello.opts, opts); > + LSA_24_SETLO(hello.opts, area_ospf_options(iface->area)); > hello.opts = htonl(hello.opts); > - > hello.hello_interval = htons(iface->hello_interval); > hello.rtr_dead_interval = htons(iface->dead_interval); > > @@ -104,10 +100,11 @@ send_hello(struct iface *iface) > if (upd_ospf_hdr(buf, iface)) > goto fail; > > - ret = send_packet(iface, buf, &dst); > + if (send_packet(iface, buf, &dst) == -1) > + goto fail; > > ibuf_free(buf); > - return (ret); > + return (0); > fail: > log_warn("send_hello"); > ibuf_free(buf); > @@ -120,7 +117,6 @@ recv_hello(struct iface *iface, struct i > { > struct hello_hdr hello; > struct nbr *nbr = NULL, *dr; > - struct area *area; > u_int32_tnbr_id, opts; > int nbr_change = 0; > > @@ -148,12 +144,9 @@ recv_hello(struct iface *iface, struct i > return; > } > > - if ((area = iface->area) == NULL) > - fatalx("interface lost area"); > - > opts = LSA_24_GETLO(ntohl(hello.opts)); > - if ((opts & OSPF_OPTION_E && area->stub) || > - ((opts & OSPF_OPTION_E) == 0 && !area->stub)) { > + if ((opts & OSPF_OPTION_E && iface->area->stub) || > + ((opts & OSPF_OPTION_E) == 0 && !iface->area->stub)) { > log_warnx("recv_hello: ExternalRoutingCapability mismatch, " > "interface %s", iface->name); > return; > @@ -161,8 +154,15 @@ recv_hello(struct iface *iface, struct i > > /* match router-id */ > LIST_FOREACH(nbr, &iface->nbr_list, entry) { > - if (nbr == iface->self) > + if (nbr == iface->self) { > + if (nbr->id.s_addr == rtr_id) { > + log_warnx("recv_hello: Router-ID collision on " > + "interface %s neighbor IP %s", iface->name, > + log_in6addr(src)); > + return; > + } > continue; > + } > if (nbr->id.s_addr == rtr_id) > break; > } >
Re: ospf6d: sync database.c with ospfd(8)
On Thu, Jan 02, 2020 at 04:05:45PM +0100, Denis Fondras wrote: > This is mostly log messages sync. ok remi@ > > Index: database.c > === > RCS file: /cvs/src/usr.sbin/ospf6d/database.c,v > retrieving revision 1.18 > diff -u -p -r1.18 database.c > --- database.c23 Dec 2019 07:33:49 - 1.18 > +++ database.c2 Jan 2020 14:31:46 - > @@ -43,7 +43,6 @@ send_db_description(struct nbr *nbr) > struct db_dscrp_hdr dd_hdr; > struct lsa_entry*le, *nle; > struct ibuf *buf; > - int ret = 0; > u_int8_t bits = 0; > > if ((buf = ibuf_open(nbr->iface->mtu - sizeof(struct ip6_hdr))) == NULL) > @@ -63,11 +62,10 @@ send_db_description(struct nbr *nbr) > case NBR_STA_INIT: > case NBR_STA_2_WAY: > case NBR_STA_SNAP: > - log_debug("send_db_description: cannot send packet in state %s," > - " neighbor ID %s", nbr_state_name(nbr->state), > - inet_ntoa(nbr->id)); > - ret = -1; > - goto done; > + log_debug("send_db_description: neighbor ID %s: " > + "cannot send packet in state %s", inet_ntoa(nbr->id), > + nbr_state_name(nbr->state)); > + goto fail; > case NBR_STA_XSTRT: > bits |= OSPF_DBD_MS | OSPF_DBD_M | OSPF_DBD_I; > nbr->dd_more = 1; > @@ -90,7 +88,7 @@ send_db_description(struct nbr *nbr) > > /* build LSA list */ > for (le = TAILQ_FIRST(&nbr->db_sum_list); le != NULL && > - buf->wpos + sizeof(struct lsa_hdr) < buf->max; le = nle) { > + ibuf_left(buf) >= sizeof(struct lsa_hdr); le = nle) { > nbr->dd_end = nle = TAILQ_NEXT(le, entry); > if (ibuf_add(buf, le->le_lsa, sizeof(struct lsa_hdr))) > goto fail; > @@ -146,10 +144,11 @@ send_db_description(struct nbr *nbr) > goto fail; > > /* transmit packet */ > - ret = send_packet(nbr->iface, buf, &dst); > -done: > + if (send_packet(nbr->iface, buf, &dst) == -1) > + goto fail; > + > ibuf_free(buf); > - return (ret); > + return (0); > fail: > log_warn("send_db_description"); > ibuf_free(buf); > @@ -163,8 +162,8 @@ recv_db_description(struct nbr *nbr, cha > int dupe = 0; > > if (len < sizeof(dd_hdr)) { > - log_warnx("recv_db_description: " > - "bad packet size, neighbor ID %s", inet_ntoa(nbr->id)); > + log_warnx("recv_db_description: neighbor ID %s: " > + "bad packet size", inet_ntoa(nbr->id)); > return; > } > memcpy(&dd_hdr, buf, sizeof(dd_hdr)); > @@ -173,9 +172,9 @@ recv_db_description(struct nbr *nbr, cha > > /* db description packet sanity checks */ > if (ntohs(dd_hdr.iface_mtu) > nbr->iface->mtu) { > - log_warnx("recv_db_description: invalid MTU %d sent by " > - "neighbor ID %s, expected %d", ntohs(dd_hdr.iface_mtu), > - inet_ntoa(nbr->id), nbr->iface->mtu); > + log_warnx("recv_db_description: neighbor ID %s: " > + "invalid MTU %d expected %d", inet_ntoa(nbr->id), > + ntohs(dd_hdr.iface_mtu), nbr->iface->mtu); > return; > } > > @@ -183,7 +182,7 @@ recv_db_description(struct nbr *nbr, cha > nbr->last_rx_bits == dd_hdr.bits && > ntohl(dd_hdr.dd_seq_num) == nbr->dd_seq_num - nbr->dd_master ? > 1 : 0) { > - log_debug("recv_db_description: dupe from ID %s", > + log_debug("recv_db_description: dupe from neighbor ID %s", > inet_ntoa(nbr->id)); > dupe = 1; > } > @@ -193,9 +192,9 @@ recv_db_description(struct nbr *nbr, cha > case NBR_STA_ATTEMPT: > case NBR_STA_2_WAY: > case NBR_STA_SNAP: > - log_debug("recv_db_description: packet ignored in state %s, " > - "neighbor ID %s", nbr_state_name(nbr->state), > - inet_ntoa(nbr->id)); > + log_debug("recv_db_description: neighbor ID %s: " > + "packet ignored in state %s", inet_ntoa(nbr->id), > + nbr_state_name(nbr->state)); > return; > case NBR_STA_INIT: > /* evaluate dr and bdr after issuing a 2-Way event */ > @@ -224,9 +223,11 @@ recv_db_description(struct nbr *nbr, cha > } else if (!(dd_hdr.bits & (OSPF_DBD_I | OSPF_DBD_MS))) { > /* M only case: we are master */ > if (ntohl(dd_hdr.dd_seq_num) != nbr->dd_seq_num) { > - log_warnx("recv_db_description: invalid " > - "seq num, mine %x his %x", > -
ospf6d: sync hello.c with ospfd
Sync with ospfd's hello.c Index: hello.c === RCS file: /cvs/src/usr.sbin/ospf6d/hello.c,v retrieving revision 1.21 diff -u -p -r1.21 hello.c --- hello.c 23 Dec 2019 11:25:41 - 1.21 +++ hello.c 2 Jan 2020 16:11:19 - @@ -41,8 +41,6 @@ send_hello(struct iface *iface) struct hello_hdr hello; struct nbr *nbr; struct ibuf *buf; - int ret; - u_int32_topts; switch (iface->type) { case IF_TYPE_POINTOPOINT: @@ -72,10 +70,8 @@ send_hello(struct iface *iface) /* hello header */ hello.iface_id = htonl(iface->ifindex); LSA_24_SETHI(hello.opts, iface->priority); - opts = area_ospf_options(iface->area); - LSA_24_SETLO(hello.opts, opts); + LSA_24_SETLO(hello.opts, area_ospf_options(iface->area)); hello.opts = htonl(hello.opts); - hello.hello_interval = htons(iface->hello_interval); hello.rtr_dead_interval = htons(iface->dead_interval); @@ -104,10 +100,11 @@ send_hello(struct iface *iface) if (upd_ospf_hdr(buf, iface)) goto fail; - ret = send_packet(iface, buf, &dst); + if (send_packet(iface, buf, &dst) == -1) + goto fail; ibuf_free(buf); - return (ret); + return (0); fail: log_warn("send_hello"); ibuf_free(buf); @@ -120,7 +117,6 @@ recv_hello(struct iface *iface, struct i { struct hello_hdr hello; struct nbr *nbr = NULL, *dr; - struct area *area; u_int32_tnbr_id, opts; int nbr_change = 0; @@ -148,12 +144,9 @@ recv_hello(struct iface *iface, struct i return; } - if ((area = iface->area) == NULL) - fatalx("interface lost area"); - opts = LSA_24_GETLO(ntohl(hello.opts)); - if ((opts & OSPF_OPTION_E && area->stub) || - ((opts & OSPF_OPTION_E) == 0 && !area->stub)) { + if ((opts & OSPF_OPTION_E && iface->area->stub) || + ((opts & OSPF_OPTION_E) == 0 && !iface->area->stub)) { log_warnx("recv_hello: ExternalRoutingCapability mismatch, " "interface %s", iface->name); return; @@ -161,8 +154,15 @@ recv_hello(struct iface *iface, struct i /* match router-id */ LIST_FOREACH(nbr, &iface->nbr_list, entry) { - if (nbr == iface->self) + if (nbr == iface->self) { + if (nbr->id.s_addr == rtr_id) { + log_warnx("recv_hello: Router-ID collision on " + "interface %s neighbor IP %s", iface->name, + log_in6addr(src)); + return; + } continue; + } if (nbr->id.s_addr == rtr_id) break; }
ospf6d: sync database.c with ospfd(8)
This is mostly log messages sync. Index: database.c === RCS file: /cvs/src/usr.sbin/ospf6d/database.c,v retrieving revision 1.18 diff -u -p -r1.18 database.c --- database.c 23 Dec 2019 07:33:49 - 1.18 +++ database.c 2 Jan 2020 14:31:46 - @@ -43,7 +43,6 @@ send_db_description(struct nbr *nbr) struct db_dscrp_hdr dd_hdr; struct lsa_entry*le, *nle; struct ibuf *buf; - int ret = 0; u_int8_t bits = 0; if ((buf = ibuf_open(nbr->iface->mtu - sizeof(struct ip6_hdr))) == NULL) @@ -63,11 +62,10 @@ send_db_description(struct nbr *nbr) case NBR_STA_INIT: case NBR_STA_2_WAY: case NBR_STA_SNAP: - log_debug("send_db_description: cannot send packet in state %s," - " neighbor ID %s", nbr_state_name(nbr->state), - inet_ntoa(nbr->id)); - ret = -1; - goto done; + log_debug("send_db_description: neighbor ID %s: " + "cannot send packet in state %s", inet_ntoa(nbr->id), + nbr_state_name(nbr->state)); + goto fail; case NBR_STA_XSTRT: bits |= OSPF_DBD_MS | OSPF_DBD_M | OSPF_DBD_I; nbr->dd_more = 1; @@ -90,7 +88,7 @@ send_db_description(struct nbr *nbr) /* build LSA list */ for (le = TAILQ_FIRST(&nbr->db_sum_list); le != NULL && - buf->wpos + sizeof(struct lsa_hdr) < buf->max; le = nle) { + ibuf_left(buf) >= sizeof(struct lsa_hdr); le = nle) { nbr->dd_end = nle = TAILQ_NEXT(le, entry); if (ibuf_add(buf, le->le_lsa, sizeof(struct lsa_hdr))) goto fail; @@ -146,10 +144,11 @@ send_db_description(struct nbr *nbr) goto fail; /* transmit packet */ - ret = send_packet(nbr->iface, buf, &dst); -done: + if (send_packet(nbr->iface, buf, &dst) == -1) + goto fail; + ibuf_free(buf); - return (ret); + return (0); fail: log_warn("send_db_description"); ibuf_free(buf); @@ -163,8 +162,8 @@ recv_db_description(struct nbr *nbr, cha int dupe = 0; if (len < sizeof(dd_hdr)) { - log_warnx("recv_db_description: " - "bad packet size, neighbor ID %s", inet_ntoa(nbr->id)); + log_warnx("recv_db_description: neighbor ID %s: " + "bad packet size", inet_ntoa(nbr->id)); return; } memcpy(&dd_hdr, buf, sizeof(dd_hdr)); @@ -173,9 +172,9 @@ recv_db_description(struct nbr *nbr, cha /* db description packet sanity checks */ if (ntohs(dd_hdr.iface_mtu) > nbr->iface->mtu) { - log_warnx("recv_db_description: invalid MTU %d sent by " - "neighbor ID %s, expected %d", ntohs(dd_hdr.iface_mtu), - inet_ntoa(nbr->id), nbr->iface->mtu); + log_warnx("recv_db_description: neighbor ID %s: " + "invalid MTU %d expected %d", inet_ntoa(nbr->id), + ntohs(dd_hdr.iface_mtu), nbr->iface->mtu); return; } @@ -183,7 +182,7 @@ recv_db_description(struct nbr *nbr, cha nbr->last_rx_bits == dd_hdr.bits && ntohl(dd_hdr.dd_seq_num) == nbr->dd_seq_num - nbr->dd_master ? 1 : 0) { - log_debug("recv_db_description: dupe from ID %s", + log_debug("recv_db_description: dupe from neighbor ID %s", inet_ntoa(nbr->id)); dupe = 1; } @@ -193,9 +192,9 @@ recv_db_description(struct nbr *nbr, cha case NBR_STA_ATTEMPT: case NBR_STA_2_WAY: case NBR_STA_SNAP: - log_debug("recv_db_description: packet ignored in state %s, " - "neighbor ID %s", nbr_state_name(nbr->state), - inet_ntoa(nbr->id)); + log_debug("recv_db_description: neighbor ID %s: " + "packet ignored in state %s", inet_ntoa(nbr->id), + nbr_state_name(nbr->state)); return; case NBR_STA_INIT: /* evaluate dr and bdr after issuing a 2-Way event */ @@ -224,9 +223,11 @@ recv_db_description(struct nbr *nbr, cha } else if (!(dd_hdr.bits & (OSPF_DBD_I | OSPF_DBD_MS))) { /* M only case: we are master */ if (ntohl(dd_hdr.dd_seq_num) != nbr->dd_seq_num) { - log_warnx("recv_db_description: invalid " - "seq num, mine %x his %x", - nbr->dd_seq_num, ntohl(dd_hdr.dd_seq_num)); + log_warnx("recv_db_description: " +
Re: ospfd: type p2p
On 17/11/2019 13:44, Remi Locherer wrote: > Yes, I'll send a separate diff for that later. > > OK for the new diff? Works for me. G
Re: ospfd: type p2p
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 - 1.108 > > > +++ ospfd.c 23 Jun 2019 21:06:44 - > > > @@ -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); > > > + LI
Re: ospfd: type p2p
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 - 1.108 > > +++ ospfd.c 23 Jun 2019 21:06:44 - > > @@ -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_debu
Re: ospfd: type p2p
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 - 1.24 > +++ hello.c 21 Sep 2019 22:06:17 - > @@ -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.c15 Jul 2019 18:26:39 - 1.46 > +++ lsupdate.c 15 Aug 2019 21:10:13 - > @@ -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.c9 Feb 2018 02:14:03 - 1.48 > +++ neighbor.c21 Sep 2019 15:28:43 - > @@ -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; &g
Re: ospfd: type p2p
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? 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 - 1.24 +++ hello.c 21 Sep 2019 22:06:17 - @@ -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 - 1.46 +++ lsupdate.c 15 Aug 2019 21:10:13 - @@ -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 - 1.48 +++ neighbor.c 21 Sep 2019 15:28:43 - @@ -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 +nbr_update_addr(u_int32_t peerid, struct in_addr addr) { + + struct nbr *nbr = NULL; + + nbr = nbr_find_peerid(peerid); + if (nbr == NULL) + return (1); + + /* XXX neighbor address shouldn't be stored on virtual links */ + nbr->addr.s_addr = addr.s_addr;
Re: ospfd: correct function name in error message
Sebastian Benoit wrote: > > OK. Another option is to use __func__ which is always correct. > > Please definatly use __func__. With that ok benno too. I like __func__ where it makes sense, but often developers consider "i've shown an obscurely named function that i know" to be a solved problem, and then don't show a good explanation that a user can deal with. So always step back and think: Let's say the person doesn't have the source tree or the ability to read. Can they determine their next step? Often times __func__ is not an improvement, because it can be used like a crutch.
Re: ospfd: correct function name in error message
Claudio Jeker(cje...@diehard.n-r-g.com) on 2019.11.09 15:32:39 +0100: > On Sat, Nov 09, 2019 at 03:27:31PM +0100, Denis Fondras wrote: > > Fix function name in error message. > > > > Index: kroute.c > > === > > RCS file: /cvs/src/usr.sbin/ospfd/kroute.c,v > > retrieving revision 1.112 > > diff -u -p -r1.112 kroute.c > > --- kroute.c28 Dec 2018 19:25:10 - 1.112 > > +++ kroute.c9 Nov 2019 14:11:03 - > > @@ -1501,7 +1501,7 @@ rtmsg_process(char *buf, size_t len) > > if ((mpath || prio == kr_state.fib_prio) && > > (kr = kroute_matchgw(okr, nexthop)) == > > NULL) { > > - log_warnx("dispatch_rtmsg " > > + log_warnx("rtmsg_process " > > "mpath route not found"); > > /* add routes we missed out earlier */ > > goto add; > > @@ -1536,7 +1536,7 @@ rtmsg_process(char *buf, size_t len) > > add: > > if ((kr = calloc(1, > > sizeof(struct kroute_node))) == NULL) { > > - log_warn("dispatch calloc"); > > + log_warn("rtmsg_process calloc"); > > return (-1); > > } > > > > @@ -1580,7 +1580,7 @@ add: > > okr = kr; > > if (mpath && > > (kr = kroute_matchgw(kr, nexthop)) == NULL) { > > - log_warnx("dispatch_rtmsg " > > + log_warnx("rtmsg_process " > > "mpath route not found"); > > return (-1); > > } > > > > OK. Another option is to use __func__ which is always correct. Please definatly use __func__. With that ok benno too.
Re: ospfd: correct function name in error message
On Sat, Nov 09, 2019 at 03:27:31PM +0100, Denis Fondras wrote: > Fix function name in error message. > > Index: kroute.c > === > RCS file: /cvs/src/usr.sbin/ospfd/kroute.c,v > retrieving revision 1.112 > diff -u -p -r1.112 kroute.c > --- kroute.c 28 Dec 2018 19:25:10 - 1.112 > +++ kroute.c 9 Nov 2019 14:11:03 - > @@ -1501,7 +1501,7 @@ rtmsg_process(char *buf, size_t len) > if ((mpath || prio == kr_state.fib_prio) && > (kr = kroute_matchgw(okr, nexthop)) == > NULL) { > - log_warnx("dispatch_rtmsg " > + log_warnx("rtmsg_process " > "mpath route not found"); > /* add routes we missed out earlier */ > goto add; > @@ -1536,7 +1536,7 @@ rtmsg_process(char *buf, size_t len) > add: > if ((kr = calloc(1, > sizeof(struct kroute_node))) == NULL) { > - log_warn("dispatch calloc"); > + log_warn("rtmsg_process calloc"); > return (-1); > } > > @@ -1580,7 +1580,7 @@ add: > okr = kr; > if (mpath && > (kr = kroute_matchgw(kr, nexthop)) == NULL) { > - log_warnx("dispatch_rtmsg " > + log_warnx("rtmsg_process " > "mpath route not found"); > return (-1); > } > OK. Another option is to use __func__ which is always correct. -- :wq Claudio
ospfd: correct function name in error message
Fix function name in error message. Index: kroute.c === RCS file: /cvs/src/usr.sbin/ospfd/kroute.c,v retrieving revision 1.112 diff -u -p -r1.112 kroute.c --- kroute.c28 Dec 2018 19:25:10 - 1.112 +++ kroute.c9 Nov 2019 14:11:03 - @@ -1501,7 +1501,7 @@ rtmsg_process(char *buf, size_t len) if ((mpath || prio == kr_state.fib_prio) && (kr = kroute_matchgw(okr, nexthop)) == NULL) { - log_warnx("dispatch_rtmsg " + log_warnx("rtmsg_process " "mpath route not found"); /* add routes we missed out earlier */ goto add; @@ -1536,7 +1536,7 @@ rtmsg_process(char *buf, size_t len) add: if ((kr = calloc(1, sizeof(struct kroute_node))) == NULL) { - log_warn("dispatch calloc"); + log_warn("rtmsg_process calloc"); return (-1); } @@ -1580,7 +1580,7 @@ add: okr = kr; if (mpath && (kr = kroute_matchgw(kr, nexthop)) == NULL) { - log_warnx("dispatch_rtmsg " + log_warnx("rtmsg_process " "mpath route not found"); return (-1); }
Re: ospfd: type p2p
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 :) Thank you for testing! > > looking forward test it for IPv6 as well Sure, I plan to also do this this for ospf6d.
Re: ospfd: type p2p
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 > > > > 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 - 1.24 > +++ hello.c 21 Sep 2019 22:06:17 - > @@ -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.c15 Jul 2019 18:26:39 - 1.46 > +++ lsupdate.c15 Aug 2019 21:10:13 - > @@ -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.c9 Feb 2018 02:14:03 - 1.48 > +++ neighbor.c21 Sep 2019 15:28:43 - > @@ -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 > +nbr_update_addr(u_int32_t peerid, struct in_addr addr) { > + > + struct nbr *nbr = NULL; > + > + nbr = nbr_find_peerid(peerid); > + if (nbr == NULL) > + return (1); > + > + /* XXX neighbor address shouldn't be stored on virtual links */ > + nbr->addr.s_addr = addr.s_addr; >
ospfd: more explicit error message
Newer version after a comment by claudio@. Currently ospfd logs routing message type code instead of name. Make it more explicit. remi@ is OK but wonders if rtm_type_name() will be updated as needed. What do you think ? Denis Index: kroute.c === RCS file: /cvs/src/usr.sbin/ospfd/kroute.c,v retrieving revision 1.112 diff -u -p -r1.112 kroute.c --- kroute.c28 Dec 2018 19:25:10 - 1.112 +++ kroute.c1 Nov 2019 22:12:18 - @@ -1269,7 +1269,8 @@ retry: return (0); } } - log_warn("send_rtmsg: action %u, prefix %s/%u", hdr.rtm_type, + log_warn("send_rtmsg: action %s, prefix %s/%u", + rtm_type_name(hdr.rtm_type), inet_ntoa(kroute->prefix), kroute->prefixlen); return (0); } Index: logmsg.c === RCS file: /cvs/src/usr.sbin/ospfd/logmsg.c,v retrieving revision 1.1 diff -u -p -r1.1 logmsg.c --- logmsg.c2 Sep 2016 14:04:25 - 1.1 +++ logmsg.c1 Nov 2019 22:12:18 - @@ -23,6 +23,8 @@ #include #include #include +#include +#include #include "ospfd.h" #include "log.h" @@ -140,4 +142,36 @@ path_type_name(enum path_type type) } /* NOTREACHED */ return ("unknown"); +} + +const char * +rtm_type_name(int type) +{ + static char buf[13]; + + switch(type) { + case RTM_ADD: + return("RTM_ADD"); + case RTM_GET: + return("RTM_GET"); + case RTM_CHANGE: + return("RTM_CHANGE"); + case RTM_DELETE: + return("RTM_DELETE"); + case RTM_IFINFO: + return("RTM_IFINFO"); + case RTM_NEWADDR: + return("RTM_NEWADDR"); + case RTM_DELADDR: + return("RTM_DELADDR"); + case RTM_IFANNOUNCE: + return("RTM_IFANNOUNCE"); + case RTM_DESYNC: + return("RTM_DESYNC"); + case RTM_BFD: + return("RTM_BFD"); + default: + snprintf(buf, sizeof(buf), "#%d", type); + return(buf); + } } 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 - 1.104 +++ ospfd.h 1 Nov 2019 22:12:18 - @@ -598,6 +598,7 @@ const char *if_type_name(enum iface_type const char *if_auth_name(enum auth_type); const char *dst_type_name(enum dst_type); const char *path_type_name(enum path_type); +const char *rtm_type_name(int); /* name2id.c */ u_int16_t rtlabel_name2id(const char *);
ospfd: more explicit error message
Currently ospfd logs routing message type code instead of name. Make it more explicit. remi@ is OK but wonders if rtm_type_name() will be updated as needed (that's why I also log the typecode) What do you think ? Denis Index: kroute.c === RCS file: /cvs/src/usr.sbin/ospfd/kroute.c,v retrieving revision 1.112 diff -u -p -r1.112 kroute.c --- kroute.c28 Dec 2018 19:25:10 - 1.112 +++ kroute.c1 Nov 2019 18:00:31 - @@ -1269,7 +1269,8 @@ retry: return (0); } } - log_warn("send_rtmsg: action %u, prefix %s/%u", hdr.rtm_type, + log_warn("send_rtmsg: action %s(%d), prefix %s/%u", + rtm_type_name(hdr.rtm_type), hdr.rtm_type, inet_ntoa(kroute->prefix), kroute->prefixlen); return (0); } Index: logmsg.c === RCS file: /cvs/src/usr.sbin/ospfd/logmsg.c,v retrieving revision 1.1 diff -u -p -r1.1 logmsg.c --- logmsg.c2 Sep 2016 14:04:25 - 1.1 +++ logmsg.c1 Nov 2019 18:00:31 - @@ -23,6 +23,8 @@ #include #include #include +#include +#include #include "ospfd.h" #include "log.h" @@ -137,6 +139,35 @@ path_type_name(enum path_type type) return ("Type 1 ext"); case PT_TYPE2_EXT: return ("Type 2 ext"); + } + /* NOTREACHED */ + return ("unknown"); +} + +const char * +rtm_type_name(int type) +{ + switch(type) { + case RTM_ADD: + return("RTM_ADD"); + case RTM_GET: + return("RTM_GET"); + case RTM_CHANGE: + return("RTM_CHANGE"); + case RTM_DELETE: + return("RTM_DELETE"); + case RTM_IFINFO: + return("RTM_IFINFO"); + case RTM_NEWADDR: + return("RTM_NEWADDR"); + case RTM_DELADDR: + return("RTM_DELADDR"); + case RTM_IFANNOUNCE: + return("RTM_IFANNOUNCE"); + case RTM_DESYNC: + return("RTM_DESYNC"); + case RTM_BFD: + return("RTM_BFD"); } /* NOTREACHED */ return ("unknown"); 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 - 1.104 +++ ospfd.h 1 Nov 2019 18:00:31 - @@ -598,6 +598,7 @@ const char *if_type_name(enum iface_type const char *if_auth_name(enum auth_type); const char *dst_type_name(enum dst_type); const char *path_type_name(enum path_type); +const char *rtm_type_name(int); /* name2id.c */ u_int16_t rtlabel_name2id(const char *);
ospfd: type p2p
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 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 - 1.24 +++ hello.c 21 Sep 2019 22:06:17 - @@ -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 - 1.46 +++ lsupdate.c 15 Aug 2019 21:10:13 - @@ -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 - 1.48 +++ neighbor.c 21 Sep 2019 15:28:43 - @@ -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 +nbr_update_addr(u_int32_t peerid, struct in_addr addr) { + + struct nbr *nbr = NULL; + + nbr = nbr_find_peerid(peerid); + if (nbr == NULL) + return (1); + + /* 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)); + + 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 - 1.108 +++ ospfd.c 23 Jun 2019 21:06:44 - @@ -911,6 +911,22 @@ merge_interfaces(struct area *a, struct if_fsm(i, IF_EVT_UP); } + if (i->p2p != xi->p2p) { + /* re-add interfac
Re: ospfd: warn when a neighbor changes its ip address
Remi Locherer(remi.loche...@relo.ch) on 2019.08.11 11:37:27 +0200: > I'd like to get a notification when a neighbor changes the src IP address > for hello packets. Either it is a planned change or something bad happens > in the network. > > OK? ok > > Remi > > > Index: hello.c > === > RCS file: /cvs/src/usr.sbin/ospfd/hello.c,v > retrieving revision 1.23 > diff -u -p -r1.23 hello.c > --- hello.c 15 Jul 2019 18:26:39 - 1.23 > +++ hello.c 11 Aug 2019 09:36:13 - > @@ -189,10 +189,16 @@ 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; > + } > + > + 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; > } > > - /* actually the neighbor address shouldn't be stored on virtual links */ > - nbr->addr.s_addr = src.s_addr; > nbr->options = hello.opts; > > nbr_fsm(nbr, NBR_EVT_HELLO_RCVD); >
Re: ospfd: check dst addr for hello packets
reads ok Remi Locherer(remi.loche...@relo.ch) on 2019.08.11 11:21:36 +0200: > When ospfd receives a hello packet it takes the src IP address and updates > the address in its neighbor struct for the given router id unconditionally. > > In the case of broadcast interfaces this is not a problem: > find_iface() checks that the src address is from the same subnet as > the receiving interface is. It is best practice to only enable ospf in a > subnet where you control all routers. > > But in the case of point-to-point interfaces no sanity checks happen on the > src or dst IP address. > > RFC 2328 says in "9.5. Sending Hello packets": > On broadcast networks and physical point-to-point networks, > Hello packets are sent every HelloInterval seconds to the IP > multicast address AllSPFRouters. > > > I verified that ospfd does it like that. Also FastIron and Junos follow > this. > > I propose that we add a check and only accept hellos on point-to-point and > broadcast interfaces when the destination is 224.0.0.5 (AllSPFRouters). > > The check for AllDRouters is not needed in addition to the proposed check. > > OK? > > Remi > > > Index: packet.c > === > RCS file: /cvs/src/usr.sbin/ospfd/packet.c,v > retrieving revision 1.32 > diff -u -p -r1.32 packet.c > --- packet.c 15 Jul 2019 18:26:39 - 1.32 > +++ packet.c 11 Aug 2019 09:17:51 - > @@ -219,12 +219,16 @@ recv_packet(int fd, short event, void *b > /* switch OSPF packet type */ > switch (ospf_hdr->type) { > case PACKET_TYPE_HELLO: > - inet_aton(AllDRouters, &addr); > - if (ip_hdr.ip_dst.s_addr == addr.s_addr) { > - log_debug("recv_packet: invalid destination IP " > - "address"); > - break; > - } > + inet_aton(AllSPFRouters, &addr); > + if (iface->type == IF_TYPE_BROADCAST || > + iface->type == IF_TYPE_POINTOPOINT) > + if (ip_hdr.ip_dst.s_addr != addr.s_addr) { > + log_warnx("%s: hello ignored on interface %s, " > + "invalid destination IP address %s", > + __func__, iface->name, > + inet_ntoa(ip_hdr.ip_dst)); > + break; > + } > > recv_hello(iface, ip_hdr.ip_src, ospf_hdr->rtr_id, buf, len); > break; >
ospfd: warn when a neighbor changes its ip address
I'd like to get a notification when a neighbor changes the src IP address for hello packets. Either it is a planned change or something bad happens in the network. OK? Remi Index: hello.c === RCS file: /cvs/src/usr.sbin/ospfd/hello.c,v retrieving revision 1.23 diff -u -p -r1.23 hello.c --- hello.c 15 Jul 2019 18:26:39 - 1.23 +++ hello.c 11 Aug 2019 09:36:13 - @@ -189,10 +189,16 @@ 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; + } + + 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; } - /* actually the neighbor address shouldn't be stored on virtual links */ - nbr->addr.s_addr = src.s_addr; nbr->options = hello.opts; nbr_fsm(nbr, NBR_EVT_HELLO_RCVD);
ospfd: check dst addr for hello packets
When ospfd receives a hello packet it takes the src IP address and updates the address in its neighbor struct for the given router id unconditionally. In the case of broadcast interfaces this is not a problem: find_iface() checks that the src address is from the same subnet as the receiving interface is. It is best practice to only enable ospf in a subnet where you control all routers. But in the case of point-to-point interfaces no sanity checks happen on the src or dst IP address. RFC 2328 says in "9.5. Sending Hello packets": On broadcast networks and physical point-to-point networks, Hello packets are sent every HelloInterval seconds to the IP multicast address AllSPFRouters. I verified that ospfd does it like that. Also FastIron and Junos follow this. I propose that we add a check and only accept hellos on point-to-point and broadcast interfaces when the destination is 224.0.0.5 (AllSPFRouters). The check for AllDRouters is not needed in addition to the proposed check. OK? Remi Index: packet.c === RCS file: /cvs/src/usr.sbin/ospfd/packet.c,v retrieving revision 1.32 diff -u -p -r1.32 packet.c --- packet.c15 Jul 2019 18:26:39 - 1.32 +++ packet.c11 Aug 2019 09:17:51 - @@ -219,12 +219,16 @@ recv_packet(int fd, short event, void *b /* switch OSPF packet type */ switch (ospf_hdr->type) { case PACKET_TYPE_HELLO: - inet_aton(AllDRouters, &addr); - if (ip_hdr.ip_dst.s_addr == addr.s_addr) { - log_debug("recv_packet: invalid destination IP " -"address"); - break; - } + inet_aton(AllSPFRouters, &addr); + if (iface->type == IF_TYPE_BROADCAST || + iface->type == IF_TYPE_POINTOPOINT) + if (ip_hdr.ip_dst.s_addr != addr.s_addr) { + log_warnx("%s: hello ignored on interface %s, " + "invalid destination IP address %s", + __func__, iface->name, + inet_ntoa(ip_hdr.ip_dst)); + break; + } recv_hello(iface, ip_hdr.ip_src, ospf_hdr->rtr_id, buf, len); break;
Re: ospfd: improve logging when sendig packets fail
On Mon, Jul 15, 2019 at 07:32:18AM +0200, Remi Locherer wrote: > Hi, > > I'd like to improve ospfd's logging when sending a packet fails. > > I got a debug output from a ospfd user which contains "send packet: error > ...". > I guess ospfd failed to send an ls ack. With below diff applied it would be > clear which packet could not be sent and to which neighbor. > > OK? Sure, OK claudio@ Guess ospf6d could use a similar diff. > > Index: database.c > ======= > RCS file: /cvs/src/usr.sbin/ospfd/database.c,v > retrieving revision 1.33 > diff -u -p -r1.33 database.c > --- database.c18 Feb 2016 15:33:24 - 1.33 > +++ database.c13 Jul 2019 14:08:10 - > @@ -43,7 +43,6 @@ send_db_description(struct nbr *nbr) > struct db_dscrp_hdr dd_hdr; > struct lsa_entry*le, *nle; > struct ibuf *buf; > - int ret = 0; > u_int8_t bits = 0; > > if ((buf = ibuf_open(nbr->iface->mtu - sizeof(struct ip))) == NULL) > @@ -66,8 +65,7 @@ send_db_description(struct nbr *nbr) > log_debug("send_db_description: neighbor ID %s: " > "cannot send packet in state %s", inet_ntoa(nbr->id), > nbr_state_name(nbr->state)); > - ret = -1; > - goto done; > + goto fail; > case NBR_STA_XSTRT: > bits |= OSPF_DBD_MS | OSPF_DBD_M | OSPF_DBD_I; > nbr->dd_more = 1; > @@ -150,12 +148,13 @@ send_db_description(struct nbr *nbr) > goto fail; > > /* transmit packet */ > - ret = send_packet(nbr->iface, buf, &dst); > -done: > + if (send_packet(nbr->iface, buf, &dst) == -1) > + goto fail; > + > ibuf_free(buf); > - return (ret); > + return (0); > fail: > - log_warn("send_db_description"); > + log_warn("%s", __func__); > ibuf_free(buf); > return (-1); > } > Index: hello.c > === > RCS file: /cvs/src/usr.sbin/ospfd/hello.c,v > retrieving revision 1.22 > diff -u -p -r1.22 hello.c > --- hello.c 22 Feb 2018 07:42:38 - 1.22 > +++ hello.c 13 Jul 2019 14:03:27 - > @@ -41,7 +41,6 @@ send_hello(struct iface *iface) > struct hello_hdr hello; > struct nbr *nbr; > struct ibuf *buf; > - int ret; > > dst.sin_family = AF_INET; > dst.sin_len = sizeof(struct sockaddr_in); > @@ -103,11 +102,13 @@ send_hello(struct iface *iface) > if (auth_gen(buf, iface)) > goto fail; > > - ret = send_packet(iface, buf, &dst); > + if (send_packet(iface, buf, &dst) == -1) > + goto fail; > + > ibuf_free(buf); > - return (ret); > + return (0); > fail: > - log_warn("send_hello"); > + log_warn("%s", __func__); > ibuf_free(buf); > return (-1); > } > Index: lsack.c > === > RCS file: /cvs/src/usr.sbin/ospfd/lsack.c,v > retrieving revision 1.21 > diff -u -p -r1.21 lsack.c > --- lsack.c 25 Oct 2014 03:23:49 - 1.21 > +++ lsack.c 13 Jul 2019 14:04:59 - > @@ -59,7 +59,6 @@ int > send_ls_ack(struct iface *iface, struct in_addr addr, struct ibuf *buf) > { > struct sockaddr_in dst; > - int ret; > > /* update authentication and calculate checksum */ > if (auth_gen(buf, iface)) { > @@ -71,8 +70,11 @@ send_ls_ack(struct iface *iface, struct > dst.sin_len = sizeof(struct sockaddr_in); > dst.sin_addr.s_addr = addr.s_addr; > > - ret = send_packet(iface, buf, &dst); > - return (ret); > + if (send_packet(iface, buf, &dst) == -1) { > + log_warn("%s", __func__); > + return (-1); > + } > + return (0); > } > > int > Index: lsreq.c > === > RCS file: /cvs/src/usr.sbin/ospfd/lsreq.c,v > retrieving revision 1.20 > diff -u -p -r1.20 lsreq.c > --- lsreq.c 17 Jan 2013 09:02:22 - 1.20 > +++ lsreq.c 13 Jul 2019 14:04:00 - > @@ -37,7 +37,6 @@ send_ls_req(struct nbr *nbr) > struct ls_req_hdrls_req_hdr; > struct lsa_entry*le, *nle; > struct ibuf *buf; > - int re
ospfd: improve logging when sendig packets fail
Hi, I'd like to improve ospfd's logging when sending a packet fails. I got a debug output from a ospfd user which contains "send packet: error ...". I guess ospfd failed to send an ls ack. With below diff applied it would be clear which packet could not be sent and to which neighbor. OK? Remi Index: database.c === RCS file: /cvs/src/usr.sbin/ospfd/database.c,v retrieving revision 1.33 diff -u -p -r1.33 database.c --- database.c 18 Feb 2016 15:33:24 - 1.33 +++ database.c 13 Jul 2019 14:08:10 - @@ -43,7 +43,6 @@ send_db_description(struct nbr *nbr) struct db_dscrp_hdr dd_hdr; struct lsa_entry*le, *nle; struct ibuf *buf; - int ret = 0; u_int8_t bits = 0; if ((buf = ibuf_open(nbr->iface->mtu - sizeof(struct ip))) == NULL) @@ -66,8 +65,7 @@ send_db_description(struct nbr *nbr) log_debug("send_db_description: neighbor ID %s: " "cannot send packet in state %s", inet_ntoa(nbr->id), nbr_state_name(nbr->state)); - ret = -1; - goto done; + goto fail; case NBR_STA_XSTRT: bits |= OSPF_DBD_MS | OSPF_DBD_M | OSPF_DBD_I; nbr->dd_more = 1; @@ -150,12 +148,13 @@ send_db_description(struct nbr *nbr) goto fail; /* transmit packet */ - ret = send_packet(nbr->iface, buf, &dst); -done: + if (send_packet(nbr->iface, buf, &dst) == -1) + goto fail; + ibuf_free(buf); - return (ret); + return (0); fail: - log_warn("send_db_description"); + log_warn("%s", __func__); ibuf_free(buf); return (-1); } Index: hello.c ======= RCS file: /cvs/src/usr.sbin/ospfd/hello.c,v retrieving revision 1.22 diff -u -p -r1.22 hello.c --- hello.c 22 Feb 2018 07:42:38 - 1.22 +++ hello.c 13 Jul 2019 14:03:27 - @@ -41,7 +41,6 @@ send_hello(struct iface *iface) struct hello_hdr hello; struct nbr *nbr; struct ibuf *buf; - int ret; dst.sin_family = AF_INET; dst.sin_len = sizeof(struct sockaddr_in); @@ -103,11 +102,13 @@ send_hello(struct iface *iface) if (auth_gen(buf, iface)) goto fail; - ret = send_packet(iface, buf, &dst); + if (send_packet(iface, buf, &dst) == -1) + goto fail; + ibuf_free(buf); - return (ret); + return (0); fail: - log_warn("send_hello"); + log_warn("%s", __func__); ibuf_free(buf); return (-1); } Index: lsack.c === RCS file: /cvs/src/usr.sbin/ospfd/lsack.c,v retrieving revision 1.21 diff -u -p -r1.21 lsack.c --- lsack.c 25 Oct 2014 03:23:49 - 1.21 +++ lsack.c 13 Jul 2019 14:04:59 - @@ -59,7 +59,6 @@ int send_ls_ack(struct iface *iface, struct in_addr addr, struct ibuf *buf) { struct sockaddr_in dst; - int ret; /* update authentication and calculate checksum */ if (auth_gen(buf, iface)) { @@ -71,8 +70,11 @@ send_ls_ack(struct iface *iface, struct dst.sin_len = sizeof(struct sockaddr_in); dst.sin_addr.s_addr = addr.s_addr; - ret = send_packet(iface, buf, &dst); - return (ret); + if (send_packet(iface, buf, &dst) == -1) { + log_warn("%s", __func__); + return (-1); + } + return (0); } int Index: lsreq.c === RCS file: /cvs/src/usr.sbin/ospfd/lsreq.c,v retrieving revision 1.20 diff -u -p -r1.20 lsreq.c --- lsreq.c 17 Jan 2013 09:02:22 - 1.20 +++ lsreq.c 13 Jul 2019 14:04:00 - @@ -37,7 +37,6 @@ send_ls_req(struct nbr *nbr) struct ls_req_hdrls_req_hdr; struct lsa_entry*le, *nle; struct ibuf *buf; - int ret; if ((buf = ibuf_open(nbr->iface->mtu - sizeof(struct ip))) == NULL) fatal("send_ls_req"); @@ -80,12 +79,13 @@ send_ls_req(struct nbr *nbr) if (auth_gen(buf, nbr->iface)) goto fail; - ret = send_packet(nbr->iface, buf, &dst); + if (send_packet(nbr->iface, buf, &dst) == -1) + goto fail; ibuf_free(buf); - return (ret); + return (0); fail: - log_warn("send_ls_req"); + log_warn("%s", __func__); ibuf_free(buf); return (-1); } Index: lsupdate.c
Re: ospfd: point-to-point on ethernet interfaces
On Thu, Jul 04, 2019 at 09:20:59AM +0300, Kapetanakis Giannis wrote: > Hi, > > This does not work for me with IOS. > > neighbor is full, > rib is ok > fib does not list the routes to IOS and > routing table is not updated on BSD > > On IOS I do have the loopback route the BSD is announcing. Thank you for testing! Can you send me your ospfd.conf, the output from ospfd -dv and the output from tcpdump showing the ospf traffic? > On 24/06/2019 01:33, Remi Locherer wrote: > > Diff below adds to ospfd point to point support for Ethernet interfaces. > > I successfully tested this against Junos and FastIron. > > > > I first made the key word in the config "point-to-point". But then I > > changed to "type p2p". The later would allow for "type nbma" or "type p2mp" > > should we implement these types. > > > > On Junos it looks like this: > > > > area 0.0.0.0 { > > interface ge-0/0/1.0 { > > interface-type p2p; > > } > > } > > > > On FastIron it's similar to IOS: > > > > interface ethernet 1/2/1 > > ip address 10.10.10.5 255.255.255.0 > > ip ospf area 0 > > ip ospf network point-to-point > > > > Comments, test reports and OKs are welcome. > > > > Remi > > > > > > Index: interface.c > > === > > RCS file: /cvs/src/usr.sbin/ospfd/interface.c,v > > retrieving revision 1.82 > > diff -u -p -r1.82 interface.c > > --- interface.c 11 Mar 2018 13:16:49 - 1.82 > > +++ interface.c 23 Jun 2019 11:27:57 - > > @@ -190,6 +190,8 @@ if_new(struct kif *kif, struct kif_addr > > if (kif->flags & IFF_BROADCAST && > > kif->flags & IFF_MULTICAST) > > iface->type = IF_TYPE_BROADCAST; > > + if (iface->p2p) > > + iface->type = IF_TYPE_POINTOPOINT; > > if (kif->flags & IFF_LOOPBACK) { > > iface->type = IF_TYPE_POINTOPOINT; > > iface->passive = 1; > > @@ -351,6 +353,9 @@ if_act_start(struct iface *iface) > > orig_rtr_lsa(iface->area); > > return (0); > > } > > + > > + if (iface->p2p) > > + iface->type = IF_TYPE_POINTOPOINT; > > > > switch (iface->type) { > > case IF_TYPE_POINTOPOINT: > > 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 - 1.108 > > +++ ospfd.c 23 Jun 2019 21:06:44 - > > @@ -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; > > + } > > + > > 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.510 Jun 2019 06:07:15 - 1.57 > > +++ ospfd.conf.523 Jun 2019 22:10:32 - > > @@ -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
Re: ospfd: point-to-point on ethernet interfaces
Hi, This does not work for me with IOS. neighbor is full, rib is ok fib does not list the routes to IOS and routing table is not updated on BSD On IOS I do have the loopback route the BSD is announcing. G On 24/06/2019 01:33, Remi Locherer wrote: > Diff below adds to ospfd point to point support for Ethernet interfaces. > I successfully tested this against Junos and FastIron. > > I first made the key word in the config "point-to-point". But then I > changed to "type p2p". The later would allow for "type nbma" or "type p2mp" > should we implement these types. > > On Junos it looks like this: > > area 0.0.0.0 { > interface ge-0/0/1.0 { > interface-type p2p; > } > } > > On FastIron it's similar to IOS: > > interface ethernet 1/2/1 > ip address 10.10.10.5 255.255.255.0 > ip ospf area 0 > ip ospf network point-to-point > > Comments, test reports and OKs are welcome. > > Remi > > > Index: interface.c > === > RCS file: /cvs/src/usr.sbin/ospfd/interface.c,v > retrieving revision 1.82 > diff -u -p -r1.82 interface.c > --- interface.c 11 Mar 2018 13:16:49 - 1.82 > +++ interface.c 23 Jun 2019 11:27:57 - > @@ -190,6 +190,8 @@ if_new(struct kif *kif, struct kif_addr > if (kif->flags & IFF_BROADCAST && > kif->flags & IFF_MULTICAST) > iface->type = IF_TYPE_BROADCAST; > + if (iface->p2p) > + iface->type = IF_TYPE_POINTOPOINT; > if (kif->flags & IFF_LOOPBACK) { > iface->type = IF_TYPE_POINTOPOINT; > iface->passive = 1; > @@ -351,6 +353,9 @@ if_act_start(struct iface *iface) > orig_rtr_lsa(iface->area); > return (0); > } > + > + if (iface->p2p) > + iface->type = IF_TYPE_POINTOPOINT; > > switch (iface->type) { > case IF_TYPE_POINTOPOINT: > 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 - 1.108 > +++ ospfd.c 23 Jun 2019 21:06:44 - > @@ -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; > + } > + > 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 - 1.57 > +++ ospfd.conf.5 23 Jun 2019 22:10:32 - > @@ -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 - 1.104 > +++ ospfd.h 23 Jun 2019 11:28:24 - > @@ -363,6 +363,7 @@ struct iface { > u_int8_t linkstate; > u_int8_t priority; > u_int8_t passive; > + u_int8_t p2p; > }; > > struct ifaddrchange { > Index: parse.y > =
Re: ospfd: point-to-point on ethernet interfaces
Works for me no problem. tested to IOS. On 03.07.19 00:00, Remi Locherer wrote: ping On Mon, Jun 24, 2019 at 12:33:16AM +0200, Remi Locherer wrote: Diff below adds to ospfd point to point support for Ethernet interfaces. I successfully tested this against Junos and FastIron. I first made the key word in the config "point-to-point". But then I changed to "type p2p". The later would allow for "type nbma" or "type p2mp" should we implement these types. On Junos it looks like this: area 0.0.0.0 { interface ge-0/0/1.0 { interface-type p2p; } } On FastIron it's similar to IOS: interface ethernet 1/2/1 ip address 10.10.10.5 255.255.255.0 ip ospf area 0 ip ospf network point-to-point Comments, test reports and OKs are welcome. Remi Index: interface.c ======= RCS file: /cvs/src/usr.sbin/ospfd/interface.c,v retrieving revision 1.82 diff -u -p -r1.82 interface.c --- interface.c 11 Mar 2018 13:16:49 - 1.82 +++ interface.c 23 Jun 2019 11:27:57 - @@ -190,6 +190,8 @@ if_new(struct kif *kif, struct kif_addr if (kif->flags & IFF_BROADCAST && kif->flags & IFF_MULTICAST) iface->type = IF_TYPE_BROADCAST; + if (iface->p2p) + iface->type = IF_TYPE_POINTOPOINT; if (kif->flags & IFF_LOOPBACK) { iface->type = IF_TYPE_POINTOPOINT; iface->passive = 1; @@ -351,6 +353,9 @@ if_act_start(struct iface *iface) orig_rtr_lsa(iface->area); return (0); } + + if (iface->p2p) + iface->type = IF_TYPE_POINTOPOINT; switch (iface->type) { case IF_TYPE_POINTOPOINT: 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 - 1.108 +++ ospfd.c 23 Jun 2019 21:06:44 - @@ -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; + } + 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.510 Jun 2019 06:07:15 - 1.57 +++ ospfd.conf.523 Jun 2019 22:10:32 - @@ -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 - 1.104 +++ ospfd.h 23 Jun 2019 11:28:24 - @@ -363,6 +363,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 - 1.98 +++ parse.y 23 Jun 2019 22:04:22 - @@ -129,7 +129,7 @@ typedef struct { %tokenAREA INTERFACE ROUTERID FIBPRIORITY FIBUPDATE REDISTRIBUTE RTLABEL %tokenRDOMAIN RFC1583COMPAT STUB ROUTER SPFDELAY SPFHOLDTIME EXTTAG %tokenAUTHKEY AUTHTYPE AUTHMD AUTHMDKEYID -%token METRIC PASSIVE +%token METRIC P2P PASSIVE %tokenHELLOINTERVAL FASTHELLOINTERVAL TRANSMITDELAY %token
Re: ospfd: point-to-point on ethernet interfaces
ping On Mon, Jun 24, 2019 at 12:33:16AM +0200, Remi Locherer wrote: > Diff below adds to ospfd point to point support for Ethernet interfaces. > I successfully tested this against Junos and FastIron. > > I first made the key word in the config "point-to-point". But then I > changed to "type p2p". The later would allow for "type nbma" or "type p2mp" > should we implement these types. > > On Junos it looks like this: > > area 0.0.0.0 { > interface ge-0/0/1.0 { > interface-type p2p; > } > } > > On FastIron it's similar to IOS: > > interface ethernet 1/2/1 > ip address 10.10.10.5 255.255.255.0 > ip ospf area 0 > ip ospf network point-to-point > > Comments, test reports and OKs are welcome. > > Remi > > > Index: interface.c > === > RCS file: /cvs/src/usr.sbin/ospfd/interface.c,v > retrieving revision 1.82 > diff -u -p -r1.82 interface.c > --- interface.c 11 Mar 2018 13:16:49 - 1.82 > +++ interface.c 23 Jun 2019 11:27:57 - > @@ -190,6 +190,8 @@ if_new(struct kif *kif, struct kif_addr > if (kif->flags & IFF_BROADCAST && > kif->flags & IFF_MULTICAST) > iface->type = IF_TYPE_BROADCAST; > + if (iface->p2p) > + iface->type = IF_TYPE_POINTOPOINT; > if (kif->flags & IFF_LOOPBACK) { > iface->type = IF_TYPE_POINTOPOINT; > iface->passive = 1; > @@ -351,6 +353,9 @@ if_act_start(struct iface *iface) > orig_rtr_lsa(iface->area); > return (0); > } > + > + if (iface->p2p) > + iface->type = IF_TYPE_POINTOPOINT; > > switch (iface->type) { > case IF_TYPE_POINTOPOINT: > 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 - 1.108 > +++ ospfd.c 23 Jun 2019 21:06:44 - > @@ -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; > + } > + > 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 - 1.57 > +++ ospfd.conf.5 23 Jun 2019 22:10:32 - > @@ -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 - 1.104 > +++ ospfd.h 23 Jun 2019 11:28:24 - > @@ -363,6 +363,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 Ju
ospfd: point-to-point on ethernet interfaces
Diff below adds to ospfd point to point support for Ethernet interfaces. I successfully tested this against Junos and FastIron. I first made the key word in the config "point-to-point". But then I changed to "type p2p". The later would allow for "type nbma" or "type p2mp" should we implement these types. On Junos it looks like this: area 0.0.0.0 { interface ge-0/0/1.0 { interface-type p2p; } } On FastIron it's similar to IOS: interface ethernet 1/2/1 ip address 10.10.10.5 255.255.255.0 ip ospf area 0 ip ospf network point-to-point Comments, test reports and OKs are welcome. Remi Index: interface.c ======= RCS file: /cvs/src/usr.sbin/ospfd/interface.c,v retrieving revision 1.82 diff -u -p -r1.82 interface.c --- interface.c 11 Mar 2018 13:16:49 - 1.82 +++ interface.c 23 Jun 2019 11:27:57 - @@ -190,6 +190,8 @@ if_new(struct kif *kif, struct kif_addr if (kif->flags & IFF_BROADCAST && kif->flags & IFF_MULTICAST) iface->type = IF_TYPE_BROADCAST; + if (iface->p2p) + iface->type = IF_TYPE_POINTOPOINT; if (kif->flags & IFF_LOOPBACK) { iface->type = IF_TYPE_POINTOPOINT; iface->passive = 1; @@ -351,6 +353,9 @@ if_act_start(struct iface *iface) orig_rtr_lsa(iface->area); return (0); } + + if (iface->p2p) + iface->type = IF_TYPE_POINTOPOINT; switch (iface->type) { case IF_TYPE_POINTOPOINT: 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 - 1.108 +++ ospfd.c 23 Jun 2019 21:06:44 - @@ -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; + } + 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.510 Jun 2019 06:07:15 - 1.57 +++ ospfd.conf.523 Jun 2019 22:10:32 - @@ -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 - 1.104 +++ ospfd.h 23 Jun 2019 11:28:24 - @@ -363,6 +363,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 - 1.98 +++ parse.y 23 Jun 2019 22:04:22 - @@ -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,7 @@ interfaceopts_l : interfaceopts_l interf ; interface
Re: ospfd: allow specifying area by number as well as id
yes :D > On 29 May 2019, at 15:05, Remi Locherer wrote: > > Hi David, > > are you going to commit this? > > Remi > > > On Thu, May 16, 2019 at 11:14:55PM +0200, Remi Locherer wrote: >> On Thu, May 16, 2019 at 09:39:37AM +0200, Sebastian Benoit wrote: >>> >>> >>> >>> Remi Locherer(remi.loche...@relo.ch) on 2019.05.15 23:15:03 +0200: >>>> On Tue, Apr 30, 2019 at 11:10:37PM +0200, Remi Locherer wrote: >>>>> On Mon, Apr 29, 2019 at 11:10:31AM +0100, Stuart Henderson wrote: >>>>>> On 2019/04/29 11:58, Sebastian Benoit wrote: >>>>>>> David Gwynne(da...@gwynne.id.au) on 2019.04.29 19:36:51 +1000: >>>>>>>> >>>>>>>> >>>>>>>>> On 29 Apr 2019, at 4:59 pm, Remi Locherer >>>>>>>>> wrote: >>>>>>>>> >>>>>>>>> Hi David >>>>>>>>> >>>>>>>>> On Mon, Apr 29, 2019 at 11:53:27AM +1000, David Gwynne wrote: >>>>>>>>>> it's always bothered me that i config areas on a crisco using a >>>>>>>>>> number, >>>>>>>>>> but then have to think hard to convert that number to an address for >>>>>>>>>> use >>>>>>>>>> in openbsd. eg, i was given area 700 in one place, which is 0.0.2.188 >>>>>>>>>> as an address. super annoying. >>>>>>>>>> >>>>>>>>>> so this changes the ospfd parser so it accepts both a number or >>>>>>>>>> address. >>>>>>>>>> i also changed it so it prints the number by default, which may be >>>>>>>>>> contentious. the manpage is slightly tweaked too. >>>>>>>>>> >>>>>>>>>> thoughts? >>>>>>>>> >>>>>>>>> I like it to be able to use a number instead of an address! >>>>>>>>> >>>>>>>>> It worked fine in my short test I performed. >>>>>>>>> >>>>>>>>> The output with the comment looks a bit strange to me. >>>>>>>> >>>>>>>> Are you sure it doesn't look... awesome? >>>>>>> >>>>>>> I like it! >>>>>> >>>>>> I don't really, but if we change this it needs to be displayed somehow >>>>>> and I don't have an idea to make it look nicer than this (cisco's method >>>>>> seems pretty horrible and wouldn't work for us anyway - looks like they >>>>>> remember which format was used to configure an area and use that as >>>>>> the output format...) >>>>>> >>>>> >>>>> Maybe it's better when we just allow both input formats but don't change >>>>> any output. >>>> >>>> Any opinions or comments on this? I think this would be a valuable addition >>>> to ospfd. >>> >>> Yes, and diff is ok benno@ >>> >> >> David: ok remi@ for your diff without the printconf part. >> >>> What about ospf6d? >> >> I'll handle that. >> >>> >>>>> >>>>> Below diff changes ospfctl to accept the address and number format for >>>>> "ospfct show database area XXX". >>>>> >>>>> >>>>> Index: parser.c >>>>> === >>>>> RCS file: /cvs/src/usr.sbin/ospfctl/parser.c,v >>>>> retrieving revision 1.20 >>>>> diff -u -p -r1.20 parser.c >>>>> --- parser.c 9 May 2011 12:25:35 - 1.20 >>>>> +++ parser.c 30 Apr 2019 20:28:18 - >>>>> @@ -39,7 +39,8 @@ enum token_type { >>>>> ADDRESS, >>>>> FLAG, >>>>> PREFIX, >>>>> - IFNAME >>>>> + IFNAME, >>>>> + AREA >>>>> }; >>>>> >>>>> struct token { >>>>> @@ -107,7 +108,7 @@ static const struct token t_show_db[] = >>>>> }; >>>>> >>>>> static const struct token t_show_area[] = { >>>>> - {ADDRESS, "",
Re: ospfd: allow specifying area by number as well as id
Hi David, are you going to commit this? Remi On Thu, May 16, 2019 at 11:14:55PM +0200, Remi Locherer wrote: > On Thu, May 16, 2019 at 09:39:37AM +0200, Sebastian Benoit wrote: > > > > > > > > Remi Locherer(remi.loche...@relo.ch) on 2019.05.15 23:15:03 +0200: > > > On Tue, Apr 30, 2019 at 11:10:37PM +0200, Remi Locherer wrote: > > > > On Mon, Apr 29, 2019 at 11:10:31AM +0100, Stuart Henderson wrote: > > > > > On 2019/04/29 11:58, Sebastian Benoit wrote: > > > > > > David Gwynne(da...@gwynne.id.au) on 2019.04.29 19:36:51 +1000: > > > > > > > > > > > > > > > > > > > > > > On 29 Apr 2019, at 4:59 pm, Remi Locherer > > > > > > > > wrote: > > > > > > > > > > > > > > > > Hi David > > > > > > > > > > > > > > > > On Mon, Apr 29, 2019 at 11:53:27AM +1000, David Gwynne wrote: > > > > > > > >> it's always bothered me that i config areas on a crisco using > > > > > > > >> a number, > > > > > > > >> but then have to think hard to convert that number to an > > > > > > > >> address for use > > > > > > > >> in openbsd. eg, i was given area 700 in one place, which is > > > > > > > >> 0.0.2.188 > > > > > > > >> as an address. super annoying. > > > > > > > >> > > > > > > > >> so this changes the ospfd parser so it accepts both a number > > > > > > > >> or address. > > > > > > > >> i also changed it so it prints the number by default, which > > > > > > > >> may be > > > > > > > >> contentious. the manpage is slightly tweaked too. > > > > > > > >> > > > > > > > >> thoughts? > > > > > > > > > > > > > > > > I like it to be able to use a number instead of an address! > > > > > > > > > > > > > > > > It worked fine in my short test I performed. > > > > > > > > > > > > > > > > The output with the comment looks a bit strange to me. > > > > > > > > > > > > > > Are you sure it doesn't look... awesome? > > > > > > > > > > > > I like it! > > > > > > > > > > I don't really, but if we change this it needs to be displayed somehow > > > > > and I don't have an idea to make it look nicer than this (cisco's > > > > > method > > > > > seems pretty horrible and wouldn't work for us anyway - looks like > > > > > they > > > > > remember which format was used to configure an area and use that as > > > > > the output format...) > > > > > > > > > > > > > Maybe it's better when we just allow both input formats but don't change > > > > any output. > > > > > > Any opinions or comments on this? I think this would be a valuable > > > addition > > > to ospfd. > > > > Yes, and diff is ok benno@ > > > > David: ok remi@ for your diff without the printconf part. > > > What about ospf6d? > > I'll handle that. > > > > > > > > > > > Below diff changes ospfctl to accept the address and number format for > > > > "ospfct show database area XXX". > > > > > > > > > > > > Index: parser.c > > > > === > > > > RCS file: /cvs/src/usr.sbin/ospfctl/parser.c,v > > > > retrieving revision 1.20 > > > > diff -u -p -r1.20 parser.c > > > > --- parser.c9 May 2011 12:25:35 - 1.20 > > > > +++ parser.c30 Apr 2019 20:28:18 - > > > > @@ -39,7 +39,8 @@ enum token_type { > > > > ADDRESS, > > > > FLAG, > > > > PREFIX, > > > > - IFNAME > > > > + IFNAME, > > > > + AREA > > > > }; > > > > > > > > struct token { > > > > @@ -107,7 +108,7 @@ static const struct token t_show_db[] = > > > > }; > > > > > > >
Re: ospfd: allow specifying area by number as well as id
On Thu, May 16, 2019 at 09:39:37AM +0200, Sebastian Benoit wrote: > > > > Remi Locherer(remi.loche...@relo.ch) on 2019.05.15 23:15:03 +0200: > > On Tue, Apr 30, 2019 at 11:10:37PM +0200, Remi Locherer wrote: > > > On Mon, Apr 29, 2019 at 11:10:31AM +0100, Stuart Henderson wrote: > > > > On 2019/04/29 11:58, Sebastian Benoit wrote: > > > > > David Gwynne(da...@gwynne.id.au) on 2019.04.29 19:36:51 +1000: > > > > > > > > > > > > > > > > > > > On 29 Apr 2019, at 4:59 pm, Remi Locherer > > > > > > > wrote: > > > > > > > > > > > > > > Hi David > > > > > > > > > > > > > > On Mon, Apr 29, 2019 at 11:53:27AM +1000, David Gwynne wrote: > > > > > > >> it's always bothered me that i config areas on a crisco using a > > > > > > >> number, > > > > > > >> but then have to think hard to convert that number to an address > > > > > > >> for use > > > > > > >> in openbsd. eg, i was given area 700 in one place, which is > > > > > > >> 0.0.2.188 > > > > > > >> as an address. super annoying. > > > > > > >> > > > > > > >> so this changes the ospfd parser so it accepts both a number or > > > > > > >> address. > > > > > > >> i also changed it so it prints the number by default, which may > > > > > > >> be > > > > > > >> contentious. the manpage is slightly tweaked too. > > > > > > >> > > > > > > >> thoughts? > > > > > > > > > > > > > > I like it to be able to use a number instead of an address! > > > > > > > > > > > > > > It worked fine in my short test I performed. > > > > > > > > > > > > > > The output with the comment looks a bit strange to me. > > > > > > > > > > > > Are you sure it doesn't look... awesome? > > > > > > > > > > I like it! > > > > > > > > I don't really, but if we change this it needs to be displayed somehow > > > > and I don't have an idea to make it look nicer than this (cisco's method > > > > seems pretty horrible and wouldn't work for us anyway - looks like they > > > > remember which format was used to configure an area and use that as > > > > the output format...) > > > > > > > > > > Maybe it's better when we just allow both input formats but don't change > > > any output. > > > > Any opinions or comments on this? I think this would be a valuable addition > > to ospfd. > > Yes, and diff is ok benno@ > David: ok remi@ for your diff without the printconf part. > What about ospf6d? I'll handle that. > > > > > > > Below diff changes ospfctl to accept the address and number format for > > > "ospfct show database area XXX". > > > > > > > > > Index: parser.c > > > === > > > RCS file: /cvs/src/usr.sbin/ospfctl/parser.c,v > > > retrieving revision 1.20 > > > diff -u -p -r1.20 parser.c > > > --- parser.c 9 May 2011 12:25:35 - 1.20 > > > +++ parser.c 30 Apr 2019 20:28:18 - > > > @@ -39,7 +39,8 @@ enum token_type { > > > ADDRESS, > > > FLAG, > > > PREFIX, > > > - IFNAME > > > + IFNAME, > > > + AREA > > > }; > > > > > > struct token { > > > @@ -107,7 +108,7 @@ static const struct token t_show_db[] = > > > }; > > > > > > static const struct token t_show_area[] = { > > > - {ADDRESS, "", NONE, NULL}, > > > + {AREA, "", NONE, NULL}, > > > {ENDTOKEN, "", NONE, NULL} > > > }; > > > > > > @@ -218,6 +219,14 @@ match_token(const char *word, const stru > > > res->action = t->value; > > > } > > > break; > > > + case AREA: > > > + if (parse_area(word, &res->addr)) { > > > +
Re: ospfd: allow specifying area by number as well as id
Remi Locherer(remi.loche...@relo.ch) on 2019.05.15 23:15:03 +0200: > On Tue, Apr 30, 2019 at 11:10:37PM +0200, Remi Locherer wrote: > > On Mon, Apr 29, 2019 at 11:10:31AM +0100, Stuart Henderson wrote: > > > On 2019/04/29 11:58, Sebastian Benoit wrote: > > > > David Gwynne(da...@gwynne.id.au) on 2019.04.29 19:36:51 +1000: > > > > > > > > > > > > > > > > On 29 Apr 2019, at 4:59 pm, Remi Locherer > > > > > > wrote: > > > > > > > > > > > > Hi David > > > > > > > > > > > > On Mon, Apr 29, 2019 at 11:53:27AM +1000, David Gwynne wrote: > > > > > >> it's always bothered me that i config areas on a crisco using a > > > > > >> number, > > > > > >> but then have to think hard to convert that number to an address > > > > > >> for use > > > > > >> in openbsd. eg, i was given area 700 in one place, which is > > > > > >> 0.0.2.188 > > > > > >> as an address. super annoying. > > > > > >> > > > > > >> so this changes the ospfd parser so it accepts both a number or > > > > > >> address. > > > > > >> i also changed it so it prints the number by default, which may be > > > > > >> contentious. the manpage is slightly tweaked too. > > > > > >> > > > > > >> thoughts? > > > > > > > > > > > > I like it to be able to use a number instead of an address! > > > > > > > > > > > > It worked fine in my short test I performed. > > > > > > > > > > > > The output with the comment looks a bit strange to me. > > > > > > > > > > Are you sure it doesn't look... awesome? > > > > > > > > I like it! > > > > > > I don't really, but if we change this it needs to be displayed somehow > > > and I don't have an idea to make it look nicer than this (cisco's method > > > seems pretty horrible and wouldn't work for us anyway - looks like they > > > remember which format was used to configure an area and use that as > > > the output format...) > > > > > > > Maybe it's better when we just allow both input formats but don't change > > any output. > > Any opinions or comments on this? I think this would be a valuable addition > to ospfd. Yes, and diff is ok benno@ What about ospf6d? > > > > Below diff changes ospfctl to accept the address and number format for > > "ospfct show database area XXX". > > > > > > Index: parser.c > > === > > RCS file: /cvs/src/usr.sbin/ospfctl/parser.c,v > > retrieving revision 1.20 > > diff -u -p -r1.20 parser.c > > --- parser.c9 May 2011 12:25:35 - 1.20 > > +++ parser.c30 Apr 2019 20:28:18 - > > @@ -39,7 +39,8 @@ enum token_type { > > ADDRESS, > > FLAG, > > PREFIX, > > - IFNAME > > + IFNAME, > > + AREA > > }; > > > > struct token { > > @@ -107,7 +108,7 @@ static const struct token t_show_db[] = > > }; > > > > static const struct token t_show_area[] = { > > - {ADDRESS, "", NONE, NULL}, > > + {AREA, "", NONE, NULL}, > > {ENDTOKEN, "", NONE, NULL} > > }; > > > > @@ -218,6 +219,14 @@ match_token(const char *word, const stru > > res->action = t->value; > > } > > break; > > + case AREA: > > + if (parse_area(word, &res->addr)) { > > + match++; > > + t = &table[i]; > > + if (t->value) > > + res->action = t->value; > > + } > > + break; > > case PREFIX: > > if (parse_prefix(word, &res->addr, &res->prefixlen)) { > > match++; > > @@ -274,6 +283,9 @@ show_valid_args(const struct token *tabl > > case ADDRESS: > > fprintf(stderr, " \n"); &
Re: ospfd: allow specifying area by number as well as id
On Wed, May 15, 2019 at 11:15:03PM +0200, Remi Locherer wrote: > Any opinions or comments on this? I think this would be a valuable addition > to ospfd. > I can't see any harm in it. OK denis@ > > > > Below diff changes ospfctl to accept the address and number format for > > "ospfct show database area XXX". > > > > > > Index: parser.c > > === > > RCS file: /cvs/src/usr.sbin/ospfctl/parser.c,v > > retrieving revision 1.20 > > diff -u -p -r1.20 parser.c > > --- parser.c9 May 2011 12:25:35 - 1.20 > > +++ parser.c30 Apr 2019 20:28:18 - > > @@ -39,7 +39,8 @@ enum token_type { > > ADDRESS, > > FLAG, > > PREFIX, > > - IFNAME > > + IFNAME, > > + AREA > > }; > > > > struct token { > > @@ -107,7 +108,7 @@ static const struct token t_show_db[] = > > }; > > > > static const struct token t_show_area[] = { > > - {ADDRESS, "", NONE, NULL}, > > + {AREA, "", NONE, NULL}, > > {ENDTOKEN, "", NONE, NULL} > > }; > > > > @@ -218,6 +219,14 @@ match_token(const char *word, const stru > > res->action = t->value; > > } > > break; > > + case AREA: > > + if (parse_area(word, &res->addr)) { > > + match++; > > + t = &table[i]; > > + if (t->value) > > + res->action = t->value; > > + } > > + break; > > case PREFIX: > > if (parse_prefix(word, &res->addr, &res->prefixlen)) { > > match++; > > @@ -274,6 +283,9 @@ show_valid_args(const struct token *tabl > > case ADDRESS: > > fprintf(stderr, " \n"); > > break; > > + case AREA: > > + fprintf(stderr, " \n"); > > + break; > > case PREFIX: > > fprintf(stderr, " [/]\n"); > > break; > > @@ -298,6 +310,32 @@ parse_addr(const char *word, struct in_a > > bzero(&ina, sizeof(ina)); > > > > if (inet_pton(AF_INET, word, &ina)) { > > + addr->s_addr = ina.s_addr; > > + return (1); > > + } > > + > > + return (0); > > +} > > + > > +int > > +parse_area(const char *word, struct in_addr *addr) > > +{ > > + struct in_addr ina; > > + const char *errstr; > > + > > + if (word == NULL) > > + return (0); > > + > > + bzero(addr, sizeof(struct in_addr)); > > + bzero(&ina, sizeof(ina)); > > + > > + if (inet_pton(AF_INET, word, &ina)) { > > + addr->s_addr = ina.s_addr; > > + return (1); > > + } > > + > > + ina.s_addr = htonl(strtonum(word, 0, 0x, &errstr)); > > + if (errstr == NULL) { > > addr->s_addr = ina.s_addr; > > return (1); > > } > > Index: parser.h > > === > > RCS file: /cvs/src/usr.sbin/ospfctl/parser.h,v > > retrieving revision 1.13 > > diff -u -p -r1.13 parser.h > > --- parser.h9 May 2011 12:25:35 - 1.13 > > +++ parser.h30 Apr 2019 20:28:52 - > > @@ -64,6 +64,7 @@ struct parse_result { > > > > struct parse_result*parse(int, char *[]); > > int parse_addr(const char *, struct in_addr *); > > +int parse_area(const char *, struct in_addr *); > > int parse_prefix(const char *, struct in_addr *, > > u_int8_t *); > > > > >
Re: ospfd: allow specifying area by number as well as id
On Tue, Apr 30, 2019 at 11:10:37PM +0200, Remi Locherer wrote: > On Mon, Apr 29, 2019 at 11:10:31AM +0100, Stuart Henderson wrote: > > On 2019/04/29 11:58, Sebastian Benoit wrote: > > > David Gwynne(da...@gwynne.id.au) on 2019.04.29 19:36:51 +1000: > > > > > > > > > > > > > On 29 Apr 2019, at 4:59 pm, Remi Locherer > > > > > wrote: > > > > > > > > > > Hi David > > > > > > > > > > On Mon, Apr 29, 2019 at 11:53:27AM +1000, David Gwynne wrote: > > > > >> it's always bothered me that i config areas on a crisco using a > > > > >> number, > > > > >> but then have to think hard to convert that number to an address for > > > > >> use > > > > >> in openbsd. eg, i was given area 700 in one place, which is 0.0.2.188 > > > > >> as an address. super annoying. > > > > >> > > > > >> so this changes the ospfd parser so it accepts both a number or > > > > >> address. > > > > >> i also changed it so it prints the number by default, which may be > > > > >> contentious. the manpage is slightly tweaked too. > > > > >> > > > > >> thoughts? > > > > > > > > > > I like it to be able to use a number instead of an address! > > > > > > > > > > It worked fine in my short test I performed. > > > > > > > > > > The output with the comment looks a bit strange to me. > > > > > > > > Are you sure it doesn't look... awesome? > > > > > > I like it! > > > > I don't really, but if we change this it needs to be displayed somehow > > and I don't have an idea to make it look nicer than this (cisco's method > > seems pretty horrible and wouldn't work for us anyway - looks like they > > remember which format was used to configure an area and use that as > > the output format...) > > > > Maybe it's better when we just allow both input formats but don't change > any output. Any opinions or comments on this? I think this would be a valuable addition to ospfd. > > Below diff changes ospfctl to accept the address and number format for > "ospfct show database area XXX". > > > Index: parser.c > === > RCS file: /cvs/src/usr.sbin/ospfctl/parser.c,v > retrieving revision 1.20 > diff -u -p -r1.20 parser.c > --- parser.c 9 May 2011 12:25:35 - 1.20 > +++ parser.c 30 Apr 2019 20:28:18 - > @@ -39,7 +39,8 @@ enum token_type { > ADDRESS, > FLAG, > PREFIX, > - IFNAME > + IFNAME, > + AREA > }; > > struct token { > @@ -107,7 +108,7 @@ static const struct token t_show_db[] = > }; > > static const struct token t_show_area[] = { > - {ADDRESS, "", NONE, NULL}, > + {AREA, "", NONE, NULL}, > {ENDTOKEN, "", NONE, NULL} > }; > > @@ -218,6 +219,14 @@ match_token(const char *word, const stru > res->action = t->value; > } > break; > + case AREA: > + if (parse_area(word, &res->addr)) { > + match++; > + t = &table[i]; > + if (t->value) > + res->action = t->value; > + } > + break; > case PREFIX: > if (parse_prefix(word, &res->addr, &res->prefixlen)) { > match++; > @@ -274,6 +283,9 @@ show_valid_args(const struct token *tabl > case ADDRESS: > fprintf(stderr, " \n"); > break; > + case AREA: > + fprintf(stderr, " \n"); > + break; > case PREFIX: > fprintf(stderr, " [/]\n"); > break; > @@ -298,6 +310,32 @@ parse_addr(const char *word, struct in_a > bzero(&ina, sizeof(ina)); > > if (inet_pton(AF_INET, word, &ina)) { > + addr->s_addr = ina.s_addr; > + return (1); > + } > + > + return (0); > +} > + &
Re: ospfd: do not change router-id on reload if unspecified
On Wed, May 15, 2019 at 03:52:57PM +0200, Denis Fondras wrote: > When router-id is unspecified, ospfd will choose the lowest IP address of the > host. I added an area and an IP lower than the existing ones and on reload > ospfd asked me to restart and did not activate the new area. > > Why would it update the router-id in such a case ? > > This diff changes this behaviour. When router-id is not explicitely changed, > keep the existing setting. makes sense to me. OK remi@ > > Index: ospfd.c > === > RCS file: /cvs/src/usr.sbin/ospfd/ospfd.c,v > retrieving revision 1.107 > diff -u -p -r1.107 ospfd.c > --- ospfd.c 26 Mar 2019 20:39:33 - 1.107 > +++ ospfd.c 15 May 2019 13:19:52 - > @@ -185,6 +185,8 @@ main(int argc, char *argv[]) > kif_clear(); > exit(1); > } > +if (ospfd_conf->rtr_id.s_addr == 0) > +ospfd_conf->rtr_id.s_addr = get_rtr_id(); > > if (sockname == NULL) { > if (asprintf(&sockname, "%s.%d", OSPFD_SOCKET, > @@ -641,6 +643,10 @@ ospf_reload(void) > > if ((xconf = parse_config(conffile, ospfd_conf->opts)) == NULL) > return (-1); > + > + /* No router-id was specified, keep existing value */ > +if (xconf->rtr_id.s_addr == 0) > +xconf->rtr_id.s_addr = ospfd_conf->rtr_id.s_addr; > > /* Abort the reload if rtr_id changed */ > if (ospfd_conf->rtr_id.s_addr != xconf->rtr_id.s_addr) { > Index: ospfd.h > === > RCS file: /cvs/src/usr.sbin/ospfd/ospfd.h,v > retrieving revision 1.103 > diff -u -p -r1.103 ospfd.h > --- ospfd.h 28 Dec 2018 19:25:10 - 1.103 > +++ ospfd.h 15 May 2019 13:19:52 - > @@ -561,6 +561,7 @@ intcarp_demote_set(char *, int); > > /* parse.y */ > struct ospfd_conf*parse_config(char *, int); > +u_int32_t get_rtr_id(void); > int cmdline_symset(char *); > void conf_clear_redist_list(struct redist_list *); > > Index: parse.y > === > RCS file: /cvs/src/usr.sbin/ospfd/parse.y,v > retrieving revision 1.96 > diff -u -p -r1.96 parse.y > --- parse.y 29 Apr 2019 05:14:38 - 1.96 > +++ parse.y 15 May 2019 13:19:52 - > @@ -83,7 +83,6 @@ int symset(const char *, const char *, > char *symget(const char *); > > void clear_config(struct ospfd_conf *xconf); > -u_int32_t get_rtr_id(void); > int host(const char *, struct in_addr *, struct in_addr *); > > static struct ospfd_conf *conf; > @@ -1253,9 +1252,6 @@ parse_config(char *filename, int opts) > clear_config(conf); > return (NULL); > } > - > - if (conf->rtr_id.s_addr == 0) > - conf->rtr_id.s_addr = get_rtr_id(); > > return (conf); > } >
ospfd: do not change router-id on reload if unspecified
When router-id is unspecified, ospfd will choose the lowest IP address of the host. I added an area and an IP lower than the existing ones and on reload ospfd asked me to restart and did not activate the new area. Why would it update the router-id in such a case ? This diff changes this behaviour. When router-id is not explicitely changed, keep the existing setting. Index: ospfd.c === RCS file: /cvs/src/usr.sbin/ospfd/ospfd.c,v retrieving revision 1.107 diff -u -p -r1.107 ospfd.c --- ospfd.c 26 Mar 2019 20:39:33 - 1.107 +++ ospfd.c 15 May 2019 13:19:52 - @@ -185,6 +185,8 @@ main(int argc, char *argv[]) kif_clear(); exit(1); } +if (ospfd_conf->rtr_id.s_addr == 0) +ospfd_conf->rtr_id.s_addr = get_rtr_id(); if (sockname == NULL) { if (asprintf(&sockname, "%s.%d", OSPFD_SOCKET, @@ -641,6 +643,10 @@ ospf_reload(void) if ((xconf = parse_config(conffile, ospfd_conf->opts)) == NULL) return (-1); + + /* No router-id was specified, keep existing value */ +if (xconf->rtr_id.s_addr == 0) +xconf->rtr_id.s_addr = ospfd_conf->rtr_id.s_addr; /* Abort the reload if rtr_id changed */ if (ospfd_conf->rtr_id.s_addr != xconf->rtr_id.s_addr) { Index: ospfd.h === RCS file: /cvs/src/usr.sbin/ospfd/ospfd.h,v retrieving revision 1.103 diff -u -p -r1.103 ospfd.h --- ospfd.h 28 Dec 2018 19:25:10 - 1.103 +++ ospfd.h 15 May 2019 13:19:52 - @@ -561,6 +561,7 @@ int carp_demote_set(char *, int); /* parse.y */ struct ospfd_conf *parse_config(char *, int); +u_int32_t get_rtr_id(void); int cmdline_symset(char *); voidconf_clear_redist_list(struct redist_list *); Index: parse.y === RCS file: /cvs/src/usr.sbin/ospfd/parse.y,v retrieving revision 1.96 diff -u -p -r1.96 parse.y --- parse.y 29 Apr 2019 05:14:38 - 1.96 +++ parse.y 15 May 2019 13:19:52 - @@ -83,7 +83,6 @@ intsymset(const char *, const char *, char *symget(const char *); voidclear_config(struct ospfd_conf *xconf); -u_int32_t get_rtr_id(void); int host(const char *, struct in_addr *, struct in_addr *); static struct ospfd_conf *conf; @@ -1253,9 +1252,6 @@ parse_config(char *filename, int opts) clear_config(conf); return (NULL); } - - if (conf->rtr_id.s_addr == 0) - conf->rtr_id.s_addr = get_rtr_id(); return (conf); }
Re: ospfd: allow specifying area by number as well as id
On Mon, Apr 29, 2019 at 11:10:31AM +0100, Stuart Henderson wrote: > On 2019/04/29 11:58, Sebastian Benoit wrote: > > David Gwynne(da...@gwynne.id.au) on 2019.04.29 19:36:51 +1000: > > > > > > > > > > On 29 Apr 2019, at 4:59 pm, Remi Locherer wrote: > > > > > > > > Hi David > > > > > > > > On Mon, Apr 29, 2019 at 11:53:27AM +1000, David Gwynne wrote: > > > >> it's always bothered me that i config areas on a crisco using a number, > > > >> but then have to think hard to convert that number to an address for > > > >> use > > > >> in openbsd. eg, i was given area 700 in one place, which is 0.0.2.188 > > > >> as an address. super annoying. > > > >> > > > >> so this changes the ospfd parser so it accepts both a number or > > > >> address. > > > >> i also changed it so it prints the number by default, which may be > > > >> contentious. the manpage is slightly tweaked too. > > > >> > > > >> thoughts? > > > > > > > > I like it to be able to use a number instead of an address! > > > > > > > > It worked fine in my short test I performed. > > > > > > > > The output with the comment looks a bit strange to me. > > > > > > Are you sure it doesn't look... awesome? > > > > I like it! > > I don't really, but if we change this it needs to be displayed somehow > and I don't have an idea to make it look nicer than this (cisco's method > seems pretty horrible and wouldn't work for us anyway - looks like they > remember which format was used to configure an area and use that as > the output format...) > Maybe it's better when we just allow both input formats but don't change any output. Below diff changes ospfctl to accept the address and number format for "ospfct show database area XXX". Index: parser.c === RCS file: /cvs/src/usr.sbin/ospfctl/parser.c,v retrieving revision 1.20 diff -u -p -r1.20 parser.c --- parser.c9 May 2011 12:25:35 - 1.20 +++ parser.c30 Apr 2019 20:28:18 - @@ -39,7 +39,8 @@ enum token_type { ADDRESS, FLAG, PREFIX, - IFNAME + IFNAME, + AREA }; struct token { @@ -107,7 +108,7 @@ static const struct token t_show_db[] = }; static const struct token t_show_area[] = { - {ADDRESS, "", NONE, NULL}, + {AREA, "", NONE, NULL}, {ENDTOKEN, "", NONE, NULL} }; @@ -218,6 +219,14 @@ match_token(const char *word, const stru res->action = t->value; } break; + case AREA: + if (parse_area(word, &res->addr)) { + match++; + t = &table[i]; + if (t->value) + res->action = t->value; + } + break; case PREFIX: if (parse_prefix(word, &res->addr, &res->prefixlen)) { match++; @@ -274,6 +283,9 @@ show_valid_args(const struct token *tabl case ADDRESS: fprintf(stderr, " \n"); break; + case AREA: + fprintf(stderr, " \n"); + break; case PREFIX: fprintf(stderr, " [/]\n"); break; @@ -298,6 +310,32 @@ parse_addr(const char *word, struct in_a bzero(&ina, sizeof(ina)); if (inet_pton(AF_INET, word, &ina)) { + addr->s_addr = ina.s_addr; + return (1); + } + + return (0); +} + +int +parse_area(const char *word, struct in_addr *addr) +{ + struct in_addr ina; + const char *errstr; + + if (word == NULL) + return (0); + + bzero(addr, sizeof(struct in_addr)); + bzero(&ina, sizeof(ina)); + + if (inet_pton(AF_INET, word, &ina)) { + addr->s_addr = ina.s_addr; + return (1); + } + + ina.s_addr = htonl(strtonum(word, 0, 0x, &errstr)); + if (errstr == NULL) { addr->s_addr = ina.s_addr; return (1); } Index: parser.h === RCS file: /cvs/src/usr.sbin/ospfctl/parser.h,v retrieving revision 1.13 diff -u -p -r1.13 parser.h --- parser.h9 May 2011 12:25:35 - 1.13 +++ parser.h30 Apr 2019 20:28:52 - @@ -64,6 +64,7 @@ struct parse_result { struct parse_result*parse(int, char *[]); int parse_addr(const char *, struct in_addr *); +int parse_area(const char *, struct in_addr *); int parse_prefix(const char *, struct in_addr *, u_int8_t *);
Re: ospfd: allow specifying area by number as well as id
On 29.04.19 04:53, David Gwynne wrote: it's always bothered me that i config areas on a crisco using a number, but then have to think hard to convert that number to an address for use in openbsd. eg, i was given area 700 in one place, which is 0.0.2.188 as an address. super annoying. so this changes the ospfd parser so it accepts both a number or address. i also changed it so it prints the number by default, which may be contentious. the manpage is slightly tweaked too. thoughts? why don't just add something like 'convert' to ospfctl? wouldn't it be easier? $ ospfctl convert 700 0.0.2.188 $ ospfctl convert 0.0.2.188 700
Re: ospfd: allow specifying area by number as well as id
On Mon, Apr 29, 2019 at 11:53:27AM +1000, David Gwynne wrote: > it's always bothered me that i config areas on a crisco using a number, > but then have to think hard to convert that number to an address for use > in openbsd. eg, i was given area 700 in one place, which is 0.0.2.188 > as an address. super annoying. I don't use Cisco and I don't use numbers. Fine to me if you make ospfd.conf more flexible. The only thing about notation I found in RFC 2328 is: The OSPF backbone is the special OSPF Area 0 (often written as Area 0.0.0.0, since OSPF Area ID's are typically formatted as IP addresses). > so this changes the ospfd parser so it accepts both a number or address. > i also changed it so it prints the number by default, which may be > contentious. the manpage is slightly tweaked too. Please keep it as it is. ospfctl show database uses the dot notation everywhere. It would break my scripts if you change that. bluhm
Re: ospfd: allow specifying area by number as well as id
On 2019/04/29 11:58, Sebastian Benoit wrote: > David Gwynne(da...@gwynne.id.au) on 2019.04.29 19:36:51 +1000: > > > > > > > On 29 Apr 2019, at 4:59 pm, Remi Locherer wrote: > > > > > > Hi David > > > > > > On Mon, Apr 29, 2019 at 11:53:27AM +1000, David Gwynne wrote: > > >> it's always bothered me that i config areas on a crisco using a number, > > >> but then have to think hard to convert that number to an address for use > > >> in openbsd. eg, i was given area 700 in one place, which is 0.0.2.188 > > >> as an address. super annoying. > > >> > > >> so this changes the ospfd parser so it accepts both a number or address. > > >> i also changed it so it prints the number by default, which may be > > >> contentious. the manpage is slightly tweaked too. > > >> > > >> thoughts? > > > > > > I like it to be able to use a number instead of an address! > > > > > > It worked fine in my short test I performed. > > > > > > The output with the comment looks a bit strange to me. > > > > Are you sure it doesn't look... awesome? > > I like it! I don't really, but if we change this it needs to be displayed somehow and I don't have an idea to make it look nicer than this (cisco's method seems pretty horrible and wouldn't work for us anyway - looks like they remember which format was used to configure an area and use that as the output format...)
Re: ospfd: allow specifying area by number as well as id
David Gwynne(da...@gwynne.id.au) on 2019.04.29 19:36:51 +1000: > > > > On 29 Apr 2019, at 4:59 pm, Remi Locherer wrote: > > > > Hi David > > > > On Mon, Apr 29, 2019 at 11:53:27AM +1000, David Gwynne wrote: > >> it's always bothered me that i config areas on a crisco using a number, > >> but then have to think hard to convert that number to an address for use > >> in openbsd. eg, i was given area 700 in one place, which is 0.0.2.188 > >> as an address. super annoying. > >> > >> so this changes the ospfd parser so it accepts both a number or address. > >> i also changed it so it prints the number by default, which may be > >> contentious. the manpage is slightly tweaked too. > >> > >> thoughts? > > > > I like it to be able to use a number instead of an address! > > > > It worked fine in my short test I performed. > > > > The output with the comment looks a bit strange to me. > > Are you sure it doesn't look... awesome? I like it! > > > typhoon ..sbin/ospfd$ doas obj/ospfd -nv > > > > router-id 0.0.0.7 > > fib-update yes > > fib-priority 32 > > rfc1583compat no > > spf-delay msec 1000 > > spf-holdtime msec 5000 > > > > area 7 { # 0.0.0.7 > > ^ > >interface pair7:10.77.77.1 { > >metric 10 > >retransmit-interval 5 > >router-dead-time 40 > > > > > > I'd prefer if we settle for one output format and then use only that. The > > number format is more common but that would be a change for the users. I'm > > fine with either format for outputs. > > I lean toward the number too. I don't think it would hurt to change it so > only one is output, so long input works either way. We need a way to show both: to make migration easier, and to avoid the same problem you encountered when entering the area in some other equipment. I dont care if thats in the config printer or in ospfctl output though. > > > There is also "ospfctl show database area 0.0.0.0" and ospf6d. ;-) > > Are you offering to help with the implementation of those? > > dlg > > > > > Regards, > > Remi > > > > > >> > >> with this diff, i can do the following and things keep > >> working: > >> > >> --- /etc/ospfd.confMon Apr 29 11:29:56 2019 > >> +++ /etc/ospfd.conf.newMon Apr 29 11:39:45 2019 > >> @@ -7,5 +7,5 @@ > >> redistribute rtlabel "backup" set metric 65535 > >> > >> -area 0.0.2.188 { > >> +area 700 { > >>router-dead-time minimal > >>fast-hello-interval msec 300 > >> > >> Index: ospfd.conf.5 > >> === > >> RCS file: /cvs/src/usr.sbin/ospfd/ospfd.conf.5,v > >> retrieving revision 1.55 > >> diff -u -p -r1.55 ospfd.conf.5 > >> --- ospfd.conf.5 28 Dec 2018 19:25:10 - 1.55 > >> +++ ospfd.conf.5 29 Apr 2019 01:45:40 - > >> @@ -68,7 +68,7 @@ Macros are not expanded inside quotes. > >> For example: > >> .Bd -literal -offset indent > >> hi="5" > >> -area 0.0.0.0 { > >> +area 0 { > >> interface em0 { > >>hello-interval $hi > >>} > >> @@ -257,10 +257,10 @@ Areas are used for grouping interfaces. > >> All interface-specific parameters can > >> be configured per area, overruling the global settings. > >> .Bl -tag -width Ds > >> -.It Ic area Ar address > >> +.It Ic area Ar id Ns | Ns Ar address > >> Specify an area section, grouping one or more interfaces. > >> .Bd -literal -offset indent > >> -area 0.0.0.0 { > >> +area 0 { > >>interface em0 > >>interface em1 { > >>metric 10 > >> Index: parse.y > >> === > >> RCS file: /cvs/src/usr.sbin/ospfd/parse.y,v > >> retrieving revision 1.95 > >> diff -u -p -r1.95 parse.y > >> --- parse.y13 Feb 2019 22:57:08 - 1.95 > >> +++ parse.y29 Apr 2019 01:45:40 - > >> @@ -120,6 +120,7 @@ typedef struct { > >>int64_t number; > >>char*string; > >>struct redistribute *redist; > >> + struct in_addr id; > >>} v; > >>int lineno; >
Re: ospfd: allow specifying area by number as well as id
> On 29 Apr 2019, at 4:59 pm, Remi Locherer wrote: > > Hi David > > On Mon, Apr 29, 2019 at 11:53:27AM +1000, David Gwynne wrote: >> it's always bothered me that i config areas on a crisco using a number, >> but then have to think hard to convert that number to an address for use >> in openbsd. eg, i was given area 700 in one place, which is 0.0.2.188 >> as an address. super annoying. >> >> so this changes the ospfd parser so it accepts both a number or address. >> i also changed it so it prints the number by default, which may be >> contentious. the manpage is slightly tweaked too. >> >> thoughts? > > I like it to be able to use a number instead of an address! > > It worked fine in my short test I performed. > > The output with the comment looks a bit strange to me. Are you sure it doesn't look... awesome? > typhoon ..sbin/ospfd$ doas obj/ospfd -nv > > router-id 0.0.0.7 > fib-update yes > fib-priority 32 > rfc1583compat no > spf-delay msec 1000 > spf-holdtime msec 5000 > > area 7 { # 0.0.0.7 > ^ >interface pair7:10.77.77.1 { >metric 10 >retransmit-interval 5 >router-dead-time 40 > > > I'd prefer if we settle for one output format and then use only that. The > number format is more common but that would be a change for the users. I'm > fine with either format for outputs. I lean toward the number too. I don't think it would hurt to change it so only one is output, so long input works either way. > There is also "ospfctl show database area 0.0.0.0" and ospf6d. ;-) Are you offering to help with the implementation of those? dlg > > Regards, > Remi > > >> >> with this diff, i can do the following and things keep >> working: >> >> --- /etc/ospfd.conf Mon Apr 29 11:29:56 2019 >> +++ /etc/ospfd.conf.new Mon Apr 29 11:39:45 2019 >> @@ -7,5 +7,5 @@ >> redistribute rtlabel "backup" set metric 65535 >> >> -area 0.0.2.188 { >> +area 700 { >> router-dead-time minimal >> fast-hello-interval msec 300 >> >> Index: ospfd.conf.5 >> === >> RCS file: /cvs/src/usr.sbin/ospfd/ospfd.conf.5,v >> retrieving revision 1.55 >> diff -u -p -r1.55 ospfd.conf.5 >> --- ospfd.conf.5 28 Dec 2018 19:25:10 - 1.55 >> +++ ospfd.conf.5 29 Apr 2019 01:45:40 - >> @@ -68,7 +68,7 @@ Macros are not expanded inside quotes. >> For example: >> .Bd -literal -offset indent >> hi="5" >> -area 0.0.0.0 { >> +area 0 { >> interface em0 { >> hello-interval $hi >> } >> @@ -257,10 +257,10 @@ Areas are used for grouping interfaces. >> All interface-specific parameters can >> be configured per area, overruling the global settings. >> .Bl -tag -width Ds >> -.It Ic area Ar address >> +.It Ic area Ar id Ns | Ns Ar address >> Specify an area section, grouping one or more interfaces. >> .Bd -literal -offset indent >> -area 0.0.0.0 { >> +area 0 { >> interface em0 >> interface em1 { >> metric 10 >> Index: parse.y >> === >> RCS file: /cvs/src/usr.sbin/ospfd/parse.y,v >> retrieving revision 1.95 >> diff -u -p -r1.95 parse.y >> --- parse.y 13 Feb 2019 22:57:08 - 1.95 >> +++ parse.y 29 Apr 2019 01:45:40 - >> @@ -120,6 +120,7 @@ typedef struct { >> int64_t number; >> char*string; >> struct redistribute *redist; >> +struct in_addr id; >> } v; >> int lineno; >> } YYSTYPE; >> @@ -145,6 +146,7 @@ typedef struct { >> %type deadtime >> %type string dependon >> %type redistribute >> +%type areaid >> >> %% >> >> @@ -588,15 +590,8 @@ comma : ',' >> | /*empty*/ >> ; >> >> -area: AREA STRING { >> -struct in_addr id; >> -if (inet_aton($2, &id) == 0) { >> -yyerror("error parsing area"); >> -free($2); >> -YYERROR; >> -} >> -free($2); >> -area = conf_get_area(id); >> +area
Re: ospfd: allow specifying area by number as well as id
Hi David On Mon, Apr 29, 2019 at 11:53:27AM +1000, David Gwynne wrote: > it's always bothered me that i config areas on a crisco using a number, > but then have to think hard to convert that number to an address for use > in openbsd. eg, i was given area 700 in one place, which is 0.0.2.188 > as an address. super annoying. > > so this changes the ospfd parser so it accepts both a number or address. > i also changed it so it prints the number by default, which may be > contentious. the manpage is slightly tweaked too. > > thoughts? I like it to be able to use a number instead of an address! It worked fine in my short test I performed. The output with the comment looks a bit strange to me. typhoon ..sbin/ospfd$ doas obj/ospfd -nv router-id 0.0.0.7 fib-update yes fib-priority 32 rfc1583compat no spf-delay msec 1000 spf-holdtime msec 5000 area 7 { # 0.0.0.7 ^ interface pair7:10.77.77.1 { metric 10 retransmit-interval 5 router-dead-time 40 I'd prefer if we settle for one output format and then use only that. The number format is more common but that would be a change for the users. I'm fine with either format for outputs. There is also "ospfctl show database area 0.0.0.0" and ospf6d. ;-) Regards, Remi > > with this diff, i can do the following and things keep > working: > > --- /etc/ospfd.conf Mon Apr 29 11:29:56 2019 > +++ /etc/ospfd.conf.new Mon Apr 29 11:39:45 2019 > @@ -7,5 +7,5 @@ > redistribute rtlabel "backup" set metric 65535 > > -area 0.0.2.188 { > +area 700 { > router-dead-time minimal > fast-hello-interval msec 300 > > Index: ospfd.conf.5 > === > RCS file: /cvs/src/usr.sbin/ospfd/ospfd.conf.5,v > retrieving revision 1.55 > diff -u -p -r1.55 ospfd.conf.5 > --- ospfd.conf.5 28 Dec 2018 19:25:10 - 1.55 > +++ ospfd.conf.5 29 Apr 2019 01:45:40 - > @@ -68,7 +68,7 @@ Macros are not expanded inside quotes. > For example: > .Bd -literal -offset indent > hi="5" > -area 0.0.0.0 { > +area 0 { > interface em0 { > hello-interval $hi > } > @@ -257,10 +257,10 @@ Areas are used for grouping interfaces. > All interface-specific parameters can > be configured per area, overruling the global settings. > .Bl -tag -width Ds > -.It Ic area Ar address > +.It Ic area Ar id Ns | Ns Ar address > Specify an area section, grouping one or more interfaces. > .Bd -literal -offset indent > -area 0.0.0.0 { > +area 0 { > interface em0 > interface em1 { > metric 10 > Index: parse.y > === > RCS file: /cvs/src/usr.sbin/ospfd/parse.y,v > retrieving revision 1.95 > diff -u -p -r1.95 parse.y > --- parse.y 13 Feb 2019 22:57:08 - 1.95 > +++ parse.y 29 Apr 2019 01:45:40 - > @@ -120,6 +120,7 @@ typedef struct { > int64_t number; > char*string; > struct redistribute *redist; > + struct in_addr id; > } v; > int lineno; > } YYSTYPE; > @@ -145,6 +146,7 @@ typedef struct { > %type deadtime > %type string dependon > %type redistribute > +%type areaid > > %% > > @@ -588,15 +590,8 @@ comma: ',' > | /*empty*/ > ; > > -area : AREA STRING { > - struct in_addr id; > - if (inet_aton($2, &id) == 0) { > - yyerror("error parsing area"); > - free($2); > - YYERROR; > - } > - free($2); > - area = conf_get_area(id); > +area : AREA areaid { > + area = conf_get_area($2); > > memcpy(&areadefs, defs, sizeof(areadefs)); > md_list_copy(&areadefs.md_list, &defs->md_list); > @@ -610,6 +605,23 @@ area : AREA STRING { > > demotecount : NUMBER{ $$ = $1; } > | /*empty*/ { $$ = 1; } > + ; > + > +areaid : NUMBER { > + if ($1 < 0 || $1 > 0x) { > + yyerror("invalid area id"); > + YYERROR; > + } > + $$.s_addr = htonl($1); > + } > + | STRING { > +
ospfd: allow specifying area by number as well as id
it's always bothered me that i config areas on a crisco using a number, but then have to think hard to convert that number to an address for use in openbsd. eg, i was given area 700 in one place, which is 0.0.2.188 as an address. super annoying. so this changes the ospfd parser so it accepts both a number or address. i also changed it so it prints the number by default, which may be contentious. the manpage is slightly tweaked too. thoughts? with this diff, i can do the following and things keep working: --- /etc/ospfd.conf Mon Apr 29 11:29:56 2019 +++ /etc/ospfd.conf.new Mon Apr 29 11:39:45 2019 @@ -7,5 +7,5 @@ redistribute rtlabel "backup" set metric 65535 -area 0.0.2.188 { +area 700 { router-dead-time minimal fast-hello-interval msec 300 Index: ospfd.conf.5 === RCS file: /cvs/src/usr.sbin/ospfd/ospfd.conf.5,v retrieving revision 1.55 diff -u -p -r1.55 ospfd.conf.5 --- ospfd.conf.528 Dec 2018 19:25:10 - 1.55 +++ ospfd.conf.529 Apr 2019 01:45:40 - @@ -68,7 +68,7 @@ Macros are not expanded inside quotes. For example: .Bd -literal -offset indent hi="5" -area 0.0.0.0 { +area 0 { interface em0 { hello-interval $hi } @@ -257,10 +257,10 @@ Areas are used for grouping interfaces. All interface-specific parameters can be configured per area, overruling the global settings. .Bl -tag -width Ds -.It Ic area Ar address +.It Ic area Ar id Ns | Ns Ar address Specify an area section, grouping one or more interfaces. .Bd -literal -offset indent -area 0.0.0.0 { +area 0 { interface em0 interface em1 { metric 10 Index: parse.y === RCS file: /cvs/src/usr.sbin/ospfd/parse.y,v retrieving revision 1.95 diff -u -p -r1.95 parse.y --- parse.y 13 Feb 2019 22:57:08 - 1.95 +++ parse.y 29 Apr 2019 01:45:40 - @@ -120,6 +120,7 @@ typedef struct { int64_t number; char*string; struct redistribute *redist; + struct in_addr id; } v; int lineno; } YYSTYPE; @@ -145,6 +146,7 @@ typedef struct { %typedeadtime %typestring dependon %typeredistribute +%typeareaid %% @@ -588,15 +590,8 @@ comma : ',' | /*empty*/ ; -area : AREA STRING { - struct in_addr id; - if (inet_aton($2, &id) == 0) { - yyerror("error parsing area"); - free($2); - YYERROR; - } - free($2); - area = conf_get_area(id); +area : AREA areaid { + area = conf_get_area($2); memcpy(&areadefs, defs, sizeof(areadefs)); md_list_copy(&areadefs.md_list, &defs->md_list); @@ -610,6 +605,23 @@ area : AREA STRING { demotecount: NUMBER{ $$ = $1; } | /*empty*/ { $$ = 1; } + ; + +areaid : NUMBER { + if ($1 < 0 || $1 > 0x) { + yyerror("invalid area id"); + YYERROR; + } + $$.s_addr = htonl($1); + } + | STRING { + if (inet_aton($1, &$$) == 0) { + yyerror("error parsing area"); + free($1); + YYERROR; + } + free($1); + } ; areaopts_l : areaopts_l areaoptsl nl 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 - 1.20 +++ printconf.c 29 Apr 2019 01:45:40 - @@ -181,7 +181,8 @@ print_config(struct ospfd_conf *conf) printf("\n"); LIST_FOREACH(area, &conf->area_list, entry) { - printf("area %s {\n", inet_ntoa(area->id)); + printf("area %u { # %s\n", ntohl(area->id.s_addr), + inet_ntoa(area->id)); if (area->stub) { printf("\tstub"); if (SIMPLEQ_EMPTY(&area->redist_list))
Re: ospfd: Apply netmask to stub prefixes before adding the route to the route table
On Thu, Apr 04, 2019 at 05:29:40PM +0200, Remi Locherer wrote: > On Tue, Apr 02, 2019 at 07:27:07PM +1000, Mitchell Krome wrote: > > On 2/04/2019 3:30 pm, Remi Locherer wrote: > > > Hi Mitchell > > > > > > On Sat, Mar 30, 2019 at 04:10:09PM +1000, Mitchell Krome wrote: > > >> I kept finding I had a lingering /30 route when I turned off one of my > > >> test boxes. I tracked it down to ospfd sending RTM_ADD for a stub > > >> network with the non-masked prefix. The RTM_ADD path applies the mask > > >> inside the kernel, so the route got added as expected, but the > > >> RTM_DELETE enforces an exact match, so it could never remove the route. > > >> > > >> The advertised stub network was as follows: > > >> > > >> Link connected to: Stub Network > > >> Link ID (Network ID): 10.10.20.2 > > >> Link Data (Network Mask): 255.255.255.252 > > >> Metric: 10 > > > > > > Please send the details of your setup so it is easy to reproduce the > > > issue. > > > - OpenBSD version > > > - ospfd.conf > > > - interface configs > > > - routing table > > > > I am running a kernel I compiled myself with source from ~2 weeks ago. > > See the bottom for other info. > > > > > > > >> ospfd sends the interface address rather than network address as the > > >> link ID. The RFC says "set the Link ID of the Type 3 link to the > > >> subnet's IP address" which to me means we probably should also apply the > > >> mask before we add the stub to the LSA to avoid getting into this place > > >> to start with? > > > > > > This only applies to Type 3 LSAs. Below table is from RFC 2328 > > > chapter 12.1.4: > > > > > > LS Type Link State ID > > > ___ > > > 1 The originating router's Router ID. > > > 2 The IP interface address of the > > > network's Designated Router. > > > 3 The destination network's IP address. > > > 4 The Router ID of the described AS > > > boundary router. > > > 5 The destination network's IP address. > > > > > >> > > >> The patch below just masks the stub network before it gets added to the > > >> route table, so that we can properly delete it. I can send a patch to > > >> mask it before sending the LSA too if the consensus is that is how it > > >> should be. > > > > > > With your patch you change the case "LSA_TYPE_ROUTER" (LS Type 1) and not > > > LS type 3. > > > > Inside the LSA type 1 there is a type 3 link which is a "stub network". > > That is what I was changing. Under 12.4.1.1 second dotpoint it says for > > a point to point network add a type 3 link. Maybe I got the terminology > > wrong, but this was definitely the thing I intended to change > > > >Link type Description Link ID > >__ > >1 Point-to-pointNeighbor Router ID > >link > >2 Link to transit Interface address of > >network Designated Router > >3 Link to stub IP network number > > network > >4 Virtual link Neighbor Router ID > > > > > >Table 18: Link descriptions in the > > router-LSA. > > > > > > Thank you Mitchell for your analysis and great explanation! > > I think your proposed fix is correct. I never noticed this warning bevor > because I always used a /32 mask on point-to-point interfaces. > > Below again the diff from Mitchell. I tested this and it is OK remi@. Looks good to me OK claudio@ > > Index: rde_spf.c > === > RCS file: /cvs/src/usr.sbin/ospfd/rde_spf.c,v > retrieving revision 1.76 > diff -u -p -r1.76 rde_spf.c > --- rde_spf.c 22 Nov 2015 13:09:10 - 1.76 > +++ rde_spf.c 2 Apr 2019 20:13:40 - > @@ -195,7 +195,7 @@ rt_calc(struct vertex *v, struct area *a > if (rtr_link->type != LINK_TYPE_STUB_NET) > continue; > > - addr.s_addr = rtr_link->id; > + addr.s_addr = rtr_link->id & rtr_link->data; > adv_rtr.s_addr = htonl(v->adv_rtr); > > rt_update(addr, mask2prefixlen(rtr_link->data), > -- :wq Claudio
Re: ospfd: Apply netmask to stub prefixes before adding the route to the route table
On Tue, Apr 02, 2019 at 07:27:07PM +1000, Mitchell Krome wrote: > On 2/04/2019 3:30 pm, Remi Locherer wrote: > > Hi Mitchell > > > > On Sat, Mar 30, 2019 at 04:10:09PM +1000, Mitchell Krome wrote: > >> I kept finding I had a lingering /30 route when I turned off one of my > >> test boxes. I tracked it down to ospfd sending RTM_ADD for a stub > >> network with the non-masked prefix. The RTM_ADD path applies the mask > >> inside the kernel, so the route got added as expected, but the > >> RTM_DELETE enforces an exact match, so it could never remove the route. > >> > >> The advertised stub network was as follows: > >> > >> Link connected to: Stub Network > >>Link ID (Network ID): 10.10.20.2 > >> Link Data (Network Mask): 255.255.255.252 > >>Metric: 10 > > > > Please send the details of your setup so it is easy to reproduce the issue. > > - OpenBSD version > > - ospfd.conf > > - interface configs > > - routing table > > I am running a kernel I compiled myself with source from ~2 weeks ago. > See the bottom for other info. > > > > >> ospfd sends the interface address rather than network address as the > >> link ID. The RFC says "set the Link ID of the Type 3 link to the > >> subnet's IP address" which to me means we probably should also apply the > >> mask before we add the stub to the LSA to avoid getting into this place > >> to start with? > > > > This only applies to Type 3 LSAs. Below table is from RFC 2328 > > chapter 12.1.4: > > > > LS Type Link State ID > > ___ > > 1 The originating router's Router ID. > > 2 The IP interface address of the > > network's Designated Router. > > 3 The destination network's IP address. > > 4 The Router ID of the described AS > > boundary router. > > 5 The destination network's IP address. > > > >> > >> The patch below just masks the stub network before it gets added to the > >> route table, so that we can properly delete it. I can send a patch to > >> mask it before sending the LSA too if the consensus is that is how it > >> should be. > > > > With your patch you change the case "LSA_TYPE_ROUTER" (LS Type 1) and not > > LS type 3. > > Inside the LSA type 1 there is a type 3 link which is a "stub network". > That is what I was changing. Under 12.4.1.1 second dotpoint it says for > a point to point network add a type 3 link. Maybe I got the terminology > wrong, but this was definitely the thing I intended to change > >Link type Description Link ID >__ >1 Point-to-pointNeighbor Router ID >link >2 Link to transit Interface address of >network Designated Router >3 Link to stub IP network number >network >4 Virtual link Neighbor Router ID > > > Table 18: Link descriptions in the > router-LSA. > > Thank you Mitchell for your analysis and great explanation! I think your proposed fix is correct. I never noticed this warning bevor because I always used a /32 mask on point-to-point interfaces. Below again the diff from Mitchell. I tested this and it is OK remi@. Index: rde_spf.c === RCS file: /cvs/src/usr.sbin/ospfd/rde_spf.c,v retrieving revision 1.76 diff -u -p -r1.76 rde_spf.c --- rde_spf.c 22 Nov 2015 13:09:10 - 1.76 +++ rde_spf.c 2 Apr 2019 20:13:40 - @@ -195,7 +195,7 @@ rt_calc(struct vertex *v, struct area *a if (rtr_link->type != LINK_TYPE_STUB_NET) continue; - addr.s_addr = rtr_link->id; + addr.s_addr = rtr_link->id & rtr_link->data; adv_rtr.s_addr = htonl(v->adv_rtr); rt_update(addr, mask2prefixlen(rtr_link->data),
Re: ospfd: Apply netmask to stub prefixes before adding the route to the route table
On 2/04/2019 3:30 pm, Remi Locherer wrote: > Hi Mitchell > > On Sat, Mar 30, 2019 at 04:10:09PM +1000, Mitchell Krome wrote: >> I kept finding I had a lingering /30 route when I turned off one of my >> test boxes. I tracked it down to ospfd sending RTM_ADD for a stub >> network with the non-masked prefix. The RTM_ADD path applies the mask >> inside the kernel, so the route got added as expected, but the >> RTM_DELETE enforces an exact match, so it could never remove the route. >> >> The advertised stub network was as follows: >> >> Link connected to: Stub Network >> Link ID (Network ID): 10.10.20.2 >> Link Data (Network Mask): 255.255.255.252 >> Metric: 10 > > Please send the details of your setup so it is easy to reproduce the issue. > - OpenBSD version > - ospfd.conf > - interface configs > - routing table I am running a kernel I compiled myself with source from ~2 weeks ago. See the bottom for other info. > >> ospfd sends the interface address rather than network address as the >> link ID. The RFC says "set the Link ID of the Type 3 link to the >> subnet's IP address" which to me means we probably should also apply the >> mask before we add the stub to the LSA to avoid getting into this place >> to start with? > > This only applies to Type 3 LSAs. Below table is from RFC 2328 > chapter 12.1.4: > > LS Type Link State ID > ___ > 1 The originating router's Router ID. > 2 The IP interface address of the > network's Designated Router. > 3 The destination network's IP address. > 4 The Router ID of the described AS > boundary router. > 5 The destination network's IP address. > >> >> The patch below just masks the stub network before it gets added to the >> route table, so that we can properly delete it. I can send a patch to >> mask it before sending the LSA too if the consensus is that is how it >> should be. > > With your patch you change the case "LSA_TYPE_ROUTER" (LS Type 1) and not > LS type 3. Inside the LSA type 1 there is a type 3 link which is a "stub network". That is what I was changing. Under 12.4.1.1 second dotpoint it says for a point to point network add a type 3 link. Maybe I got the terminology wrong, but this was definitely the thing I intended to change Link type Description Link ID __ 1 Point-to-pointNeighbor Router ID link 2 Link to transit Interface address of network Designated Router 3 Link to stub IP network number network 4 Virtual link Neighbor Router ID Table 18: Link descriptions in the router-LSA. > > Remi > Box 1: openbsd1# cat /etc/hostname.lo1 inet 10.0.0.1/32 openbsd1# cat /etc/hostname.gre0 up mpls tunnel 10.10.10.1 10.10.10.2 tunneldomain 10 inet 10.10.20.1/30 dest 10.10.20.2 openbsd1# cat /etc/ospfd.conf area 0.0.0.0 { interface lo1 { passive } interface gre0 } Box 2: openbsd2# cat /etc/hostname.lo1 inet 10.0.0.2/32 openbsd2# cat /etc/hostname.gre0 up mpls tunnel 10.10.10.2 10.10.10.1 tunneldomain 10 inet 10.10.20.2/30 dest 10.10.20.1 openbsd2# cat /etc/hostname.gre1 up mpls tunnel 10.10.10.5 10.10.10.6 tunneldomain 10 inet 10.10.20.5/30 dest 10.10.20.6 openbsd2# cat /etc/ospfd.conf area 0.0.0.0 { interface lo1 { passive } interface gre0 interface gre1 } Box 3: openbsd3# cat /etc/hostname.lo1 inet 10.0.0.3/32 openbsd3# cat /etc/hostname.gre0 up mpls tunnel 10.10.10.6 10.10.10.5 tunneldomain 10 inet 10.10.20.6/30 dest 10.10.20.5 openbsd3# cat /etc/ospfd.conf area 0.0.0.0 { interface lo1 { passive } interface gre0 } 1: Box 1 has ospfd disabled. Route table on box 3: openbsd3# route show Routing tables Internet: DestinationGatewayFlags Refs Use Mtu Prio Iface 10.0.0.2/3210.10.20.5 UG 11 -32 gre0 10.0.0.3 10.0.0.3 UHl0 156 32768 1 lo1 10.10.20.0/30 10.10.20.5 UG 00 -32 gre0 10.10.20.5 10.10.20.6 UHh 5 431 - 8 gre0 10.10.20.6 10.10.20.5
Re: ospfd: Apply netmask to stub prefixes before adding the route to the route table
Hi Mitchell On Sat, Mar 30, 2019 at 04:10:09PM +1000, Mitchell Krome wrote: > I kept finding I had a lingering /30 route when I turned off one of my > test boxes. I tracked it down to ospfd sending RTM_ADD for a stub > network with the non-masked prefix. The RTM_ADD path applies the mask > inside the kernel, so the route got added as expected, but the > RTM_DELETE enforces an exact match, so it could never remove the route. > > The advertised stub network was as follows: > > Link connected to: Stub Network > Link ID (Network ID): 10.10.20.2 > Link Data (Network Mask): 255.255.255.252 > Metric: 10 Please send the details of your setup so it is easy to reproduce the issue. - OpenBSD version - ospfd.conf - interface configs - routing table > ospfd sends the interface address rather than network address as the > link ID. The RFC says "set the Link ID of the Type 3 link to the > subnet's IP address" which to me means we probably should also apply the > mask before we add the stub to the LSA to avoid getting into this place > to start with? This only applies to Type 3 LSAs. Below table is from RFC 2328 chapter 12.1.4: LS Type Link State ID ___ 1 The originating router's Router ID. 2 The IP interface address of the network's Designated Router. 3 The destination network's IP address. 4 The Router ID of the described AS boundary router. 5 The destination network's IP address. > > The patch below just masks the stub network before it gets added to the > route table, so that we can properly delete it. I can send a patch to > mask it before sending the LSA too if the consensus is that is how it > should be. With your patch you change the case "LSA_TYPE_ROUTER" (LS Type 1) and not LS type 3. Remi > > Mitchell > > diff --git usr.sbin/ospfd/rde_spf.c usr.sbin/ospfd/rde_spf.c > index 736f2e575..d842a2c20 100644 > --- usr.sbin/ospfd/rde_spf.c > +++ usr.sbin/ospfd/rde_spf.c > @@ -195,7 +195,7 @@ rt_calc(struct vertex *v, struct area *area, struct > ospfd_conf *conf) > if (rtr_link->type != LINK_TYPE_STUB_NET) > continue; > > - addr.s_addr = rtr_link->id; > + addr.s_addr = rtr_link->id & rtr_link->data; > adv_rtr.s_addr = htonl(v->adv_rtr); > > rt_update(addr, mask2prefixlen(rtr_link->data), >
ospfd: Apply netmask to stub prefixes before adding the route to the route table
I kept finding I had a lingering /30 route when I turned off one of my test boxes. I tracked it down to ospfd sending RTM_ADD for a stub network with the non-masked prefix. The RTM_ADD path applies the mask inside the kernel, so the route got added as expected, but the RTM_DELETE enforces an exact match, so it could never remove the route. The advertised stub network was as follows: Link connected to: Stub Network Link ID (Network ID): 10.10.20.2 Link Data (Network Mask): 255.255.255.252 Metric: 10 ospfd sends the interface address rather than network address as the link ID. The RFC says "set the Link ID of the Type 3 link to the subnet's IP address" which to me means we probably should also apply the mask before we add the stub to the LSA to avoid getting into this place to start with? The patch below just masks the stub network before it gets added to the route table, so that we can properly delete it. I can send a patch to mask it before sending the LSA too if the consensus is that is how it should be. Mitchell diff --git usr.sbin/ospfd/rde_spf.c usr.sbin/ospfd/rde_spf.c index 736f2e575..d842a2c20 100644 --- usr.sbin/ospfd/rde_spf.c +++ usr.sbin/ospfd/rde_spf.c @@ -195,7 +195,7 @@ rt_calc(struct vertex *v, struct area *area, struct ospfd_conf *conf) if (rtr_link->type != LINK_TYPE_STUB_NET) continue; - addr.s_addr = rtr_link->id; + addr.s_addr = rtr_link->id & rtr_link->data; adv_rtr.s_addr = htonl(v->adv_rtr); rt_update(addr, mask2prefixlen(rtr_link->data),
Re: ospfd: Warn when the router ID changes during config reload
On Mon, Mar 25 2019, Remi Locherer wrote: [...] > This works and it makes sense to me. > > The log message is a bit lengthy compared to other log messages produced > by ospfd. Maybe something like this: "router-id changed: restart required" Yep, fine with me. > But the patch is also OK remi@ as it is now. ok jca@ for your wording instead -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: ospfd: Warn when the router ID changes during config reload
On Mon, Mar 25, 2019 at 02:43:26PM +0100, Jeremie Courreges-Anglas wrote: > On Sun, Mar 24 2019, Mitchell Krome wrote: > > On 24/03/2019 7:23 am, Theo de Raadt wrote: > >> Sebastian Benoit wrote: > >> > >>> Mitchell Krome(mitchellkr...@gmail.com) on 2019.03.23 20:27:17 +1000: > >>>> Was messing around with ospf and got myself into a situation where the > >>>> router ID's were the same on two boxes because I only did a reload on > >>>> one of them when I changed the loopback IP's. > >>> > >>> Thats sub optimal i believe... > >>> > >>>> This adds a warning when reloading if the router ID changes (there was > >>>> already a comment saying as much). Same patch can probably be applied to > >>>> ospf6d if people think it's useful. > > ospf6d currently doesn't support config reloads at all. It might be > worth adding an XXX comment there. > > >>> I think it would be better to abort the reload if the router-id is > >>> changed, > >>> i.e. not load the new config at all. > >> > >> That's the right approach in all our other daemons: > >> > >> if the configuration change cannot be installed correctly, consider > >> it invalid and abort. Someone would need to write code to make it > >> valid.. > >> > > > > That makes sense. I checked the manuals for the routers I use at work > > and they also required the ospf process to be restarted for the config > > to take effect after changing the router id. > > > > I moved the check up into ospf_reload because it doesn't make sense > > sending the config to all the children if we know we're going to abort. > > Your patch was mangled (long line wrapped) but the changes looked good. > Here's an updated version which tweaks punctuation and case (to match > the router-id keyword). Works for me in my simple test setup. > > Comments/oks? This works and it makes sense to me. The log message is a bit lengthy compared to other log messages produced by ospfd. Maybe something like this: "router-id changed: restart required" But the patch is also OK remi@ as it is now. > > > Index: ospfd.c > === > RCS file: /cvs/src/usr.sbin/ospfd/ospfd.c,v > retrieving revision 1.105 > diff -u -p -r1.105 ospfd.c > --- ospfd.c 15 Jan 2019 22:18:10 - 1.105 > +++ ospfd.c 25 Mar 2019 13:33:43 - > @@ -642,6 +642,13 @@ ospf_reload(void) > if ((xconf = parse_config(conffile, ospfd_conf->opts)) == NULL) > return (-1); > > + /* Abort the reload if rtr_id changed */ > + if (ospfd_conf->rtr_id.s_addr != xconf->rtr_id.s_addr) { > + log_warnx("router-id changed in new configuration, " > + "this requires ospfd to be restarted."); > + return (-1); > + } > + > /* send config to childs */ > if (ospf_sendboth(IMSG_RECONF_CONF, xconf, sizeof(*xconf)) == -1) > return (-1); > @@ -693,7 +700,6 @@ merge_config(struct ospfd_conf *conf, st > struct redistribute *r; > int rchange = 0; > > - /* change of rtr_id needs a restart */ > conf->flags = xconf->flags; > conf->spf_delay = xconf->spf_delay; > conf->spf_hold_time = xconf->spf_hold_time; > > > -- > jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE >
Re: ospfd: Warn when the router ID changes during config reload
ok Jeremie Courreges-Anglas(j...@wxcvbn.org) on 2019.03.25 14:43:26 +0100: > On Sun, Mar 24 2019, Mitchell Krome wrote: > > On 24/03/2019 7:23 am, Theo de Raadt wrote: > >> Sebastian Benoit wrote: > >> > >>> Mitchell Krome(mitchellkr...@gmail.com) on 2019.03.23 20:27:17 +1000: > >>>> Was messing around with ospf and got myself into a situation where the > >>>> router ID's were the same on two boxes because I only did a reload on > >>>> one of them when I changed the loopback IP's. > >>> > >>> Thats sub optimal i believe... > >>> > >>>> This adds a warning when reloading if the router ID changes (there was > >>>> already a comment saying as much). Same patch can probably be applied to > >>>> ospf6d if people think it's useful. > > ospf6d currently doesn't support config reloads at all. It might be > worth adding an XXX comment there. > > >>> I think it would be better to abort the reload if the router-id is > >>> changed, > >>> i.e. not load the new config at all. > >> > >> That's the right approach in all our other daemons: > >> > >> if the configuration change cannot be installed correctly, consider > >> it invalid and abort. Someone would need to write code to make it > >> valid.. > >> > > > > That makes sense. I checked the manuals for the routers I use at work > > and they also required the ospf process to be restarted for the config > > to take effect after changing the router id. > > > > I moved the check up into ospf_reload because it doesn't make sense > > sending the config to all the children if we know we're going to abort. > > Your patch was mangled (long line wrapped) but the changes looked good. > Here's an updated version which tweaks punctuation and case (to match > the router-id keyword). Works for me in my simple test setup. > > Comments/oks? > > > Index: ospfd.c > === > RCS file: /cvs/src/usr.sbin/ospfd/ospfd.c,v > retrieving revision 1.105 > diff -u -p -r1.105 ospfd.c > --- ospfd.c 15 Jan 2019 22:18:10 - 1.105 > +++ ospfd.c 25 Mar 2019 13:33:43 - > @@ -642,6 +642,13 @@ ospf_reload(void) > if ((xconf = parse_config(conffile, ospfd_conf->opts)) == NULL) > return (-1); > > + /* Abort the reload if rtr_id changed */ > + if (ospfd_conf->rtr_id.s_addr != xconf->rtr_id.s_addr) { > + log_warnx("router-id changed in new configuration, " > + "this requires ospfd to be restarted."); > + return (-1); > + } > + > /* send config to childs */ > if (ospf_sendboth(IMSG_RECONF_CONF, xconf, sizeof(*xconf)) == -1) > return (-1); > @@ -693,7 +700,6 @@ merge_config(struct ospfd_conf *conf, st > struct redistribute *r; > int rchange = 0; > > - /* change of rtr_id needs a restart */ > conf->flags = xconf->flags; > conf->spf_delay = xconf->spf_delay; > conf->spf_hold_time = xconf->spf_hold_time; > > > -- > jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE >
Re: ospfd: Warn when the router ID changes during config reload
On Sun, Mar 24 2019, Mitchell Krome wrote: > On 24/03/2019 7:23 am, Theo de Raadt wrote: >> Sebastian Benoit wrote: >> >>> Mitchell Krome(mitchellkr...@gmail.com) on 2019.03.23 20:27:17 +1000: >>>> Was messing around with ospf and got myself into a situation where the >>>> router ID's were the same on two boxes because I only did a reload on >>>> one of them when I changed the loopback IP's. >>> >>> Thats sub optimal i believe... >>> >>>> This adds a warning when reloading if the router ID changes (there was >>>> already a comment saying as much). Same patch can probably be applied to >>>> ospf6d if people think it's useful. ospf6d currently doesn't support config reloads at all. It might be worth adding an XXX comment there. >>> I think it would be better to abort the reload if the router-id is changed, >>> i.e. not load the new config at all. >> >> That's the right approach in all our other daemons: >> >> if the configuration change cannot be installed correctly, consider >> it invalid and abort. Someone would need to write code to make it >> valid.. >> > > That makes sense. I checked the manuals for the routers I use at work > and they also required the ospf process to be restarted for the config > to take effect after changing the router id. > > I moved the check up into ospf_reload because it doesn't make sense > sending the config to all the children if we know we're going to abort. Your patch was mangled (long line wrapped) but the changes looked good. Here's an updated version which tweaks punctuation and case (to match the router-id keyword). Works for me in my simple test setup. Comments/oks? Index: ospfd.c === RCS file: /cvs/src/usr.sbin/ospfd/ospfd.c,v retrieving revision 1.105 diff -u -p -r1.105 ospfd.c --- ospfd.c 15 Jan 2019 22:18:10 - 1.105 +++ ospfd.c 25 Mar 2019 13:33:43 - @@ -642,6 +642,13 @@ ospf_reload(void) if ((xconf = parse_config(conffile, ospfd_conf->opts)) == NULL) return (-1); + /* Abort the reload if rtr_id changed */ + if (ospfd_conf->rtr_id.s_addr != xconf->rtr_id.s_addr) { + log_warnx("router-id changed in new configuration, " + "this requires ospfd to be restarted."); + return (-1); + } + /* send config to childs */ if (ospf_sendboth(IMSG_RECONF_CONF, xconf, sizeof(*xconf)) == -1) return (-1); @@ -693,7 +700,6 @@ merge_config(struct ospfd_conf *conf, st struct redistribute *r; int rchange = 0; - /* change of rtr_id needs a restart */ conf->flags = xconf->flags; conf->spf_delay = xconf->spf_delay; conf->spf_hold_time = xconf->spf_hold_time; -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: ospfd: Warn when the router ID changes during config reload
On 24/03/2019 7:23 am, Theo de Raadt wrote: > Sebastian Benoit wrote: > >> Mitchell Krome(mitchellkr...@gmail.com) on 2019.03.23 20:27:17 +1000: >>> Was messing around with ospf and got myself into a situation where the >>> router ID's were the same on two boxes because I only did a reload on >>> one of them when I changed the loopback IP's. >> >> Thats sub optimal i believe... >> >>> This adds a warning when reloading if the router ID changes (there was >>> already a comment saying as much). Same patch can probably be applied to >>> ospf6d if people think it's useful. >> >> I think it would be better to abort the reload if the router-id is changed, >> i.e. not load the new config at all. > > That's the right approach in all our other daemons: > > if the configuration change cannot be installed correctly, consider > it invalid and abort. Someone would need to write code to make it > valid.. > That makes sense. I checked the manuals for the routers I use at work and they also required the ospf process to be restarted for the config to take effect after changing the router id. I moved the check up into ospf_reload because it doesn't make sense sending the config to all the children if we know we're going to abort. Mitchell diff --git a/usr.sbin/ospfd/ospfd.c b/usr.sbin/ospfd/ospfd.c index d01a2fa66..bc2d007a7 100644 --- a/usr.sbin/ospfd/ospfd.c +++ b/usr.sbin/ospfd/ospfd.c @@ -642,6 +642,13 @@ ospf_reload(void) if ((xconf = parse_config(conffile, ospfd_conf->opts)) == NULL) return (-1); + /* Abort the reload if rtr_id changed */ + if (ospfd_conf->rtr_id.s_addr != xconf->rtr_id.s_addr) { + log_warnx("Router-ID changed in new configuration. This " + "requires ospfd to be restarted"); + return (-1); + } + /* send config to childs */ if (ospf_sendboth(IMSG_RECONF_CONF, xconf, sizeof(*xconf)) == -1) return (-1); @@ -693,7 +700,6 @@ merge_config(struct ospfd_conf *conf, struct ospfd_conf *xconf) struct redistribute *r; int rchange = 0; - /* change of rtr_id needs a restart */ conf->flags = xconf->flags; conf->spf_delay = xconf->spf_delay; conf->spf_hold_time = xconf->spf_hold_time;
Re: ospfd: Warn when the router ID changes during config reload
Sebastian Benoit wrote: > Mitchell Krome(mitchellkr...@gmail.com) on 2019.03.23 20:27:17 +1000: > > Was messing around with ospf and got myself into a situation where the > > router ID's were the same on two boxes because I only did a reload on > > one of them when I changed the loopback IP's. > > Thats sub optimal i believe... > > > This adds a warning when reloading if the router ID changes (there was > > already a comment saying as much). Same patch can probably be applied to > > ospf6d if people think it's useful. > > I think it would be better to abort the reload if the router-id is changed, > i.e. not load the new config at all. That's the right approach in all our other daemons: if the configuration change cannot be installed correctly, consider it invalid and abort. Someone would need to write code to make it valid..
Re: ospfd: Warn when the router ID changes during config reload
Mitchell Krome(mitchellkr...@gmail.com) on 2019.03.23 20:27:17 +1000: > Was messing around with ospf and got myself into a situation where the > router ID's were the same on two boxes because I only did a reload on > one of them when I changed the loopback IP's. Thats sub optimal i believe... > This adds a warning when reloading if the router ID changes (there was > already a comment saying as much). Same patch can probably be applied to > ospf6d if people think it's useful. I think it would be better to abort the reload if the router-id is changed, i.e. not load the new config at all. You just add a warning, but who reads the log at that point? ospf6d cant reload its config at this time, so not a problem there. /Benno > Mitchell > > > diff --git a/usr.sbin/ospfd/ospfd.c b/usr.sbin/ospfd/ospfd.c > index d01a2fa66..db59fc718 100644 > --- a/usr.sbin/ospfd/ospfd.c > +++ b/usr.sbin/ospfd/ospfd.c > @@ -694,6 +694,10 @@ merge_config(struct ospfd_conf *conf, struct > ospfd_conf *xconf) > int rchange = 0; > > /* change of rtr_id needs a restart */ > + if (conf->rtr_id.s_addr != xconf->rtr_id.s_addr && > + ospfd_process == PROC_MAIN) > + log_warnx("Router-ID changed in new configuration. This will " > + "not apply until ospfd is restarted."); > conf->flags = xconf->flags; > conf->spf_delay = xconf->spf_delay; > conf->spf_hold_time = xconf->spf_hold_time; >
ospfd: Warn when the router ID changes during config reload
Was messing around with ospf and got myself into a situation where the router ID's were the same on two boxes because I only did a reload on one of them when I changed the loopback IP's. This adds a warning when reloading if the router ID changes (there was already a comment saying as much). Same patch can probably be applied to ospf6d if people think it's useful. Mitchell diff --git a/usr.sbin/ospfd/ospfd.c b/usr.sbin/ospfd/ospfd.c index d01a2fa66..db59fc718 100644 --- a/usr.sbin/ospfd/ospfd.c +++ b/usr.sbin/ospfd/ospfd.c @@ -694,6 +694,10 @@ merge_config(struct ospfd_conf *conf, struct ospfd_conf *xconf) int rchange = 0; /* change of rtr_id needs a restart */ + if (conf->rtr_id.s_addr != xconf->rtr_id.s_addr && + ospfd_process == PROC_MAIN) + log_warnx("Router-ID changed in new configuration. This will " + "not apply until ospfd is restarted."); conf->flags = xconf->flags; conf->spf_delay = xconf->spf_delay; conf->spf_hold_time = xconf->spf_hold_time;
Re: ospfd: send router lsa when removing an interface
On Tue, Jan 01, 2019 at 10:30:55PM +0100, Remi Locherer wrote: > Hi tech, > > when removing an interface from ospdf.conf and doing a reload other > OSPF routers should get a router LSA update. Then they can remove the > affected route. But currently this does not happen. The affected route > might be used by other routers a long time after removing it from the > config (until the LSA ages out). > > Below diff fixes this. > > OK? Makes sense, OK claudio@ > Index: ospfd.c > ======= > RCS file: /cvs/src/usr.sbin/ospfd/ospfd.c,v > retrieving revision 1.102 > diff -u -p -r1.102 ospfd.c > --- ospfd.c 28 Dec 2018 19:25:10 - 1.102 > +++ ospfd.c 1 Jan 2019 21:23:38 - > @@ -827,7 +827,7 @@ merge_interfaces(struct area *a, struct > > /* problems: >* - new interfaces (easy) > - * - deleted interfaces (needs to be done via fsm?) > + * - deleted interfaces >* - changing passive (painful?) >*/ > for (i = LIST_FIRST(&a->iface_list); i != NULL; i = ni) { > @@ -842,6 +842,7 @@ merge_interfaces(struct area *a, struct > rde_nbr_iface_del(i); > LIST_REMOVE(i, entry); > if_del(i); > + dirty = 1; /* force rtr LSA update */ > } > } > > -- :wq Claudio
ospfd: send router lsa when removing an interface
Hi tech, when removing an interface from ospdf.conf and doing a reload other OSPF routers should get a router LSA update. Then they can remove the affected route. But currently this does not happen. The affected route might be used by other routers a long time after removing it from the config (until the LSA ages out). Below diff fixes this. OK? Remi Index: ospfd.c === RCS file: /cvs/src/usr.sbin/ospfd/ospfd.c,v retrieving revision 1.102 diff -u -p -r1.102 ospfd.c --- ospfd.c 28 Dec 2018 19:25:10 - 1.102 +++ ospfd.c 1 Jan 2019 21:23:38 - @@ -827,7 +827,7 @@ merge_interfaces(struct area *a, struct /* problems: * - new interfaces (easy) -* - deleted interfaces (needs to be done via fsm?) +* - deleted interfaces * - changing passive (painful?) */ for (i = LIST_FIRST(&a->iface_list); i != NULL; i = ni) { @@ -842,6 +842,7 @@ merge_interfaces(struct area *a, struct rde_nbr_iface_del(i); LIST_REMOVE(i, entry); if_del(i); + dirty = 1; /* force rtr LSA update */ } }
Re: ospfd: fib-priority
On Fri, Dec 28, 2018 at 02:32:54PM +0100, Remi Locherer wrote: > ping OK claudio@ > On Mon, Dec 10, 2018 at 10:40:22AM +0100, Remi Locherer wrote: > > Hi, > > > > below patch adds "fib-priority" to ospfd.conf which allows to set a > > custom priority to routes. 32 is still the default if not set. Changing > > the priority with a reload is also supported. > > > > A discussion about the feature can be found here: > > https://marc.info/?l=openbsd-tech&m=138360663119816&w=2 > > > > My first idea was to add an additional parameter to the functions that > > need it. But that that is not practical since then need the event that calls > > kr_dispatch_msg() needs to be reset. Because of that I added fib_prio to > > struct kr_state. > > > > > > OK? > > > > Remi > > > > > > > > cvs diff: Diffing . > > Index: kroute.c > > === > > RCS file: /cvs/src/usr.sbin/ospfd/kroute.c,v > > retrieving revision 1.111 > > diff -u -p -r1.111 kroute.c > > --- kroute.c10 Jul 2018 11:49:04 - 1.111 > > +++ kroute.c9 Dec 2018 21:39:46 - > > @@ -45,6 +45,7 @@ struct { > > pid_t pid; > > int fib_sync; > > int fib_serial; > > + u_int8_tfib_prio; > > int fd; > > struct eventev; > > struct eventreload; > > @@ -127,14 +128,15 @@ kif_init(void) > > } > > > > int > > -kr_init(int fs, u_int rdomain, int redis_label_or_prefix) > > +kr_init(int fs, u_int rdomain, int redis_label_or_prefix, u_int8_t > > fib_prio) > > { > > int opt = 0, rcvbuf, default_rcvbuf; > > socklen_t optlen; > > - int filter_prio = RTP_OSPF; > > + int filter_prio = fib_prio; > > > > kr_state.fib_sync = fs; > > kr_state.rdomain = rdomain; > > + kr_state.fib_prio = fib_prio; > > > > if ((kr_state.fd = socket(AF_ROUTE, > > SOCK_RAW | SOCK_CLOEXEC | SOCK_NONBLOCK, AF_INET)) == -1) { > > @@ -262,7 +264,7 @@ kr_change_fib(struct kroute_node *kr, st > > kn->r.prefixlen = kroute[i].prefixlen; > > kn->r.nexthop.s_addr = kroute[i].nexthop.s_addr; > > kn->r.flags = kroute[i].flags | F_OSPFD_INSERTED; > > - kn->r.priority = RTP_OSPF; > > + kn->r.priority = kr_state.fib_prio; > > kn->r.ext_tag = kroute[i].ext_tag; > > rtlabel_unref(kn->r.rtlabel); /* for RTM_CHANGE */ > > kn->r.rtlabel = kroute[i].rtlabel; > > @@ -286,7 +288,8 @@ kr_change(struct kroute *kroute, int krc > > > > kroute->rtlabel = rtlabel_tag2id(kroute->ext_tag); > > > > - kr = kroute_find(kroute->prefix.s_addr, kroute->prefixlen, RTP_OSPF); > > + kr = kroute_find(kroute->prefix.s_addr, kroute->prefixlen, > > + kr_state.fib_prio); > > if (kr != NULL && kr->next == NULL && krcount == 1) > > /* single path OSPF route */ > > action = RTM_CHANGE; > > @@ -297,7 +300,7 @@ kr_change(struct kroute *kroute, int krc > > int > > kr_delete_fib(struct kroute_node *kr) > > { > > - if (kr->r.priority != RTP_OSPF) > > + if (kr->r.priority != kr_state.fib_prio) > > log_warn("kr_delete_fib: %s/%d has wrong priority %d", > > inet_ntoa(kr->r.prefix), kr->r.prefixlen, kr->r.priority); > > > > @@ -316,7 +319,7 @@ kr_delete(struct kroute *kroute) > > struct kroute_node *kr, *nkr; > > > > if ((kr = kroute_find(kroute->prefix.s_addr, kroute->prefixlen, > > - RTP_OSPF)) == NULL) > > + kr_state.fib_prio)) == NULL) > > return (0); > > > > while (kr != NULL) { > > @@ -348,7 +351,7 @@ kr_fib_couple(void) > > kr_state.fib_sync = 1; > > > > RB_FOREACH(kr, kroute_tree, &krt) > > - if (kr->r.priority == RTP_OSPF) > > + if (kr->r.priority == kr_state.fib_prio) > > for (kn = kr; kn != NULL; kn = kn->next) > > send_rtmsg(kr_state.fd, RTM_ADD, &kn->r); > > > > @@ -365,7 +368,7 @@ kr_fib_decouple(void) > > return; > > > > RB_FOREACH(kr, kroute_tree, &krt
Re: ospfd: fib-priority
ping On Mon, Dec 10, 2018 at 10:40:22AM +0100, Remi Locherer wrote: > Hi, > > below patch adds "fib-priority" to ospfd.conf which allows to set a > custom priority to routes. 32 is still the default if not set. Changing > the priority with a reload is also supported. > > A discussion about the feature can be found here: > https://marc.info/?l=openbsd-tech&m=138360663119816&w=2 > > My first idea was to add an additional parameter to the functions that > need it. But that that is not practical since then need the event that calls > kr_dispatch_msg() needs to be reset. Because of that I added fib_prio to > struct kr_state. > > > OK? > > Remi > > > > cvs diff: Diffing . > Index: kroute.c > === > RCS file: /cvs/src/usr.sbin/ospfd/kroute.c,v > retrieving revision 1.111 > diff -u -p -r1.111 kroute.c > --- kroute.c 10 Jul 2018 11:49:04 - 1.111 > +++ kroute.c 9 Dec 2018 21:39:46 - > @@ -45,6 +45,7 @@ struct { > pid_t pid; > int fib_sync; > int fib_serial; > + u_int8_tfib_prio; > int fd; > struct eventev; > struct eventreload; > @@ -127,14 +128,15 @@ kif_init(void) > } > > int > -kr_init(int fs, u_int rdomain, int redis_label_or_prefix) > +kr_init(int fs, u_int rdomain, int redis_label_or_prefix, u_int8_t fib_prio) > { > int opt = 0, rcvbuf, default_rcvbuf; > socklen_t optlen; > - int filter_prio = RTP_OSPF; > + int filter_prio = fib_prio; > > kr_state.fib_sync = fs; > kr_state.rdomain = rdomain; > + kr_state.fib_prio = fib_prio; > > if ((kr_state.fd = socket(AF_ROUTE, > SOCK_RAW | SOCK_CLOEXEC | SOCK_NONBLOCK, AF_INET)) == -1) { > @@ -262,7 +264,7 @@ kr_change_fib(struct kroute_node *kr, st > kn->r.prefixlen = kroute[i].prefixlen; > kn->r.nexthop.s_addr = kroute[i].nexthop.s_addr; > kn->r.flags = kroute[i].flags | F_OSPFD_INSERTED; > - kn->r.priority = RTP_OSPF; > + kn->r.priority = kr_state.fib_prio; > kn->r.ext_tag = kroute[i].ext_tag; > rtlabel_unref(kn->r.rtlabel); /* for RTM_CHANGE */ > kn->r.rtlabel = kroute[i].rtlabel; > @@ -286,7 +288,8 @@ kr_change(struct kroute *kroute, int krc > > kroute->rtlabel = rtlabel_tag2id(kroute->ext_tag); > > - kr = kroute_find(kroute->prefix.s_addr, kroute->prefixlen, RTP_OSPF); > + kr = kroute_find(kroute->prefix.s_addr, kroute->prefixlen, > + kr_state.fib_prio); > if (kr != NULL && kr->next == NULL && krcount == 1) > /* single path OSPF route */ > action = RTM_CHANGE; > @@ -297,7 +300,7 @@ kr_change(struct kroute *kroute, int krc > int > kr_delete_fib(struct kroute_node *kr) > { > - if (kr->r.priority != RTP_OSPF) > + if (kr->r.priority != kr_state.fib_prio) > log_warn("kr_delete_fib: %s/%d has wrong priority %d", > inet_ntoa(kr->r.prefix), kr->r.prefixlen, kr->r.priority); > > @@ -316,7 +319,7 @@ kr_delete(struct kroute *kroute) > struct kroute_node *kr, *nkr; > > if ((kr = kroute_find(kroute->prefix.s_addr, kroute->prefixlen, > - RTP_OSPF)) == NULL) > + kr_state.fib_prio)) == NULL) > return (0); > > while (kr != NULL) { > @@ -348,7 +351,7 @@ kr_fib_couple(void) > kr_state.fib_sync = 1; > > RB_FOREACH(kr, kroute_tree, &krt) > - if (kr->r.priority == RTP_OSPF) > + if (kr->r.priority == kr_state.fib_prio) > for (kn = kr; kn != NULL; kn = kn->next) > send_rtmsg(kr_state.fd, RTM_ADD, &kn->r); > > @@ -365,7 +368,7 @@ kr_fib_decouple(void) > return; > > RB_FOREACH(kr, kroute_tree, &krt) > - if (kr->r.priority == RTP_OSPF) > + if (kr->r.priority == kr_state.fib_prio) > for (kn = kr; kn != NULL; kn = kn->next) > send_rtmsg(kr_state.fd, RTM_DELETE, &kn->r); > > @@ -418,7 +421,7 @@ kr_fib_reload() > kn = kr->next; > > if (kr->serial != kr_state.fib_serial) { > - if (kr->r.priority == RTP_OSPF) { > +
ospfd: fib-priority
Hi, below patch adds "fib-priority" to ospfd.conf which allows to set a custom priority to routes. 32 is still the default if not set. Changing the priority with a reload is also supported. A discussion about the feature can be found here: https://marc.info/?l=openbsd-tech&m=138360663119816&w=2 My first idea was to add an additional parameter to the functions that need it. But that that is not practical since then need the event that calls kr_dispatch_msg() needs to be reset. Because of that I added fib_prio to struct kr_state. OK? Remi cvs diff: Diffing . Index: kroute.c === RCS file: /cvs/src/usr.sbin/ospfd/kroute.c,v retrieving revision 1.111 diff -u -p -r1.111 kroute.c --- kroute.c10 Jul 2018 11:49:04 - 1.111 +++ kroute.c9 Dec 2018 21:39:46 - @@ -45,6 +45,7 @@ struct { pid_t pid; int fib_sync; int fib_serial; + u_int8_tfib_prio; int fd; struct eventev; struct eventreload; @@ -127,14 +128,15 @@ kif_init(void) } int -kr_init(int fs, u_int rdomain, int redis_label_or_prefix) +kr_init(int fs, u_int rdomain, int redis_label_or_prefix, u_int8_t fib_prio) { int opt = 0, rcvbuf, default_rcvbuf; socklen_t optlen; - int filter_prio = RTP_OSPF; + int filter_prio = fib_prio; kr_state.fib_sync = fs; kr_state.rdomain = rdomain; + kr_state.fib_prio = fib_prio; if ((kr_state.fd = socket(AF_ROUTE, SOCK_RAW | SOCK_CLOEXEC | SOCK_NONBLOCK, AF_INET)) == -1) { @@ -262,7 +264,7 @@ kr_change_fib(struct kroute_node *kr, st kn->r.prefixlen = kroute[i].prefixlen; kn->r.nexthop.s_addr = kroute[i].nexthop.s_addr; kn->r.flags = kroute[i].flags | F_OSPFD_INSERTED; - kn->r.priority = RTP_OSPF; + kn->r.priority = kr_state.fib_prio; kn->r.ext_tag = kroute[i].ext_tag; rtlabel_unref(kn->r.rtlabel); /* for RTM_CHANGE */ kn->r.rtlabel = kroute[i].rtlabel; @@ -286,7 +288,8 @@ kr_change(struct kroute *kroute, int krc kroute->rtlabel = rtlabel_tag2id(kroute->ext_tag); - kr = kroute_find(kroute->prefix.s_addr, kroute->prefixlen, RTP_OSPF); + kr = kroute_find(kroute->prefix.s_addr, kroute->prefixlen, + kr_state.fib_prio); if (kr != NULL && kr->next == NULL && krcount == 1) /* single path OSPF route */ action = RTM_CHANGE; @@ -297,7 +300,7 @@ kr_change(struct kroute *kroute, int krc int kr_delete_fib(struct kroute_node *kr) { - if (kr->r.priority != RTP_OSPF) + if (kr->r.priority != kr_state.fib_prio) log_warn("kr_delete_fib: %s/%d has wrong priority %d", inet_ntoa(kr->r.prefix), kr->r.prefixlen, kr->r.priority); @@ -316,7 +319,7 @@ kr_delete(struct kroute *kroute) struct kroute_node *kr, *nkr; if ((kr = kroute_find(kroute->prefix.s_addr, kroute->prefixlen, - RTP_OSPF)) == NULL) + kr_state.fib_prio)) == NULL) return (0); while (kr != NULL) { @@ -348,7 +351,7 @@ kr_fib_couple(void) kr_state.fib_sync = 1; RB_FOREACH(kr, kroute_tree, &krt) - if (kr->r.priority == RTP_OSPF) + if (kr->r.priority == kr_state.fib_prio) for (kn = kr; kn != NULL; kn = kn->next) send_rtmsg(kr_state.fd, RTM_ADD, &kn->r); @@ -365,7 +368,7 @@ kr_fib_decouple(void) return; RB_FOREACH(kr, kroute_tree, &krt) - if (kr->r.priority == RTP_OSPF) + if (kr->r.priority == kr_state.fib_prio) for (kn = kr; kn != NULL; kn = kn->next) send_rtmsg(kr_state.fd, RTM_DELETE, &kn->r); @@ -418,7 +421,7 @@ kr_fib_reload() kn = kr->next; if (kr->serial != kr_state.fib_serial) { - if (kr->r.priority == RTP_OSPF) { + if (kr->r.priority == kr_state.fib_prio) { kr->serial = kr_state.fib_serial; if (send_rtmsg(kr_state.fd, RTM_ADD, &kr->r) != 0) @@ -431,6 +434,21 @@ kr_fib_reload() } } +void +kr_fib_update_prio(u_int8_t fib_prio) +{ + struct kroute_node *kr; + + RB_FOREACH(kr, kroute_tree, &krt) + if ((kr->r.flags & F_OSPFD_INSERTED)
Re: ospfd: pledge parent process
On Sat, Sep 01, 2018 at 10:38:09PM +0200, Sebastian Benoit wrote: > Remi Locherer(remi.loche...@relo.ch) on 2018.09.01 21:53:21 +0200: > > Hi, > > > > Since slaacd is able to use pledge in the parent process I thought it may > > be possible for ospfd too. > > > > It works fine until ospfd gets reloaded. At this point it uses setsockopt > > to set the priority filter on the routing socket. > > > > Since I could not find a promise for this I extended wroute. Does this make > > sense? Would another promise or something completely different be better? > > just route would be good enough, because route is for receiving routes, > and the route filter just changes which routes you get. > > does the > > area ... { > demote carp > > } > > feature and the > > > interface if { demote carp ... } > > feature still work with this pledge? No, it does not: 79534 ospfdCALL recvmsg(3,0x7f7f8a40,0) 79534 ospfd GIO fd 3 read 36 bytes "8\0\0\0$\0\0\0\0\0\0\0Wx\^A\0carp\0\0\0\0\0\0\0\0\0\0\0\0\^A\0\0\0" 79534 ospfdSTRU struct msghdr { name=0x0, namelen=0, iov=0x7f7f8a30, iovlen=1, control=0x7f7f8a70, controllen=0, flags=0x80 } 79534 ospfdSTRU struct iovec { base=0xcc3203c5034, len=65499 } 79534 ospfdRET recvmsg 36/0x24 79534 ospfdCALL socket(AF_INET,0x2,0) 79534 ospfdPLDG socket, "dns", errno 1 Operation not permitted 79534 ospfdPSIG SIGABRT SIG_DFL 79534 ospfdNAMI "ospfd.core" This is from socket(AF_INET, SOCK_DGRAM, 0) in carp_demote_get. The same function needs ioctl(s, SIOCGIFGATTR, (caddr_t)&ifgr) afterwards.
Re: ospfd: pledge parent process
On Sun, Sep 02, 2018 at 08:05:55AM +0200, Remi Locherer wrote: > On Sat, Sep 01, 2018 at 10:38:09PM +0200, Sebastian Benoit wrote: > > Remi Locherer(remi.loche...@relo.ch) on 2018.09.01 21:53:21 +0200: > > > Hi, > > > > > > Since slaacd is able to use pledge in the parent process I thought it may > > > be possible for ospfd too. > > > > > > It works fine until ospfd gets reloaded. At this point it uses setsockopt > > > to set the priority filter on the routing socket. > > > > > > Since I could not find a promise for this I extended wroute. Does this > > > make > > > sense? Would another promise or something completely different be better? > > > > just route would be good enough, because route is for receiving routes, > > and the route filter just changes which routes you get. > > ospfd is not happy with "pledge("stdio rpath sendfd route", NULL)" > > During reload: > > kr_reload: priority filter disabled > orig_rtr_lsa: area 0.0.0.0 > Abort trap (core dumped) > orig_rtr_lsa: stub net, interface pair0 > orig_rtr_lsa: stub net, interface vether0 > [...] > > ospfd[2432]: pledge "inet", syscall 105 Theo pointed out a ktrace is needed here to understand what is going on: 18069 ospfdGIO fd 2 wrote 36 bytes "kr_reload: priority filter disabled " 18069 ospfd RET write 36/0x24 18069 ospfdCALL setsockopt(6,17,3,0x7f7efb70,4) 18069 ospfdPLDG setsockopt, "inet", errno 1 Operation not permitted 18069 ospfdPSIG SIGABRT SIG_DFL 18069 ospfdNAMI "ospfd.core" This is from the following line in osfpd/kroute.c if (setsockopt(kr_state.fd, AF_ROUTE, ROUTE_PRIOFILTER, &filter_prio, sizeof(filter_prio)) == -1) {
Re: ospfd: pledge parent process
On Sat, Sep 01, 2018 at 10:38:09PM +0200, Sebastian Benoit wrote: > Remi Locherer(remi.loche...@relo.ch) on 2018.09.01 21:53:21 +0200: > > Hi, > > > > Since slaacd is able to use pledge in the parent process I thought it may > > be possible for ospfd too. > > > > It works fine until ospfd gets reloaded. At this point it uses setsockopt > > to set the priority filter on the routing socket. > > > > Since I could not find a promise for this I extended wroute. Does this make > > sense? Would another promise or something completely different be better? > > just route would be good enough, because route is for receiving routes, > and the route filter just changes which routes you get. ospfd is not happy with "pledge("stdio rpath sendfd route", NULL)" During reload: kr_reload: priority filter disabled orig_rtr_lsa: area 0.0.0.0 Abort trap (core dumped) orig_rtr_lsa: stub net, interface pair0 orig_rtr_lsa: stub net, interface vether0 [...] ospfd[2432]: pledge "inet", syscall 105 > > does the > > area ... { > demote carp > > } I didn't test this initially. But to me it looks like this feature does not work anymore. With my little test setup the demote counter never increases. I'll have a closer look. > > feature and the > > >interface if { demote carp ... } > > feature still work with this pledge?
Re: ospfd: pledge parent process
Remi Locherer(remi.loche...@relo.ch) on 2018.09.01 21:53:21 +0200: > Hi, > > Since slaacd is able to use pledge in the parent process I thought it may > be possible for ospfd too. > > It works fine until ospfd gets reloaded. At this point it uses setsockopt > to set the priority filter on the routing socket. > > Since I could not find a promise for this I extended wroute. Does this make > sense? Would another promise or something completely different be better? just route would be good enough, because route is for receiving routes, and the route filter just changes which routes you get. does the area ... { demote carp } feature and the interface if { demote carp ... } feature still work with this pledge? > Index: kern_pledge.c > === > RCS file: /cvs/src/sys/kern/kern_pledge.c,v > retrieving revision 1.242 > diff -u -p -r1.242 kern_pledge.c > --- kern_pledge.c 20 Aug 2018 10:00:04 - 1.242 > +++ kern_pledge.c 1 Sep 2018 12:56:27 - > @@ -1295,7 +1295,7 @@ pledge_sockopt(struct proc *p, int set, > break; > } > > - if ((p->p_p->ps_pledge & > (PLEDGE_INET|PLEDGE_UNIX|PLEDGE_DNS|PLEDGE_YPACTIVE)) == 0) > + if ((p->p_p->ps_pledge & > (PLEDGE_INET|PLEDGE_ROUTE|PLEDGE_UNIX|PLEDGE_DNS|PLEDGE_YPACTIVE)) == 0) > return pledge_fail(p, EPERM, PLEDGE_INET); > /* In use by some service libraries */ > switch (level) { > @@ -1335,6 +1335,13 @@ pledge_sockopt(struct proc *p, int set, > return (0); > } > break; > + } > + } > + > + if (p->p_p->ps_pledge & PLEDGE_WROUTE) { > + switch (level) { > + case AF_ROUTE: > + return (0); > } > } > > > > Index: control.c > === > RCS file: /cvs/src/usr.sbin/ospfd/control.c,v > retrieving revision 1.45 > diff -u -p -r1.45 control.c > --- control.c 29 Aug 2018 08:43:16 - 1.45 > +++ control.c 31 Aug 2018 14:11:53 - > @@ -124,16 +124,6 @@ control_listen(void) > return (0); > } > > -void > -control_cleanup(char *path) > -{ > - if (path == NULL) > - return; > - event_del(&control_state.ev); > - event_del(&control_state.evt); > - unlink(path); > -} > - > /* ARGSUSED */ > void > control_accept(int listenfd, short event, void *bula) > Index: control.h > === > RCS file: /cvs/src/usr.sbin/ospfd/control.h,v > retrieving revision 1.7 > diff -u -p -r1.7 control.h > --- control.h 29 Aug 2018 08:43:16 - 1.7 > +++ control.h 31 Aug 2018 14:12:09 - > @@ -40,6 +40,5 @@ int control_listen(void); > void control_accept(int, short, void *); > void control_dispatch_imsg(int, short, void *); > int control_imsg_relay(struct imsg *); > -void control_cleanup(char *); > > #endif /* _CONTROL_H_ */ > Index: ospfd.c > === > RCS file: /cvs/src/usr.sbin/ospfd/ospfd.c,v > retrieving revision 1.100 > diff -u -p -r1.100 ospfd.c > --- ospfd.c 29 Aug 2018 08:43:17 - 1.100 > +++ ospfd.c 1 Sep 2018 12:57:46 - > @@ -282,6 +282,9 @@ main(int argc, char *argv[]) > ospfd_conf->rdomain, ospfd_conf->redist_label_or_prefix) == -1) > fatalx("kr_init failed"); > > + if (pledge("stdio rpath sendfd wroute", NULL) == -1) > + fatal("pledge"); > + > /* remove unneeded stuff from config */ > while ((a = LIST_FIRST(&ospfd_conf->area_list)) != NULL) { > LIST_REMOVE(a, entry); > @@ -308,7 +311,6 @@ ospfd_shutdown(void) > msgbuf_clear(&iev_rde->ibuf.w); > close(iev_rde->ibuf.fd); > > - control_cleanup(ospfd_conf->csock); > while ((r = SIMPLEQ_FIRST(&ospfd_conf->redist_list)) != NULL) { > SIMPLEQ_REMOVE_HEAD(&ospfd_conf->redist_list, entry); > free(r);
ospfd: pledge parent process
Hi, Since slaacd is able to use pledge in the parent process I thought it may be possible for ospfd too. It works fine until ospfd gets reloaded. At this point it uses setsockopt to set the priority filter on the routing socket. Since I could not find a promise for this I extended wroute. Does this make sense? Would another promise or something completely different be better? Remi Index: kern_pledge.c === RCS file: /cvs/src/sys/kern/kern_pledge.c,v retrieving revision 1.242 diff -u -p -r1.242 kern_pledge.c --- kern_pledge.c 20 Aug 2018 10:00:04 - 1.242 +++ kern_pledge.c 1 Sep 2018 12:56:27 - @@ -1295,7 +1295,7 @@ pledge_sockopt(struct proc *p, int set, break; } - if ((p->p_p->ps_pledge & (PLEDGE_INET|PLEDGE_UNIX|PLEDGE_DNS|PLEDGE_YPACTIVE)) == 0) + if ((p->p_p->ps_pledge & (PLEDGE_INET|PLEDGE_ROUTE|PLEDGE_UNIX|PLEDGE_DNS|PLEDGE_YPACTIVE)) == 0) return pledge_fail(p, EPERM, PLEDGE_INET); /* In use by some service libraries */ switch (level) { @@ -1335,6 +1335,13 @@ pledge_sockopt(struct proc *p, int set, return (0); } break; + } + } + + if (p->p_p->ps_pledge & PLEDGE_WROUTE) { + switch (level) { + case AF_ROUTE: + return (0); } } Index: control.c === RCS file: /cvs/src/usr.sbin/ospfd/control.c,v retrieving revision 1.45 diff -u -p -r1.45 control.c --- control.c 29 Aug 2018 08:43:16 - 1.45 +++ control.c 31 Aug 2018 14:11:53 - @@ -124,16 +124,6 @@ control_listen(void) return (0); } -void -control_cleanup(char *path) -{ - if (path == NULL) - return; - event_del(&control_state.ev); - event_del(&control_state.evt); - unlink(path); -} - /* ARGSUSED */ void control_accept(int listenfd, short event, void *bula) Index: control.h === RCS file: /cvs/src/usr.sbin/ospfd/control.h,v retrieving revision 1.7 diff -u -p -r1.7 control.h --- control.h 29 Aug 2018 08:43:16 - 1.7 +++ control.h 31 Aug 2018 14:12:09 - @@ -40,6 +40,5 @@ int control_listen(void); void control_accept(int, short, void *); void control_dispatch_imsg(int, short, void *); intcontrol_imsg_relay(struct imsg *); -void control_cleanup(char *); #endif /* _CONTROL_H_ */ Index: ospfd.c === RCS file: /cvs/src/usr.sbin/ospfd/ospfd.c,v retrieving revision 1.100 diff -u -p -r1.100 ospfd.c --- ospfd.c 29 Aug 2018 08:43:17 - 1.100 +++ ospfd.c 1 Sep 2018 12:57:46 - @@ -282,6 +282,9 @@ main(int argc, char *argv[]) ospfd_conf->rdomain, ospfd_conf->redist_label_or_prefix) == -1) fatalx("kr_init failed"); + if (pledge("stdio rpath sendfd wroute", NULL) == -1) + fatal("pledge"); + /* remove unneeded stuff from config */ while ((a = LIST_FIRST(&ospfd_conf->area_list)) != NULL) { LIST_REMOVE(a, entry); @@ -308,7 +311,6 @@ ospfd_shutdown(void) msgbuf_clear(&iev_rde->ibuf.w); close(iev_rde->ibuf.fd); - control_cleanup(ospfd_conf->csock); while ((r = SIMPLEQ_FIRST(&ospfd_conf->redist_list)) != NULL) { SIMPLEQ_REMOVE_HEAD(&ospfd_conf->redist_list, entry); free(r);
Re: ospfd: prevent additional ospfd from starting
OK florian@ On Tue, Aug 28, 2018 at 01:19:39PM +0200, Remi Locherer wrote: > On Tue, Aug 28, 2018 at 07:56:43AM +0200, Claudio Jeker wrote: > > On Mon, Aug 27, 2018 at 11:33:19PM +0200, Remi Locherer wrote: > > > On Fri, Aug 24, 2018 at 12:21:31PM +0200, Remi Locherer wrote: > > > > On Fri, Aug 24, 2018 at 08:58:12AM +0200, Claudio Jeker wrote: > > [ snip ] > > > > > > Why are we not checking the control socket in the parent? > > > > > Also it may be better to create the control socket in the parent and > > > > > pass > > > > > it to the ospfe. This is what bgpd is doing and allows to change the > > > > > path > > > > > during runtime with a config reload. > > > > > > > > This makes sense to me. I'll come up with a new diff once I found some > > > > time for it. > > > > > > > > But I'm not sure about changing the socket path with a reload. I plan to > > > > pledge (stdio rpath sendfd wroute) and eventually unveil (read > > > > ospfd.conf) > > > > the main process. > > > > > > New diff below creates the control socket in the main process and passes > > > it > > > to the ospf engine later on. The connect check on the control socket now > > > happens very early. > > > > > > The diff in action looks like this: > > > > > > typhoon ..sbin/ospfd$ doas obj/ospfd -dv > > > startup > > > control_init: socket in use > > > fatal in ospfd: control socket setup failed > > > typhoon 1 ..sbin/ospfd$ > > > > > > > > > I borrowed the fd passing code from slaacd. > > > > > > > > > > > > > > > > > Could there be a case where this causes ospfd to hang on start in the > > > > > connect? Not sure if we can sleep doing a connect() to a AF_UNIX > > > > > socket. > > > > > > I never observed a hangin ospfctl which also does a connect on the control > > > socket. But I could not find the definitiv answer. > > > > > [ snip ] > > > I would prefer if the check happens before the daemon() call since then > > the rc script notice this easily. > > sure > > > Also between here and sending the socket > > we spawn off the rde and ospfe processes. So currently you are leaking > > control_fd into those processes. > > You could probably just add the fd as argument to rde() and ospfe() and > > not use the fd passing at all. But the moment ospfd is using fork&exec > > then the fd passing will be needed again. > > How about the new diff below: I moved the check from control_init into its > own function control_check and call only this before daemon(). control_init > happens later. With this the childs do not have the control fd. > > The time frame where another process can start using the socket is a little > bit bigger this way. We can reduce this again when implementing fork&exec > for ospfd. > > One could also argue that with control_check as separate function fd passing > is not strictly needed. But I think this a step towards fork&exec. > > The diff should also address the other suggestions. > > > > Index: control.c > === > RCS file: /cvs/src/usr.sbin/ospfd/control.c,v > retrieving revision 1.44 > diff -u -p -r1.44 control.c > --- control.c 24 Jan 2017 04:24:25 - 1.44 > +++ control.c 28 Aug 2018 09:42:11 - > @@ -39,6 +39,32 @@ struct ctl_conn*control_connbypid(pid_t > void control_close(int); > > int > +control_check(char *path) > +{ > + struct sockaddr_un sun; > + int fd; > + > + bzero(&sun, sizeof(sun)); > + sun.sun_family = AF_UNIX; > + strlcpy(sun.sun_path, path, sizeof(sun.sun_path)); > + > + if ((fd = socket(AF_UNIX, SOCK_STREAM, 0)) == -1) { > + log_warn("control_check: socket check"); > + return (-1); > + } > + > + if (connect(fd, (struct sockaddr *)&sun, sizeof(sun)) == 0) { > + log_warnx("control_check: socket in use"); > + close(fd); > + return (-1); > + } > + > + close(fd); > + > + return (0); > +} > + > +int > control_init(char *path) > { > struct sockaddr_un sun; > @@ -78,9 +104,7 @@ control_init(char *path) > return (-1); > } > > - control_state.fd = fd; > -
Re: ospfd: prevent additional ospfd from starting
On Tue, Aug 28, 2018 at 01:19:39PM +0200, Remi Locherer wrote: > On Tue, Aug 28, 2018 at 07:56:43AM +0200, Claudio Jeker wrote: > > On Mon, Aug 27, 2018 at 11:33:19PM +0200, Remi Locherer wrote: > > > On Fri, Aug 24, 2018 at 12:21:31PM +0200, Remi Locherer wrote: > > > > On Fri, Aug 24, 2018 at 08:58:12AM +0200, Claudio Jeker wrote: > > [ snip ] > > > > > > Why are we not checking the control socket in the parent? > > > > > Also it may be better to create the control socket in the parent and > > > > > pass > > > > > it to the ospfe. This is what bgpd is doing and allows to change the > > > > > path > > > > > during runtime with a config reload. > > > > > > > > This makes sense to me. I'll come up with a new diff once I found some > > > > time for it. > > > > > > > > But I'm not sure about changing the socket path with a reload. I plan to > > > > pledge (stdio rpath sendfd wroute) and eventually unveil (read > > > > ospfd.conf) > > > > the main process. > > > > > > New diff below creates the control socket in the main process and passes > > > it > > > to the ospf engine later on. The connect check on the control socket now > > > happens very early. > > > > > > The diff in action looks like this: > > > > > > typhoon ..sbin/ospfd$ doas obj/ospfd -dv > > > startup > > > control_init: socket in use > > > fatal in ospfd: control socket setup failed > > > typhoon 1 ..sbin/ospfd$ > > > > > > > > > I borrowed the fd passing code from slaacd. > > > > > > > > > > > > > > > > > Could there be a case where this causes ospfd to hang on start in the > > > > > connect? Not sure if we can sleep doing a connect() to a AF_UNIX > > > > > socket. > > > > > > I never observed a hangin ospfctl which also does a connect on the control > > > socket. But I could not find the definitiv answer. > > > > > [ snip ] > > > I would prefer if the check happens before the daemon() call since then > > the rc script notice this easily. > > sure > > > Also between here and sending the socket > > we spawn off the rde and ospfe processes. So currently you are leaking > > control_fd into those processes. > > You could probably just add the fd as argument to rde() and ospfe() and > > not use the fd passing at all. But the moment ospfd is using fork&exec > > then the fd passing will be needed again. > > How about the new diff below: I moved the check from control_init into its > own function control_check and call only this before daemon(). control_init > happens later. With this the childs do not have the control fd. > > The time frame where another process can start using the socket is a little > bit bigger this way. We can reduce this again when implementing fork&exec > for ospfd. > > One could also argue that with control_check as separate function fd passing > is not strictly needed. But I think this a step towards fork&exec. > > The diff should also address the other suggestions. > Looks good to me. > > Index: control.c > === > RCS file: /cvs/src/usr.sbin/ospfd/control.c,v > retrieving revision 1.44 > diff -u -p -r1.44 control.c > --- control.c 24 Jan 2017 04:24:25 - 1.44 > +++ control.c 28 Aug 2018 09:42:11 - > @@ -39,6 +39,32 @@ struct ctl_conn*control_connbypid(pid_t > void control_close(int); > > int > +control_check(char *path) > +{ > + struct sockaddr_un sun; > + int fd; > + > + bzero(&sun, sizeof(sun)); > + sun.sun_family = AF_UNIX; > + strlcpy(sun.sun_path, path, sizeof(sun.sun_path)); > + > + if ((fd = socket(AF_UNIX, SOCK_STREAM, 0)) == -1) { > + log_warn("control_check: socket check"); > + return (-1); > + } > + > + if (connect(fd, (struct sockaddr *)&sun, sizeof(sun)) == 0) { > + log_warnx("control_check: socket in use"); > + close(fd); > + return (-1); > + } > + > + close(fd); > + > + return (0); > +} > + > +int > control_init(char *path) > { > struct sockaddr_un sun; > @@ -78,9 +104,7 @@ control_init(char *path) > return (-1); > } > > - control_state.fd = fd; > -
Re: ospfd: prevent additional ospfd from starting
On Tue, Aug 28, 2018 at 07:56:43AM +0200, Claudio Jeker wrote: > On Mon, Aug 27, 2018 at 11:33:19PM +0200, Remi Locherer wrote: > > On Fri, Aug 24, 2018 at 12:21:31PM +0200, Remi Locherer wrote: > > > On Fri, Aug 24, 2018 at 08:58:12AM +0200, Claudio Jeker wrote: [ snip ] > > > > Why are we not checking the control socket in the parent? > > > > Also it may be better to create the control socket in the parent and > > > > pass > > > > it to the ospfe. This is what bgpd is doing and allows to change the > > > > path > > > > during runtime with a config reload. > > > > > > This makes sense to me. I'll come up with a new diff once I found some > > > time for it. > > > > > > But I'm not sure about changing the socket path with a reload. I plan to > > > pledge (stdio rpath sendfd wroute) and eventually unveil (read ospfd.conf) > > > the main process. > > > > New diff below creates the control socket in the main process and passes it > > to the ospf engine later on. The connect check on the control socket now > > happens very early. > > > > The diff in action looks like this: > > > > typhoon ..sbin/ospfd$ doas obj/ospfd -dv > > startup > > control_init: socket in use > > fatal in ospfd: control socket setup failed > > typhoon 1 ..sbin/ospfd$ > > > > > > I borrowed the fd passing code from slaacd. > > > > > > > > > > > > > Could there be a case where this causes ospfd to hang on start in the > > > > connect? Not sure if we can sleep doing a connect() to a AF_UNIX socket. > > > > I never observed a hangin ospfctl which also does a connect on the control > > socket. But I could not find the definitiv answer. > > [ snip ] > I would prefer if the check happens before the daemon() call since then > the rc script notice this easily. sure > Also between here and sending the socket > we spawn off the rde and ospfe processes. So currently you are leaking > control_fd into those processes. > You could probably just add the fd as argument to rde() and ospfe() and > not use the fd passing at all. But the moment ospfd is using fork&exec > then the fd passing will be needed again. How about the new diff below: I moved the check from control_init into its own function control_check and call only this before daemon(). control_init happens later. With this the childs do not have the control fd. The time frame where another process can start using the socket is a little bit bigger this way. We can reduce this again when implementing fork&exec for ospfd. One could also argue that with control_check as separate function fd passing is not strictly needed. But I think this a step towards fork&exec. The diff should also address the other suggestions. Index: control.c === RCS file: /cvs/src/usr.sbin/ospfd/control.c,v retrieving revision 1.44 diff -u -p -r1.44 control.c --- control.c 24 Jan 2017 04:24:25 - 1.44 +++ control.c 28 Aug 2018 09:42:11 - @@ -39,6 +39,32 @@ struct ctl_conn *control_connbypid(pid_t voidcontrol_close(int); int +control_check(char *path) +{ + struct sockaddr_un sun; + int fd; + + bzero(&sun, sizeof(sun)); + sun.sun_family = AF_UNIX; + strlcpy(sun.sun_path, path, sizeof(sun.sun_path)); + + if ((fd = socket(AF_UNIX, SOCK_STREAM, 0)) == -1) { + log_warn("control_check: socket check"); + return (-1); + } + + if (connect(fd, (struct sockaddr *)&sun, sizeof(sun)) == 0) { + log_warnx("control_check: socket in use"); + close(fd); + return (-1); + } + + close(fd); + + return (0); +} + +int control_init(char *path) { struct sockaddr_un sun; @@ -78,9 +104,7 @@ control_init(char *path) return (-1); } - control_state.fd = fd; - - return (0); + return (fd); } int Index: control.h === RCS file: /cvs/src/usr.sbin/ospfd/control.h,v retrieving revision 1.6 diff -u -p -r1.6 control.h --- control.h 10 Feb 2015 05:24:48 - 1.6 +++ control.h 28 Aug 2018 09:43:17 - @@ -34,6 +34,7 @@ struct ctl_conn { struct imsgev iev; }; +intcontrol_check(char *); intcontrol_init(char *); intcontrol_listen(void); void control_accept(int, short, void *); Index: ospfd.c === RCS file: /cvs/src/usr.sbin/ospfd/ospfd.c,v retrievi
Re: ospfd: prevent additional ospfd from starting
On Mon, Aug 27, 2018 at 11:33:19PM +0200, Remi Locherer wrote: > On Fri, Aug 24, 2018 at 12:21:31PM +0200, Remi Locherer wrote: > > On Fri, Aug 24, 2018 at 08:58:12AM +0200, Claudio Jeker wrote: > > > On Wed, Aug 22, 2018 at 12:12:10AM +0200, Remi Locherer wrote: > > > > On Tue, Aug 21, 2018 at 05:54:18PM +0100, Stuart Henderson wrote: > > > > > On 2018/08/21 17:16, Remi Locherer wrote: > > > > > > Hi tech, > > > > > > > > > > > > recently we had a short outage in our network. A script started an > > > > > > additional > > > > > > ospfd instance because the -n flag for config test was missing. > > > > > > > > > > This is a problem with bgpd as well, last time I did this it killed > > > > > one of the > > > > > *other* routers on the network (i.e. not just the one where I > > > > > accidentally ran > > > > > 2x bgpd...). > > > > > > > > > > > What then happend was not nice: > > > > > > - The new ospfd unlinked the control socket of the first ospfd > > > > > > - The new ospfd removed all routes from the first ospfd > > > > > > - The new ospfd was not able to build up an adjacency and therefore > > > > > > could > > > > > > not install the routes needed for a recovery. > > > > > > - Both ospfd instances were running but non-functional. > > > > > > > > > > > > Of course the faulty script is fixed by now. ;-) > > > > > > > > > > > > It would be nice if ospfd could prevent such a situation. > > > > > > > > > > > > Below diff does these things: > > > > > > - Detect a running ospfd by first doing a connect on the control > > > > > > socket. > > > > > > - Do not delete the control socket on exit. > > > > > > - This could delete the socket of another instance. > > > > > > - Unlinking the socket on shutdown will be in the way once we add > > > > > > pledge > > > > > > to the main process. It was removed recently from various > > > > > > daemons. > > > > > > > > > > This all sounds very sensible. > > > > > > > > > > > - Do not delete routes added by another process even if they have > > > > > > prio RTP_OSPF. Without this the new ospfd will remove all the > > > > > > routes > > > > > > of the first one. > > > > > > > > > > I'm unsure about this, the above changes stop the new ospfd from > > > > > running > > > > > don't they, so that shouldn't be a problem? > > > > > > > > It stops to late. kr_init happens before and kill all existing routes > > > > with > > > > priority 32. And again in the shutdown function of ospfd. > > > > > > > > > > If an ospfd blows up for whatever reason, it would be quite > > > > > inconvenient > > > > > if it needs manual route tweaks rather than just 'rcctl start ospfd' > > > > > to fix it .. > > > > > > > > Yes, this is not optimal. > > > > > > > > The new diff below defers kr_init until the ospf engine notifies the > > > > parent > > > > that the control socket is ready. In case the ospf engine exits because > > > > the > > > > control socket is already in use no routes are known that could be > > > > removed. > > > > > > > > With this ospfd keeps the behaviour of removing foreign routes with > > > > priority 32. > > > > > > > > Better? > > > > > > > > > > Why are we not checking the control socket in the parent? > > > Also it may be better to create the control socket in the parent and pass > > > it to the ospfe. This is what bgpd is doing and allows to change the path > > > during runtime with a config reload. > > > > This makes sense to me. I'll come up with a new diff once I found some > > time for it. > > > > But I'm not sure about changing the socket path with a reload. I plan to > > pledge (stdio rpath sendfd wroute) and eventually unveil (read ospfd.conf) > > the main process. > > New diff below creates the cont
Re: ospfd: prevent additional ospfd from starting
On Fri, Aug 24, 2018 at 12:21:31PM +0200, Remi Locherer wrote: > On Fri, Aug 24, 2018 at 08:58:12AM +0200, Claudio Jeker wrote: > > On Wed, Aug 22, 2018 at 12:12:10AM +0200, Remi Locherer wrote: > > > On Tue, Aug 21, 2018 at 05:54:18PM +0100, Stuart Henderson wrote: > > > > On 2018/08/21 17:16, Remi Locherer wrote: > > > > > Hi tech, > > > > > > > > > > recently we had a short outage in our network. A script started an > > > > > additional > > > > > ospfd instance because the -n flag for config test was missing. > > > > > > > > This is a problem with bgpd as well, last time I did this it killed one > > > > of the > > > > *other* routers on the network (i.e. not just the one where I > > > > accidentally ran > > > > 2x bgpd...). > > > > > > > > > What then happend was not nice: > > > > > - The new ospfd unlinked the control socket of the first ospfd > > > > > - The new ospfd removed all routes from the first ospfd > > > > > - The new ospfd was not able to build up an adjacency and therefore > > > > > could > > > > > not install the routes needed for a recovery. > > > > > - Both ospfd instances were running but non-functional. > > > > > > > > > > Of course the faulty script is fixed by now. ;-) > > > > > > > > > > It would be nice if ospfd could prevent such a situation. > > > > > > > > > > Below diff does these things: > > > > > - Detect a running ospfd by first doing a connect on the control > > > > > socket. > > > > > - Do not delete the control socket on exit. > > > > > - This could delete the socket of another instance. > > > > > - Unlinking the socket on shutdown will be in the way once we add > > > > > pledge > > > > > to the main process. It was removed recently from various daemons. > > > > > > > > This all sounds very sensible. > > > > > > > > > - Do not delete routes added by another process even if they have > > > > > prio RTP_OSPF. Without this the new ospfd will remove all the routes > > > > > of the first one. > > > > > > > > I'm unsure about this, the above changes stop the new ospfd from running > > > > don't they, so that shouldn't be a problem? > > > > > > It stops to late. kr_init happens before and kill all existing routes with > > > priority 32. And again in the shutdown function of ospfd. > > > > > > > > If an ospfd blows up for whatever reason, it would be quite inconvenient > > > > if it needs manual route tweaks rather than just 'rcctl start ospfd' to > > > > fix it .. > > > > > > Yes, this is not optimal. > > > > > > The new diff below defers kr_init until the ospf engine notifies the > > > parent > > > that the control socket is ready. In case the ospf engine exits because > > > the > > > control socket is already in use no routes are known that could be > > > removed. > > > > > > With this ospfd keeps the behaviour of removing foreign routes with > > > priority 32. > > > > > > Better? > > > > > > > Why are we not checking the control socket in the parent? > > Also it may be better to create the control socket in the parent and pass > > it to the ospfe. This is what bgpd is doing and allows to change the path > > during runtime with a config reload. > > This makes sense to me. I'll come up with a new diff once I found some > time for it. > > But I'm not sure about changing the socket path with a reload. I plan to > pledge (stdio rpath sendfd wroute) and eventually unveil (read ospfd.conf) > the main process. New diff below creates the control socket in the main process and passes it to the ospf engine later on. The connect check on the control socket now happens very early. The diff in action looks like this: typhoon ..sbin/ospfd$ doas obj/ospfd -dv startup control_init: socket in use fatal in ospfd: control socket setup failed typhoon 1 ..sbin/ospfd$ I borrowed the fd passing code from slaacd. > > > > > Could there be a case where this causes ospfd to hang on start in the > > connect? Not sure if we can sleep doing a connect() to a AF_UNIX socket. I never observed a hangin ospfctl which also does a connect on the control
Re: ospfd: prevent additional ospfd from starting
On Fri, Aug 24, 2018 at 08:58:12AM +0200, Claudio Jeker wrote: > On Wed, Aug 22, 2018 at 12:12:10AM +0200, Remi Locherer wrote: > > On Tue, Aug 21, 2018 at 05:54:18PM +0100, Stuart Henderson wrote: > > > On 2018/08/21 17:16, Remi Locherer wrote: > > > > Hi tech, > > > > > > > > recently we had a short outage in our network. A script started an > > > > additional > > > > ospfd instance because the -n flag for config test was missing. > > > > > > This is a problem with bgpd as well, last time I did this it killed one > > > of the > > > *other* routers on the network (i.e. not just the one where I > > > accidentally ran > > > 2x bgpd...). > > > > > > > What then happend was not nice: > > > > - The new ospfd unlinked the control socket of the first ospfd > > > > - The new ospfd removed all routes from the first ospfd > > > > - The new ospfd was not able to build up an adjacency and therefore > > > > could > > > > not install the routes needed for a recovery. > > > > - Both ospfd instances were running but non-functional. > > > > > > > > Of course the faulty script is fixed by now. ;-) > > > > > > > > It would be nice if ospfd could prevent such a situation. > > > > > > > > Below diff does these things: > > > > - Detect a running ospfd by first doing a connect on the control socket. > > > > - Do not delete the control socket on exit. > > > > - This could delete the socket of another instance. > > > > - Unlinking the socket on shutdown will be in the way once we add > > > > pledge > > > > to the main process. It was removed recently from various daemons. > > > > > > This all sounds very sensible. > > > > > > > - Do not delete routes added by another process even if they have > > > > prio RTP_OSPF. Without this the new ospfd will remove all the routes > > > > of the first one. > > > > > > I'm unsure about this, the above changes stop the new ospfd from running > > > don't they, so that shouldn't be a problem? > > > > It stops to late. kr_init happens before and kill all existing routes with > > priority 32. And again in the shutdown function of ospfd. > > > > > > If an ospfd blows up for whatever reason, it would be quite inconvenient > > > if it needs manual route tweaks rather than just 'rcctl start ospfd' to > > > fix it .. > > > > Yes, this is not optimal. > > > > The new diff below defers kr_init until the ospf engine notifies the parent > > that the control socket is ready. In case the ospf engine exits because the > > control socket is already in use no routes are known that could be removed. > > > > With this ospfd keeps the behaviour of removing foreign routes with > > priority 32. > > > > Better? > > > > Why are we not checking the control socket in the parent? > Also it may be better to create the control socket in the parent and pass > it to the ospfe. This is what bgpd is doing and allows to change the path > during runtime with a config reload. This makes sense to me. I'll come up with a new diff once I found some time for it. But I'm not sure about changing the socket path with a reload. I plan to pledge (stdio rpath sendfd wroute) and eventually unveil (read ospfd.conf) the main process. > > Could there be a case where this causes ospfd to hang on start in the > connect? Not sure if we can sleep doing a connect() to a AF_UNIX socket.
Re: ospfd: prevent additional ospfd from starting
On Wed, Aug 22, 2018 at 12:12:10AM +0200, Remi Locherer wrote: > On Tue, Aug 21, 2018 at 05:54:18PM +0100, Stuart Henderson wrote: > > On 2018/08/21 17:16, Remi Locherer wrote: > > > Hi tech, > > > > > > recently we had a short outage in our network. A script started an > > > additional > > > ospfd instance because the -n flag for config test was missing. > > > > This is a problem with bgpd as well, last time I did this it killed one of > > the > > *other* routers on the network (i.e. not just the one where I accidentally > > ran > > 2x bgpd...). > > > > > What then happend was not nice: > > > - The new ospfd unlinked the control socket of the first ospfd > > > - The new ospfd removed all routes from the first ospfd > > > - The new ospfd was not able to build up an adjacency and therefore could > > > not install the routes needed for a recovery. > > > - Both ospfd instances were running but non-functional. > > > > > > Of course the faulty script is fixed by now. ;-) > > > > > > It would be nice if ospfd could prevent such a situation. > > > > > > Below diff does these things: > > > - Detect a running ospfd by first doing a connect on the control socket. > > > - Do not delete the control socket on exit. > > > - This could delete the socket of another instance. > > > - Unlinking the socket on shutdown will be in the way once we add pledge > > > to the main process. It was removed recently from various daemons. > > > > This all sounds very sensible. > > > > > - Do not delete routes added by another process even if they have > > > prio RTP_OSPF. Without this the new ospfd will remove all the routes > > > of the first one. > > > > I'm unsure about this, the above changes stop the new ospfd from running > > don't they, so that shouldn't be a problem? > > It stops to late. kr_init happens before and kill all existing routes with > priority 32. And again in the shutdown function of ospfd. > > > > If an ospfd blows up for whatever reason, it would be quite inconvenient > > if it needs manual route tweaks rather than just 'rcctl start ospfd' to fix > > it .. > > Yes, this is not optimal. > > The new diff below defers kr_init until the ospf engine notifies the parent > that the control socket is ready. In case the ospf engine exits because the > control socket is already in use no routes are known that could be removed. > > With this ospfd keeps the behaviour of removing foreign routes with > priority 32. > > Better? > Why are we not checking the control socket in the parent? Also it may be better to create the control socket in the parent and pass it to the ospfe. This is what bgpd is doing and allows to change the path during runtime with a config reload. Could there be a case where this causes ospfd to hang on start in the connect? Not sure if we can sleep doing a connect() to a AF_UNIX socket. > Index: control.c > === > RCS file: /cvs/src/usr.sbin/ospfd/control.c,v > retrieving revision 1.44 > diff -u -p -r1.44 control.c > --- control.c 24 Jan 2017 04:24:25 - 1.44 > +++ control.c 17 Aug 2018 22:41:43 - > @@ -42,19 +42,29 @@ int > control_init(char *path) > { > struct sockaddr_un sun; > - int fd; > + int fd, fd_check; > mode_t old_umask; > > + bzero(&sun, sizeof(sun)); > + sun.sun_family = AF_UNIX; > + strlcpy(sun.sun_path, path, sizeof(sun.sun_path)); > + > + if ((fd_check = socket(AF_UNIX, SOCK_STREAM, 0)) == -1) { > + log_warn("control_init: socket check"); > + return (-1); > + } > + if (connect(fd_check, (struct sockaddr *)&sun, sizeof(sun)) == 0) { > + log_warnx("control_init: socket in use"); > + return (-1); > + } > + close(fd_check); > + > if ((fd = socket(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC | SOCK_NONBLOCK, > 0)) == -1) { > log_warn("control_init: socket"); > return (-1); > } > > - bzero(&sun, sizeof(sun)); > - sun.sun_family = AF_UNIX; > - strlcpy(sun.sun_path, path, sizeof(sun.sun_path)); > - > if (unlink(path) == -1) > if (errno != ENOENT) { > log_warn("control_init: unlink %s", path); > @@ -98,16 +108,6 @@ control_listen(void) > evti
Re: ospfd: prevent additional ospfd from starting
On Tue, Aug 21, 2018 at 05:54:18PM +0100, Stuart Henderson wrote: > On 2018/08/21 17:16, Remi Locherer wrote: > > Hi tech, > > > > recently we had a short outage in our network. A script started an > > additional > > ospfd instance because the -n flag for config test was missing. > > This is a problem with bgpd as well, last time I did this it killed one of the > *other* routers on the network (i.e. not just the one where I accidentally ran > 2x bgpd...). > > > What then happend was not nice: > > - The new ospfd unlinked the control socket of the first ospfd > > - The new ospfd removed all routes from the first ospfd > > - The new ospfd was not able to build up an adjacency and therefore could > > not install the routes needed for a recovery. > > - Both ospfd instances were running but non-functional. > > > > Of course the faulty script is fixed by now. ;-) > > > > It would be nice if ospfd could prevent such a situation. > > > > Below diff does these things: > > - Detect a running ospfd by first doing a connect on the control socket. > > - Do not delete the control socket on exit. > > - This could delete the socket of another instance. > > - Unlinking the socket on shutdown will be in the way once we add pledge > > to the main process. It was removed recently from various daemons. > > This all sounds very sensible. > > > - Do not delete routes added by another process even if they have > > prio RTP_OSPF. Without this the new ospfd will remove all the routes > > of the first one. > > I'm unsure about this, the above changes stop the new ospfd from running > don't they, so that shouldn't be a problem? It stops to late. kr_init happens before and kill all existing routes with priority 32. And again in the shutdown function of ospfd. > > If an ospfd blows up for whatever reason, it would be quite inconvenient > if it needs manual route tweaks rather than just 'rcctl start ospfd' to fix > it .. Yes, this is not optimal. The new diff below defers kr_init until the ospf engine notifies the parent that the control socket is ready. In case the ospf engine exits because the control socket is already in use no routes are known that could be removed. With this ospfd keeps the behaviour of removing foreign routes with priority 32. Better? Index: control.c === RCS file: /cvs/src/usr.sbin/ospfd/control.c,v retrieving revision 1.44 diff -u -p -r1.44 control.c --- control.c 24 Jan 2017 04:24:25 - 1.44 +++ control.c 17 Aug 2018 22:41:43 - @@ -42,19 +42,29 @@ int control_init(char *path) { struct sockaddr_un sun; - int fd; + int fd, fd_check; mode_t old_umask; + bzero(&sun, sizeof(sun)); + sun.sun_family = AF_UNIX; + strlcpy(sun.sun_path, path, sizeof(sun.sun_path)); + + if ((fd_check = socket(AF_UNIX, SOCK_STREAM, 0)) == -1) { + log_warn("control_init: socket check"); + return (-1); + } + if (connect(fd_check, (struct sockaddr *)&sun, sizeof(sun)) == 0) { + log_warnx("control_init: socket in use"); + return (-1); + } + close(fd_check); + if ((fd = socket(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC | SOCK_NONBLOCK, 0)) == -1) { log_warn("control_init: socket"); return (-1); } - bzero(&sun, sizeof(sun)); - sun.sun_family = AF_UNIX; - strlcpy(sun.sun_path, path, sizeof(sun.sun_path)); - if (unlink(path) == -1) if (errno != ENOENT) { log_warn("control_init: unlink %s", path); @@ -98,16 +108,6 @@ control_listen(void) evtimer_set(&control_state.evt, control_accept, NULL); return (0); -} - -void -control_cleanup(char *path) -{ - if (path == NULL) - return; - event_del(&control_state.ev); - event_del(&control_state.evt); - unlink(path); } /* ARGSUSED */ Index: control.h === RCS file: /cvs/src/usr.sbin/ospfd/control.h,v retrieving revision 1.6 diff -u -p -r1.6 control.h --- control.h 10 Feb 2015 05:24:48 - 1.6 +++ control.h 17 Aug 2018 21:02:36 - @@ -39,6 +39,5 @@ int control_listen(void); void control_accept(int, short, void *); void control_dispatch_imsg(int, short, void *); intcontrol_imsg_relay(struct imsg *); -void control_cleanup(char *); #endif /* _CONTROL_H_ */ Index: ospfd.c === RCS file: /
Re: ospfd: prevent additional ospfd from starting
On Tue, Aug 21, 2018 at 05:16:47PM +0200, Remi Locherer wrote: > Hi tech, > > recently we had a short outage in our network. A script started an additional > ospfd instance because the -n flag for config test was missing. > > What then happend was not nice: > - The new ospfd unlinked the control socket of the first ospfd > - The new ospfd removed all routes from the first ospfd > - The new ospfd was not able to build up an adjacency and therefore could > not install the routes needed for a recovery. > - Both ospfd instances were running but non-functional. > > Of course the faulty script is fixed by now. ;-) > > It would be nice if ospfd could prevent such a situation. > > Below diff does these things: > - Detect a running ospfd by first doing a connect on the control socket. > - Do not delete the control socket on exit. > - This could delete the socket of another instance. > - Unlinking the socket on shutdown will be in the way once we add pledge > to the main process. It was removed recently from various daemons. > - Do not delete routes added by another process even if they have > prio RTP_OSPF. Without this the new ospfd will remove all the routes > of the first one. > > A side effect of this is that alien OSPF routes are now only logged but > not removed anymore. Should a crashed ospfd leave some routes behind the > next ospfd does not clean them up anymore. The admin would need to check > the logs and remove them manually with the route command. > > Does this make sense? > Manually removing routes does not :)