Index | Thread | Search

From:
Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject:
bgpd: adjust adjout_prefix_dump code for the reaper
To:
tech@openbsd.org
Date:
Thu, 21 May 2026 17:19:41 +0200

Download raw body.

Thread
  • Claudio Jeker:

    bgpd: adjust adjout_prefix_dump code for the reaper

Right now adjout_prefix_dump uses peer_get() to provide a peer pointer to
the callback. This makes the peer reaper unhappy since we want to remove
the peer from the lookup table before calling the peer_reaper walker.

So in other words the peer_reaper walker is currently not working at all
and so another peer may start with a bad Adj-RIB-Out which is hopefully
all cleaned up by the peer_dump call. The full effect of this bug is not
quite clear.

First of all the adjout_prefix_dump context should just use the adjout_bid
and stop using the peer->conf.id. Doing that trickles some additional
challenges since peer can no longer be used.

- The callback drop the struct rde_peer argument (but get a currently unused
  uint32_t bid argument).
- adjout_prefix_first() also needs to switch to using the adjout bitmask
  id and drop the peer argument.
- also change adjout_prefix_next() just to be in sync with
  adjout_prefix_first(). It is not strictly needed but it makes the most
  sense.
- Most callbacks already pass the peer as ctx_arg pointer around so little
  changes are needed there.
- Once again another big surgery is needed in rde_dump_ctx_new()
- The rde_dump_adjout_upcall() now needs to use peer_get() to resolve the
  peer. That's not great, maybe passing the peer pointer in rde_dump_ctx
  would work, but that's for later.

If the peer has no adj-rib-out then exit adjout_prefix_dump_new() early by
calling its done callback. Because of that rde_dump_ctx_new() needs to
enqueue the rde_dump_ctx before calling adjout_prefix_dump_new().

I hope I covered all the crazy in this diff.
-- 
:wq Claudio

Index: rde.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v
diff -u -p -r1.702 rde.c
--- rde.c	20 May 2026 15:29:46 -0000	1.702
+++ rde.c	21 May 2026 13:03:31 -0000
@@ -3207,10 +3207,14 @@ rde_dump_upcall(struct rib_entry *re, vo
 }
 
 static void
-rde_dump_adjout_upcall(struct rde_peer *peer, struct pt_entry *pte,
-    struct adjout_prefix *p, void *ptr)
+rde_dump_adjout_upcall(struct pt_entry *pte, struct adjout_prefix *p,
+    uint32_t bid, void *ptr)
 {
 	struct rde_dump_ctx	*ctx = ptr;
+	struct rde_peer		*peer;
+
+	if ((peer = peer_get(ctx->peerid)) == NULL)
+		return;
 
 	rde_dump_adjout_filter(peer, pte, p, &ctx->req);
 }
