From: Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject: Re: bgpd: better sync template clones in merge_peers
To: Theo Buehler <tb@theobuehler.org>
Cc: tech@openbsd.org
Date: Mon, 11 May 2026 22:30:41 +0200

On Mon, May 11, 2026 at 09:23:46PM +0200, Theo Buehler wrote:
> On Mon, May 11, 2026 at 09:08:01PM +0200, Claudio Jeker wrote:
> > When introducing local_bgpid in the session code the case to update cloned
> > peers was missed. merge_peers only updates regular peers and templates.
> > 
> > Note: session_template_clone() copies the template->conf into conf so all
> > the other fixups inside conf (holdtime, etc) get taken over. The problem
> > is that local_bgpid is not inside conf and so this was missed.
> > Another option to the diff below is to do the local_bgpid in
> > session_template_clone() by copying peer->template->local_bgpid into
> > peer->local_bgpid. It is a bit less direct than the diff below, not sure
> > if it is worth it.
> 
> Your call. The below seems fine. If I understand correctly, it doesn't
> matter for the second session_template_clone() call where the
> local_peerip is copied from loose in the memcpy already.

In the call in getpeerbyip() the session_template_clone() is followed by
init_peer() which does set local_bgpid. So there it really doesn't matter.
In my view session_template_clone() is really just a fixup function that
adjusts the template config for a single cloned peer. Setting the AS
number, ebgp / ibgp settings, etc.

So adjusting local_bgpid there feels a bit off to me.
 
> ok for this diff

Thanks.
 
> > 
> > This should fix this oversight.
> > -- 
> > :wq Claudio
> > 
> > Index: session.c
> > ===================================================================
> > RCS file: /cvs/src/usr.sbin/bgpd/session.c,v
> > diff -u -p -r1.534 session.c
> > --- session.c	8 May 2026 12:03:50 -0000	1.534
> > +++ session.c	11 May 2026 19:02:43 -0000
> > @@ -1968,6 +1968,8 @@ merge_peers(struct bgpd_config *c, struc
> >  				session_template_clone(xp, NULL, xp->conf.id,
> >  				    xp->conf.remote_as);
> >  
> > +				xp->local_bgpid = nc->bgpid;
> > +
> >  				if (xp->rdesession)
> >  					imsg_rde(IMSG_SESSION_ADD,
> >  					    xp->conf.id, &xp->conf,
> > 
> 

-- 
:wq Claudio