From: claudio <claudio@openbsd.org>
Date: Wed, 20 Mar 2024 09:35:46 +0000 (+0000)
Subject: Cleanup AID handling.
X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=110c1584e6f06f8c1d2e120f71e691b8225c4c2f;p=openbsd

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@
---

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 <henning@openbsd.org>
@@ -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 <henning@openbsd.org>
@@ -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 <henning@openbsd.org>
@@ -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 <claudio@openbsd.org>
@@ -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 <henning@openbsd.org>
@@ -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 <claudio@openbsd.org>
@@ -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);