From: Theo Buehler <tb@theobuehler.org>
Subject: Re: bgpd: rework rde_enqueue_updates() to simplify the rib queue handling
To: tech@openbsd.org
Date: Wed, 1 Jul 2026 11:19:22 +0200

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.

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