On Wed, Jul 01, 2026 at 11:30:05AM +0200, Claudio Jeker wrote:
> 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.

Great, thanks. Go ahead then :)

> 
> > > 
> > > -- 
> > > :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