Download raw body.
bgpd: adjust adjout_prefix_dump code for the reaper
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);
>
>
bgpd: adjust adjout_prefix_dump code for the reaper