From: Theo Buehler <tb@theobuehler.org>
Subject: Re: bgpd: adjust adjout_prefix_dump code for the reaper
To: tech@openbsd.org
Date: Thu, 28 May 2026 06:33:42 +0200

On Thu, May 21, 2026 at 11:31:53PM +0200, Claudio Jeker wrote:
> On Thu, May 21, 2026 at 05:19:41PM +0200, Claudio Jeker wrote:
> > 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.
> 
> Doing the early exit in adjout_prefix_dump_new() by calling the done
> callback is a can of worms. Most code does not expect the callback to be
> run that early. Instead of trying to fix all the mess caused by that move
> the check into adjout_prefix_dump_r() were the peer_get was done before.

I think this diff is fine. Just some minor stuff below.

ok tb

> 
> -- 
> :wq Claudio
> 
> Index: rde.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v
> diff -u -p -r1.703 rde.c
> --- rde.c	21 May 2026 15:20:27 -0000	1.703
> +++ rde.c	21 May 2026 21:05:39 -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);
>  }
> @@ -3342,8 +3346,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;

I found this bid -> peerid -> bid back and forth a bit confusing. I
think you can spare a line or two and do something a la:

				ctx->peerid = peer->conf.id;

				bid = peer->adjout_bid;
				if (bid == 0)
					continue;

> +
>  				if (req->flags & F_SHORTER) {
>  					for (plen = 0; plen <= req->prefixlen;
>  					    plen++) {
> @@ -3353,12 +3362,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)) {

There are 8 spaces before bid. I could live with this being a tab. I think
style(9) really wants this wrapping instead:

						    p = adjout_prefix_next(pte,
						    bid, p)) {

I find it hard to read code that starts in column 50+, especially in
diff form. I understand that we have bigger fish to fry.

>  							rde_dump_adjout_upcall(
> -							    peer, pte, p, ctx);
> +							    pte, p, bid, ctx);
>  						}
>  					}
>  					continue;
> @@ -3374,12 +3383,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 +3405,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);
> @@ -3649,9 +3658,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;

The upcalls in rde_peer have an extra empty line here.

>  	adjout_prefix_withdraw(peer, pte, p);
>  }
>  
> Index: rde.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rde.h,v
> diff -u -p -r1.351 rde.h
> --- rde.h	21 May 2026 15:20:27 -0000	1.351
> +++ rde.h	21 May 2026 18:27:08 -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 20:41:05 -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,10 +614,10 @@ 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)
> +	/* no adjout_bid -> no adj-rib-out */
> +	if (ctx->ctx_id == 0)
>  		goto done;

Maybe assign ctx->ctx_id to a uint32_t adjout_bid?

>  
>  	if (ctx->ctx_pt == NULL && ctx->ctx_subtree.aid == AID_UNSPEC)
> @@ -656,13 +643,13 @@ 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:
> + done:
>  	if (ctx->ctx_done)
>  		ctx->ctx_done(ctx->ctx_arg, ctx->ctx_aid);
>  	LIST_REMOVE(ctx, entry);
> @@ -672,8 +659,7 @@ 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 *))
>  {
> @@ -681,7 +667,7 @@ adjout_prefix_dump_new(struct rde_peer *
>  
>  	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,8 +687,7 @@ 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 *))
>  {
> @@ -710,7 +695,7 @@ adjout_prefix_dump_subtree(struct rde_pe
>  
>  	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.74 rde_peer.c
> --- rde_peer.c	21 May 2026 15:20:27 -0000	1.74
> +++ rde_peer.c	21 May 2026 18:27:08 -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.296 rde_rib.c
> --- rde_rib.c	21 May 2026 15:20:27 -0000	1.296
> +++ rde_rib.c	21 May 2026 21:13:17 -0000
> @@ -507,7 +507,7 @@ rib_dump_abort(uint16_t id)
>  	struct rib_context *ctx, *next;
>  
>  	LIST_FOREACH_SAFE(ctx, &rib_dumps, entry, next) {
> -		if (id != ctx->ctx_id)
> +		if (ctx->ctx_re != NULL && id != ctx->ctx_id)
>  			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);
>  
>