@@ -3312,6 +3316,7 @@ rde_dump_ctx_new(struct ctl_show_rib_req
 		ctx->peerid = peer->conf.id;
 		switch (ctx->req.type) {
 		case IMSG_CTL_SHOW_RIB:
+			LIST_INSERT_HEAD(&rde_dump_h, ctx, entry);
 			if (adjout_prefix_dump_new(peer, ctx->req.aid,
 			    CTL_MSG_HIGH_MARK, ctx, rde_dump_adjout_upcall,
 			    rde_dump_done, rde_dump_throttled) == -1)
@@ -3319,6 +3324,7 @@ rde_dump_ctx_new(struct ctl_show_rib_req
 			break;
 		case IMSG_CTL_SHOW_RIB_PREFIX:
 			if (req->flags & F_LONGER) {
+				LIST_INSERT_HEAD(&rde_dump_h, ctx, entry);
 				if (adjout_prefix_dump_subtree(peer,
 				    &req->prefix, req->prefixlen,
 				    CTL_MSG_HIGH_MARK, ctx,
@@ -3342,8 +3348,13 @@ rde_dump_ctx_new(struct ctl_show_rib_req
 
 			do {
 				struct pt_entry *pte;
+				uint32_t bid = peer->adjout_bid;
 				int found;
 
+				ctx->peerid = peer->conf.id;
+				if (bid == 0)
+					continue;
+
 				if (req->flags & F_SHORTER) {
 					for (plen = 0; plen <= req->prefixlen;
 					    plen++) {
@@ -3353,12 +3364,12 @@ rde_dump_ctx_new(struct ctl_show_rib_req
 							continue;
 						/* dump all matching paths */
 						for (p = adjout_prefix_first(
-						    peer, pte);
+						    pte, bid);
 						    p != NULL;
-						    p = adjout_prefix_next(
-						    peer, pte, p)) {
+						    p = adjout_prefix_next(pte,
+						        bid, p)) {
 							rde_dump_adjout_upcall(
-							    peer, pte, p, ctx);
+							    pte, p, bid, ctx);
 						}
 					}
 					continue;
@@ -3374,12 +3385,12 @@ rde_dump_ctx_new(struct ctl_show_rib_req
 				do {
 					/* dump all matching paths */
 					found = 0;
-					for (p = adjout_prefix_first(peer, pte);
+					for (p = adjout_prefix_first(pte, bid);
 					    p != NULL;
-					    p = adjout_prefix_next(peer, pte,
+					    p = adjout_prefix_next(pte, bid,
 					    p)) {
-						rde_dump_adjout_upcall(peer,
-						    pte, p, ctx);
+						rde_dump_adjout_upcall(pte, p,
+						    bid, ctx);
 						found = 1;
 					}
 					plen = pte->prefixlen - 1;
@@ -3396,7 +3407,7 @@ rde_dump_ctx_new(struct ctl_show_rib_req
 					}
 				} while (!found && pte != NULL);
 			} while ((peer = peer_match(&req->neighbor,
-			    peer->conf.id)));
+			    ctx->peerid)) != NULL);
 
 			imsg_compose(ibuf_se_ctl, IMSG_CTL_END, 0, ctx->req.pid,
 			    -1, NULL, 0);
@@ -3411,7 +3422,6 @@ rde_dump_ctx_new(struct ctl_show_rib_req
 			return;
 		}
 
-		LIST_INSERT_HEAD(&rde_dump_h, ctx, entry);
 		return;
 	} else if ((rid = rib_find(req->rib)) == RIB_NOTFOUND) {
 		log_warnx("%s: no such rib %s", __func__, req->rib);
@@ -3649,9 +3659,10 @@ rde_evaluate_all(void)
 
 /* flush Adj-RIB-Out by withdrawing all prefixes */
 static void
-rde_up_flush_upcall(struct rde_peer *peer, struct pt_entry *pte,
-    struct adjout_prefix *p, void *ptr)
+rde_up_flush_upcall(struct pt_entry *pte, struct adjout_prefix *p,
+    uint32_t bid, void *ptr)
 {
+	struct rde_peer *peer = ptr;
 	adjout_prefix_withdraw(peer, pte, p);
 }
 
Index: rde.h
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde.h,v
diff -u -p -r1.350 rde.h
--- rde.h	20 May 2026 18:33:21 -0000	1.350
+++ rde.h	21 May 2026 09:28:46 -0000
@@ -369,8 +369,8 @@ struct rib_context {
 	struct pt_entry			*ctx_pt;
 	uint32_t			 ctx_id;
 	void		(*ctx_rib_call)(struct rib_entry *, void *);
-	void		(*ctx_prefix_call)(struct rde_peer *,
-			    struct pt_entry *, struct adjout_prefix *, void *);
+	void		(*ctx_prefix_call)(struct pt_entry *,
+			    struct adjout_prefix *, uint32_t, void *);
 	void		(*ctx_done)(void *, uint8_t);
 	int		(*ctx_throttle)(void *);
 	void				*ctx_arg;
@@ -767,10 +767,9 @@ int		 nexthop_unref(struct nexthop *);
 void			 adjout_init(void);
 struct adjout_prefix	*adjout_prefix_get(struct rde_peer *, uint32_t,
 			    struct pt_entry *);
-struct adjout_prefix	*adjout_prefix_first(struct rde_peer *,
-			    struct pt_entry *);
-struct adjout_prefix	*adjout_prefix_next(struct rde_peer *,
-			    struct pt_entry *, struct adjout_prefix *);
+struct adjout_prefix	*adjout_prefix_first(struct pt_entry *, uint32_t);
+struct adjout_prefix	*adjout_prefix_next(struct pt_entry *, uint32_t,
+			    struct adjout_prefix *);
 
 void		 adjout_prefix_update(struct adjout_prefix *, struct rde_peer *,
 		    struct filterstate *, struct pt_entry *, uint32_t, int);
@@ -781,13 +780,13 @@ void		 adjout_prefix_dump_cleanup(struct
 void		 adjout_prefix_dump_r(struct rib_context *);
 int		 adjout_prefix_dump_new(struct rde_peer *, uint8_t,
 		    unsigned int, void *,
-		    void (*)(struct rde_peer *, struct pt_entry *,
-			struct adjout_prefix *, void *),
+		    void (*)(struct pt_entry *, struct adjout_prefix *,
+		    uint32_t, void *),
 		    void (*)(void *, uint8_t), int (*)(void *));
 int		 adjout_prefix_dump_subtree(struct rde_peer *,
 		    struct bgpd_addr *, uint8_t, unsigned int, void *,
-		    void (*)(struct rde_peer *, struct pt_entry *,
-			struct adjout_prefix *, void *),
+		    void (*)(struct pt_entry *, struct adjout_prefix *,
+		    uint32_t, void *),
 		    void (*)(void *, uint8_t), int (*)(void *));
 void		 adjout_peer_init(struct rde_peer *);
 void		 adjout_peer_flush_pending(struct rde_peer *);
Index: rde_adjout.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde_adjout.c,v
diff -u -p -r1.19 rde_adjout.c
--- rde_adjout.c	20 May 2026 18:33:21 -0000	1.19
+++ rde_adjout.c	21 May 2026 11:15:47 -0000
@@ -479,21 +479,15 @@ adjout_prefix_with_attrs(struct pt_entry
  * Returns NULL if not found.
  */
 struct adjout_prefix *
-adjout_prefix_first(struct rde_peer *peer, struct pt_entry *pte)
+adjout_prefix_first(struct pt_entry *pte, uint32_t bid)
 {
 	struct adjout_prefix *p;
 	uint32_t i;
-	int has_add_path = 0;
-
-	if (peer_has_add_path(peer, pte->aid, CAPA_AP_SEND))
-		has_add_path = 1;
 
 	for (i = 0; i < pte->adjoutlen; i++) {
 		p = &pte->adjout[i];
-		if (bitmap_test(&p->peermap, peer->adjout_bid))
+		if (bitmap_test(&p->peermap, bid))
 			return p;
-		if (!has_add_path && p->path_id_tx != 0)
-			return NULL;
 	}
 
 	return NULL;
@@ -503,22 +497,19 @@ adjout_prefix_first(struct rde_peer *pee
  * Return next prefix for peer after last.
  */
 struct adjout_prefix *
-adjout_prefix_next(struct rde_peer *peer, struct pt_entry *pte,
+adjout_prefix_next(struct pt_entry *pte, uint32_t bid,
     struct adjout_prefix *last)
 {
 	struct adjout_prefix *p;
 	uint32_t i;
 
-	if (!peer_has_add_path(peer, pte->aid, CAPA_AP_SEND))
-		return NULL;
-
 	i = adjout_prefix_index(pte, last);
 	for (; i < pte->adjoutlen; i++)
 		if (pte->adjout[i].path_id_tx != last->path_id_tx)
 			break;
 	for (; i < pte->adjoutlen; i++) {
 		p = &pte->adjout[i];
-		if (bitmap_test(&p->peermap, peer->adjout_bid))
+		if (bitmap_test(&p->peermap, bid))
 			return p;
 	}
 
@@ -599,10 +590,6 @@ static struct pt_entry *
 prefix_restart(struct rib_context *ctx)
 {
 	struct pt_entry *pte = NULL;
-	struct rde_peer *peer;
-
-	if ((peer = peer_get(ctx->ctx_id)) == NULL)
-		return NULL;
 
 	/* be careful when this is the last reference to pte */
 	if (ctx->ctx_pt != NULL) {
@@ -627,12 +614,8 @@ adjout_prefix_dump_r(struct rib_context 
 {
 	struct pt_entry *pte, *next;
 	struct adjout_prefix *p;
-	struct rde_peer *peer;
 	unsigned int i;
 
-	if ((peer = peer_get(ctx->ctx_id)) == NULL)
-		goto done;
-
 	if (ctx->ctx_pt == NULL && ctx->ctx_subtree.aid == AID_UNSPEC)
 		pte = pt_first(ctx->ctx_aid);
 	else
@@ -656,13 +639,12 @@ adjout_prefix_dump_r(struct rib_context 
 			ctx->ctx_pt = pt_ref(pte);
 			return;
 		}
-		p = adjout_prefix_first(peer, pte);
+		p = adjout_prefix_first(pte, ctx->ctx_id);
 		if (p == NULL)
 			continue;
-		ctx->ctx_prefix_call(peer, pte, p, ctx->ctx_arg);
+		ctx->ctx_prefix_call(pte, p, ctx->ctx_id, ctx->ctx_arg);
 	}
 
-done:
 	if (ctx->ctx_done)
 		ctx->ctx_done(ctx->ctx_arg, ctx->ctx_aid);
 	LIST_REMOVE(ctx, entry);
@@ -672,16 +654,22 @@ done:
 int
 adjout_prefix_dump_new(struct rde_peer *peer, uint8_t aid,
     unsigned int count, void *arg,
-    void (*upcall)(struct rde_peer *, struct pt_entry *,
-	struct adjout_prefix *, void *),
+    void (*upcall)(struct pt_entry *, struct adjout_prefix *, uint32_t, void *),
     void (*done)(void *, uint8_t),
     int (*throttle)(void *))
 {
 	struct rib_context *ctx;
 
+	/* no adjout_bid -> no adj-rib-out */
+	if (peer->adjout_bid == 0) {
+		if (done)
+			done(arg, aid);
+		return 0;
+	}
+
 	if ((ctx = calloc(1, sizeof(*ctx))) == NULL)
 		return -1;
-	ctx->ctx_id = peer->conf.id;
+	ctx->ctx_id = peer->adjout_bid;
 	ctx->ctx_aid = aid;
 	ctx->ctx_count = count;
 	ctx->ctx_arg = arg;
@@ -701,16 +689,22 @@ adjout_prefix_dump_new(struct rde_peer *
 int
 adjout_prefix_dump_subtree(struct rde_peer *peer, struct bgpd_addr *subtree,
     uint8_t subtreelen, unsigned int count, void *arg,
-    void (*upcall)(struct rde_peer *, struct pt_entry *,
-	struct adjout_prefix *, void *),
+    void (*upcall)(struct pt_entry *, struct adjout_prefix *, uint32_t, void *),
     void (*done)(void *, uint8_t),
     int (*throttle)(void *))
 {
 	struct rib_context *ctx;
 
+	/* no adjout_bid -> no adj-rib-out */
+	if (peer->adjout_bid == 0) {
+		if (done)
+			done(arg, subtree->aid);
+		return 0;
+	}
+
 	if ((ctx = calloc(1, sizeof(*ctx))) == NULL)
 		return -1;
-	ctx->ctx_id = peer->conf.id;
+	ctx->ctx_id = peer->adjout_bid;
 	ctx->ctx_aid = subtree->aid;
 	ctx->ctx_count = count;
 	ctx->ctx_arg = arg;
Index: rde_peer.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde_peer.c,v
diff -u -p -r1.73 rde_peer.c
--- rde_peer.c	20 May 2026 18:33:21 -0000	1.73
+++ rde_peer.c	21 May 2026 09:16:16 -0000
@@ -513,9 +513,11 @@ peer_down(struct rde_peer *peer)
  * RIB walker callback for peer_delete / the reaper.
  */
 static void
-peer_reaper_upcall(struct rde_peer *peer, struct pt_entry *pte,
-    struct adjout_prefix *p, void *ptr)
+peer_reaper_upcall(struct pt_entry *pte, struct adjout_prefix *p,
+    uint32_t bid, void *ptr)
 {
+	struct rde_peer		*peer = ptr;
+
 	adjout_prefix_withdraw(peer, pte, p);
 }
 
@@ -617,9 +619,11 @@ peer_stale(struct rde_peer *peer, uint8_
  * Enqueue a prefix onto the update queue so it can be sent out.
  */
 static void
-peer_blast_upcall(struct rde_peer *peer, struct pt_entry *pte,
-    struct adjout_prefix *p, void *ptr)
+peer_blast_upcall(struct pt_entry *pte, struct adjout_prefix *p,
+    uint32_t bid, void *ptr)
 {
+	struct rde_peer		*peer = ptr;
+
 	pend_prefix_add(peer, p->attrs, pte, p->path_id_tx);
 }
 
Index: rde_rib.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde_rib.c,v
diff -u -p -r1.295 rde_rib.c
--- rde_rib.c	20 May 2026 20:32:50 -0000	1.295
+++ rde_rib.c	21 May 2026 15:01:38 -0000
@@ -519,7 +519,7 @@ rib_dump_terminate(void *arg)
 	struct rib_context *ctx, *next;
 
 	LIST_FOREACH_SAFE(ctx, &rib_dumps, entry, next) {
-		if (ctx->ctx_arg != arg)
+		if (ctx->ctx_re && ctx->ctx_arg != arg)
 			continue;
 		rib_dump_free(ctx);
 	}
Index: rde_update.c
===================================================================
RCS file: /cvs/src/usr.sbin/bgpd/rde_update.c,v
diff -u -p -r1.196 rde_update.c
--- rde_update.c	20 May 2026 18:33:21 -0000	1.196
+++ rde_update.c	21 May 2026 09:30:15 -0000
@@ -226,7 +226,7 @@ up_generate_updates(struct rde_peer *pee
 	struct prefix		*new;
 	struct adjout_prefix	*p;
 
-	p = adjout_prefix_first(peer, re->prefix);
+	p = adjout_prefix_first(re->prefix, peer->adjout_bid);
 
 	new = prefix_best(re);
 	while (new != NULL) {
@@ -271,8 +271,9 @@ up_generate_addpath(struct rde_peer *pee
 	unsigned int		pidx = 0, i;
 
 	/* collect all current paths */
-	head = adjout_prefix_first(peer, re->prefix);
-	for (p = head; p != NULL; p = adjout_prefix_next(peer, re->prefix, p)) {
+	head = adjout_prefix_first(re->prefix, peer->adjout_bid);
+	for (p = head; p != NULL;
+	    p = adjout_prefix_next(re->prefix, peer->adjout_bid, p)) {
 		addpath_prefix_list[pidx++] = p->path_id_tx;
 		if (pidx >= nitems(addpath_prefix_list))
 			fatalx("too many addpath paths to select from");
@@ -450,7 +451,7 @@ up_generate_default(struct rde_peer *pee
 	pte = pt_get(&addr, 0);
 	if (pte == NULL)
 		pte = pt_add(&addr, 0);
-	p = adjout_prefix_first(peer, pte);
+	p = adjout_prefix_first(pte, peer->adjout_bid);
 	adjout_prefix_update(p, peer, &state, pte, 0, 1);
 	rde_filterstate_clean(&state);