untrusted comment: verify with openbsd-71-base.pub RWR2eHwZTOEiTSfgYxxQkgK3ygNXvoqXAblB1qJlG9cOGU7Ao1Z6+lUASLgYq5NycH8kgrAN5PMfn/MWPaftIcKQ5oHJO8afrQU= OpenBSD 7.1 errata 008, August 2, 2022: bgpd(8) could fail to invalidate nexthops and incorrectly leave them in the FIB or Adj-RIB-Out. Apply by doing: signify -Vep /etc/signify/openbsd-71-base.pub -x 008_bgpd.patch.sig \ -m - | (cd /usr/src && patch -p0) And then rebuild and install bgpd(8): cd /usr/src/usr.sbin/bgpd make obj make make install Index: usr.sbin/bgpd/rde.c =================================================================== RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v retrieving revision 1.544 diff -u -p -r1.544 rde.c --- usr.sbin/bgpd/rde.c 22 Mar 2022 10:53:08 -0000 1.544 +++ usr.sbin/bgpd/rde.c 25 Jul 2022 12:57:14 -0000 @@ -4170,8 +4170,7 @@ network_dump_upcall(struct rib_entry *re bzero(&k, sizeof(k)); memcpy(&k.prefix, &addr, sizeof(k.prefix)); - if (prefix_nexthop(p) == NULL || - prefix_nexthop(p)->state != NEXTHOP_REACH) + if (prefix_nhvalid(p)) k.nexthop.aid = k.prefix.aid; else memcpy(&k.nexthop, &prefix_nexthop(p)->true_nexthop, Index: usr.sbin/bgpd/rde.h =================================================================== RCS file: /cvs/src/usr.sbin/bgpd/rde.h,v retrieving revision 1.251 diff -u -p -r1.251 rde.h --- usr.sbin/bgpd/rde.h 22 Mar 2022 10:53:08 -0000 1.251 +++ usr.sbin/bgpd/rde.h 25 Jul 2022 12:44:00 -0000 @@ -352,6 +352,8 @@ struct prefix { #define NEXTHOP_REJECT 0x02 #define NEXTHOP_BLACKHOLE 0x04 #define NEXTHOP_NOMODIFY 0x08 +#define NEXTHOP_MASK 0x0f +#define NEXTHOP_VALID 0x80 struct filterstate { struct rde_aspath aspath; @@ -504,6 +506,8 @@ int prefix_eligible(struct prefix *); struct prefix *prefix_best(struct rib_entry *); void prefix_evaluate(struct rib_entry *, struct prefix *, struct prefix *); +void prefix_evaluate_nexthop(struct prefix *, enum nexthop_state, + enum nexthop_state); /* rde_filter.c */ void rde_apply_set(struct filter_set_head *, struct rde_peer *, @@ -643,7 +647,13 @@ prefix_nexthop(struct prefix *p) static inline uint8_t prefix_nhflags(struct prefix *p) { - return (p->nhflags); + return (p->nhflags & NEXTHOP_MASK); +} + +static inline int +prefix_nhvalid(struct prefix *p) +{ + return ((p->nhflags & NEXTHOP_VALID) != 0); } static inline uint8_t Index: usr.sbin/bgpd/rde_decide.c =================================================================== RCS file: /cvs/src/usr.sbin/bgpd/rde_decide.c,v retrieving revision 1.91 diff -u -p -r1.91 rde_decide.c --- usr.sbin/bgpd/rde_decide.c 22 Mar 2022 10:53:08 -0000 1.91 +++ usr.sbin/bgpd/rde_decide.c 25 Jul 2022 13:20:50 -0000 @@ -138,28 +138,10 @@ prefix_cmp(struct prefix *p1, struct pre peer1 = prefix_peer(p1); peer2 = prefix_peer(p2); - /* pathes with errors are not eligible */ - if (asp1 == NULL || asp1->flags & F_ATTR_PARSE_ERR) - return -1; - if (asp2 == NULL || asp2->flags & F_ATTR_PARSE_ERR) - return 1; - - /* only loop free pathes are eligible */ - if (asp1->flags & F_ATTR_LOOP) - return -1; - if (asp2->flags & F_ATTR_LOOP) - return 1; - - /* - * 1. check if prefix is eligible a.k.a reachable - * A NULL nexthop is eligible since it is used for locally - * announced networks. - */ - if (prefix_nexthop(p2) != NULL && - prefix_nexthop(p2)->state != NEXTHOP_REACH) + /* 1. check if prefix is eligible a.k.a reachable */ + if (!prefix_eligible(p2)) return 1; - if (prefix_nexthop(p1) != NULL && - prefix_nexthop(p1)->state != NEXTHOP_REACH) + if (!prefix_eligible(p1)) return -1; /* 2. local preference of prefix, bigger is better */ @@ -416,16 +398,13 @@ int prefix_eligible(struct prefix *p) { struct rde_aspath *asp = prefix_aspath(p); - struct nexthop *nh = prefix_nexthop(p); /* The aspath needs to be loop and error free */ if (asp == NULL || asp->flags & (F_ATTR_LOOP|F_ATTR_PARSE_ERR)) return 0; - /* - * If the nexthop exists it must be reachable. - * It is OK if the nexthop does not exist (local announcement). - */ - if (nh != NULL && nh->state != NEXTHOP_REACH) + + /* The nexthop must be valid. */ + if (!prefix_nhvalid(p)) return 0; return 1; @@ -472,16 +451,11 @@ prefix_evaluate(struct rib_entry *re, st } active = prefix_best(re); - if (old != NULL) prefix_remove(old, re); - if (new != NULL) prefix_insert(new, NULL, re); - - xp = TAILQ_FIRST(&re->prefix_h); - if (xp != NULL && !prefix_eligible(xp)) - xp = NULL; + xp = prefix_best(re); /* * If the active prefix changed or the active prefix was removed @@ -506,5 +480,76 @@ prefix_evaluate(struct rib_entry *re, st */ if (rde_evaluate_all()) if ((new != NULL && prefix_eligible(new)) || old != NULL) - rde_generate_updates(rib, prefix_best(re), NULL, 1); + rde_generate_updates(rib, xp, NULL, 1); } + +void +prefix_evaluate_nexthop(struct prefix *p, enum nexthop_state state, + enum nexthop_state oldstate) +{ + struct rib_entry *re = prefix_re(p); + struct prefix *newbest, *oldbest; + struct rib *rib; + + /* Skip non local-RIBs or RIBs that are flagged as noeval. */ + rib = re_rib(re); + if (rib->flags & F_RIB_NOEVALUATE) { + log_warnx("%s: prefix with F_RIB_NOEVALUATE hit", __func__); + return; + } + + if (oldstate == state) { + /* + * The state of the nexthop did not change. The only + * thing that may have changed is the true_nexthop + * or other internal infos. This will not change + * the routing decision so shortcut here. + * XXX needs to be changed for ECMP + */ + if (state == NEXTHOP_REACH) { + if ((rib->flags & F_RIB_NOFIB) == 0 && + p == prefix_best(re)) + rde_send_kroute(rib, p, NULL); + } + return; + } + + /* + * Re-evaluate the prefix by removing the prefix then updating the + * nexthop state and reinserting the prefix again. + */ + oldbest = prefix_best(re); + prefix_remove(p, re); + + if (state == NEXTHOP_REACH) + p->nhflags |= NEXTHOP_VALID; + else + p->nhflags &= ~NEXTHOP_VALID; + + prefix_insert(p, NULL, re); + newbest = prefix_best(re); + + /* + * If the active prefix changed or the active prefix was removed + * and added again then generate an update. + */ + if (oldbest != newbest || newbest == p) { + /* + * Send update withdrawing oldbest and adding newbest + * but remember that newbest may be NULL aka ineligible. + * Additional decision may be made by the called functions. + */ + rde_generate_updates(rib, newbest, oldbest, 0); + if ((rib->flags & F_RIB_NOFIB) == 0) + rde_send_kroute(rib, newbest, oldbest); + return; + } + + /* + * If there are peers with 'rde evaluate all' every update needs + * to be passed on (not only a change of the best prefix). + * rde_generate_updates() will then take care of distribution. + */ + if (rde_evaluate_all()) + rde_generate_updates(rib, newbest, NULL, 1); + } Index: usr.sbin/bgpd/rde_rib.c =================================================================== RCS file: /cvs/src/usr.sbin/bgpd/rde_rib.c,v retrieving revision 1.237 diff -u -p -r1.237 rde_rib.c --- usr.sbin/bgpd/rde_rib.c 22 Mar 2022 10:53:08 -0000 1.237 +++ usr.sbin/bgpd/rde_rib.c 25 Jul 2022 12:54:13 -0000 @@ -1502,37 +1502,6 @@ prefix_bypeer(struct rib_entry *re, stru return (NULL); } -static void -prefix_evaluate_all(struct prefix *p, enum nexthop_state state, - enum nexthop_state oldstate) -{ - struct rib_entry *re = prefix_re(p); - - /* Skip non local-RIBs or RIBs that are flagged as noeval. */ - if (re_rib(re)->flags & F_RIB_NOEVALUATE) { - log_warnx("%s: prefix with F_RIB_NOEVALUATE hit", __func__); - return; - } - - if (oldstate == state) { - /* - * The state of the nexthop did not change. The only - * thing that may have changed is the true_nexthop - * or other internal infos. This will not change - * the routing decision so shortcut here. - */ - if (state == NEXTHOP_REACH) { - if ((re_rib(re)->flags & F_RIB_NOFIB) == 0 && - p == prefix_best(re)) - rde_send_kroute(re_rib(re), p, NULL); - } - return; - } - - /* redo the route decision */ - prefix_evaluate(prefix_re(p), p, p); -} - /* kill a prefix. */ void prefix_destroy(struct prefix *p) @@ -1704,7 +1673,7 @@ nexthop_runner(void) p = nh->next_prefix; for (j = 0; p != NULL && j < RDE_RUNNER_ROUNDS; j++) { - prefix_evaluate_all(p, nh->state, nh->oldstate); + prefix_evaluate_nexthop(p, nh->state, nh->oldstate); p = LIST_NEXT(p, entry.list.nexthop); } @@ -1806,15 +1775,24 @@ nexthop_modify(struct nexthop *setnh, en void nexthop_link(struct prefix *p) { - if (p->nexthop == NULL) - return; - if (p->flags & PREFIX_FLAG_ADJOUT) + p->nhflags &= ~NEXTHOP_VALID; + + if (p->flags & PREFIX_FLAG_ADJOUT) { + /* All nexthops are valid in Adj-RIB-Out */ + p->nhflags |= NEXTHOP_VALID; return; + } /* no need to link prefixes in RIBs that have no decision process */ if (re_rib(prefix_re(p))->flags & F_RIB_NOEVALUATE) return; + /* self-announce networks use nexthop NULL and are valid as well */ + if (p->nexthop == NULL || p->nexthop->state == NEXTHOP_REACH) + p->nhflags |= NEXTHOP_VALID; + + if (p->nexthop == NULL) + return; p->flags |= PREFIX_NEXTHOP_LINKED; LIST_INSERT_HEAD(&p->nexthop->prefix_h, p, entry.list.nexthop); } @@ -1822,6 +1800,8 @@ nexthop_link(struct prefix *p) void nexthop_unlink(struct prefix *p) { + p->nhflags &= ~NEXTHOP_VALID; + if (p->nexthop == NULL || (p->flags & PREFIX_NEXTHOP_LINKED) == 0) return;