Cleanup AID handling.

- Loops over all valid AID should start with AID_MIN and go up to AID_MAX - 1
   e.g. for (i = AID_MIN; i < AID_MAX; i++)
  If for some reason AID_UNSPEC must be handled make that explicit in the
  for loop.

- aid2afi() now returns an error for AID_UNSPEC since there is no valid
  AFI SAFI combo for AID_UNSPEC.

- Add additional checks for AID_MIN where currently only AID_MAX was checked.
  This affects imsg for route refresh and graceful restart.

- Simplify add-path capability handling. Only the negotiated add_path capa
  sets the flag for AID_UNSPEC to help code to quickly check if any add-path
  is active.

OK tb@

diff --git a/usr.sbin/bgpd/parse.y b/usr.sbin/bgpd/parse.y
index ea47a65..2fc507e 100644 (file)
--- a/usr.sbin/bgpd/parse.y
+++ b/usr.sbin/bgpd/parse.y
@@ -1,4 +1,4 @@
-/*     $OpenBSD: parse.y,v 1.456 2024/03/18 14:54:52 claudio Exp $ */
+/*     $OpenBSD: parse.y,v 1.457 2024/03/20 09:35:46 claudio Exp $ */
 
 /*
  * Copyright (c) 2002, 2003, 2004 Henning Brauer <henning@openbsd.org>
@@ -1894,7 +1894,7 @@ peeropts  : REMOTEAS as4number    {
                        uint16_t        afi;
 
                        if ($3 == SAFI_NONE) {
-                               for (aid = 0; aid < AID_MAX; aid++) {
+                               for (aid = AID_MIN; aid < AID_MAX; aid++) {
                                        if (aid2afi(aid, &afi, &safi) == -1 ||
                                            afi != $2)
                                                continue;
@@ -1927,11 +1927,11 @@ peeropts        : REMOTEAS as4number    {
                        int8_t *ap = curpeer->conf.capabilities.add_path;
                        uint8_t i;
 
-                       for (i = 0; i < AID_MAX; i++)
+                       for (i = AID_MIN; i < AID_MAX; i++)
                                if ($4)
-                                       *ap++ |= CAPA_AP_RECV;
+                                       ap[i] |= CAPA_AP_RECV;
                                else
-                                       *ap++ &= ~CAPA_AP_RECV;
+                                       ap[i] &= ~CAPA_AP_RECV;
                }
                | ANNOUNCE ADDPATH SEND STRING addpathextra addpathmax {
                        int8_t *ap = curpeer->conf.capabilities.add_path;
@@ -1945,9 +1945,7 @@ peeropts  : REMOTEAS as4number    {
                                            "for 'add-path send no'");
                                        YYERROR;
                                }
-                               for (i = 0; i < AID_MAX; i++)
-                                       *ap++ &= ~CAPA_AP_SEND;
-                               break;
+                               mode = ADDPATH_EVAL_NONE;
                        } else if (!strcmp($4, "all")) {
                                free($4);
                                if ($5 != 0 || $6 != 0) {
@@ -1971,8 +1969,12 @@ peeropts : REMOTEAS as4number    {
                                free($4);
                                YYERROR;
                        }
-                       for (i = 0; i < AID_MAX; i++)
-                               *ap++ |= CAPA_AP_SEND;
+                       for (i = AID_MIN; i < AID_MAX; i++) {
+                               if (mode != ADDPATH_EVAL_NONE)
+                                       ap[i] |= CAPA_AP_SEND;
+                               else
+                                       ap[i] &= ~CAPA_AP_SEND;
+                       }
                        curpeer->conf.eval.mode = mode;
                        curpeer->conf.eval.extrapaths = $5;
                        curpeer->conf.eval.maxpaths = $6;
@@ -4611,7 +4613,6 @@ struct peer *
 alloc_peer(void)
 {
        struct peer     *p;
-       uint8_t          i;
 
        if ((p = calloc(1, sizeof(struct peer))) == NULL)
                fatal("new_peer");
@@ -4622,8 +4623,6 @@ alloc_peer(void)
        p->conf.distance = 1;
        p->conf.export_type = EXPORT_UNSET;
        p->conf.announce_capa = 1;
-       for (i = 0; i < AID_MAX; i++)
-               p->conf.capabilities.mp[i] = 0;
        p->conf.capabilities.refresh = 1;
        p->conf.capabilities.grestart.restart = 1;
        p->conf.capabilities.as4byte = 1;

diff --git a/usr.sbin/bgpd/printconf.c b/usr.sbin/bgpd/printconf.c
index 5f39b69..74095dd 100644 (file)
--- a/usr.sbin/bgpd/printconf.c
+++ b/usr.sbin/bgpd/printconf.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: printconf.c,v 1.169 2024/01/10 13:31:09 claudio Exp $ */
+/*     $OpenBSD: printconf.c,v 1.170 2024/03/20 09:35:46 claudio Exp $ */
 
 /*
  * Copyright (c) 2003, 2004 Henning Brauer <henning@openbsd.org>
@@ -920,7 +920,7 @@ print_announce(struct peer_config *p, const char *c)
        if (p->announce_capa == 0)
                printf("%s\tannounce capabilities no\n", c);
 
-       for (aid = 0; aid < AID_MAX; aid++)
+       for (aid = AID_MIN; aid < AID_MAX; aid++)
                if (p->capabilities.mp[aid])
                        printf("%s\tannounce %s\n", c, aid2str(aid));
 

diff --git a/usr.sbin/bgpd/rde.c b/usr.sbin/bgpd/rde.c
index 62acf44..73dce0a 100644 (file)
--- a/usr.sbin/bgpd/rde.c
+++ b/usr.sbin/bgpd/rde.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: rde.c,v 1.623 2024/02/22 06:45:22 miod Exp $ */
+/*     $OpenBSD: rde.c,v 1.624 2024/03/20 09:35:46 claudio Exp $ */
 
 /*
  * Copyright (c) 2003, 2004 Henning Brauer <henning@openbsd.org>
@@ -442,7 +442,7 @@ rde_dispatch_imsg_session(struct imsgbuf *imsgbuf)
                                log_warnx("%s: wrong imsg len", __func__);
                                break;
                        }
-                       if (aid >= AID_MAX) {
+                       if (aid < AID_MIN || aid >= AID_MAX) {
                                log_warnx("%s: bad AID", __func__);
                                break;
                        }
@@ -1328,7 +1328,7 @@ rde_dispatch_imsg_peer(struct rde_peer *peer, void *bula)
                        log_warnx("route refresh: wrong imsg len");
                        break;
                }
-               if (rr.aid >= AID_MAX) {
+               if (rr.aid < AID_MIN || rr.aid >= AID_MAX) {
                        log_peer_warnx(&peer->conf,
                            "route refresh: bad AID %d", rr.aid);
                        break;
@@ -3326,7 +3326,7 @@ rde_update_queue_pending(void)
                        continue;
                if (peer->throttled)
                        continue;
-               for (aid = 0; aid < AID_MAX; aid++) {
+               for (aid = AID_MIN; aid < AID_MAX; aid++) {
                        if (!RB_EMPTY(&peer->updates[aid]) ||
                            !RB_EMPTY(&peer->withdraws[aid]))
                                return 1;
@@ -3821,7 +3821,7 @@ rde_softreconfig_in_done(void *arg, uint8_t dummy)
                                peer->reconf_out = 0;
                        } else if (peer->export_type == EXPORT_DEFAULT_ROUTE) {
                                /* just resend the default route */
