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:30:05 +0200

On Wed, Jul 01, 2026 at 11:19:22AM +0200, Theo Buehler wrote:
> On Wed, Jul 01, 2026 at 11:09:52AM +0200, Claudio Jeker wrote:
> > 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.
> 
> Makes sense. One thing: old_pathid_tx != 0 implies old != NULL, but not
> necessarily the other way around (it could be that old->path_id_tx == 0).
> If this doesn't matter or is not the case, I think this is a good fix.

old->path_id_tx must be != 0 since 0 is a special marker meaning that
there is no path. Valid path_id_tx values are all != 0.

> > 
> > -- 
> > :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);
> >  	}
> >  }
> >  
> 

-- 
:wq Claudio