From: Theo Buehler <tb@theobuehler.org>
Subject: Re: bgpd: trigger session down timer during graceful restart
To: tech@openbsd.org
Date: Thu, 7 May 2026 17:43:03 +0200

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

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