On Wed, Jul 06, 2022 at 06:56:28PM +0200, Claudio Jeker wrote:
> On Wed, Jul 06, 2022 at 06:15:45PM +0200, Theo Buehler wrote:
> > On Wed, Jul 06, 2022 at 05:07:45PM +0200, Claudio Jeker wrote:
> > > This diff changes various loops which call into up_generate_update() so
> > > that all these loops call the same function peer_generate_update() which
> > > then calls up_generate_update(). This is a step to add an alternative path
> > > to generate updates for add-path send support without altering many
> > > code-paths.
> > >
> > > If I did not fool myself this should not alter current behaviour.
> > > rde_up_dump_upcall() gets a few more checks but all those checks should be
> > > false in this case (e.g. peer_dump checks the export_type already).
> >
> > This looks good and I agree that the behavior is mostly preserved. I
> > noticed are two changes of behavior and I have a small question for
> > rde_up_dump_upcall():
> >
> > > @@ -317,20 +374,10 @@ rde_up_dump_upcall(struct rib_entry *re,
> > > struct rde_peer *peer = ptr;
> > > struct prefix *p;
> > >
> > > - if (peer->state != PEER_UP)
> > > - return;
> > > - if (re->rib_id != peer->loc_rib_id)
> > > - fatalx("%s: Unexpected RIB %u != %u.", __func__, re->rib_id,
> > > - peer->loc_rib_id);
> >
> > Failure of this check will now be silent.
>
> This is fine, that code path was acting as an assert().
> rde_up_dump_upcall() is called by a RIB table walk for the
> peer->loc_rib_id RIB (see peer_dump()) so there should be no way that
> if (re->rib_id != peer->loc_rib_id) is true.
Indeed.
>
> > > - if (peer->capa.mp[re->prefix->aid] == 0)
> > > - fatalx("%s: Unexpected %s prefix", __func__,
> > > - aid2str(re->prefix->aid));
> >
> > Same here.
>
> Similar reason. The rib walks each address family independent:
> for (i = 0; i < AID_MAX; i++) {
> if (peer->capa.mp[i])
> peer_dump(peer, i);
> }
>
> Now this check is missing in the IMSG_REFRESH case in rde.c but other than
> that there should be no way to reach this code with a wrong aid.
> Skipping routes with non-negotiated address families should not matter
> here but it will make the code more robust.
>
Makes sense.
>
> > I got lost when checking that re->prefix->aid == p->pt->aid. I assume
> > that follows from the definition of the rib_entry's prefix queue?
>
> If the prefix p is on the list of prefixes in re (part of re->prefix_h)
> then re->prefix == p->pt and so the aid check also needs to match.
> We cache the struct pt_entry * in a few places to simplify access.
> Since the code uses prefix_best(re) to get the p we know that p is part of
> the re list of prefixes.
All these comments made things a lot clearer. Thanks
>
> > > -
> > > /* no eligible prefix, not even for 'evaluate all' */
> > > if ((p = prefix_best(re)) == NULL)
> > > return;
> > > -
> > > - up_generate_updates(out_rules, peer, p, NULL);
> > > + peer_generate_update(peer, re->rib_id, p, NULL, 0);
> > > }
> > >
> > > static void
>
> Updated diff below that adds the extra aid check in the IMSG_REFRESH case.
ok tb
>
> --
> :wq Claudio
>
> Index: rde.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rde.c,v
> retrieving revision 1.547
> diff -u -p -r1.547 rde.c
> --- rde.c 27 Jun 2022 13:26:51 -0000 1.547
> +++ rde.c 6 Jul 2022 16:43:22 -0000
> @@ -1123,12 +1123,19 @@ rde_dispatch_imsg_peer(struct rde_peer *
> break;
> case IMSG_REFRESH:
> if (imsg.hdr.len - IMSG_HEADER_SIZE != sizeof(rr)) {
> - log_warnx("%s: wrong imsg len", __func__);
> + log_warnx("route refresh: wrong imsg len");
> break;
> }
> memcpy(&rr, imsg.data, sizeof(rr));
> if (rr.aid >= AID_MAX) {
> - log_warnx("%s: bad AID", __func__);
> + log_peer_warnx(&peer->conf,
> + "route refresh: bad AID %d", rr.aid);
> + break;
> + }
> + if (peer->capa.mp[rr.aid]) {
> + log_peer_warnx(&peer->conf,
> + "route refresh: AID %s not negotiated",
> + aid2str(rr.aid));
> break;
> }
> switch (rr.subtype) {
> @@ -1156,7 +1163,8 @@ rde_dispatch_imsg_peer(struct rde_peer *
> aid2str(rr.aid));
> break;
> default:
> - log_warnx("%s: bad subtype %d", __func__, rr.subtype);
> + log_peer_warnx(&peer->conf,
> + "route refresh: bad subtype %d", rr.subtype);
> break;
> }
> break;
> @@ -3037,59 +3045,6 @@ rde_evaluate_all(void)
> return rde_eval_all;
> }
>
> -static int
> -rde_skip_peer(struct rde_peer *peer, uint16_t rib_id, uint8_t aid)
> -{
> - /* skip ourself */
> - if (peer == peerself)
> - return 1;
> - if (peer->state != PEER_UP)
> - return 1;
> - /* skip peers using a different rib */
> - if (peer->loc_rib_id != rib_id)
> - return 1;
> - /* check if peer actually supports the address family */
> - if (peer->capa.mp[aid] == 0)
> - return 1;
> - /* skip peers with special export types */
> - if (peer->export_type == EXPORT_NONE ||
> - peer->export_type == EXPORT_DEFAULT_ROUTE)
> - return 1;
> -
> - return 0;
> -}
> -
> -void
> -rde_generate_updates(struct rib *rib, struct prefix *new, struct prefix *old,
> - int eval_all)
> -{
> - struct rde_peer *peer;
> - uint8_t aid;
> -
> - /*
> - * If old is != NULL we know it was active and should be removed.
> - * If new is != NULL we know it is reachable and then we should
> - * generate an update.
> - */
> - if (old == NULL && new == NULL)
> - return;
> -
> - if (new)
> - aid = new->pt->aid;
> - else
> - aid = old->pt->aid;
> -
> - LIST_FOREACH(peer, &peerlist, peer_l) {
> - if (rde_skip_peer(peer, rib->id, aid))
> - continue;
> - /* skip regular peers if the best path didn't change */
> - if ((peer->flags & PEERFLAG_EVALUATE_ALL) == 0 && eval_all)
> - continue;
> -
> - up_generate_updates(out_rules, peer, new, old);
> - }
> -}
> -
> /* flush Adj-RIB-Out by withdrawing all prefixes */
> static void
> rde_up_flush_upcall(struct prefix *p, void *ptr)
> @@ -3760,26 +3715,16 @@ rde_softreconfig_in(struct rib_entry *re
> }
>
> static void
> -rde_softreconfig_out(struct rib_entry *re, void *bula)
> +rde_softreconfig_out(struct rib_entry *re, void *arg)
> {
> - struct prefix *p;
> - struct rde_peer *peer;
> - uint8_t aid = re->prefix->aid;
> + struct rib *rib = arg;
> + struct prefix *p;
>
> if ((p = prefix_best(re)) == NULL)
> /* no valid path for prefix */
> return;
>
> - LIST_FOREACH(peer, &peerlist, peer_l) {
> - if (rde_skip_peer(peer, re->rib_id, aid))
> - continue;
> - /* skip peers which don't need to reconfigure */
> - if (peer->reconf_out == 0)
> - continue;
> -
> - /* Regenerate all updates. */
> - up_generate_updates(out_rules, peer, p, p);
> - }
> + rde_generate_updates(rib, p, NULL, EVAL_RECONF);
> }
>
> static void
> Index: rde.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rde.h,v
> retrieving revision 1.254
> diff -u -p -r1.254 rde.h
> --- rde.h 27 Jun 2022 13:26:51 -0000 1.254
> +++ rde.h 6 Jul 2022 16:54:39 -0000
> @@ -362,6 +362,12 @@ struct filterstate {
> uint8_t nhflags;
> };
>
> +enum eval_mode {
> + EVAL_DEFAULT,
> + EVAL_ALL,
> + EVAL_RECONF,
> +};
> +
> extern struct rde_memstats rdemem;
>
> /* prototypes */
> @@ -384,7 +390,7 @@ void rde_pftable_del(uint16_t, struct p
>
> int rde_evaluate_all(void);
> void rde_generate_updates(struct rib *, struct prefix *,
> - struct prefix *, int);
> + struct prefix *, enum eval_mode);
> uint32_t rde_local_as(void);
> int rde_decisionflags(void);
> void rde_peer_send_rrefresh(struct rde_peer *, uint8_t, uint8_t);
> Index: rde_decide.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rde_decide.c,v
> retrieving revision 1.91
> diff -u -p -r1.91 rde_decide.c
> --- rde_decide.c 22 Mar 2022 10:53:08 -0000 1.91
> +++ rde_decide.c 6 Jul 2022 14:54:58 -0000
> @@ -493,7 +493,7 @@ prefix_evaluate(struct rib_entry *re, st
> * but remember that xp may be NULL aka ineligible.
> * Additional decision may be made by the called functions.
> */
> - rde_generate_updates(rib, xp, active, 0);
> + rde_generate_updates(rib, xp, active, EVAL_DEFAULT);
> if ((rib->flags & F_RIB_NOFIB) == 0)
> rde_send_kroute(rib, xp, active);
> return;
> @@ -506,5 +506,6 @@ prefix_evaluate(struct rib_entry *re, st
> */
> if (rde_evaluate_all())
> if ((new != NULL && prefix_eligible(new)) || old != NULL)
> - rde_generate_updates(rib, prefix_best(re), NULL, 1);
> + rde_generate_updates(rib, prefix_best(re), NULL,
> + EVAL_ALL);
> }
> Index: rde_peer.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rde_peer.c,v
> retrieving revision 1.17
> diff -u -p -r1.17 rde_peer.c
> --- rde_peer.c 27 Jun 2022 13:26:51 -0000 1.17
> +++ rde_peer.c 6 Jul 2022 16:46:56 -0000
> @@ -212,6 +212,63 @@ peer_add(uint32_t id, struct peer_config
> return (peer);
> }
>
> +static void
> +peer_generate_update(struct rde_peer *peer, uint16_t rib_id,
> + struct prefix *new, struct prefix *old, enum eval_mode mode)
> +{
> + uint8_t aid;
> +
> + if (new != NULL)
> + aid = new->pt->aid;
> + else if (old != NULL)
> + aid = old->pt->aid;
> + else
> + return;
> +
> + /* skip ourself */
> + if (peer == peerself)
> + return;
> + if (peer->state != PEER_UP)
> + return;
> + /* skip peers using a different rib */
> + if (peer->loc_rib_id != rib_id)
> + return;
> + /* check if peer actually supports the address family */
> + if (peer->capa.mp[aid] == 0)
> + return;
> + /* skip peers with special export types */
> + if (peer->export_type == EXPORT_NONE ||
> + peer->export_type == EXPORT_DEFAULT_ROUTE)
> + return;
> +
> + /* if reconf skip peers which don't need to reconfigure */
> + if (mode == EVAL_RECONF && peer->reconf_out == 0)
> + return;
> + /* skip regular peers if the best path didn't change */
> + if (mode == EVAL_ALL && (peer->flags & PEERFLAG_EVALUATE_ALL) == 0)
> + return;
> +
> + up_generate_updates(out_rules, peer, new, old);
> +}
> +
> +void
> +rde_generate_updates(struct rib *rib, struct prefix *new, struct prefix *old,
> + enum eval_mode mode)
> +{
> + struct rde_peer *peer;
> +
> + /*
> + * If old is != NULL we know it was active and should be removed.
> + * If new is != NULL we know it is reachable and then we should
> + * generate an update.
> + */
> + if (old == NULL && new == NULL)
> + return;
> +
> + LIST_FOREACH(peer, &peerlist, peer_l)
> + peer_generate_update(peer, rib->id, new, old, mode);
> +}
> +
> /*
> * Various RIB walker callbacks.
> */
> @@ -317,20 +374,10 @@ rde_up_dump_upcall(struct rib_entry *re,
> struct rde_peer *peer = ptr;
> struct prefix *p;
>
> - if (peer->state != PEER_UP)
> - return;
> - if (re->rib_id != peer->loc_rib_id)
> - fatalx("%s: Unexpected RIB %u != %u.", __func__, re->rib_id,
> - peer->loc_rib_id);
> - if (peer->capa.mp[re->prefix->aid] == 0)
> - fatalx("%s: Unexpected %s prefix", __func__,
> - aid2str(re->prefix->aid));
> -
> /* no eligible prefix, not even for 'evaluate all' */
> if ((p = prefix_best(re)) == NULL)
> return;
> -
> - up_generate_updates(out_rules, peer, p, NULL);
> + peer_generate_update(peer, re->rib_id, p, NULL, 0);
> }
>
> static void
> @@ -461,6 +508,7 @@ peer_stale(struct rde_peer *peer, uint8_
> peer->staletime[aid] = now = getmonotime();
> peer->state = PEER_DOWN;
>
> + /* XXX this is not quite correct */
> /* mark Adj-RIB-Out stale for this peer */
> if (prefix_dump_new(peer, AID_UNSPEC, 0, NULL,
> peer_adjout_stale_upcall, NULL, NULL) == -1)
