From: Claudio Jeker Subject: bgpd: fix restart timer behaviour for graceful restart To: tech@openbsd.org Date: Thu, 24 Sep 2026 15:26:35 +0200 When graceful restart is active on a multi-AFI/SAFI session then the first EoR will clear the refresh timer and so stale routes can remain in the other AFI/SAFI RIB (if the neighbor does not finish and sends an EoR message for those too). The timer is global for all AFI/SAFI. So instead of clearing the CAPA_GR_RESTARTING flag introduce a CAPA_GR_FINISHED flag that indicates that the restart handling was done. Now for this to work some extra things had to be adjusted: - session_graceful_restart() needs to ensure CAPA_GR_FINISHED does not remain set. This is done by just setting CAPA_GR_RESTARTING or clearing flags (capa_neg_calc only checks CAPA_GR_RESTARTING | CAPA_GR_FINISHED). - Always check for (flags & (CAPA_GR_RESTARTING | CAPA_GR_FINISHED)) == CAPA_GR_RESTARTING to know that the retart is still going. - never stop the Restart Timer and stop clearing CAPA_GR_RESTARTING - In parse_capabilities() the code was not quite right. First always start with a clean slate, since 'the receiver ... MUST ignore all but the last instance'. Also handle the N flag when no AFI/SAFI pairs are present (helper only role). - In capa_neg_calc() only write the capa.neg data if grestart is actually turned on on our side. Normally capa.neg.grestart.restart is first checked and so data in capa.neg.grestart.flag wont matter but I still find this a bit cleaner. What is currently still missing is proper handling of the CAPA_GR_R_FLAG when announcing the capability. The current code is OK but not quite right. -- :wq Claudio Index: bgpd.h =================================================================== RCS file: /cvs/src/usr.sbin/bgpd/bgpd.h,v diff -u -p -r1.548 bgpd.h --- bgpd.h 21 Sep 2026 19:06:16 -0000 1.548 +++ bgpd.h 24 Sep 2026 09:34:50 -0000 @@ -471,6 +471,7 @@ enum capa_codes { #define CAPA_GR_RESTART 0x02 #define CAPA_GR_FORWARD 0x04 #define CAPA_GR_RESTARTING 0x08 +#define CAPA_GR_FINISHED 0x10 #define CAPA_GR_TIMEMASK 0x0fff #define CAPA_GR_R_FLAG 0x8000 #define CAPA_GR_N_FLAG 0x4000 Index: session.c =================================================================== RCS file: /cvs/src/usr.sbin/bgpd/session.c,v diff -u -p -r1.541 session.c --- session.c 14 Sep 2026 13:27:14 -0000 1.541 +++ session.c 24 Sep 2026 13:04:18 -0000 @@ -1065,7 +1065,7 @@ session_graceful_restart(struct peer *p) log_peer_warnx(&p->conf, "graceful restart of %s, keeping routes", aid2str(i)); - p->capa.neg.grestart.flags[i] |= CAPA_GR_RESTARTING; + p->capa.neg.grestart.flags[i] = CAPA_GR_RESTARTING; } else if (p->capa.neg.mp[i]) { imsg_rde(IMSG_SESSION_NOGRACE, p->conf.id, &i, sizeof(i)); @@ -1087,9 +1087,10 @@ session_graceful_stop(struct peer *p) * In all other cases the session was already flushed when the * session went down or when the new open message was parsed. */ - if (p->capa.neg.grestart.flags[i] & CAPA_GR_RESTARTING) + if ((p->capa.neg.grestart.flags[i] & (CAPA_GR_RESTARTING | + CAPA_GR_FINISHED)) == CAPA_GR_RESTARTING) session_graceful_flush(p, i, "time-out"); - p->capa.neg.grestart.flags[i] &= ~CAPA_GR_RESTARTING; + p->capa.neg.grestart.flags[i] |= CAPA_GR_FINISHED; } } @@ -1552,18 +1553,18 @@ session_dispatch_imsg(struct imsgbuf *im } if (aid < AID_MIN || aid >= AID_MAX) fatalx("IMSG_SESSION_RESTARTED: bad AID"); - if (p->capa.neg.grestart.flags[aid] & + if ((p->capa.neg.grestart.flags[aid] & + (CAPA_GR_RESTARTING | CAPA_GR_FINISHED)) == CAPA_GR_RESTARTING) { log_peer_warnx(&p->conf, "graceful restart of %s finished", aid2str(aid)); - p->capa.neg.grestart.flags[aid] &= - ~CAPA_GR_RESTARTING; - timer_stop(&p->timers, Timer_RestartTimeout); - /* signal back to RDE to cleanup stale routes */ imsg_rde(IMSG_SESSION_RESTARTED, peerid, &aid, sizeof(aid)); + + p->capa.neg.grestart.flags[aid] |= + CAPA_GR_FINISHED; } break; default: Index: session_bgp.c =================================================================== RCS file: /cvs/src/usr.sbin/bgpd/session_bgp.c,v diff -u -p -r1.13 session_bgp.c --- session_bgp.c 30 Aug 2026 23:43:23 -0000 1.13 +++ session_bgp.c 24 Sep 2026 13:02:38 -0000 @@ -769,16 +769,12 @@ parse_capabilities(struct peer *peer, st peer->remote_role = capa2role(role); break; case CAPA_RESTART: - if (capa_len == 2) { - /* peer only supports EoR marker */ - peer->capa.peer.grestart.restart = 1; - peer->capa.peer.grestart.timeout = 0; - break; - } else if (capa_len % 4 != 2) { + memset(&peer->capa.peer.grestart, 0, + sizeof(peer->capa.peer.grestart)); + + if (capa_len % 4 != 2) { log_peer_warnx(&peer->conf, "Bad graceful restart capability"); - peer->capa.peer.grestart.restart = 0; - peer->capa.peer.grestart.timeout = 0; break; } @@ -786,8 +782,17 @@ parse_capabilities(struct peer *peer, st bad_gr_restart: log_peer_warnx(&peer->conf, "Bad graceful restart capability"); - peer->capa.peer.grestart.restart = 0; - peer->capa.peer.grestart.timeout = 0; + memset(&peer->capa.peer.grestart, 0, + sizeof(peer->capa.peer.grestart)); + break; + } + + if (gr_header & CAPA_GR_N_FLAG) + peer->capa.peer.grestart.grnotification = 1; + + if (capa_len == 2) { + /* peer only supports EoR marker */ + peer->capa.peer.grestart.restart = 1; break; } @@ -796,7 +801,8 @@ parse_capabilities(struct peer *peer, st if (peer->capa.peer.grestart.timeout == 0) { log_peer_warnx(&peer->conf, "Received " "graceful restart with zero timeout"); - peer->capa.peer.grestart.restart = 0; + memset(&peer->capa.peer.grestart, 0, + sizeof(peer->capa.peer.grestart)); break; } @@ -812,7 +818,7 @@ parse_capabilities(struct peer *peer, st afi, safi); continue; } - peer->capa.peer.grestart.flags[aid] |= + peer->capa.peer.grestart.flags[aid] = CAPA_GR_PRESENT; if (flags & CAPA_GR_F_FLAG) peer->capa.peer.grestart.flags[aid] |= @@ -822,8 +828,6 @@ parse_capabilities(struct peer *peer, st CAPA_GR_RESTART; peer->capa.peer.grestart.restart = 2; } - if (gr_header & CAPA_GR_N_FLAG) - peer->capa.peer.grestart.grnotification = 1; break; case CAPA_AS4BYTE: if (capa_len != 4 || @@ -1318,24 +1322,26 @@ capa_neg_calc(struct peer *p) */ for (i = AID_MIN; i < AID_MAX; i++) { - int8_t negflags; + int8_t negflags, peerflags; /* disable GR if the AFI/SAFI is not present */ - if ((p->capa.peer.grestart.flags[i] & CAPA_GR_PRESENT && - p->capa.neg.mp[i] == 0)) + if (p->capa.peer.grestart.flags[i] & CAPA_GR_PRESENT && + p->capa.neg.mp[i] == 0) p->capa.peer.grestart.flags[i] = 0; /* disable */ /* look at current GR state and decide what to do */ negflags = p->capa.neg.grestart.flags[i]; - p->capa.neg.grestart.flags[i] = p->capa.peer.grestart.flags[i]; - if (negflags & CAPA_GR_RESTARTING) { + peerflags = p->capa.peer.grestart.flags[i]; + if ((negflags & (CAPA_GR_RESTARTING | CAPA_GR_FINISHED)) == + CAPA_GR_RESTARTING) { if (p->capa.ann.grestart.restart != 0 && - p->capa.peer.grestart.flags[i] & CAPA_GR_FORWARD) { - p->capa.neg.grestart.flags[i] |= - CAPA_GR_RESTARTING; + peerflags & CAPA_GR_FORWARD) { + peerflags |= CAPA_GR_RESTARTING; } else { session_graceful_flush(p, i, "not restarted"); } } + if (p->capa.ann.grestart.restart != 0) + p->capa.neg.grestart.flags[i] = peerflags; } p->capa.neg.grestart.timeout = p->capa.peer.grestart.timeout; p->capa.neg.grestart.restart = p->capa.peer.grestart.restart;