From: Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject: Re: bgpd: trigger session down timer during graceful restart
To: Theo Buehler <tb@theobuehler.org>
Cc: tech@openbsd.org
Date: Thu, 7 May 2026 20:00:04 +0200

On Thu, May 07, 2026 at 05:43:03PM +0200, Theo Buehler wrote:
> On Thu, May 07, 2026 at 04:19:25PM +0200, Claudio Jeker wrote:
> > When a peer goes down it can either call session_down() or go into
> > session_graceful_restart() to do a graceful reload.
> > 
> > In session_down the Timer_SessionDown timer is armed and will fire in 1h.
> > In the GR case this is not done. If the session fails to recover the
> > RDE will keep on syncing its Adj-RIB-Out which is not helpful. Also
> > templated peers are no longer cleaned up.
> > 
> > Now the maximum GR timeout is 4095 (since it is a 12bit field) and that is
> > longer than INTERVAL_SESSION_DOWN. So do a quick check to either use
> > INTERVAL_SESSION_DOWN or staletime + INTERVAL_SESSION_DOWN depending on
> > which one is bigger.
> 
> I think you mean "INTERVAL_SESSION_DOWN or staletime + INTERVAL_STALE"
> ... (see below)
> 
> > 
> > While there also update last_updown in the stats struct since the peer
> > either lost or flapped the session.
> > 
> > I switch the grestart.timeout in struct capabilities to uint16_t to
> > silence another signed vs unsigned warnings.
> > -- 
> > :wq Claudio
> > 
> > Index: bgpd.h
> > ===================================================================
> > RCS file: /cvs/src/usr.sbin/bgpd/bgpd.h,v
> > diff -u -p -r1.540 bgpd.h
> > --- bgpd.h	5 May 2026 09:12:04 -0000	1.540
> > +++ bgpd.h	7 May 2026 13:31:04 -0000
> > @@ -437,10 +437,10 @@ struct auth_config {
> >  
> >  struct capabilities {
> >  	struct {
> > -		int16_t	timeout;	/* graceful restart timeout */
> > -		int8_t	flags[AID_MAX];	/* graceful restart per AID flags */
> > -		int8_t	restart;	/* graceful restart, RFC 4724 */
> > -		int8_t	grnotification;	/* graceful notification, RFC 8538 */
> > +		uint16_t	timeout;	/* gr restart timeout */
> > +		int8_t		flags[AID_MAX];	/* gr restart per AID flags */
> > +		int8_t		restart;	/* gr restart, RFC 4724 */
> > +		int8_t		grnotification;	/* gr notification, RFC 8538 */
> >  	}	grestart;
> >  	int8_t	mp[AID_MAX];		/* multiprotocol extensions, RFC 4760 */
> >  	int8_t	add_path[AID_MAX];	/* ADD_PATH, RFC 7911 */
> > Index: parse.y
> > ===================================================================
> > RCS file: /cvs/src/usr.sbin/bgpd/parse.y,v
> > diff -u -p -r1.488 parse.y
> > --- parse.y	2 Mar 2026 09:51:48 -0000	1.488
> > +++ parse.y	7 May 2026 13:28:57 -0000
> > @@ -772,9 +772,9 @@ conf_main	: AS as4number		{
> >  			conf->min_holdtime = $3;
> >  		}
> >  		| STALETIME NUMBER	{
> > -			if ($2 < MIN_HOLDTIME || $2 > USHRT_MAX) {
> > +			if ($2 < MIN_HOLDTIME || $2 > CAPA_GR_TIMEMASK) {
> >  				yyerror("staletime must be between %u and %u",
> > -				    MIN_HOLDTIME, USHRT_MAX);
> > +				    MIN_HOLDTIME, CAPA_GR_TIMEMASK);
> >  				YYERROR;
> >  			}
> >  			conf->staletime = $2;
> > @@ -1937,9 +1937,9 @@ peeropts	: REMOTEAS as4number	{
> >  			curpeer->conf.min_holdtime = $3;
> >  		}
> >  		| STALETIME NUMBER	{
> > -			if ($2 < MIN_HOLDTIME || $2 > USHRT_MAX) {
> > +			if ($2 < MIN_HOLDTIME || $2 > CAPA_GR_TIMEMASK) {
> >  				yyerror("staletime must be between %u and %u",
> > -				    MIN_HOLDTIME, USHRT_MAX);
> > +				    MIN_HOLDTIME, CAPA_GR_TIMEMASK);
> >  				YYERROR;
> >  			}
> >  			curpeer->conf.staletime = $2;
> > Index: session.c
> > ===================================================================
> > RCS file: /cvs/src/usr.sbin/bgpd/session.c,v
> > diff -u -p -r1.531 session.c
> > --- session.c	7 May 2026 09:17:27 -0000	1.531
> > +++ session.c	7 May 2026 13:34:11 -0000
> > @@ -1048,7 +1048,7 @@ void
> >  session_graceful_restart(struct peer *p)
> >  {
> >  	uint8_t	i;
> > -	uint16_t staletime = conf->staletime;
> > +	u_int staletime = conf->staletime;
> >  
> >  	if (p->conf.staletime)
> >  		staletime = p->conf.staletime;
> > @@ -1057,6 +1057,14 @@ session_graceful_restart(struct peer *p)
> >  	if (staletime > p->capa.neg.grestart.timeout)
> >  		staletime = p->capa.neg.grestart.timeout;
> >  	timer_set(&p->timers, Timer_RestartTimeout, staletime);
> > +
> > +	if (staletime < INTERVAL_SESSION_DOWN - INTERVAL_STALE)
> > +		staletime = INTERVAL_SESSION_DOWN - INTERVAL_STALE;
> > +
> > +	/* bits from session_down that are also needed here */
> > +	p->stats.last_updown = getmonotime();
> > +	timer_set(&p->timers, Timer_SessionDown,
> > +	    staletime + INTERVAL_SESSION_DOWN);
> 
> ... and this should be:
> 
> 	    staletime + INTERVAL_STALE);
> 
> then it's ok

Thanks for spotting this.
 
> >  
> >  	for (i = AID_MIN; i < AID_MAX; i++) {
> >  		if (p->capa.neg.grestart.flags[i] & CAPA_GR_PRESENT) {
> > 
> 

-- 
:wq Claudio