From 110c1584e6f06f8c1d2e120f71e691b8225c4c2f Mon Sep 17 00:00:00 2001 From: claudio Date: Wed, 20 Mar 2024 09:35:46 +0000 Subject: [PATCH] Cleanup AID handling. - Loops over all valid AID should start with AID_MIN and go up to AID_MAX - 1 e.g. for (i = AID_MIN; i < AID_MAX; i++) If for some reason AID_UNSPEC must be handled make that explicit in the for loop. - aid2afi() now returns an error for AID_UNSPEC since there is no valid AFI SAFI combo for AID_UNSPEC. - Add additional checks for AID_MIN where currently only AID_MAX was checked. This affects imsg for route refresh and graceful restart. - Simplify add-path capability handling. Only the negotiated add_path capa sets the flag for AID_UNSPEC to help code to quickly check if any add-path is active. OK tb@ --- usr.sbin/bgpd/parse.y | 25 ++++++++-------- usr.sbin/bgpd/printconf.c | 4 +-- usr.sbin/bgpd/rde.c | 12 ++++---- usr.sbin/bgpd/rde_peer.c | 17 +++++------ usr.sbin/bgpd/session.c | 61 ++++++++++++++++++--------------------- usr.sbin/bgpd/util.c | 8 ++--- 6 files changed, 59 insertions(+), 68 deletions(-) diff --git a/usr.sbin/bgpd/parse.y b/usr.sbin/bgpd/parse.y index ea47a659b1a..2fc507e86a6 100644 --- a/usr.sbin/bgpd/parse.y +++ b/usr.sbin/bgpd/parse.y @@ -1,4 +1,4 @@ -/* $OpenBSD: parse.y,v 1.456 2024/03/18 14:54:52 claudio Exp $ */ +/* $OpenBSD: parse.y,v 1.457 2024/03/20 09:35:46 claudio Exp $ */ /* * Copyright (c) 2002, 2003, 2004 Henning Brauer @@ -1894,7 +1894,7 @@ peeropts : REMOTEAS as4number { uint16_t afi; if ($3 == SAFI_NONE) { - for (aid = 0; aid < AID_MAX; aid++) { + for (aid = AID_MIN; aid < AID_MAX; aid++) { if (aid2afi(aid, &afi, &safi) == -1 || afi != $2) continue; @@ -1927,11 +1927,11 @@ peeropts : REMOTEAS as4number { int8_t *ap = curpeer->conf.capabilities.add_path; uint8_t i; - for (i = 0; i < AID_MAX; i++) + for (i = AID_MIN; i < AID_MAX; i++) if ($4) - *ap++ |= CAPA_AP_RECV; + ap[i] |= CAPA_AP_RECV; else - *ap++ &= ~CAPA_AP_RECV; + ap[i] &= ~CAPA_AP_RECV; } | ANNOUNCE ADDPATH SEND STRING addpathextra addpathmax { int8_t *ap = curpeer->conf.capabilities.add_path; @@ -1945,9 +1945,7 @@ peeropts : REMOTEAS as4number { "for 'add-path send no'"); YYERROR; } - for (i = 0; i < AID_MAX; i++) - *ap++ &= ~CAPA_AP_SEND; - break; + mode = ADDPATH_EVAL_NONE; } else if (!strcmp($4, "all")) { free($4); if ($5 != 0 || $6 != 0) { @@ -1971,8 +1969,12 @@ peeropts : REMOTEAS as4number { free($4); YYERROR; } - for (i = 0; i < AID_MAX; i++) - *ap++ |= CAPA_AP_SEND; + for (i = AID_MIN; i < AID_MAX; i++) { + if (mode != ADDPATH_EVAL_NONE) + ap[i] |= CAPA_AP_SEND; + else + ap[i] &= ~CAPA_AP_SEND; + } curpeer->conf.eval.mode = mode; curpeer->conf.eval.extrapaths = $5; curpeer->conf.eval.maxpaths = $6; @@ -4611,7 +4613,6 @@ struct peer * alloc_peer(void) { struct peer *p; - uint8_t i; if ((p = calloc(1, sizeof(struct peer))) == NULL) fatal("new_peer"); @@ -4622,8 +4623,6 @@ alloc_peer(void) p->conf.distance = 1; p->conf.export_type = EXPORT_UNSET; p->conf.announce_capa = 1; - for (i = 0; i < AID_MAX; i++) - p->conf.capabilities.mp[i] = 0; p->conf.capabilities.refresh = 1; p->conf.capabilities.grestart.restart = 1; p->conf.capabilities.as4byte = 1; diff --git a/usr.sbin/bgpd/printconf.c b/usr.sbin/bgpd/printconf.c index 5f39b696c9d..74095ddb49c 100644 --- a/usr.sbin/bgpd/printconf.c +++ b/usr.sbin/bgpd/printconf.c @@ -1,4 +1,4 @@ -/* $OpenBSD: printconf.c,v 1.169 2024/01/10 13:31:09 claudio Exp $ */ +/* $OpenBSD: printconf.c,v 1.170 2024/03/20 09:35:46 claudio Exp $ */ /* * Copyright (c) 2003, 2004 Henning Brauer @@ -920,7 +920,7 @@ print_announce(struct peer_config *p, const char *c) if (p->announce_capa == 0) printf("%s\tannounce capabilities no\n", c); - for (aid = 0; aid < AID_MAX; aid++) + for (aid = AID_MIN; aid < AID_MAX; aid++) if (p->capabilities.mp[aid]) printf("%s\tannounce %s\n", c, aid2str(aid)); diff --git a/usr.sbin/bgpd/rde.c b/usr.sbin/bgpd/rde.c index 62acf44f070..73dce0a5bb3 100644 --- a/usr.sbin/bgpd/rde.c +++ b/usr.sbin/bgpd/rde.c @@ -1,4 +1,4 @@ -/* $OpenBSD: rde.c,v 1.623 2024/02/22 06:45:22 miod Exp $ */ +/* $OpenBSD: rde.c,v 1.624 2024/03/20 09:35:46 claudio Exp $ */ /* * Copyright (c) 2003, 2004 Henning Brauer @@ -442,7 +442,7 @@ rde_dispatch_imsg_session(struct imsgbuf *imsgbuf) log_warnx("%s: wrong imsg len", __func__); break; } - if (aid >= AID_MAX) { + if (aid < AID_MIN || aid >= AID_MAX) { log_warnx("%s: bad AID", __func__); break; } @@ -1328,7 +1328,7 @@ rde_dispatch_imsg_peer(struct rde_peer *peer, void *bula) log_warnx("route refresh: wrong imsg len"); break; } - if (rr.aid >= AID_MAX) { + if (rr.aid < AID_MIN || rr.aid >= AID_MAX) { log_peer_warnx(&peer->conf, "route refresh: bad AID %d", rr.aid); break; @@ -3326,7 +3326,7 @@ rde_update_queue_pending(void) continue; if (peer->throttled) continue; - for (aid = 0; aid < AID_MAX; aid++) { + for (aid = AID_MIN; aid < AID_MAX; aid++) { if (!RB_EMPTY(&peer->updates[aid]) || !RB_EMPTY(&peer->withdraws[aid])) return 1; @@ -3821,7 +3821,7 @@ rde_softreconfig_in_done(void *arg, uint8_t dummy) peer->reconf_out = 0; } else if (peer->export_type == EXPORT_DEFAULT_ROUTE) { /* just resend the default route */ - for (aid = 0; aid < AID_MAX; aid++) { + for (aid = AID_MIN; aid < AID_MAX; aid++) { if (peer->capa.mp[aid]) up_generate_default(peer, aid); } @@ -3831,7 +3831,7 @@ rde_softreconfig_in_done(void *arg, uint8_t dummy) RECONF_RELOAD; } else if (peer->reconf_rib) { /* dump the full table to neighbors that changed rib */ - for (aid = 0; aid < AID_MAX; aid++) { + for (aid = AID_MIN; aid < AID_MAX; aid++) { if (peer->capa.mp[aid]) peer_dump(peer, aid); } diff --git a/usr.sbin/bgpd/rde_peer.c b/usr.sbin/bgpd/rde_peer.c index 84e952620ed..00d12f9afec 100644 --- a/usr.sbin/bgpd/rde_peer.c +++ b/usr.sbin/bgpd/rde_peer.c @@ -1,4 +1,4 @@ -/* $OpenBSD: rde_peer.c,v 1.35 2024/02/03 09:26:52 jsg Exp $ */ +/* $OpenBSD: rde_peer.c,v 1.36 2024/03/20 09:35:46 claudio Exp $ */ /* * Copyright (c) 2019 Claudio Jeker @@ -44,18 +44,15 @@ peer_has_as4byte(struct rde_peer *peer) return (peer->capa.as4byte); } +/* + * Check if ADD_PATH is enabled for aid and mode (rx / tx). If aid is + * AID_UNSPEC then the function returns true if any aid has mode enabled. + */ int peer_has_add_path(struct rde_peer *peer, uint8_t aid, int mode) { if (aid >= AID_MAX) return 0; - if (aid == AID_UNSPEC) { - /* check if at capability is set for at least one AID */ - for (aid = AID_MIN; aid < AID_MAX; aid++) - if (peer->capa.add_path[aid] & mode) - return 1; - return 0; - } return (peer->capa.add_path[aid] & mode); } @@ -444,7 +441,7 @@ peer_up(struct rde_peer *peer, struct session_up *sup) } peer->state = PEER_UP; - for (i = 0; i < AID_MAX; i++) { + for (i = AID_MIN; i < AID_MAX; i++) { if (peer->capa.mp[i]) peer_dump(peer, i); } @@ -500,7 +497,7 @@ peer_flush(struct rde_peer *peer, uint8_t aid, time_t staletime) /* every route is gone so reset staletime */ if (aid == AID_UNSPEC) { uint8_t i; - for (i = 0; i < AID_MAX; i++) + for (i = AID_MIN; i < AID_MAX; i++) peer->staletime[i] = 0; } else { peer->staletime[aid] = 0; diff --git a/usr.sbin/bgpd/session.c b/usr.sbin/bgpd/session.c index fddd2f41584..3aeec7767df 100644 --- a/usr.sbin/bgpd/session.c +++ b/usr.sbin/bgpd/session.c @@ -1,4 +1,4 @@ -/* $OpenBSD: session.c,v 1.463 2024/02/19 10:15:35 job Exp $ */ +/* $OpenBSD: session.c,v 1.464 2024/03/20 09:35:46 claudio Exp $ */ /* * Copyright (c) 2003, 2004, 2005 Henning Brauer @@ -68,7 +68,7 @@ void session_tcp_established(struct peer *); void session_capa_ann_none(struct peer *); int session_capa_add(struct ibuf *, uint8_t, uint8_t); int session_capa_add_mp(struct ibuf *, uint8_t); -int session_capa_add_afi(struct peer *, struct ibuf *, uint8_t, uint8_t); +int session_capa_add_afi(struct ibuf *, uint8_t, uint8_t); struct bgp_msg *session_newmsg(enum msg_type, uint16_t); int session_sendmsg(struct bgp_msg *, struct peer *); void session_open(struct peer *); @@ -1357,8 +1357,7 @@ session_capa_add_mp(struct ibuf *buf, uint8_t aid) } int -session_capa_add_afi(struct peer *p, struct ibuf *b, uint8_t aid, - uint8_t flags) +session_capa_add_afi(struct ibuf *b, uint8_t aid, uint8_t flags) { u_int errs = 0; uint16_t afi; @@ -1492,7 +1491,7 @@ session_open(struct peer *p) } /* multiprotocol extensions, RFC 4760 */ - for (i = 0; i < AID_MAX; i++) + for (i = AID_MIN; i < AID_MAX; i++) if (p->capa.ann.mp[i]) { /* 4 bytes data */ errs += session_capa_add(opb, CAPA_MP, 4); errs += session_capa_add_mp(opb, i); @@ -1517,7 +1516,7 @@ session_open(struct peer *p) int rst = 0; uint16_t hdr = 0; - for (i = 0; i < AID_MAX; i++) { + for (i = AID_MIN; i < AID_MAX; i++) { if (p->capa.neg.grestart.flags[i] & CAPA_GR_RESTARTING) rst++; } @@ -1536,7 +1535,7 @@ session_open(struct peer *p) } /* advertisement of multiple paths, RFC7911 */ - if (p->capa.ann.add_path[0]) { /* variable */ + if (p->capa.ann.add_path[AID_MIN]) { /* variable */ uint8_t aplen; if (mpcapa) @@ -1547,12 +1546,12 @@ session_open(struct peer *p) if (mpcapa) { for (i = AID_MIN; i < AID_MAX; i++) { if (p->capa.ann.mp[i]) { - errs += session_capa_add_afi(p, opb, + errs += session_capa_add_afi(opb, i, p->capa.ann.add_path[i]); } } } else { /* AID_INET */ - errs += session_capa_add_afi(p, opb, AID_INET, + errs += session_capa_add_afi(opb, AID_INET, p->capa.ann.add_path[AID_INET]); } } @@ -1759,7 +1758,7 @@ session_neighbor_rrefresh(struct peer *p) if (!(p->capa.neg.refresh || p->capa.neg.enhanced_rr)) return (-1); - for (i = 0; i < AID_MAX; i++) { + for (i = AID_MIN; i < AID_MAX; i++) { if (p->capa.neg.mp[i] != 0) session_rrefresh(p, i, ROUTE_REFRESH_REQUEST); } @@ -1828,7 +1827,7 @@ session_graceful_restart(struct peer *p) timer_set(&p->timers, Timer_RestartTimeout, p->capa.neg.grestart.timeout); - for (i = 0; i < AID_MAX; i++) { + for (i = AID_MIN; i < AID_MAX; i++) { if (p->capa.neg.grestart.flags[i] & CAPA_GR_PRESENT) { if (imsg_rde(IMSG_SESSION_STALE, p->conf.id, &i, sizeof(i)) == -1) @@ -1854,7 +1853,7 @@ session_graceful_stop(struct peer *p) { uint8_t i; - for (i = 0; i < AID_MAX; i++) { + for (i = AID_MIN; i < AID_MAX; i++) { /* * Only flush if the peer is restarting and the timeout fired. * In all other cases the session was already flushed when the @@ -2485,7 +2484,6 @@ parse_notification(struct peer *peer) uint8_t capa_code; uint8_t capa_len; size_t reason_len; - uint8_t i; /* just log */ p = peer->rbuf->rptr; @@ -2545,8 +2543,8 @@ parse_notification(struct peer *peer) datalen -= capa_len; switch (capa_code) { case CAPA_MP: - for (i = 0; i < AID_MAX; i++) - peer->capa.ann.mp[i] = 0; + memset(peer->capa.ann.mp, 0, + sizeof(peer->capa.ann.mp)); log_peer_warnx(&peer->conf, "disabling multiprotocol capability"); break; @@ -2840,12 +2838,11 @@ capa_neg_calc(struct peer *p, uint8_t *suberr) (p->capa.ann.refresh && p->capa.peer.refresh) != 0; p->capa.neg.enhanced_rr = (p->capa.ann.enhanced_rr && p->capa.peer.enhanced_rr) != 0; - p->capa.neg.as4byte = (p->capa.ann.as4byte && p->capa.peer.as4byte) != 0; /* MP: both side must agree on the AFI,SAFI pair */ - for (i = 0; i < AID_MAX; i++) { + for (i = AID_MIN; i < AID_MAX; i++) { if (p->capa.ann.mp[i] && p->capa.peer.mp[i]) p->capa.neg.mp[i] = 1; else @@ -2866,7 +2863,7 @@ capa_neg_calc(struct peer *p, uint8_t *suberr) * supporting graceful restart. */ - for (i = 0; i < AID_MAX; i++) { + for (i = AID_MIN; i < AID_MAX; i++) { int8_t negflags; /* disable GR if the AFI/SAFI is not present */ @@ -2898,26 +2895,24 @@ capa_neg_calc(struct peer *p, uint8_t *suberr) if (p->capa.ann.grestart.restart == 0) p->capa.neg.grestart.restart = 0; - /* * ADD-PATH: set only those bits where both sides agree. * For this compare our send bit with the recv bit from the peer * and vice versa. * The flags are stored from this systems view point. + * At index 0 the flags are set if any per-AID flag is set. */ memset(p->capa.neg.add_path, 0, sizeof(p->capa.neg.add_path)); - if (p->capa.ann.add_path[0]) { - for (i = AID_MIN; i < AID_MAX; i++) { - if ((p->capa.ann.add_path[i] & CAPA_AP_RECV) && - (p->capa.peer.add_path[i] & CAPA_AP_SEND)) { - p->capa.neg.add_path[i] |= CAPA_AP_RECV; - p->capa.neg.add_path[0] |= CAPA_AP_RECV; - } - if ((p->capa.ann.add_path[i] & CAPA_AP_SEND) && - (p->capa.peer.add_path[i] & CAPA_AP_RECV)) { - p->capa.neg.add_path[i] |= CAPA_AP_SEND; - p->capa.neg.add_path[0] |= CAPA_AP_SEND; - } + for (i = AID_MIN; i < AID_MAX; i++) { + if ((p->capa.ann.add_path[i] & CAPA_AP_RECV) && + (p->capa.peer.add_path[i] & CAPA_AP_SEND)) { + p->capa.neg.add_path[i] |= CAPA_AP_RECV; + p->capa.neg.add_path[0] |= CAPA_AP_RECV; + } + if ((p->capa.ann.add_path[i] & CAPA_AP_SEND) && + (p->capa.peer.add_path[i] & CAPA_AP_RECV)) { + p->capa.neg.add_path[i] |= CAPA_AP_SEND; + p->capa.neg.add_path[0] |= CAPA_AP_SEND; } } @@ -3323,7 +3318,7 @@ session_dispatch_imsg(struct imsgbuf *imsgbuf, int idx, u_int *listener_cnt) log_warnx("no such peer: id=%u", peerid); break; } - if (rr.aid >= AID_MAX) + if (rr.aid < AID_MIN || rr.aid >= AID_MAX) fatalx("IMSG_REFRESH: bad AID"); session_rrefresh(p, rr.aid, rr.subtype); break; @@ -3338,7 +3333,7 @@ session_dispatch_imsg(struct imsgbuf *imsgbuf, int idx, u_int *listener_cnt) log_warnx("no such peer: id=%u", peerid); break; } - if (aid >= AID_MAX) + if (aid < AID_MIN || aid >= AID_MAX) fatalx("IMSG_SESSION_RESTARTED: bad AID"); if (p->capa.neg.grestart.flags[aid] & CAPA_GR_RESTARTING) { diff --git a/usr.sbin/bgpd/util.c b/usr.sbin/bgpd/util.c index 0f9e89bc248..db4f5e75765 100644 --- a/usr.sbin/bgpd/util.c +++ b/usr.sbin/bgpd/util.c @@ -1,4 +1,4 @@ -/* $OpenBSD: util.c,v 1.82 2024/02/22 06:45:22 miod Exp $ */ +/* $OpenBSD: util.c,v 1.83 2024/03/20 09:35:46 claudio Exp $ */ /* * Copyright (c) 2006 Claudio Jeker @@ -922,7 +922,7 @@ aid2str(uint8_t aid) int aid2afi(uint8_t aid, uint16_t *afi, uint8_t *safi) { - if (aid < AID_MAX) { + if (aid != AID_UNSPEC && aid < AID_MAX) { *afi = aid_vals[aid].afi; *safi = aid_vals[aid].safi; return (0); @@ -935,7 +935,7 @@ afi2aid(uint16_t afi, uint8_t safi, uint8_t *aid) { uint8_t i; - for (i = 0; i < AID_MAX; i++) + for (i = AID_MIN; i < AID_MAX; i++) if (aid_vals[i].afi == afi && aid_vals[i].safi == safi) { *aid = i; return (0); @@ -960,7 +960,7 @@ af2aid(sa_family_t af, uint8_t safi, uint8_t *aid) if (safi == 0) /* default to unicast subclass */ safi = SAFI_UNICAST; - for (i = 0; i < AID_MAX; i++) + for (i = AID_UNSPEC; i < AID_MAX; i++) if (aid_vals[i].af == af && aid_vals[i].safi == safi) { *aid = i; return (0); -- 2.20.1