From: Theo Buehler Subject: Re: bgpd: make imsg_rde void and fail hard To: tech@openbsd.org Date: Wed, 26 Feb 2025 17:58:32 +0100 On Wed, Feb 26, 2025 at 05:52:13PM +0100, Claudio Jeker wrote: > On Wed, Feb 26, 2025 at 05:44:44PM +0100, Theo Buehler wrote: > > On Wed, Feb 26, 2025 at 05:37:42PM +0100, Claudio Jeker wrote: > > > On Wed, Feb 26, 2025 at 05:30:38PM +0100, Theo Buehler wrote: > > > > On Wed, Feb 26, 2025 at 05:18:58PM +0100, Claudio Jeker wrote: > > > > > While working on all the other bits in the session engine I came to the > > > > > conclusion that if imsg_rde() fails the game is over. So just admit to > > > > > that and fatal in that case. > > > > > > > > > > This simplifies all the callers since there is no need to check the return > > > > > value. This also allows some other functions to be come void. > > > > > > > > ok > > > > > > > > Should session_handle_{update,rrefresh}() also become void? Wouldn't > > > > it be more consistent if the return (0) was pushed to parse_update() > > > > and parse_rrefresh()? > > > > > > Maybe we should make them all void including parse_update() and > > > parse_rrefresh(). The return value of parse_rrefresh is not checked and > > > the code around parse_update() is most probably wrong for the error case. > > > In the end when a parse error happens we need to generate a notification > > > and properly throw an error in the FSM but there is no need to bubble up > > > the error. > > > > Right, that makes sense. > > That would look like this. ok tb > > -- > :wq Claudio > > Index: session.c > =================================================================== > RCS file: /cvs/src/usr.sbin/bgpd/session.c,v > diff -u -p -r1.523 session.c > --- session.c 26 Feb 2025 16:39:18 -0000 1.523 > +++ session.c 26 Feb 2025 16:50:36 -0000 > @@ -1016,19 +1016,17 @@ get_alternate_addr(struct bgpd_addr *loc > freeifaddrs(ifap); > } > > -int > +void > session_handle_update(struct peer *peer, struct ibuf *msg) > { > /* pass the message verbatim to the rde. */ > imsg_rde(IMSG_UPDATE, peer->conf.id, ibuf_data(msg), ibuf_size(msg)); > - return (0); > } > > -int > +void > session_handle_rrefresh(struct peer *peer, struct route_refresh *rr) > { > imsg_rde(IMSG_REFRESH, peer->conf.id, rr, sizeof(*rr)); > - return (0); > } > > void > Index: session.h > =================================================================== > RCS file: /cvs/src/usr.sbin/bgpd/session.h,v > diff -u -p -r1.190 session.h > --- session.h 26 Feb 2025 16:39:18 -0000 1.190 > +++ session.h 26 Feb 2025 16:50:36 -0000 > @@ -335,8 +335,8 @@ void get_alternate_addr(struct bgpd_ad > struct peer *getpeerbydesc(struct bgpd_config *, const char *); > struct peer *getpeerbyip(struct bgpd_config *, struct sockaddr *); > struct peer *getpeerbyid(struct bgpd_config *, uint32_t); > -int session_handle_update(struct peer *, struct ibuf *); > -int session_handle_rrefresh(struct peer *, struct route_refresh *); > +void session_handle_update(struct peer *, struct ibuf *); > +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, > Index: session_bgp.c > =================================================================== > RCS file: /cvs/src/usr.sbin/bgpd/session_bgp.c,v > diff -u -p -r1.2 session_bgp.c > --- session_bgp.c 26 Feb 2025 16:39:18 -0000 1.2 > +++ session_bgp.c 26 Feb 2025 16:50:36 -0000 > @@ -1079,13 +1079,13 @@ parse_open(struct peer *peer, struct ibu > return (0); > } > > -static int > +static void > parse_update(struct peer *peer, struct ibuf *msg) > { > - return session_handle_update(peer, msg); > + session_handle_update(peer, msg); > } > > -static int > +static void > parse_rrefresh(struct peer *peer, struct ibuf *msg) > { > struct route_refresh rr; > @@ -1114,7 +1114,7 @@ parse_rrefresh(struct peer *peer, struct > session_notification_data(peer, ERR_HEADER, > ERR_HDR_LEN, &datalen, sizeof(datalen)); > bgp_fsm(peer, EVNT_CON_FATAL, NULL); > - return (-1); > + return; > } > peer->stats.refresh_rcvd_req++; > break; > @@ -1129,7 +1129,7 @@ parse_rrefresh(struct peer *peer, struct > session_notification(peer, ERR_RREFRESH, > ERR_RR_INV_LEN, msg); > bgp_fsm(peer, EVNT_CON_FATAL, NULL); > - return (-1); > + return; > } > if (subtype == ROUTE_REFRESH_BEGIN_RR) > peer->stats.refresh_rcvd_borr++; > @@ -1139,7 +1139,7 @@ parse_rrefresh(struct peer *peer, struct > default: > log_peer_warnx(&peer->conf, "peer sent bad refresh, " > "bad subtype %d", subtype); > - return (0); > + return; > } > } else { > /* force subtype to default */ > @@ -1151,18 +1151,18 @@ parse_rrefresh(struct peer *peer, struct > if (afi2aid(afi, safi, &aid) == -1) { > log_peer_warnx(&peer->conf, "peer sent bad refresh, " > "invalid afi/safi pair"); > - return (0); > + return; > } > > if (!peer->capa.neg.refresh && !peer->capa.neg.enhanced_rr) { > log_peer_warnx(&peer->conf, "peer sent unexpected refresh"); > - return (0); > + return; > } > > rr.aid = aid; > rr.subtype = subtype; > > - return session_handle_rrefresh(peer, &rr); > + session_handle_rrefresh(peer, &rr); > } > > static void > @@ -1757,10 +1757,7 @@ bgp_fsm(struct peer *peer, enum session_ > break; > case EVNT_RCVD_UPDATE: > start_timer_holdtime(peer); > - if (parse_update(peer, msg)) > - change_state(peer, STATE_IDLE, event); > - else > - start_timer_holdtime(peer); > + parse_update(peer, msg); > break; > case EVNT_RCVD_NOTIFICATION: > parse_notification(peer, msg);