From: Claudio Jeker Subject: bgpd: Adjust BGP FSM handling in change_state To: tech@openbsd.org Date: Mon, 14 Apr 2025 16:59:17 +0200 change_state() switches the peer FSM form one state to another. One thing I dislike is that it does a lot of work before changing the state and so it is possible to shoot yourself with e.g. session_up() that is called with a peer->state that is not ESTABLISHED. I prefer to first update the peer state (and prev_state) and then do all the fumbling using peer->state and peer->prev_state. This makes the code less of a mind bender. As a side-effect the mrt_state_dump call becomes a lot simpler and log_statechange() would also but there is this 'don't clutter the logs' magic that needs 3 states. But even that is less horrible like this. -- :wq Claudio Index: logmsg.c =================================================================== RCS file: /cvs/src/usr.sbin/bgpd/logmsg.c,v diff -u -p -r1.14 logmsg.c --- logmsg.c 20 May 2024 10:00:00 -0000 1.14 +++ logmsg.c 14 Apr 2025 14:53:43 -0000 @@ -111,24 +111,21 @@ log_peer_warnx(const struct peer_config } void -log_statechange(struct peer *peer, enum session_state nstate, +log_statechange(struct peer *peer, enum session_state ostate, enum session_events event) { - char *p; - /* don't clutter the logs with constant Connect -> Active -> Connect */ - if (nstate == STATE_CONNECT && peer->state == STATE_ACTIVE && - peer->prev_state == STATE_CONNECT) + if (peer->state == STATE_CONNECT && peer->prev_state == STATE_ACTIVE && + ostate == STATE_CONNECT) return; - if (nstate == STATE_ACTIVE && peer->state == STATE_CONNECT && - peer->prev_state == STATE_ACTIVE) + if (peer->state == STATE_ACTIVE && peer->prev_state == STATE_CONNECT && + ostate == STATE_ACTIVE) return; peer->lasterr = 0; - p = log_fmt_peer(&peer->conf); - logit(LOG_INFO, "%s: state change %s -> %s, reason: %s", - p, statenames[peer->state], statenames[nstate], eventnames[event]); - free(p); + log_peer_info(&peer->conf, "state change %s -> %s, reason: %s", + statenames[peer->prev_state], statenames[peer->state], + eventnames[event]); } void Index: mrt.c =================================================================== RCS file: /cvs/src/usr.sbin/bgpd/mrt.c,v diff -u -p -r1.126 mrt.c --- mrt.c 20 Feb 2025 19:47:31 -0000 1.126 +++ mrt.c 14 Apr 2025 14:53:43 -0000 @@ -190,8 +190,7 @@ fail: } void -mrt_dump_state(struct mrt *mrt, uint16_t old_state, uint16_t new_state, - struct peer *peer) +mrt_dump_state(struct mrt *mrt, struct peer *peer) { struct ibuf *buf; uint16_t subtype = BGP4MP_STATE_CHANGE; @@ -203,9 +202,9 @@ mrt_dump_state(struct mrt *mrt, uint16_t 2 * sizeof(short), 0) == -1) goto fail; - if (ibuf_add_n16(buf, old_state) == -1) + if (ibuf_add_n16(buf, peer->prev_state) == -1) goto fail; - if (ibuf_add_n16(buf, new_state) == -1) + if (ibuf_add_n16(buf, peer->state) == -1) goto fail; ibuf_close(mrt->wbuf, buf); Index: session.c =================================================================== RCS file: /cvs/src/usr.sbin/bgpd/session.c,v diff -u -p -r1.525 session.c --- session.c 14 Apr 2025 14:52:34 -0000 1.525 +++ session.c 14 Apr 2025 14:53:44 -0000 @@ -1089,8 +1089,7 @@ session_graceful_flush(struct peer *p, u } void -session_mrt_dump_state(struct peer *p, enum session_state oldstate, - enum session_state newstate) +session_mrt_dump_state(struct peer *p) { struct mrt *mrt; @@ -1100,7 +1099,7 @@ session_mrt_dump_state(struct peer *p, e if ((mrt->peer_id == 0 && mrt->group_id == 0) || mrt->peer_id == p->conf.id || (mrt->group_id != 0 && mrt->group_id == p->conf.groupid)) - mrt_dump_state(mrt, oldstate, newstate, p); + mrt_dump_state(mrt, p); } } Index: session.h =================================================================== RCS file: /cvs/src/usr.sbin/bgpd/session.h,v diff -u -p -r1.191 session.h --- session.h 26 Feb 2025 19:31:31 -0000 1.191 +++ session.h 14 Apr 2025 14:53:44 -0000 @@ -263,7 +263,7 @@ unsigned int control_accept(int, int); /* log.c */ char *log_fmt_peer(const struct peer_config *); -void log_statechange(struct peer *, enum session_state, +void log_statechange(struct peer *, enum session_state, enum session_events); void log_notification(const struct peer *, uint8_t, uint8_t, const struct ibuf *, const char *); @@ -273,8 +273,7 @@ void log_conn_attempt(const struct peer /* mrt.c */ void mrt_dump_bgp_msg(struct mrt *, struct ibuf *, struct peer *, enum msg_type); -void mrt_dump_state(struct mrt *, uint16_t, uint16_t, - struct peer *); +void mrt_dump_state(struct mrt *, struct peer *); void mrt_done(struct mrt *); /* pfkey.c */ @@ -339,8 +338,7 @@ void session_handle_update(struct peer void session_handle_rrefresh(struct peer *, struct route_refresh *); void session_graceful_restart(struct peer *); void session_graceful_flush(struct peer *, uint8_t, const char *); -void session_mrt_dump_state(struct peer *, enum session_state, - enum session_state); +void session_mrt_dump_state(struct peer *); void session_mrt_dump_bgp_msg(struct peer *, struct ibuf *, enum msg_type, enum directions); int peer_matched(struct peer *, struct ctl_neighbor *); Index: session_bgp.c =================================================================== RCS file: /cvs/src/usr.sbin/bgpd/session_bgp.c,v diff -u -p -r1.4 session_bgp.c --- session_bgp.c 27 Feb 2025 13:35:00 -0000 1.4 +++ session_bgp.c 14 Apr 2025 14:53:44 -0000 @@ -1799,10 +1799,18 @@ void change_state(struct peer *peer, enum session_state state, enum session_events event) { - switch (state) { + enum session_state ostate; + + /* first apply new state */ + ostate = peer->prev_state; + peer->prev_state = peer->state; + peer->state = state; + + /* then act on it */ + switch (peer->state) { case STATE_IDLE: /* carp demotion first. new peers handled in init_peer */ - if (peer->state == STATE_ESTABLISHED && + if (peer->prev_state == STATE_ESTABLISHED && peer->conf.demote_group[0] && !peer->demoted) session_demote(peer, +1); @@ -1810,7 +1818,7 @@ change_state(struct peer *peer, enum ses * try to write out what's buffered (maybe a notification), * don't bother if it fails */ - if (peer->state >= STATE_OPENSENT && + if (peer->prev_state >= STATE_OPENSENT && msgbuf_queuelen(peer->wbuf) > 0) ibuf_write(peer->fd, peer->wbuf); @@ -1835,7 +1843,7 @@ change_state(struct peer *peer, enum ses memset(&peer->capa.peer, 0, sizeof(peer->capa.peer)); session_md5_reload(peer); - if (peer->state == STATE_ESTABLISHED) { + if (peer->prev_state == STATE_ESTABLISHED) { if (peer->capa.neg.grestart.restart == 2 && (event == EVNT_CON_CLOSED || event == EVNT_CON_FATAL || @@ -1864,15 +1872,15 @@ change_state(struct peer *peer, enum ses peer->IdleHoldTime *= 2; } - if (peer->state == STATE_NONE || - peer->state == STATE_ESTABLISHED) { + 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)); } break; case STATE_CONNECT: - if (peer->state == STATE_ESTABLISHED && + if (peer->prev_state == STATE_ESTABLISHED && peer->capa.neg.grestart.restart == 2) { /* do the graceful restart dance */ session_graceful_restart(peer); @@ -1907,10 +1915,6 @@ change_state(struct peer *peer, enum ses break; } - log_statechange(peer, state, event); - - session_mrt_dump_state(peer, peer->state, state); - - peer->prev_state = peer->state; - peer->state = state; + log_statechange(peer, ostate, event); + session_mrt_dump_state(peer); }