On Thu, Oct 12, 2023 at 03:21:50PM +0200, Claudio Jeker wrote:
> I optimized "announce add-path send all" to not always re-evaluate all
> possible prefixes. While doing that I introduced a small bug. The problem
> is that the new prefix passed to up_generate_addpath_all() could be not
> eligible. This causes up_process_prefix() -> up_test_update() to error
> out.
>
> The below fix should solve this twice. The rde_update.c diff ensures that
> up_generate_addpath_all() checks the new path to be eligible and is the
> minimal fix. The rde_decide.c diff fixes the problem at the root.
> I came to the conclusion that I want both.
>
> Btw. the other up_generate_* functions don't have this issue since they
> use prefix_best() to get new and that already calls prefix_eligible().
ok tb
One small comment below.
> --
> :wq Claudio
>
> Index: rde_decide.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rde_decide.c,v
> retrieving revision 1.101
> diff -u -p -r1.101 rde_decide.c
> --- rde_decide.c 13 Mar 2023 16:52:42 -0000 1.101
> +++ rde_decide.c 12 Oct 2023 12:56:59 -0000
> @@ -568,9 +568,12 @@ prefix_evaluate(struct rib_entry *re, st
> * to be passed on (not only a change of the best prefix).
> * rde_generate_updates() will then take care of distribution.
> */
> - if (rde_evaluate_all())
> - if ((new != NULL && prefix_eligible(new)) || old != NULL)
> + if (rde_evaluate_all()) {
> + if (new != NULL && !prefix_eligible(new))
> + new = NULL;
> + if (new != NULL || old != NULL)
> rde_generate_updates(re, new, old, EVAL_ALL);
> + }
> }
>
> void
> @@ -578,7 +581,7 @@ prefix_evaluate_nexthop(struct prefix *p
> enum nexthop_state oldstate)
> {
> struct rib_entry *re = prefix_re(p);
> - struct prefix *newbest, *oldbest;
> + struct prefix *newbest, *oldbest, *new, *old;
> struct rib *rib;
>
> /* Skip non local-RIBs or RIBs that are flagged as noeval. */
> @@ -608,6 +611,7 @@ prefix_evaluate_nexthop(struct prefix *p
> * Re-evaluate the prefix by removing the prefix then updating the
> * nexthop state and reinserting the prefix again.
> */
> + old = p;
> oldbest = prefix_best(re);
> prefix_remove(p, re);
>
> @@ -618,6 +622,9 @@ prefix_evaluate_nexthop(struct prefix *p
>
> prefix_insert(p, NULL, re);
> newbest = prefix_best(re);
> + new = p;
> + if (!prefix_eligible(new))
> + new = NULL;
>
> /*
> * If the active prefix changed or the active prefix was removed
> @@ -631,7 +638,7 @@ prefix_evaluate_nexthop(struct prefix *p
> */
> if ((rib->flags & F_RIB_NOFIB) == 0)
> rde_send_kroute(rib, newbest, oldbest);
> - rde_generate_updates(re, p, p, EVAL_DEFAULT);
> + rde_generate_updates(re, new, old, EVAL_DEFAULT);
> return;
> }
>
> @@ -641,5 +648,5 @@ prefix_evaluate_nexthop(struct prefix *p
> * rde_generate_updates() will then take care of distribution.
> */
> if (rde_evaluate_all())
> - rde_generate_updates(re, p, p, EVAL_ALL);
> + rde_generate_updates(re, new, old, EVAL_ALL);
> }
> Index: rde_update.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/rde_update.c,v
> retrieving revision 1.163
> diff -u -p -r1.163 rde_update.c
> --- rde_update.c 12 Jul 2023 14:45:43 -0000 1.163
> +++ rde_update.c 12 Oct 2023 12:54:05 -0000
> @@ -352,6 +352,11 @@ up_generate_addpath_all(struct rde_peer
> all = 1;
> }
>
Not sure if it's worth the complication, but this check could be done in
an else branch to the previous if.
> + if (new != NULL && !prefix_eligible(new)) {
> + /* only allow valid prefixes */
> + new = NULL;
> + }
> +
> if (old != NULL) {
> /* withdraw stale paths */
> p = prefix_adjout_get(peer, old->path_id_tx, old->pt);
>
