Index | Thread | Search

From:
Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject:
bgpd: fix min_holdtime handling and simplify holdtime
To:
tech@openbsd.org
Date:
Mon, 17 Feb 2025 16:26:47 +0100

Download raw body.

Thread
Tripped over this today. min_holdtime and holdtime are both in the global
bgpd_config and in the peer_config. If the value in peer_config is 0 then
the global value should be used.

The problem is this is only done for holdtime but not for min_holdtime.
I realy dislike this either or handling inside message processing code
and decided to instead set the peer_config holdtime and min_holdtime value
in init_peer() so the rest of the code does not need to care. Only during
config reloads we need to reapply the values again since the config is
copied over and so it is set back to 0.

The below diff does that.
-- 
:wq Claudio

Index: session.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/session.c,v
diff -u -p -r1.515 session.c
--- session.c	12 Feb 2025 16:49:56 -0000	1.515
+++ session.c	17 Feb 2025 14:57:55 -0000
@@ -56,7 +56,7 @@
 
 void	session_sighdlr(int);
 int	setup_listeners(u_int *);
-void	init_peer(struct peer *);
+void	init_peer(struct peer *, struct bgpd_config *);
 void	start_timer_holdtime(struct peer *);
 void	start_timer_sendholdtime(struct peer *);
 void	start_timer_keepalive(struct peer *);
@@ -257,7 +257,7 @@ session_main(int debug, int verbose)
 			RB_FOREACH_SAFE(p, peer_head, &conf->peers, next) {
 				/* new peer that needs init? */
 				if (p->state == STATE_NONE)
-					init_peer(p);
+					init_peer(p, conf);
 
 				/* deletion due? */
 				if (p->reconf_action == RECONF_DELETE) {
@@ -571,7 +571,7 @@ session_main(int debug, int verbose)
 }
 
 void
-init_peer(struct peer *p)
+init_peer(struct peer *p, struct bgpd_config *c)
 {
 	TAILQ_INIT(&p->timers);
 	p->fd = -1;
@@ -587,6 +587,12 @@ init_peer(struct peer *p)
 	else
 		p->depend_ok = 1;
 
+	/* apply holdtime and min_holdtime settings */
+	if (p->conf.holdtime == 0)
+		p->conf.holdtime = c->holdtime;
+	if (p->conf.min_holdtime == 0)
+		p->conf.min_holdtime = c->min_holdtime;
+
 	peer_cnt++;
 
 	change_state(p, STATE_IDLE, EVNT_NONE);
@@ -1509,7 +1515,6 @@ session_open(struct peer *p)
 {
 	struct ibuf		*buf, *opb;
 	size_t			 len, optparamlen;
-	uint16_t		 holdtime;
 	uint8_t			 i;
 	int			 errs = 0, extlen = 0;
 	int			 mpcapa = 0;
@@ -1641,14 +1646,9 @@ session_open(struct peer *p)
 		return;
 	}
 
-	if (p->conf.holdtime)
-		holdtime = p->conf.holdtime;
-	else
-		holdtime = conf->holdtime;
-
 	errs += ibuf_add_n8(buf, 4);
 	errs += ibuf_add_n16(buf, p->conf.local_short_as);
-	errs += ibuf_add_n16(buf, holdtime);
+	errs += ibuf_add_n16(buf, p->conf.holdtime);
 	/* is already in network byte order */
 	errs += ibuf_add_n32(buf, conf->bgpid);
 	errs += ibuf_add_n8(buf, optparamlen);
@@ -2231,7 +2231,7 @@ parse_open(struct peer *peer, struct ibu
 {
 	uint8_t		 version, rversion;
 	uint16_t	 short_as;
-	uint16_t	 holdtime, myholdtime;
+	uint16_t	 holdtime;
 	uint32_t	 as, bgpid;
 	uint8_t		 optparamlen;
 
@@ -2264,7 +2264,7 @@ parse_open(struct peer *peer, struct ibu
 		return (-1);
 	}
 
-	if (holdtime && holdtime < peer->conf.min_holdtime) {
+	if (holdtime != 0 && holdtime < peer->conf.min_holdtime) {
 		log_peer_warnx(&peer->conf,
 		    "peer requests unacceptable holdtime %u", holdtime);
 		session_notification(peer, ERR_OPEN, ERR_OPEN_HOLDTIME, NULL);
@@ -2272,13 +2272,10 @@ parse_open(struct peer *peer, struct ibu
 		return (-1);
 	}
 
-	myholdtime = peer->conf.holdtime;
-	if (!myholdtime)
-		myholdtime = conf->holdtime;
-	if (holdtime < myholdtime)
+	if (holdtime < peer->conf.holdtime)
 		peer->holdtime = holdtime;
 	else
-		peer->holdtime = myholdtime;
+		peer->holdtime = peer->conf.holdtime;
 
 	/* check bgpid for validity - just disallow 0 */
 	if (bgpid == 0) {
@@ -3545,7 +3542,7 @@ getpeerbyip(struct bgpd_config *c, struc
 		newpeer->reconf_action = RECONF_KEEP;
 		newpeer->rpending = 0;
 		newpeer->wbuf = NULL;
-		init_peer(newpeer);
+		init_peer(newpeer, c);
 		/* start delete timer, it is stopped when session goes up. */
 		timer_set(&newpeer->timers, Timer_SessionDown,
 		    INTERVAL_SESSION_DOWN);
@@ -3829,6 +3826,12 @@ merge_peers(struct bgpd_config *c, struc
 		free(np);
 
 		p->reconf_action = RECONF_KEEP;
+
+		/* reapply holdtime and min_holdtime settings */
+		if (p->conf.holdtime == 0)
+			p->conf.holdtime = conf->holdtime;
+		if (p->conf.min_holdtime == 0)
+			p->conf.min_holdtime = conf->min_holdtime;
 
 		/* had demotion, is demoted, demote removed? */
 		if (p->demoted && !p->conf.demote_group[0])