Index | Thread | Search

From:
Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject:
bgpd: flip bgpid and clusterid to host byte order
To:
tech@openbsd.org
Date:
Mon, 20 May 2024 15:42:43 +0200

Download raw body.

Thread
Inspired by a discussion with tb@. I think keeping those values in host
byte order makes more sense. Especially since remote_bgpid in the RDE is
already in host byte order.

I hope I got all usages of bgpid, remore_bgpid and clusterid and converted
them correctly. Regress still pass which is a good first step :)
-- 
:wq Claudio

Index: bgpctl/bgpctl.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpctl/bgpctl.c,v
diff -u -p -r1.305 bgpctl.c
--- bgpctl/bgpctl.c	1 Feb 2024 11:37:10 -0000	1.305
+++ bgpctl/bgpctl.c	20 May 2024 13:31:04 -0000
@@ -1548,15 +1548,16 @@ show_mrt_capabilities(struct ibuf *b)
 static void
 show_mrt_open(struct ibuf *b)
 {
+	struct in_addr ina;
+	uint32_t bgpid;
 	uint16_t short_as, holdtime;
 	uint8_t version, optparamlen;
-	struct in_addr bgpid;
 
 	/* length check up to optparamlen already happened */
 	if (ibuf_get_n8(b, &version) == -1 ||
 	    ibuf_get_n16(b, &short_as) == -1 ||
 	    ibuf_get_n16(b, &holdtime) == -1 ||
-	    ibuf_get(b, &bgpid, sizeof(bgpid)) == -1 ||
+	    ibuf_get_n32(b, &bgpid) == -1 ||
 	    ibuf_get_n8(b, &optparamlen) == -1) {
  trunc:
 		printf("truncated message");
@@ -1564,8 +1565,9 @@ show_mrt_open(struct ibuf *b)
 	}
 
 	printf("\n    ");
+	ina.s_addr = htonl(bgpid);
 	printf("Version: %d AS: %u Holdtime: %u BGP Id: %s Paramlen: %u",
-	    version, short_as, holdtime, inet_ntoa(bgpid), optparamlen);
+	    version, short_as, holdtime, inet_ntoa(ina), optparamlen);
 	if (optparamlen != ibuf_size(b)) {
 		/* XXX missing support for RFC9072 */
 		printf("optional parameter length mismatch");
Index: bgpctl/output.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpctl/output.c,v
diff -u -p -r1.50 output.c
--- bgpctl/output.c	31 Jan 2024 11:23:20 -0000	1.50
+++ bgpctl/output.c	20 May 2024 13:31:16 -0000
@@ -299,7 +299,7 @@ show_neighbor_full(struct peer *p, struc
 		printf("\n");
 
 	if (p->state == STATE_ESTABLISHED) {
-		ina.s_addr = p->remote_bgpid;
+		ina.s_addr = htonl(p->remote_bgpid);
 		printf("  BGP version 4, remote router-id %s",
 		    inet_ntoa(ina));
 		printf("%s\n", fmt_auth_method(p->auth.method));
Index: bgpctl/output_json.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpctl/output_json.c,v
diff -u -p -r1.43 output_json.c
--- bgpctl/output_json.c	24 Apr 2024 10:42:09 -0000	1.43
+++ bgpctl/output_json.c	20 May 2024 13:31:24 -0000
@@ -324,7 +324,7 @@ json_neighbor(struct peer *p, struct par
 		    log_addr(&p->conf.remote_addr), p->conf.remote_masklen);
 	if (p->state == STATE_ESTABLISHED) {
 		struct in_addr ina;
-		ina.s_addr = p->remote_bgpid;
+		ina.s_addr = htonl(p->remote_bgpid);
 		json_do_string("bgpid", inet_ntoa(ina));
 	}
 	json_do_string("state", statenames[p->state]);
Index: bgpd/config.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/config.c,v
diff -u -p -r1.108 config.c
--- bgpd/config.c	16 Aug 2023 08:26:35 -0000	1.108
+++ bgpd/config.c	20 May 2024 13:20:41 -0000
@@ -466,7 +466,7 @@ get_bgpid(void)
 	struct ifaddrs		*ifap, *ifa;
 	uint32_t		 ip = 0, cur, localnet;
 
-	localnet = htonl(INADDR_LOOPBACK & IN_CLASSA_NET);
+	localnet = INADDR_LOOPBACK & IN_CLASSA_NET;
 
 	if (getifaddrs(&ifap) == -1)
 		fatal("getifaddrs");
@@ -476,9 +476,10 @@ get_bgpid(void)
 		    ifa->ifa_addr->sa_family != AF_INET)
 			continue;
 		cur = ((struct sockaddr_in *)ifa->ifa_addr)->sin_addr.s_addr;
+		cur = ntohl(cur);
 		if ((cur & localnet) == localnet)	/* skip 127/8 */
 			continue;
-		if (ntohl(cur) > ntohl(ip))
+		if (cur > ip)
 			ip = cur;
 	}
 	freeifaddrs(ifap);
Index: bgpd/mrt.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/mrt.c,v
diff -u -p -r1.116 mrt.c
--- bgpd/mrt.c	14 Jul 2023 10:30:53 -0000	1.116
+++ bgpd/mrt.c	20 May 2024 13:21:06 -0000
@@ -824,7 +824,7 @@ mrt_dump_v2_hdr(struct mrt *mrt, struct 
 		return (-1);
 	}
 
-	if (ibuf_add(buf, &conf->bgpid, sizeof(conf->bgpid)) == -1)
+	if (ibuf_add_n32(buf, conf->bgpid) == -1)
 		goto fail;
 	nlen = strlen(mrt->rib);
 	if (nlen > 0)
Index: bgpd/parse.y
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/parse.y,v
diff -u -p -r1.462 parse.y
--- bgpd/parse.y	24 Apr 2024 10:41:34 -0000	1.462
+++ bgpd/parse.y	20 May 2024 13:26:31 -0000
@@ -742,7 +742,7 @@ conf_main	: AS as4number		{
 				yyerror("router-id must be an IPv4 address");
 				YYERROR;
 			}
-			conf->bgpid = $2.v4.s_addr;
+			conf->bgpid = ntohl($2.v4.s_addr);
 		}
 		| HOLDTIME NUMBER	{
 			if ($2 < MIN_HOLDTIME || $2 > USHRT_MAX) {
@@ -2236,14 +2236,14 @@ peeropts	: REMOTEAS as4number	{
 				YYERROR;
 			}
 			if ((conf->flags & BGPD_FLAG_REFLECTOR) &&
-			    conf->clusterid != $2.v4.s_addr) {
+			    conf->clusterid != ntohl($2.v4.s_addr)) {
 				yyerror("only one route reflector "
 				    "cluster allowed");
 				YYERROR;
 			}
 			conf->flags |= BGPD_FLAG_REFLECTOR;
 			curpeer->conf.reflector_client = 1;
-			conf->clusterid = $2.v4.s_addr;
+			conf->clusterid = ntohl($2.v4.s_addr);
 		}
 		| DEPEND ON STRING	{
 			if (strlcpy(curpeer->conf.if_depend, $3,
Index: bgpd/printconf.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/printconf.c,v
diff -u -p -r1.172 printconf.c
--- bgpd/printconf.c	24 Apr 2024 10:41:34 -0000	1.172
+++ bgpd/printconf.c	20 May 2024 13:29:00 -0000
@@ -383,7 +383,7 @@ print_mainconf(struct bgpd_config *conf)
 	printf("AS %s", log_as(conf->as));
 	if (conf->as > USHRT_MAX && conf->short_as != AS_TRANS)
 		printf(" %u", conf->short_as);
-	ina.s_addr = conf->bgpid;
+	ina.s_addr = htonl(conf->bgpid);
 	printf("\nrouter-id %s\n", inet_ntoa(ina));
 
 	printf("socket \"%s\"\n", conf->csock);
@@ -800,7 +800,7 @@ print_peer(struct peer_config *p, struct
 		if (conf->clusterid == 0)
 			printf("%s\troute-reflector\n", c);
 		else {
-			ina.s_addr = conf->clusterid;
+			ina.s_addr = htonl(conf->clusterid);
 			printf("%s\troute-reflector %s\n", c,
 			    inet_ntoa(ina));
 		}
Index: bgpd/rde.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v
diff -u -p -r1.624 rde.c
--- bgpd/rde.c	20 Mar 2024 09:35:46 -0000	1.624
+++ bgpd/rde.c	20 May 2024 13:28:50 -0000
@@ -2647,48 +2647,49 @@ rde_reflector(struct rde_peer *peer, str
 
 	/* check for originator id if eq router_id drop */
 	if ((a = attr_optget(asp, ATTR_ORIGINATOR_ID)) != NULL) {
-		if (memcmp(&conf->bgpid, a->data, sizeof(conf->bgpid)) == 0) {
+		id = htonl(conf->bgpid);
+		if (memcmp(&id, a->data, sizeof(id)) == 0) {
 			/* this is coming from myself */
 			asp->flags |= F_ATTR_LOOP;
 			return;
 		}
 	} else if (conf->flags & BGPD_FLAG_REFLECTOR) {
 		if (peer->conf.ebgp)
-			id = conf->bgpid;
+			id = htonl(conf->bgpid);
 		else
 			id = htonl(peer->remote_bgpid);
 		if (attr_optadd(asp, ATTR_OPTIONAL, ATTR_ORIGINATOR_ID,
-		    &id, sizeof(uint32_t)) == -1)
+		    &id, sizeof(id)) == -1)
 			fatalx("attr_optadd failed but impossible");
 	}
 
 	/* check for own id in the cluster list */
 	if (conf->flags & BGPD_FLAG_REFLECTOR) {
+		id = htonl(conf->clusterid);
 		if ((a = attr_optget(asp, ATTR_CLUSTER_LIST)) != NULL) {
-			for (len = 0; len < a->len;
-			    len += sizeof(conf->clusterid))
+			for (len = 0; len < a->len; len += sizeof(id))
 				/* check if coming from my cluster */
-				if (memcmp(&conf->clusterid, a->data + len,
-				    sizeof(conf->clusterid)) == 0) {
+				if (memcmp(&id, a->data + len,
+				    sizeof(id)) == 0) {
 					asp->flags |= F_ATTR_LOOP;
 					return;
 				}
 
 			/* prepend own clusterid by replacing attribute */
-			len = a->len + sizeof(conf->clusterid);
+			len = a->len + sizeof(id);
 			if (len < a->len)
 				fatalx("rde_reflector: cluster-list overflow");
 			if ((p = malloc(len)) == NULL)
 				fatal("rde_reflector");
-			memcpy(p, &conf->clusterid, sizeof(conf->clusterid));
-			memcpy(p + sizeof(conf->clusterid), a->data, a->len);
+			memcpy(p, &id, sizeof(id));
+			memcpy(p + sizeof(id), a->data, a->len);
 			attr_free(asp, a);
 			if (attr_optadd(asp, ATTR_OPTIONAL, ATTR_CLUSTER_LIST,
 			    p, len) == -1)
 				fatalx("attr_optadd failed but impossible");
 			free(p);
 		} else if (attr_optadd(asp, ATTR_OPTIONAL, ATTR_CLUSTER_LIST,
-		    &conf->clusterid, sizeof(conf->clusterid)) == -1)
+		    &id, sizeof(id)) == -1)
 			fatalx("attr_optadd failed but impossible");
 	}
 }
@@ -3584,11 +3585,11 @@ rde_reload_done(void)
 	nconf = NULL;
 
 	/* sync peerself with conf */
-	peerself->remote_bgpid = ntohl(conf->bgpid);
+	peerself->remote_bgpid = conf->bgpid;
 	peerself->conf.local_as = conf->as;
 	peerself->conf.remote_as = conf->as;
 	peerself->conf.remote_addr.aid = AID_INET;
-	peerself->conf.remote_addr.v4.s_addr = conf->bgpid;
+	peerself->conf.remote_addr.v4.s_addr = htonl(conf->bgpid);
 	peerself->conf.remote_masklen = 32;
 	peerself->short_as = conf->short_as;
 
Index: bgpd/rde_peer.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde_peer.c,v
diff -u -p -r1.36 rde_peer.c
--- bgpd/rde_peer.c	20 Mar 2024 09:35:46 -0000	1.36
+++ bgpd/rde_peer.c	20 May 2024 13:24:58 -0000
@@ -422,7 +422,7 @@ peer_up(struct rde_peer *peer, struct se
 		peer->stats.prefix_out_cnt = 0;
 		peer->state = PEER_DOWN;
 	}
-	peer->remote_bgpid = ntohl(sup->remote_bgpid);
+	peer->remote_bgpid = sup->remote_bgpid;
 	peer->short_as = sup->short_as;
 	peer->remote_addr = sup->remote_addr;
 	peer->local_v4_addr = sup->local_v4_addr;
Index: bgpd/session.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/session.c,v
diff -u -p -r1.477 session.c
--- bgpd/session.c	20 May 2024 10:01:52 -0000	1.477
+++ bgpd/session.c	20 May 2024 13:36:53 -0000
@@ -1602,7 +1602,7 @@ session_open(struct peer *p)
 	errs += ibuf_add_n16(buf->buf, p->conf.local_short_as);
 	errs += ibuf_add_n16(buf->buf, holdtime);
 	/* is already in network byte order */
-	errs += ibuf_add(buf->buf, &conf->bgpid, sizeof(conf->bgpid));
+	errs += ibuf_add_n32(buf->buf, conf->bgpid);
 	errs += ibuf_add_n8(buf->buf, optparamlen);
 
 	if (extlen) {
@@ -2228,8 +2228,7 @@ parse_open(struct peer *peer)
 		change_state(peer, STATE_IDLE, EVNT_RCVD_OPEN);
 		return (-1);
 	}
-	/* strore remote_bgpid in network byte order */
-	peer->remote_bgpid = htonl(bgpid);
+	peer->remote_bgpid = bgpid;
 
 	if (optparamlen != 0) {
 		struct ibuf oparams, op;
@@ -2332,8 +2331,10 @@ parse_open(struct peer *peer)
 
 	/* on iBGP sessions check for bgpid collision */
 	if (!peer->conf.ebgp && peer->remote_bgpid == conf->bgpid) {
-		log_peer_warnx(&peer->conf, "peer BGPID %u conflicts with ours",
-		    ntohl(bgpid));
+		struct in_addr ina;
+		ina.s_addr = htonl(bgpid);
+		log_peer_warnx(&peer->conf, "peer BGPID %s conflicts with ours",
+		    inet_ntoa(ina));
 		session_notification(peer, ERR_OPEN, ERR_OPEN_BGPID, NULL);
 		change_state(peer, STATE_IDLE, EVNT_RCVD_OPEN);
 		return (-1);