-                               for (aid = 0; aid < AID_MAX; aid++) {
+                               for (aid = AID_MIN; aid < AID_MAX; aid++) {
                                        if (peer->capa.mp[aid])
                                                up_generate_default(peer, aid);
                                }
@@ -3831,7 +3831,7 @@ rde_softreconfig_in_done(void *arg, uint8_t dummy)
                                    RECONF_RELOAD;
                } else if (peer->reconf_rib) {
                        /* dump the full table to neighbors that changed rib */
-                       for (aid = 0; aid < AID_MAX; aid++) {
+                       for (aid = AID_MIN; aid < AID_MAX; aid++) {
                                if (peer->capa.mp[aid])
                                        peer_dump(peer, aid);
                        }

diff --git a/usr.sbin/bgpd/rde_peer.c b/usr.sbin/bgpd/rde_peer.c
index 84e9526..00d12f9 100644 (file)
--- a/usr.sbin/bgpd/rde_peer.c
+++ b/usr.sbin/bgpd/rde_peer.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: rde_peer.c,v 1.35 2024/02/03 09:26:52 jsg Exp $ */
+/*     $OpenBSD: rde_peer.c,v 1.36 2024/03/20 09:35:46 claudio Exp $ */
 
 /*
  * Copyright (c) 2019 Claudio Jeker <claudio@openbsd.org>
@@ -44,18 +44,15 @@ peer_has_as4byte(struct rde_peer *peer)
        return (peer->capa.as4byte);
 }
 
+/*
+ * Check if ADD_PATH is enabled for aid and mode (rx / tx). If aid is
+ * AID_UNSPEC then the function returns true if any aid has mode enabled.
+ */
 int
 peer_has_add_path(struct rde_peer *peer, uint8_t aid, int mode)
 {
        if (aid >= AID_MAX)
                return 0;
-       if (aid == AID_UNSPEC) {
-               /* check if at capability is set for at least one AID */
-               for (aid = AID_MIN; aid < AID_MAX; aid++)
-                       if (peer->capa.add_path[aid] & mode)
-                               return 1;
-               return 0;
-       }
        return (peer->capa.add_path[aid] & mode);
 }
 
@@ -444,7 +441,7 @@ peer_up(struct rde_peer *peer, struct session_up *sup)
        }
        peer->state = PEER_UP;
 
-       for (i = 0; i < AID_MAX; i++) {
+       for (i = AID_MIN; i < AID_MAX; i++) {
                if (peer->capa.mp[i])
                        peer_dump(peer, i);
        }
@@ -500,7 +497,7 @@ peer_flush(struct rde_peer *peer, uint8_t aid, time_t staletime)
        /* every route is gone so reset staletime */
        if (aid == AID_UNSPEC) {
                uint8_t i;
-               for (i = 0; i < AID_MAX; i++)
+               for (i = AID_MIN; i < AID_MAX; i++)
                        peer->staletime[i] = 0;
        } else {
                peer->staletime[aid] = 0;

diff --git a/usr.sbin/bgpd/session.c b/usr.sbin/bgpd/session.c
index fddd2f4..3aeec77 100644 (file)
--- a/usr.sbin/bgpd/session.c
+++ b/usr.sbin/bgpd/session.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: session.c,v 1.463 2024/02/19 10:15:35 job Exp $ */
+/*     $OpenBSD: session.c,v 1.464 2024/03/20 09:35:46 claudio Exp $ */
 
 /*
  * Copyright (c) 2003, 2004, 2005 Henning Brauer <henning@openbsd.org>
@@ -68,7 +68,7 @@ void  session_tcp_established(struct peer *);
 void   session_capa_ann_none(struct peer *);
 int    session_capa_add(struct ibuf *, uint8_t, uint8_t);
 int    session_capa_add_mp(struct ibuf *, uint8_t);
-int    session_capa_add_afi(struct peer *, struct ibuf *, uint8_t, uint8_t);
+int    session_capa_add_afi(struct ibuf *, uint8_t, uint8_t);
 struct bgp_msg *session_newmsg(enum msg_type, uint16_t);
 int    session_sendmsg(struct bgp_msg *, struct peer *);
 void   session_open(struct peer *);
@@ -1357,8 +1357,7 @@ session_capa_add_mp(struct ibuf *buf, uint8_t aid)
 }
 
 int
-session_capa_add_afi(struct peer *p, struct ibuf *b, uint8_t aid,
-    uint8_t flags)
+session_capa_add_afi(struct ibuf *b, uint8_t aid, uint8_t flags)
 {
        u_int           errs = 0;
        uint16_t        afi;
@@ -1492,7 +1491,7 @@ session_open(struct peer *p)
        }
 
        /* multiprotocol extensions, RFC 4760 */
-       for (i = 0; i < AID_MAX; i++)
+       for (i = AID_MIN; i < AID_MAX; i++)
                if (p->capa.ann.mp[i]) {        /* 4 bytes data */
                        errs += session_capa_add(opb, CAPA_MP, 4);
                        errs += session_capa_add_mp(opb, i);
@@ -1517,7 +1516,7 @@ session_open(struct peer *p)
                int             rst = 0;
                uint16_t        hdr = 0;
 
-               for (i = 0; i < AID_MAX; i++) {
+               for (i = AID_MIN; i < AID_MAX; i++) {
                        if (p->capa.neg.grestart.flags[i] & CAPA_GR_RESTARTING)
                                rst++;
                }
@@ -1536,7 +1535,7 @@ session_open(struct peer *p)
        }
 
        /* advertisement of multiple paths, RFC7911 */
-       if (p->capa.ann.add_path[0]) {  /* variable */
+       if (p->capa.ann.add_path[AID_MIN]) {    /* variable */
                uint8_t aplen;
 
                if (mpcapa)
@@ -1547,12 +1546,12 @@ session_open(struct peer *p)
                if (mpcapa) {
                        for (i = AID_MIN; i < AID_MAX; i++) {
                                if (p->capa.ann.mp[i]) {
-                                       errs += session_capa_add_afi(p, opb,
+                                       errs += session_capa_add_afi(opb,
                                            i, p->capa.ann.add_path[i]);
                                }
                        }
                } else {        /* AID_INET */
-                       errs += session_capa_add_afi(p, opb, AID_INET,
+                       errs += session_capa_add_afi(opb, AID_INET,
                            p->capa.ann.add_path[AID_INET]);
                }
        }
@@ -1759,7 +1758,7 @@ session_neighbor_rrefresh(struct peer *p)
        if (!(p->capa.neg.refresh || p->capa.neg.enhanced_rr))
                return (-1);
 
-       for (i = 0; i < AID_MAX; i++) {
+       for (i = AID_MIN; i < AID_MAX; i++) {
                if (p->capa.neg.mp[i] != 0)
                        session_rrefresh(p, i, ROUTE_REFRESH_REQUEST);
        }
@@ -1828,7 +1827,7 @@ session_graceful_restart(struct peer *p)
        timer_set(&p->timers, Timer_RestartTimeout,
            p->capa.neg.grestart.timeout);
 
-       for (i = 0; i < AID_MAX; i++) {
+       for (i = AID_MIN; i < AID_MAX; i++) {
                if (p->capa.neg.grestart.flags[i] & CAPA_GR_PRESENT) {
                        if (imsg_rde(IMSG_SESSION_STALE, p->conf.id,
                            &i, sizeof(i)) == -1)
@@ -1854,7 +1853,7 @@ session_graceful_stop(struct peer *p)
 {
        uint8_t i;
 
-       for (i = 0; i < AID_MAX; i++) {
+       for (i = AID_MIN; i < AID_MAX; i++) {
                /*
                 * Only flush if the peer is restarting and the timeout fired.
                 * In all other cases the session was already flushed when the
@@ -2485,7 +2484,6 @@ parse_notification(struct peer *peer)
        uint8_t          capa_code;
        uint8_t          capa_len;
        size_t           reason_len;
-       uint8_t          i;
 
        /* just log */
        p = peer->rbuf->rptr;
@@ -2545,8 +2543,8 @@ parse_notification(struct peer *peer)
                        datalen -= capa_len;
                        switch (capa_code) {
                        case CAPA_MP:
-                               for (i = 0; i < AID_MAX; i++)
-                                       peer->capa.ann.mp[i] = 0;
+                               memset(peer->capa.ann.mp, 0,
+                                   sizeof(peer->capa.ann.mp));
                                log_peer_warnx(&peer->conf,
                                    "disabling multiprotocol capability");
                                break;
@@ -2840,12 +2838,11 @@ capa_neg_calc(struct peer *p, uint8_t *suberr)
            (p->capa.ann.refresh && p->capa.peer.refresh) != 0;
        p->capa.neg.enhanced_rr =
            (p->capa.ann.enhanced_rr && p->capa.peer.enhanced_rr) != 0;
-
        p->capa.neg.as4byte =
            (p->capa.ann.as4byte && p->capa.peer.as4byte) != 0;
 
        /* MP: both side must agree on the AFI,SAFI pair */
-       for (i = 0; i < AID_MAX; i++) {
+       for (i = AID_MIN; i < AID_MAX; i++) {
                if (p->capa.ann.mp[i] && p->capa.peer.mp[i])
                        p->capa.neg.mp[i] = 1;
                else
@@ -2866,7 +2863,7 @@ capa_neg_calc(struct peer *p, uint8_t *suberr)
         * supporting graceful restart.
         */
 
-       for (i = 0; i < AID_MAX; i++) {
+       for (i = AID_MIN; i < AID_MAX; i++) {
                int8_t  negflags;
 
                /* disable GR if the AFI/SAFI is not present */
@@ -2898,26 +2895,24 @@ capa_neg_calc(struct peer *p, uint8_t *suberr)
        if (p->capa.ann.grestart.restart == 0)
                p->capa.neg.grestart.restart = 0;
 
-
        /*
         * ADD-PATH: set only those bits where both sides agree.
         * For this compare our send bit with the recv bit from the peer
         * and vice versa.
         * The flags are stored from this systems view point.
+        * At index 0 the flags are set if any per-AID flag is set.
         */
        memset(p->capa.neg.add_path, 0, sizeof(p->capa.neg.add_path));
-       if (p->capa.ann.add_path[0]) {
-               for (i = AID_MIN; i < AID_MAX; i++) {
-                       if ((p->capa.ann.add_path[i] & CAPA_AP_RECV) &&
-                           (p->capa.peer.add_path[i] & CAPA_AP_SEND)) {
-                               p->capa.neg.add_path[i] |= CAPA_AP_RECV;
-                               p->capa.neg.add_path[0] |= CAPA_AP_RECV;
-                       }
-                       if ((p->capa.ann.add_path[i] & CAPA_AP_SEND) &&
-                           (p->capa.peer.add_path[i] & CAPA_AP_RECV)) {
-                               p->capa.neg.add_path[i] |= CAPA_AP_SEND;
-                               p->capa.neg.add_path[0] |= CAPA_AP_SEND;
-                       }
+       for (i = AID_MIN; i < AID_MAX; i++) {
+               if ((p->capa.ann.add_path[i] & CAPA_AP_RECV) &&
+                   (p->capa.peer.add_path[i] & CAPA_AP_SEND)) {
+                       p->capa.neg.add_path[i] |= CAPA_AP_RECV;
+                       p->capa.neg.add_path[0] |= CAPA_AP_RECV;
+               }
+               if ((p->capa.ann.add_path[i] & CAPA_AP_SEND) &&
+                   (p->capa.peer.add_path[i] & CAPA_AP_RECV)) {
+                       p->capa.neg.add_path[i] |= CAPA_AP_SEND;
+                       p->capa.neg.add_path[0] |= CAPA_AP_SEND;
                }
        }
 
@@ -3323,7 +3318,7 @@ session_dispatch_imsg(struct imsgbuf *imsgbuf, int idx, u_int *listener_cnt)
                                log_warnx("no such peer: id=%u", peerid);
                                break;
                        }
-                       if (rr.aid >= AID_MAX)
+                       if (rr.aid < AID_MIN || rr.aid >= AID_MAX)
                                fatalx("IMSG_REFRESH: bad AID");
                        session_rrefresh(p, rr.aid, rr.subtype);
                        break;
@@ -3338,7 +3333,7 @@ session_dispatch_imsg(struct imsgbuf *imsgbuf, int idx, u_int *listener_cnt)
                                log_warnx("no such peer: id=%u", peerid);
                                break;
                        }
-                       if (aid >= AID_MAX)
+                       if (aid < AID_MIN || aid >= AID_MAX)
                                fatalx("IMSG_SESSION_RESTARTED: bad AID");
                        if (p->capa.neg.grestart.flags[aid] &
                            CAPA_GR_RESTARTING) {

diff --git a/usr.sbin/bgpd/util.c b/usr.sbin/bgpd/util.c
index 0f9e89b..db4f5e7 100644 (file)
--- a/usr.sbin/bgpd/util.c
+++ b/usr.sbin/bgpd/util.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: util.c,v 1.82 2024/02/22 06:45:22 miod Exp $ */
+/*     $OpenBSD: util.c,v 1.83 2024/03/20 09:35:46 claudio Exp $ */
 
 /*
  * Copyright (c) 2006 Claudio Jeker <claudio@openbsd.org>
@@ -922,7 +922,7 @@ aid2str(uint8_t aid)
 int
 aid2afi(uint8_t aid, uint16_t *afi, uint8_t *safi)
 {
-       if (aid < AID_MAX) {
+       if (aid != AID_UNSPEC && aid < AID_MAX) {
                *afi = aid_vals[aid].afi;
                *safi = aid_vals[aid].safi;
                return (0);
@@ -935,7 +935,7 @@ afi2aid(uint16_t afi, uint8_t safi, uint8_t *aid)
 {
        uint8_t i;
 
-       for (i = 0; i < AID_MAX; i++)
+       for (i = AID_MIN; i < AID_MAX; i++)
                if (aid_vals[i].afi == afi && aid_vals[i].safi == safi) {
                        *aid = i;
                        return (0);
@@ -960,7 +960,7 @@ af2aid(sa_family_t af, uint8_t safi, uint8_t *aid)
        if (safi == 0) /* default to unicast subclass */
                safi = SAFI_UNICAST;
 
-       for (i = 0; i < AID_MAX; i++)
+       for (i = AID_UNSPEC; i < AID_MAX; i++)
                if (aid_vals[i].af == af && aid_vals[i].safi == safi) {
                        *aid = i;
                        return (0);