From bb8f17e74fb97171e80b5c0db2b54df45b900ff2 Mon Sep 17 00:00:00 2001 From: claudio Date: Sun, 9 Sep 2018 12:33:51 +0000 Subject: [PATCH] Clean up prefix flag handling. First of all the dynamic networks no longer need this and are now treated equally to the network statement in the config. This makes bgpctl network delete also remove a network which was defined in the config. While there remove the other use of flag which was done to support Adj-RIB-Out but the direction we're taking is no longer needing that. Makes code simpler again. OK benno@ --- usr.sbin/bgpd/rde.c | 31 +++++---- usr.sbin/bgpd/rde.h | 21 +++--- usr.sbin/bgpd/rde_rib.c | 130 +++++++++++-------------------------- usr.sbin/bgpd/rde_update.c | 3 +- 4 files changed, 61 insertions(+), 124 deletions(-) diff --git a/usr.sbin/bgpd/rde.c b/usr.sbin/bgpd/rde.c index 059834cc4a6..72e55237f71 100644 --- a/usr.sbin/bgpd/rde.c +++ b/usr.sbin/bgpd/rde.c @@ -1,4 +1,4 @@ -/* $OpenBSD: rde.c,v 1.420 2018/09/07 10:49:22 claudio Exp $ */ +/* $OpenBSD: rde.c,v 1.421 2018/09/09 12:33:51 claudio Exp $ */ /* * Copyright (c) 2003, 2004 Henning Brauer @@ -574,8 +574,7 @@ badnetdel: log_warnx("rde_dispatch: wrong imsg len"); break; } - prefix_network_clean(peerself, time(NULL), - F_ANN_DYNAMIC); + prefix_network_clean(peerself); break; case IMSG_FILTER_SET: if (imsg.hdr.len - IMSG_HEADER_SIZE != @@ -1357,7 +1356,7 @@ rde_update_update(struct rde_peer *peer, struct filterstate *in, peer->prefix_rcvd_update++; /* add original path to the Adj-RIB-In */ - if (path_update(&ribs[RIB_ADJ_IN].rib, peer, in, prefix, prefixlen, 0)) + if (path_update(&ribs[RIB_ADJ_IN].rib, peer, in, prefix, prefixlen)) peer->prefix_cnt++; /* max prefix checker */ @@ -1371,7 +1370,7 @@ rde_update_update(struct rde_peer *peer, struct filterstate *in, if (in->aspath.flags & F_ATTR_PARSE_ERR) wmsg = "path invalid, withdraw"; - p = prefix_get(&ribs[RIB_ADJ_IN].rib, peer, prefix, prefixlen, 0); + p = prefix_get(&ribs[RIB_ADJ_IN].rib, peer, prefix, prefixlen); if (p == NULL) fatalx("rde_update_update: no prefix in Adj-RIB-In"); @@ -1388,9 +1387,9 @@ rde_update_update(struct rde_peer *peer, struct filterstate *in, &state.nexthop->exit_nexthop, prefix, prefixlen); path_update(&ribs[i].rib, peer, &state, prefix, - prefixlen, 0); - } else if (prefix_remove(&ribs[i].rib, peer, prefix, prefixlen, - 0)) { + prefixlen); + } else if (prefix_remove(&ribs[i].rib, peer, prefix, + prefixlen)) { rde_update_log(wmsg, i, peer, NULL, prefix, prefixlen); } @@ -1410,13 +1409,13 @@ rde_update_withdraw(struct rde_peer *peer, struct bgpd_addr *prefix, for (i = RIB_LOC_START; i < rib_size; i++) { if (!rib_valid(i)) break; - if (prefix_remove(&ribs[i].rib, peer, prefix, prefixlen, 0)) + if (prefix_remove(&ribs[i].rib, peer, prefix, prefixlen)) rde_update_log("withdraw", i, peer, NULL, prefix, prefixlen); } /* remove original path form the Adj-RIB-In */ - if (prefix_remove(&ribs[RIB_ADJ_IN].rib, peer, prefix, prefixlen, 0)) + if (prefix_remove(&ribs[RIB_ADJ_IN].rib, peer, prefix, prefixlen)) peer->prefix_cnt--; peer->prefix_rcvd_withdraw++; @@ -3068,11 +3067,11 @@ rde_softreconfig_in(struct rib_entry *re, void *bula) if (action == ACTION_ALLOW) { /* update Local-RIB */ path_update(&rib->rib, peer, &state, &addr, - pt->prefixlen, 0); + pt->prefixlen); } else if (action == ACTION_DENY) { /* remove from Local-RIB */ prefix_remove(&rib->rib, peer, &addr, - pt->prefixlen, 0); + pt->prefixlen); } rde_filterstate_clean(&state); @@ -3637,7 +3636,7 @@ network_add(struct network_config *nc, int flagstatic) peerself); if (path_update(&ribs[RIB_ADJ_IN].rib, peerself, &state, &nc->prefix, - nc->prefixlen, 0)) + nc->prefixlen)) peerself->prefix_cnt++; for (i = RIB_LOC_START; i < rib_size; i++) { if (!rib_valid(i)) @@ -3646,7 +3645,7 @@ network_add(struct network_config *nc, int flagstatic) state.nexthop ? &state.nexthop->exit_nexthop : NULL, &nc->prefix, nc->prefixlen); path_update(&ribs[i].rib, peerself, &state, &nc->prefix, - nc->prefixlen, 0); + nc->prefixlen); } rde_filterstate_clean(&state); path_put(asp); @@ -3695,13 +3694,13 @@ network_delete(struct network_config *nc, int flagstatic) if (!rib_valid(i)) break; if (prefix_remove(&ribs[i].rib, peerself, &nc->prefix, - nc->prefixlen, flags)) + nc->prefixlen)) rde_update_log("withdraw announce", i, peerself, NULL, &nc->prefix, nc->prefixlen); } if (prefix_remove(&ribs[RIB_ADJ_IN].rib, peerself, &nc->prefix, - nc->prefixlen, flags)) + nc->prefixlen)) peerself->prefix_cnt--; } diff --git a/usr.sbin/bgpd/rde.h b/usr.sbin/bgpd/rde.h index 2d699c2481d..cf58e25c55f 100644 --- a/usr.sbin/bgpd/rde.h +++ b/usr.sbin/bgpd/rde.h @@ -1,4 +1,4 @@ -/* $OpenBSD: rde.h,v 1.189 2018/09/08 15:25:27 benno Exp $ */ +/* $OpenBSD: rde.h,v 1.190 2018/09/09 12:33:51 claudio Exp $ */ /* * Copyright (c) 2003, 2004 Claudio Jeker and @@ -168,7 +168,6 @@ struct mpattr { #define F_ANN_DYNAMIC 0x00800 #define F_ATTR_PARSE_ERR 0x10000 /* parse error, not eligable */ #define F_ATTR_LINKED 0x20000 /* if set path is on various lists */ -#define F_ATTR_UPDATE 0x20000 /* if set linked on update_l */ #define ORIGIN_IGP 0 @@ -179,8 +178,8 @@ struct mpattr { struct rde_aspath { LIST_ENTRY(rde_aspath) path_l; - TAILQ_ENTRY(rde_aspath) peer_l, update_l; - struct prefix_queue prefixes, updates; + TAILQ_ENTRY(rde_aspath) peer_l; + struct prefix_queue prefixes; struct attr **others; struct rde_peer *peer; struct aspath *aspath; @@ -306,12 +305,9 @@ struct prefix { struct rde_peer *peer; struct nexthop *nexthop; /* may be NULL */ time_t lastchange; - u_int8_t flags; u_int8_t nhflags; }; -#define F_PREFIX_USE_UPDATES 0x01 /* linked onto the updates list */ - #define NEXTHOP_SELF 0x01 #define NEXTHOP_REJECT 0x02 #define NEXTHOP_BLACKHOLE 0x04 @@ -498,7 +494,7 @@ void path_init(u_int32_t); void path_shutdown(void); void path_hash_stats(struct rde_hashstats *); int path_update(struct rib *, struct rde_peer *, - struct filterstate *, struct bgpd_addr *, int, int); + struct filterstate *, struct bgpd_addr *, int); int path_compare(struct rde_aspath *, struct rde_aspath *); void path_remove(struct rde_aspath *); u_int32_t path_remove_stale(struct rde_aspath *, u_int8_t, time_t); @@ -512,17 +508,16 @@ void path_put(struct rde_aspath *); #define PREFIX_SIZE(x) (((x) + 7) / 8 + 1) struct prefix *prefix_get(struct rib *, struct rde_peer *, - struct bgpd_addr *, int, u_int32_t); + struct bgpd_addr *, int); int prefix_remove(struct rib *, struct rde_peer *, - struct bgpd_addr *, int, u_int32_t); + struct bgpd_addr *, int); int prefix_write(u_char *, int, struct bgpd_addr *, u_int8_t, int); int prefix_writebuf(struct ibuf *, struct bgpd_addr *, u_int8_t); -struct prefix *prefix_bypeer(struct rib_entry *, struct rde_peer *, - u_int32_t); +struct prefix *prefix_bypeer(struct rib_entry *, struct rde_peer *); void prefix_updateall(struct prefix *, enum nexthop_state, enum nexthop_state); void prefix_destroy(struct prefix *); -void prefix_network_clean(struct rde_peer *, time_t, u_int32_t); +void prefix_network_clean(struct rde_peer *); void prefix_relink(struct prefix *, struct rde_aspath *, int); static inline struct rde_peer * diff --git a/usr.sbin/bgpd/rde_rib.c b/usr.sbin/bgpd/rde_rib.c index 80ca3d609e0..57b6c6682d9 100644 --- a/usr.sbin/bgpd/rde_rib.c +++ b/usr.sbin/bgpd/rde_rib.c @@ -1,4 +1,4 @@ -/* $OpenBSD: rde_rib.c,v 1.175 2018/08/08 06:54:50 benno Exp $ */ +/* $OpenBSD: rde_rib.c,v 1.176 2018/09/09 12:33:51 claudio Exp $ */ /* * Copyright (c) 2003, 2004 Claudio Jeker @@ -50,9 +50,9 @@ RB_GENERATE(rib_tree, rib_entry, rib_e, rib_compare); int prefix_add(struct bgpd_addr *, int, struct rib *, struct rde_peer *, struct rde_aspath *, - struct filterstate *, int); + struct filterstate *); int prefix_move(struct prefix *, struct rde_peer *, - struct rde_aspath *, struct filterstate *, int); + struct rde_aspath *, struct filterstate *); static inline void re_lock(struct rib_entry *re) @@ -419,11 +419,10 @@ path_hash_stats(struct rde_hashstats *hs) int path_update(struct rib *rib, struct rde_peer *peer, struct filterstate *state, - struct bgpd_addr *prefix, int prefixlen, int flag) + struct bgpd_addr *prefix, int prefixlen) { struct rde_aspath *asp, *nasp = &state->aspath; struct prefix *p; - int pflag = 0; if (nasp->pftableid) { rde_send_pftable(nasp->pftableid, prefix, prefixlen, 0); @@ -433,7 +432,7 @@ path_update(struct rib *rib, struct rde_peer *peer, struct filterstate *state, /* * First try to find a prefix in the specified RIB. */ - if ((p = prefix_get(rib, peer, prefix, prefixlen, 0)) != NULL) { + if ((p = prefix_get(rib, peer, prefix, prefixlen)) != NULL) { if (path_compare(nasp, prefix_aspath(p)) == 0 && prefix_nexthop(p) == state->nexthop && prefix_nhflags(p) == state->nhflags) { @@ -456,10 +455,10 @@ path_update(struct rib *rib, struct rde_peer *peer, struct filterstate *state, /* If the prefix was found move it else add it to the aspath. */ if (p != NULL) - return (prefix_move(p, peer, asp, state, pflag)); + return (prefix_move(p, peer, asp, state)); else return (prefix_add(prefix, prefixlen, rib, peer, asp, - state, pflag)); + state)); } int @@ -473,11 +472,9 @@ path_compare(struct rde_aspath *a, struct rde_aspath *b) return (1); else if (a == NULL) return (-1); - if ((a->flags & ~(F_ATTR_LINKED | F_ATTR_UPDATE)) > - (b->flags & ~(F_ATTR_LINKED | F_ATTR_UPDATE))) + if ((a->flags & ~F_ATTR_LINKED) > (b->flags & ~F_ATTR_LINKED)) return (1); - if ((a->flags & ~(F_ATTR_LINKED | F_ATTR_UPDATE)) < - (b->flags & ~(F_ATTR_LINKED | F_ATTR_UPDATE))) + if ((a->flags & ~F_ATTR_LINKED) < (b->flags & ~F_ATTR_LINKED)) return (-1); if (a->origin > b->origin) return (1); @@ -558,14 +555,6 @@ void path_remove(struct rde_aspath *asp) { struct prefix *p, *np; - int has_updates; - - /* - * Must check if we actually have updates before removing prefixes - * because if this is the case than the last prefix on prefixes will - * free the asp and so the access to updates is a use after free. - */ - has_updates = !TAILQ_EMPTY(&asp->updates); for (p = TAILQ_FIRST(&asp->prefixes); p != NULL; p = np) { np = TAILQ_NEXT(p, path_l); @@ -579,12 +568,6 @@ path_remove(struct rde_aspath *asp) } prefix_destroy(p); } - if (has_updates) - for (p = TAILQ_FIRST(&asp->updates); p != NULL; p = np) { - np = TAILQ_NEXT(p, path_l); - /* no need to worry about pftable on Adj-RIB-Out */ - prefix_destroy(p); - } } /* remove all stale routes or if staletime is 0 remove all routes for @@ -594,15 +577,12 @@ path_remove_stale(struct rde_aspath *asp, u_int8_t aid, time_t staletime) { struct prefix *p, *np; u_int32_t rprefixes; - int has_updates; rprefixes=0; /* - * Same magic as in path_remove() but probably not needed here. * This is called when a session flapped and during that time * the pending updates for that peer are getting reset. */ - has_updates = !TAILQ_EMPTY(&asp->updates); for (p = TAILQ_FIRST(&asp->prefixes); p != NULL; p = np) { np = TAILQ_NEXT(p, path_l); if (p->re->prefix->aid != aid) @@ -626,18 +606,6 @@ path_remove_stale(struct rde_aspath *asp, u_int8_t aid, time_t staletime) prefix_destroy(p); } - if (has_updates) - for (p = TAILQ_FIRST(&asp->updates); p != NULL; p = np) { - np = TAILQ_NEXT(p, path_l); - if (p->re->prefix->aid != aid) - continue; - - if (staletime && p->lastchange > staletime) - continue; - - /* no need to worry about pftable on Adj-RIB-Out */ - prefix_destroy(p); - } return (rprefixes); } @@ -650,7 +618,7 @@ void path_destroy(struct rde_aspath *asp) { /* path_destroy can only unlink and free empty rde_aspath */ - if (!TAILQ_EMPTY(&asp->prefixes) || !TAILQ_EMPTY(&asp->updates)) + if (!TAILQ_EMPTY(&asp->prefixes)) log_warnx("path_destroy: still has prefixes, leaking"); LIST_REMOVE(asp, path_l); @@ -664,7 +632,7 @@ path_destroy(struct rde_aspath *asp) int path_empty(struct rde_aspath *asp) { - return TAILQ_EMPTY(&asp->prefixes) && TAILQ_EMPTY(&asp->updates); + return TAILQ_EMPTY(&asp->prefixes); } /* @@ -708,7 +676,7 @@ path_copy(struct rde_aspath *dst, const struct rde_aspath *src) dst->rtlabelid = rtlabel_ref(src->rtlabelid); dst->pftableid = pftable_ref(src->pftableid); - dst->flags = src->flags & ~(F_ATTR_LINKED | F_ATTR_UPDATE); + dst->flags = src->flags & ~F_ATTR_LINKED; attr_copy(dst, src); return (dst); @@ -720,7 +688,6 @@ path_prep(struct rde_aspath *asp) { memset(asp, 0, sizeof(*asp)); TAILQ_INIT(&asp->prefixes); - TAILQ_INIT(&asp->updates); asp->origin = ORIGIN_INCOMPLETE; asp->lpref = DEFAULT_LPREF; @@ -772,7 +739,7 @@ static struct prefix *prefix_alloc(void); static void prefix_free(struct prefix *); static void prefix_link(struct prefix *, struct rib_entry *, struct rde_peer *, struct rde_aspath *, - struct filterstate *, int); + struct filterstate *); static void prefix_unlink(struct prefix *); /* @@ -780,14 +747,14 @@ static void prefix_unlink(struct prefix *); */ struct prefix * prefix_get(struct rib *rib, struct rde_peer *peer, struct bgpd_addr *prefix, - int prefixlen, u_int32_t flags) + int prefixlen) { struct rib_entry *re; re = rib_get(rib, prefix, prefixlen); if (re == NULL) return (NULL); - return (prefix_bypeer(re, peer, flags)); + return (prefix_bypeer(re, peer)); } /* @@ -795,8 +762,7 @@ prefix_get(struct rib *rib, struct rde_peer *peer, struct bgpd_addr *prefix, */ int prefix_add(struct bgpd_addr *prefix, int prefixlen, struct rib *rib, - struct rde_peer *peer, struct rde_aspath *asp, struct filterstate *state, - int flag) + struct rde_peer *peer, struct rde_aspath *asp, struct filterstate *state) { struct prefix *p; struct rib_entry *re; @@ -805,17 +771,17 @@ prefix_add(struct bgpd_addr *prefix, int prefixlen, struct rib *rib, if (re == NULL) re = rib_add(rib, prefix, prefixlen); - p = prefix_bypeer(re, asp->peer, asp->flags); + p = prefix_bypeer(re, asp->peer); if (p == NULL) { p = prefix_alloc(); - prefix_link(p, re, peer, asp, state, flag); + prefix_link(p, re, peer, asp, state); return (1); } else { if (prefix_aspath(p) != asp || prefix_nexthop(p) != state->nexthop || prefix_nhflags(p) != state->nhflags) { /* prefix metadata changed therefor move */ - return (prefix_move(p, peer, asp, state, flag)); + return (prefix_move(p, peer, asp, state)); } p->lastchange = time(NULL); return (0); @@ -827,7 +793,7 @@ prefix_add(struct bgpd_addr *prefix, int prefixlen, struct rib *rib, */ int prefix_move(struct prefix *p, struct rde_peer *peer, - struct rde_aspath *asp, struct filterstate *state, int flag) + struct rde_aspath *asp, struct filterstate *state) { struct prefix *np; struct rde_aspath *oasp; @@ -842,14 +808,11 @@ prefix_move(struct prefix *p, struct rde_peer *peer, np->peer = peer; np->re = p->re; np->lastchange = time(NULL); - np->flags = flag; np->nhflags = state->nhflags; /* add to new as path */ - if (np->flags & F_PREFIX_USE_UPDATES) - TAILQ_INSERT_HEAD(&asp->updates, np, path_l); - else - TAILQ_INSERT_HEAD(&asp->prefixes, np, path_l); + TAILQ_INSERT_HEAD(&asp->prefixes, np, path_l); + /* * no need to update the peer prefix count because we are only moving * the prefix without changing the peer. @@ -868,10 +831,7 @@ prefix_move(struct prefix *p, struct rde_peer *peer, /* remove old prefix node */ oasp = prefix_aspath(p); - if (p->flags & F_PREFIX_USE_UPDATES) - TAILQ_REMOVE(&oasp->updates, p, path_l); - else - TAILQ_REMOVE(&oasp->prefixes, p, path_l); + TAILQ_REMOVE(&oasp->prefixes, p, path_l); /* as before peer count needs no update because of move */ /* destroy all references to other objects and free the old prefix */ @@ -896,7 +856,7 @@ prefix_move(struct prefix *p, struct rde_peer *peer, */ int prefix_remove(struct rib *rib, struct rde_peer *peer, struct bgpd_addr *prefix, - int prefixlen, u_int32_t flags) + int prefixlen) { struct prefix *p; struct rib_entry *re; @@ -906,7 +866,7 @@ prefix_remove(struct rib *rib, struct rde_peer *peer, struct bgpd_addr *prefix, if (re == NULL) /* Got a dummy withdrawn request */ return (0); - p = prefix_bypeer(re, peer, flags); + p = prefix_bypeer(re, peer); if (p == NULL) /* Got a dummy withdrawn request. */ return (0); @@ -1004,19 +964,13 @@ prefix_writebuf(struct ibuf *buf, struct bgpd_addr *prefix, u_int8_t plen) * belonging to the peer peer. Returns NULL if no match found. */ struct prefix * -prefix_bypeer(struct rib_entry *re, struct rde_peer *peer, u_int32_t flags) +prefix_bypeer(struct rib_entry *re, struct rde_peer *peer) { struct prefix *p; - LIST_FOREACH(p, &re->prefix_h, rib_l) { - if (prefix_peer(p) != peer) - continue; - if (prefix_aspath(p)->flags & flags && - (flags & F_ANN_DYNAMIC) != - (prefix_aspath(p)->flags & F_ANN_DYNAMIC)) - continue; - return (p); - } + LIST_FOREACH(p, &re->prefix_h, rib_l) + if (prefix_peer(p) == peer) + return (p); return (NULL); } @@ -1076,24 +1030,22 @@ prefix_destroy(struct prefix *p) } /* - * helper function to clean up the connected networks after a reload + * helper function to clean up the dynamically added networks */ void -prefix_network_clean(struct rde_peer *peer, time_t reloadtime, u_int32_t flags) +prefix_network_clean(struct rde_peer *peer) { struct rde_aspath *asp, *xasp; struct prefix *p, *xp; for (asp = TAILQ_FIRST(&peer->path_h); asp != NULL; asp = xasp) { xasp = TAILQ_NEXT(asp, peer_l); - if ((asp->flags & F_ANN_DYNAMIC) != flags) + if ((asp->flags & F_ANN_DYNAMIC) != F_ANN_DYNAMIC) continue; for (p = TAILQ_FIRST(&asp->prefixes); p != NULL; p = xp) { xp = TAILQ_NEXT(p, path_l); - if (reloadtime > p->lastchange) { - prefix_unlink(p); - prefix_free(p); - } + prefix_unlink(p); + prefix_free(p); } if (path_empty(asp)) path_destroy(asp); @@ -1105,12 +1057,9 @@ prefix_network_clean(struct rde_peer *peer, time_t reloadtime, u_int32_t flags) */ static void prefix_link(struct prefix *pref, struct rib_entry *re, struct rde_peer *peer, - struct rde_aspath *asp, struct filterstate *state, int flag) + struct rde_aspath *asp, struct filterstate *state) { - if (flag & F_PREFIX_USE_UPDATES) - TAILQ_INSERT_HEAD(&asp->updates, pref, path_l); - else - TAILQ_INSERT_HEAD(&asp->prefixes, pref, path_l); + TAILQ_INSERT_HEAD(&asp->prefixes, pref, path_l); pref->aspath = asp; pref->peer = peer; @@ -1118,7 +1067,6 @@ prefix_link(struct prefix *pref, struct rib_entry *re, struct rde_peer *peer, nexthop_link(pref); pref->re = re; pref->lastchange = time(NULL); - pref->flags = flag; pref->nhflags = state->nhflags; /* make route decision */ @@ -1138,10 +1086,7 @@ prefix_unlink(struct prefix *pref) LIST_REMOVE(pref, rib_l); prefix_evaluate(NULL, re); - if (pref->flags & F_PREFIX_USE_UPDATES) - pq = &prefix_aspath(pref)->updates; - else - pq = &prefix_aspath(pref)->prefixes; + pq = &prefix_aspath(pref)->prefixes; TAILQ_REMOVE(pq, pref, path_l); if (rib_empty(re)) @@ -1154,7 +1099,6 @@ prefix_unlink(struct prefix *pref) pref->aspath = NULL; pref->peer = NULL; pref->re = NULL; - pref->flags = 0; /* * It's the caller's duty to do accounting and remove empty aspath diff --git a/usr.sbin/bgpd/rde_update.c b/usr.sbin/bgpd/rde_update.c index 8a44f0d0875..c755b377024 100644 --- a/usr.sbin/bgpd/rde_update.c +++ b/usr.sbin/bgpd/rde_update.c @@ -1,4 +1,4 @@ -/* $OpenBSD: rde_update.c,v 1.97 2018/08/08 13:49:20 claudio Exp $ */ +/* $OpenBSD: rde_update.c,v 1.98 2018/09/09 12:33:51 claudio Exp $ */ /* * Copyright (c) 2004 Claudio Jeker @@ -482,7 +482,6 @@ up_generate_default(struct filter_head *rules, struct rde_peer *peer, p.re = re; p.aspath = asp; p.peer = peer; - p.flags = 0; /* filter as usual */ rde_filterstate_prep(&state, asp, NULL, 0); -- 2.20.1