From: Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject: Re: bgpd: rework rde_enqueue_updates() to simplify the rib queue handling
To: Theo Buehler <tb@theobuehler.org>
Cc: tech@openbsd.org
Date: Wed, 1 Jul 2026 11:09:52 +0200

On Wed, Jul 01, 2026 at 10:24:04AM +0200, Theo Buehler wrote:
> > > > @@ -588,7 +591,7 @@ prefix_evaluate(struct rib_entry *re, st
> > > >  		/* no old path to remove and path is ineligible, skip rest */
> > > >  		if (old_pathid_tx == 0 && new == NULL)
> > > >  			return;
> > > > -		rde_enqueue_updates(re, new, old_pathid_tx, EVAL_ALL);
> > > > +		rde_enqueue_updates(re, peer, new, old_pathid_tx, EVAL_ALL);
> > > 
> > > While at this point we know old_pathid_tx != 0 || new != NULL, so we
> > > know that peer has been initialized, this isn't super obvious and far
> > > beyond gcc's deduction capabilities (both gcc 4.2.1 and 15.1 whine).
> > > 
> > > I don't see a good fix (if we do want to fix this). Doing peer = NULL
> > > at the top and changing the above to
> > > 
> > >  		if ((old_pathid_tx == 0 && new == NULL) || peer == NULL)
> > > 			return;
> > > 
> > > doesn't look like an improvement to me...
> > 
> > Hmmm. I'll have a closer look. We certainly can initalize peer = NULL at
> > the top. Not sure if we need to check for peer == NULL though.
> 
> Then the "problem" becomes that rde_enqueue_updates(..., EVAL_ALL) may
> deref peer, which again isn't obviously excluded.

I would like to commit this version of prefix_evaluate().

This adds an extra check that new and old are not both NULL and sets peer
to NULL. Also use old == NULL instead of old_pathid_tx == 0 in the later
check. The rest of the function also checks on old.

-- 
:wq Claudio

Index: rde_decide.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde_decide.c,v
diff -u -p -r1.108 rde_decide.c
--- rde_decide.c	21 May 2026 15:20:27 -0000	1.108
+++ rde_decide.c	1 Jul 2026 08:46:11 -0000
@@ -534,9 +534,13 @@ void
 prefix_evaluate(struct rib_entry *re, struct prefix *new, struct prefix *old)
 {
 	struct prefix	*newbest, *oldbest;
+	struct rde_peer	*peer = NULL;
 	struct rib	*rib;
 	uint32_t	 old_pathid_tx = 0;
 
+	if (old == NULL && new == NULL)
+		fatalx("king bula sez: nothing to evaluate");
+
 	rib = re_rib(re);
 	if (rib->flags & F_RIB_NOEVALUATE) {
 		/* decision process is turned off */
@@ -553,6 +557,7 @@ prefix_evaluate(struct rib_entry *re, st
 	if (old != NULL) {
 		prefix_remove(old, re);
 		old_pathid_tx = old->path_id_tx;
+		peer = prefix_peer(old);
 	}
 	if (new != NULL) {
 		prefix_insert(new, NULL, re);
@@ -560,6 +565,7 @@ prefix_evaluate(struct rib_entry *re, st
 			new = NULL;
 		else
 			old_pathid_tx = 0;
+		peer = prefix_peer(new);
 	}
 	newbest = prefix_best(re);
 
@@ -575,7 +581,7 @@ prefix_evaluate(struct rib_entry *re, st
 		 */
 		if ((rib->flags & F_RIB_NOFIB) == 0)
 			rde_send_kroute(rib, newbest, oldbest);
-		rde_enqueue_updates(re, new, old_pathid_tx, EVAL_DEFAULT);
+		rde_enqueue_updates(re, peer, new, old_pathid_tx, EVAL_DEFAULT);
 		return;
 	}
 
@@ -586,9 +592,9 @@ prefix_evaluate(struct rib_entry *re, st
 	 */
 	if (rde_evaluate_all()) {
 		/* no old path to remove and path is ineligible, skip rest */
-		if (old_pathid_tx == 0 && new == NULL)
+		if (old == NULL && new == NULL)
 			return;
-		rde_enqueue_updates(re, new, old_pathid_tx, EVAL_ALL);
+		rde_enqueue_updates(re, peer, new, old_pathid_tx, EVAL_ALL);
 	}
 }