From: Claudio Jeker Subject: bgpd: rework IdleHold part of the bgp fsm To: tech@openbsd.org Date: Tue, 14 Jul 2026 21:18:54 +0200 I realized while fixing the errcnt issue, that it actually is not needed at all. Here is a diff that solves the same quick reconnect in a slightly different way. Instead of handling this specially in session_accept() just put the session into a state that allows the connection to be accepted but then only initiate the connection after INTERVAL_IDLE_HOLD_INITIAL. So at start (and on neighbor clear/up) we set IdleHoldTime to 0. In that case the IdleHoldTimer fires immediatly and puts the session into active state. Doing that allows us to remove the session_accept() special for STATE_IDLE. As usual nothing is free so instead the bgp_fsm() needs to handle the case where the IdleHoldTime was 0 (actually INTERVAL_IDLE_HOLD_INITIAL since the code in STATE_IDLE adjust IdleHoldTime after arming the timer). In that case use the ConnectRetryTimer to delay the local connect by INTERVAL_IDLE_HOLD_INITIAL since we don't want to immediatly reopen the session but allow peer to reconnect. On top of this add code to STATE_ACTIVE / EVNT_START to also bring up a session immediately. This allows a 'bgpctl nei X up' to work also for active sessions (something that bothered me for a long time). It also allows passive peers to open up connections that way. This last part is certainly up for discussion. I myself am not sure if we really want that. Finally there is a big reshuffle of code in change_state() STATE_IDLE so that IdleHoldTime is only updated at the end. It also simplifies the EVNT_NONE handling which is only triggered by init_peer(). This seems to behave like I want but this certainly needs some testing from others as well to see if I missed some other crazy edge case. At least the regress tests are happy with this, and those do some funky business when it comes to cutting IdleHoldTime down. -- :wq Claudio Index: control.c =================================================================== RCS file: /cvs/src/usr.sbin/bgpd/control.c,v diff -u -p -r1.140 control.c --- control.c 24 Jun 2026 06:01:13 -0000 1.140 +++ control.c 14 Jul 2026 08:48:16 -0000 @@ -389,9 +389,7 @@ control_dispatch_msg(struct pollfd *pfd, bgp_fsm(p, EVNT_START, NULL); p->conf.down = 0; p->conf.reason[0] = '\0'; - p->IdleHoldTime = - INTERVAL_IDLE_HOLD_INITIAL; - p->errcnt = 0; + p->IdleHoldTime = 0; control_result(c, CTL_RES_OK); break; case IMSG_CTL_NEIGHBOR_DOWN: @@ -405,9 +403,7 @@ control_dispatch_msg(struct pollfd *pfd, case IMSG_CTL_NEIGHBOR_CLEAR: neighbor.reason[ sizeof(neighbor.reason) - 1] = '\0'; - p->IdleHoldTime = - INTERVAL_IDLE_HOLD_INITIAL; - p->errcnt = 0; + p->IdleHoldTime = 0; if (!p->conf.down) { session_stop(p, ERR_CEASE_ADMIN_RESET, Index: session.c =================================================================== RCS file: /cvs/src/usr.sbin/bgpd/session.c,v diff -u -p -r1.537 session.c --- session.c 14 May 2026 12:26:44 -0000 1.537 +++ session.c 14 Jul 2026 14:27:33 -0000 @@ -362,9 +362,7 @@ session_main(int debug, int verbose) bgp_fsm(p, EVNT_START, NULL); break; case Timer_IdleHoldReset: - p->IdleHoldTime = - INTERVAL_IDLE_HOLD_INITIAL; - p->errcnt = 0; + p->IdleHoldTime = 0; timer_stop(&p->timers, Timer_IdleHoldReset); break; @@ -714,15 +712,6 @@ session_accept(int listenfd) } p = getpeerbyip(conf, (struct sockaddr *)&cliaddr); - - if (p != NULL && p->state == STATE_IDLE && p->errcnt < 2) { - if (timer_running(&p->timers, Timer_IdleHold, NULL)) { - /* fast reconnect after clear */ - p->passive = 1; - bgp_fsm(p, EVNT_START, NULL); - } - } - if (p != NULL && (p->state == STATE_CONNECT || p->state == STATE_ACTIVE)) { if (p->fd != -1) { Index: session.h =================================================================== RCS file: /cvs/src/usr.sbin/bgpd/session.h,v diff -u -p -r1.198 session.h --- session.h 24 Jun 2026 06:01:13 -0000 1.198 +++ session.h 14 Jul 2026 08:49:21 -0000 @@ -140,7 +140,6 @@ struct peer { struct peer *template; int fd; int lasterr; - u_int errcnt; u_int IdleHoldTime; unsigned int if_scope; /* interface scope for IPv6 */ uint32_t local_bgpid; @@ -155,7 +154,6 @@ struct peer { uint16_t remote_port; uint8_t depend_ok; uint8_t demoted; - uint8_t passive; uint8_t throttled; uint8_t rpending; uint8_t rdesession; Index: session_bgp.c =================================================================== RCS file: /cvs/src/usr.sbin/bgpd/session_bgp.c,v diff -u -p -r1.10 session_bgp.c --- session_bgp.c 14 Jul 2026 07:58:05 -0000 1.10 +++ session_bgp.c 14 Jul 2026 14:31:03 -0000 @@ -512,7 +512,6 @@ session_notification(struct peer *p, uin session_sendmsg(buf, p, BGP_NOTIFICATION); p->stats.msg_sent_notification++; - p->errcnt++; p->stats.last_sent_errcode = errcode; p->stats.last_sent_suberr = subcode; } @@ -1187,7 +1186,6 @@ parse_notification(struct peer *peer, st } } - peer->errcnt++; peer->stats.last_rcvd_errcode = errcode; peer->stats.last_rcvd_suberr = subcode; @@ -1558,17 +1556,20 @@ bgp_fsm(struct peer *peer, enum session_ if (!peer->depend_ok) timer_stop(&peer->timers, Timer_ConnectRetry); - else if (peer->passive || peer->conf.passive || - peer->conf.template) { + else if (peer->conf.passive || peer->conf.template) { change_state(peer, STATE_ACTIVE, event); timer_stop(&peer->timers, Timer_ConnectRetry); + } else if (peer->IdleHoldTime == + INTERVAL_IDLE_HOLD_INITIAL) { + change_state(peer, STATE_ACTIVE, event); + timer_set(&peer->timers, Timer_ConnectRetry, + INTERVAL_IDLE_HOLD_INITIAL); } else { change_state(peer, STATE_CONNECT, event); timer_set(&peer->timers, Timer_ConnectRetry, peer->conf.connectretry); session_connect(peer); } - peer->passive = 0; break; case EVNT_STOP: timer_stop(&peer->timers, Timer_IdleHold); @@ -1610,7 +1611,12 @@ bgp_fsm(struct peer *peer, enum session_ case STATE_ACTIVE: switch (event) { case EVNT_START: - /* ignore */ + if (!peer->depend_ok || peer->conf.template) + break; + timer_set(&peer->timers, Timer_ConnectRetry, + peer->holdtime); + change_state(peer, STATE_CONNECT, event); + session_connect(peer); break; case EVNT_CON_OPEN: session_tcp_established(peer); @@ -1830,8 +1836,6 @@ change_state(struct peer *peer, enum ses * session was not established successfully before, the * starttimerinterval needs to be exponentially increased */ - if (peer->IdleHoldTime == 0) - peer->IdleHoldTime = INTERVAL_IDLE_HOLD_INITIAL; peer->holdtime = INTERVAL_HOLD_INITIAL; timer_stop(&peer->timers, Timer_ConnectRetry); timer_stop(&peer->timers, Timer_Keepalive); @@ -1845,7 +1849,22 @@ change_state(struct peer *peer, enum ses memset(&peer->capa.peer, 0, sizeof(peer->capa.peer)); session_md5_reload(peer); + if (peer->prev_state == STATE_NONE || + peer->prev_state == STATE_ESTABLISHED) { + /* initialize capability negotiation structures */ + memcpy(&peer->capa.ann, &peer->conf.capabilities, + sizeof(peer->capa.ann)); + } + + /* called from init_peer for basic setup. */ + if (peer->prev_state == STATE_NONE) + break; + if (peer->prev_state == STATE_ESTABLISHED) { + if (event == EVNT_STOP) { + session_down(peer); + break; + } if (peer->capa.neg.grestart.restart == 2 && (event == EVNT_CON_CLOSED || event == EVNT_CON_FATAL || @@ -1856,30 +1875,22 @@ change_state(struct peer *peer, enum ses /* don't punish graceful restart */ timer_set(&peer->timers, Timer_IdleHold, 0); session_graceful_restart(peer); - } else if (event != EVNT_STOP) { + break; + } else { timer_set(&peer->timers, Timer_IdleHold, peer->IdleHoldTime); - if (event != EVNT_NONE && - peer->IdleHoldTime < MAX_IDLE_HOLD / 2) - peer->IdleHoldTime *= 2; - session_down(peer); - } else { session_down(peer); } } else if (event != EVNT_STOP) { timer_set(&peer->timers, Timer_IdleHold, peer->IdleHoldTime); - if (event != EVNT_NONE && - peer->IdleHoldTime < MAX_IDLE_HOLD / 2) - peer->IdleHoldTime *= 2; } - if (peer->prev_state == STATE_NONE || - peer->prev_state == STATE_ESTABLISHED) { - /* initialize capability negotiation structures */ - memcpy(&peer->capa.ann, &peer->conf.capabilities, - sizeof(peer->capa.ann)); - } + peer->IdleHoldTime *= 2; + if (peer->IdleHoldTime == 0) + peer->IdleHoldTime = INTERVAL_IDLE_HOLD_INITIAL; + if (peer->IdleHoldTime > MAX_IDLE_HOLD) + peer->IdleHoldTime = MAX_IDLE_HOLD; break; case STATE_CONNECT: if (peer->prev_state == STATE_ESTABLISHED &&