From 6d1ae197d189d50e60279796441c6e74272ddc54 Mon Sep 17 00:00:00 2001 From: Ondrej Zajicek Date: Sun, 1 Jan 2023 20:10:23 +0100 Subject: [PATCH] Nest: Fix several issues with pflags There were some confusion about validity and usage of pflags, which caused incorrect usage after some flags from (now removed) protocol- specific area were moved to pflags. We state that pflags: - Are secondary data used by protocol-specific hooks - Can be changed on an existing route (in contrast to copy-on-write for primary data) - Are irrelevant for propagation (not propagated when changed) - Are specific to a routing table (not propagated by pipe) The patch did these fixes: - Do not compare pflags in rte_same(), as they may keep cached values like BGP_REF_STALE, causing spurious propagation. - Initialize pflags to zero in rte_get_temp(), avoid initialization in protocol code, fixing at least two forgotten initializations (krt and one case in babel). - Improve documentation about pflags --- nest/rt-dev.c | 1 - nest/rt-table.c | 21 ++++++++++++--------- proto/babel/babel.c | 1 - proto/bgp/packets.c | 1 - proto/perf/perf.c | 1 - proto/pipe/pipe.c | 7 ------- proto/rpki/rpki.c | 2 -- proto/static/static.c | 1 - 8 files changed, 12 insertions(+), 23 deletions(-) diff --git a/nest/rt-dev.c b/nest/rt-dev.c index 05e64fc3..7932b8b7 100644 --- a/nest/rt-dev.c +++ b/nest/rt-dev.c @@ -92,7 +92,6 @@ dev_ifa_notify(struct proto *P, uint flags, struct ifa *ad) a = rta_lookup(&a0); e = rte_get_temp(a, src); - e->pflags = 0; rte_update2(c, net, e, src); } } diff --git a/nest/rt-table.c b/nest/rt-table.c index 845b2483..e4b27814 100644 --- a/nest/rt-table.c +++ b/nest/rt-table.c @@ -21,11 +21,15 @@ * on the list being the best one (i.e., the one we currently use * for routing), the order of the other ones is undetermined. * - * The &rte contains information specific to the route (preference, protocol - * metrics, time of last modification etc.) and a pointer to a &rta structure - * (see the route attribute module for a precise explanation) holding the - * remaining route attributes which are expected to be shared by multiple - * routes in order to conserve memory. + * The &rte contains information about the route. There are net and src, which + * together forms a key identifying the route in a routing table. There is a + * pointer to a &rta structure (see the route attribute module for a precise + * explanation) holding the route attributes, which are primary data about the + * route. There are several technical fields used by routing table code (route + * id, REF_* flags), There is also the pflags field, holding protocol-specific + * flags. They are not used by routing table code, but by protocol-specific + * hooks. In contrast to route attributes, they are not primary data and their + * validity is also limited to the routing table. * * There are several mechanisms that allow automatic update of routes in one * routing table (dst) as a result of changes in another routing table (src). @@ -558,10 +562,9 @@ rte_find(net *net, struct rte_src *src) * rte_get_temp - get a temporary &rte * @a: attributes to assign to the new route (a &rta; in case it's * un-cached, rte_update() will create a cached copy automatically) + * @src: route source * * Create a temporary &rte and bind it with the attributes @a. - * Also set route preference to the default preference set for - * the protocol. */ rte * rte_get_temp(rta *a, struct rte_src *src) @@ -571,6 +574,7 @@ rte_get_temp(rta *a, struct rte_src *src) e->attrs = a; e->id = 0; e->flags = 0; + e->pflags = 0; rt_lock_source(e->src = src); return e; } @@ -1205,10 +1209,9 @@ rte_free_quick(rte *e) static int rte_same(rte *x, rte *y) { - /* rte.flags are not checked, as they are mostly internal to rtable */ + /* rte.flags / rte.pflags are not checked, as they are internal to rtable */ return x->attrs == y->attrs && - x->pflags == y->pflags && x->src == y->src && rte_is_filtered(x) == rte_is_filtered(y); } diff --git a/proto/babel/babel.c b/proto/babel/babel.c index 23cb6774..86cec63b 100644 --- a/proto/babel/babel.c +++ b/proto/babel/babel.c @@ -715,7 +715,6 @@ babel_announce_rte(struct babel_proto *p, struct babel_entry *e) rta *a = rta_lookup(&a0); rte *rte = rte_get_temp(a, p->p.main_source); - rte->pflags = 0; e->unreachable = 1; rte_update2(c, e->n.addr, rte, p->p.main_source); diff --git a/proto/bgp/packets.c b/proto/bgp/packets.c index 7ce2fd63..ddbc9226 100644 --- a/proto/bgp/packets.c +++ b/proto/bgp/packets.c @@ -1464,7 +1464,6 @@ bgp_rte_update(struct bgp_parse_state *s, const net_addr *n, u32 path_id, rta *a rta *a = rta_clone(s->cached_rta); rte *e = rte_get_temp(a, s->last_src); - e->pflags = 0; rte_update3(&s->channel->c, n, e, s->last_src); } diff --git a/proto/perf/perf.c b/proto/perf/perf.c index 5d228045..75e405f0 100644 --- a/proto/perf/perf.c +++ b/proto/perf/perf.c @@ -162,7 +162,6 @@ perf_loop(void *data) for (uint i=0; idata[i].a, p->p.main_source); - e->pflags = 0; rte_update(P, &(p->data[i].net), e); } diff --git a/proto/pipe/pipe.c b/proto/pipe/pipe.c index 1f1ad857..a4c5b1d4 100644 --- a/proto/pipe/pipe.c +++ b/proto/pipe/pipe.c @@ -77,13 +77,6 @@ pipe_rt_notify(struct proto *P, struct channel *src_ch, net *n, rte *new, rte *o a->cached = 0; a->hostentry = NULL; e = rte_get_temp(a, src); - e->pflags = new->pflags; - -#ifdef CONFIG_BGP - /* Hack to cleanup cached value */ - if (e->src->proto->proto == &proto_bgp) - e->pflags &= ~(BGP_REF_STALE | BGP_REF_NOT_STALE); -#endif } else { diff --git a/proto/rpki/rpki.c b/proto/rpki/rpki.c index 3c27aa1a..3e321627 100644 --- a/proto/rpki/rpki.c +++ b/proto/rpki/rpki.c @@ -130,8 +130,6 @@ rpki_table_add_roa(struct rpki_cache *cache, struct channel *channel, const net_ rta *a = rta_lookup(&a0); rte *e = rte_get_temp(a, p->p.main_source); - e->pflags = 0; - rte_update2(channel, &pfxr->n, e, e->src); } diff --git a/proto/static/static.c b/proto/static/static.c index cd31afd3..bb93305e 100644 --- a/proto/static/static.c +++ b/proto/static/static.c @@ -104,7 +104,6 @@ static_announce_rte(struct static_proto *p, struct static_route *r) /* We skip rta_lookup() here */ rte *e = rte_get_temp(a, src); - e->pflags = 0; if (r->cmds) {