The nexthop validation or actually invalidation is buggy in bgpd since
revision 1.90 of rde_decide.c. When I removed re->active and replaced it
with a value that is calculated on the spot I did not realize that this
calculation depends on the current nexthop state and not on the state used
on the previous decision process.
The decision process used prefix_best() to get the previous best prefix.
Now if that prefix just became invalid (because of a nexthop state change)
prefix_best() will return NULL and oldbest and newbest will be NULL too.
Because of this no update will be sent out because prefix_evaluate()
thinks nothing changed.
This is also the reason why prefix_set_dmetric() sees bad dmetrics during
nexthop flaps.
To solve this issue I decided to introduce a NEXTHOP_VALID flag in struct
prefix (as part of the nhflags). Which is then used to validate the
nexthop state. When a nexthop changes state nexthop_runner calls
prefix_evaluate_nexthop() which adjust the NEXTHOP_VALID flag but this is
done after removing the prefix from the evaluation list and before
inserting it again. Because of this prefix_best() works correctly and
oldbest is not NULL in the case mentioned above.
I decided to set the initial state of NEXTHOP_VALID in nexthop_link() and I
clear it in nexthop_unlink(). The link call happens after nhflags is set
and prefix_nhflags() masks out the NEXTHOP_VALID flag.
With this nexthops that turn invalid are properly removed from the FIB and
Adj-RIB-Out.
--
:wq Claudio
Index: rde.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v
retrieving revision 1.558
diff -u -p -r1.558 rde.c
--- rde.c 23 Jul 2022 08:44:06 -0000 1.558
+++ rde.c 25 Jul 2022 08:23:18 -0000
@@ -4249,8 +4249,7 @@ network_dump_upcall(struct rib_entry *re
bzero(&kf, sizeof(kf));
memcpy(&kf.prefix, &addr, sizeof(kf.prefix));
- if (prefix_nexthop(p) == NULL ||
- prefix_nexthop(p)->state != NEXTHOP_REACH)
+ if (prefix_nhvalid(p))
kf.nexthop.aid = kf.prefix.aid;
else
memcpy(&kf.nexthop, &prefix_nexthop(p)->true_nexthop,
Index: rde.h
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde.h,v
retrieving revision 1.259
diff -u -p -r1.259 rde.h
--- rde.h 11 Jul 2022 17:08:21 -0000 1.259
+++ rde.h 25 Jul 2022 08:28:38 -0000
@@ -362,6 +362,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;
@@ -522,6 +524,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 *,
@@ -663,7 +667,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: rde_decide.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde_decide.c,v
retrieving revision 1.96
diff -u -p -r1.96 rde_decide.c
--- rde_decide.c 18 Jul 2022 09:42:46 -0000 1.96
+++ rde_decide.c 25 Jul 2022 09:24:14 -0000
@@ -147,28 +147,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 -rv;
- if (asp2 == NULL || asp2->flags & F_ATTR_PARSE_ERR)
- return rv;
-
- /* only loop free pathes are eligible */
- if (asp1->flags & F_ATTR_LOOP)
- return -rv;
- if (asp2->flags & F_ATTR_LOOP)
- return rv;
-
- /*
- * 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 rv;
- if (prefix_nexthop(p1) != NULL &&
- prefix_nexthop(p1)->state != NEXTHOP_REACH)
+ if (!prefix_eligible(p1))
return -rv;
/* bump rv, from here on prefix is considered valid */
@@ -503,16 +485,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;
@@ -561,16 +540,11 @@ prefix_evaluate(struct rib_entry *re, st
}
oldbest = prefix_best(re);
-
if (old != NULL)
prefix_remove(old, re);
-
if (new != NULL)
prefix_insert(new, NULL, re);
-
- newbest = TAILQ_FIRST(&re->prefix_h);
- if (newbest != NULL && !prefix_eligible(newbest))
- newbest = NULL;
+ newbest = prefix_best(re);
/*
* If the active prefix changed or the active prefix was removed
@@ -596,4 +570,75 @@ prefix_evaluate(struct rib_entry *re, st
if (rde_evaluate_all())
if ((new != NULL && prefix_eligible(new)) || old != NULL)
rde_generate_updates(rib, newbest, NULL, EVAL_ALL);
+}
+
+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, EVAL_DEFAULT);
+ 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, EVAL_ALL);
}
Index: rde_rib.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde_rib.c,v
retrieving revision 1.240
diff -u -p -r1.240 rde_rib.c
--- rde_rib.c 8 Jul 2022 10:01:52 -0000 1.240
+++ rde_rib.c 25 Jul 2022 09:35:17 -0000
@@ -1521,37 +1521,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)
@@ -1724,7 +1693,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);
}
@@ -1826,15 +1795,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);
}
@@ -1842,6 +1820,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;
