diff options
Diffstat (limited to 'lib')
| -rw-r--r-- | lib/frr_pthread.c | 8 | ||||
| -rw-r--r-- | lib/frrcu.c | 15 | ||||
| -rw-r--r-- | lib/nexthop.c | 87 | ||||
| -rw-r--r-- | lib/nexthop.h | 104 | ||||
| -rw-r--r-- | lib/sigevent.h | 27 | ||||
| -rw-r--r-- | lib/zclient.c | 22 |
6 files changed, 146 insertions, 117 deletions
diff --git a/lib/frr_pthread.c b/lib/frr_pthread.c index c4ead01bf6..2dfe06724f 100644 --- a/lib/frr_pthread.c +++ b/lib/frr_pthread.c @@ -17,6 +17,7 @@ #include "zlog.h" #include "libfrr.h" #include "libfrr_trace.h" +#include "sigevent.h" DEFINE_MTYPE_STATIC(LIB, FRR_PTHREAD, "FRR POSIX Thread"); DEFINE_MTYPE_STATIC(LIB, PTHREAD_PRIM, "POSIX sync primitives"); @@ -152,10 +153,9 @@ int frr_pthread_run(struct frr_pthread *fpt, const pthread_attr_t *attr) assert(frr_is_after_fork || !"trying to start thread before fork()"); - /* Ensure we never handle signals on a background thread by blocking - * everything here (new thread inherits signal mask) - */ - sigfillset(&blocksigs); + sigemptyset(&blocksigs); + frr_sigset_add_mainonly(&blocksigs); + /* new thread inherits mask */ pthread_sigmask(SIG_BLOCK, &blocksigs, &oldsigs); frrtrace(1, frr_libfrr, frr_pthread_run, fpt->name); diff --git a/lib/frrcu.c b/lib/frrcu.c index c7cc655e09..6719bc9d87 100644 --- a/lib/frrcu.c +++ b/lib/frrcu.c @@ -42,6 +42,7 @@ #include "frrcu.h" #include "seqlock.h" #include "atomlist.h" +#include "sigevent.h" DEFINE_MTYPE_STATIC(LIB, RCU_THREAD, "RCU thread"); DEFINE_MTYPE_STATIC(LIB, RCU_NEXT, "RCU sequence barrier"); @@ -338,7 +339,19 @@ static void rcu_start(void) */ sigset_t oldsigs, blocksigs; - sigfillset(&blocksigs); + /* technically, the RCU thread is very poorly suited to run even just a + * crashlog handler, since zlog_sigsafe() could deadlock on transiently + * invalid (due to RCU) logging data structures + * + * but given that when we try to write a crashlog, we're already in + * b0rked territory anyway - give the crashlog handler a chance. + * + * (also cf. the SIGALRM usage in writing crashlogs to avoid hung + * processes on any kind of deadlock in crash handlers) + */ + sigemptyset(&blocksigs); + frr_sigset_add_mainonly(&blocksigs); + /* new thread inherits mask */ pthread_sigmask(SIG_BLOCK, &blocksigs, &oldsigs); rcu_active = true; diff --git a/lib/nexthop.c b/lib/nexthop.c index 3cfa72bce0..8b37290b18 100644 --- a/lib/nexthop.c +++ b/lib/nexthop.c @@ -723,68 +723,30 @@ unsigned int nexthop_level(const struct nexthop *nexthop) return rv; } -/* Only hash word-sized things, let cmp do the rest. */ -uint32_t nexthop_hash_quick(const struct nexthop *nexthop) +uint32_t nexthop_hash(const struct nexthop *nexthop) { uint32_t key = 0x45afe398; - int i; - key = jhash_3words(nexthop->type, nexthop->vrf_id, - nexthop->nh_label_type, key); - - if (nexthop->nh_label) { - int labels = nexthop->nh_label->num_labels; + /* type, vrf, ifindex, ip addresses - see nexthop.h */ + key = _nexthop_hash_bytes(nexthop, key); - i = 0; + key = jhash_1word(nexthop->flags & NEXTHOP_FLAGS_HASHED, key); - while (labels >= 3) { - key = jhash_3words(nexthop->nh_label->label[i], - nexthop->nh_label->label[i + 1], - nexthop->nh_label->label[i + 2], - key); - labels -= 3; - i += 3; - } - - if (labels >= 2) { - key = jhash_2words(nexthop->nh_label->label[i], - nexthop->nh_label->label[i + 1], - key); - labels -= 2; - i += 2; - } + if (nexthop->nh_label) { + const struct mpls_label_stack *ls = nexthop->nh_label; - if (labels >= 1) - key = jhash_1word(nexthop->nh_label->label[i], key); + /* num_labels itself isn't useful to hash, if the number of + * labels is different, the hash value will change just due to + * that already. + */ + key = jhash(ls->label, sizeof(ls->label[0]) * ls->num_labels, key); } - key = jhash_2words(nexthop->ifindex, - CHECK_FLAG(nexthop->flags, NEXTHOP_FLAG_ONLINK), - key); - /* Include backup nexthops, if present */ if (CHECK_FLAG(nexthop->flags, NEXTHOP_FLAG_HAS_BACKUP)) { int backups = nexthop->backup_num; - i = 0; - - while (backups >= 3) { - key = jhash_3words(nexthop->backup_idx[i], - nexthop->backup_idx[i + 1], - nexthop->backup_idx[i + 2], key); - backups -= 3; - i += 3; - } - - while (backups >= 2) { - key = jhash_2words(nexthop->backup_idx[i], - nexthop->backup_idx[i + 1], key); - backups -= 2; - i += 2; - } - - if (backups >= 1) - key = jhash_1word(nexthop->backup_idx[i], key); + key = jhash(nexthop->backup_idx, sizeof(nexthop->backup_idx[0]) * backups, key); } if (nexthop->nh_srv6) { @@ -819,31 +781,6 @@ uint32_t nexthop_hash_quick(const struct nexthop *nexthop) return key; } - -#define GATE_SIZE 4 /* Number of uint32_t words in struct g_addr */ - -/* For a more granular hash */ -uint32_t nexthop_hash(const struct nexthop *nexthop) -{ - uint32_t gate_src_rmap_raw[GATE_SIZE * 3] = {}; - /* Get all the quick stuff */ - uint32_t key = nexthop_hash_quick(nexthop); - - assert(((sizeof(nexthop->gate) + sizeof(nexthop->src) - + sizeof(nexthop->rmap_src)) - / 3) - == (GATE_SIZE * sizeof(uint32_t))); - - memcpy(gate_src_rmap_raw, &nexthop->gate, GATE_SIZE); - memcpy(gate_src_rmap_raw + GATE_SIZE, &nexthop->src, GATE_SIZE); - memcpy(gate_src_rmap_raw + (2 * GATE_SIZE), &nexthop->rmap_src, - GATE_SIZE); - - key = jhash2(gate_src_rmap_raw, (GATE_SIZE * 3), key); - - return key; -} - void nexthop_copy_no_recurse(struct nexthop *copy, const struct nexthop *nexthop, struct nexthop *rparent) diff --git a/lib/nexthop.h b/lib/nexthop.h index 958d06aa51..718ce2ee42 100644 --- a/lib/nexthop.h +++ b/lib/nexthop.h @@ -8,6 +8,7 @@ #ifndef _LIB_NEXTHOP_H #define _LIB_NEXTHOP_H +#include "jhash.h" #include "prefix.h" #include "mpls.h" #include "vxlan.h" @@ -56,15 +57,48 @@ struct nexthop { struct nexthop *next; struct nexthop *prev; - /* - * What vrf is this nexthop associated with? + + /* begin of hashed data - all fields from here onwards are given to + * jhash() as one consecutive chunk. DO NOT create "padding holes". + * DO NOT insert pointers that need to be deep-hashed. + * + * static_assert() below needs to be updated when fields are added */ + char _hash_begin[0]; + + /* see above */ + enum nexthop_types_t type; + + /* What vrf is this nexthop associated with? */ vrf_id_t vrf_id; /* Interface index. */ ifindex_t ifindex; - enum nexthop_types_t type; + /* Type of label(s), if any */ + enum lsp_types_t nh_label_type; + + /* padding: keep 16 byte alignment here */ + + /* Nexthop address + * make sure all 16 bytes for IPv6 are zeroed when putting in an IPv4 + * address since the entire thing is hashed as-is + */ + union { + union g_addr gate; + enum blackhole_type bh_type; + }; + union g_addr src; + union g_addr rmap_src; /* Src is set via routemap */ + + /* end of hashed data - remaining fields in this struct are not + * directly fed into jhash(). Most of them are actually part of the + * hash but have special rules or handling attached. + */ + char _hash_end[0]; + + /* Weight of the nexthop ( for unequal cost ECMP ) */ + uint8_t weight; uint16_t flags; #define NEXTHOP_FLAG_ACTIVE (1 << 0) /* This nexthop is alive. */ @@ -82,18 +116,15 @@ struct nexthop { #define NEXTHOP_FLAG_EVPN (1 << 8) /* nexthop is EVPN */ #define NEXTHOP_FLAG_LINKDOWN (1 << 9) /* is not removed on link down */ + /* which flags are part of nexthop_hash(). Should probably be split + * off into a separate field... + */ +#define NEXTHOP_FLAGS_HASHED NEXTHOP_FLAG_ONLINK + #define NEXTHOP_IS_ACTIVE(flags) \ (CHECK_FLAG(flags, NEXTHOP_FLAG_ACTIVE) \ && !CHECK_FLAG(flags, NEXTHOP_FLAG_DUPLICATE)) - /* Nexthop address */ - union { - union g_addr gate; - enum blackhole_type bh_type; - }; - union g_addr src; - union g_addr rmap_src; /* Src is set via routemap */ - /* Nexthops obtained by recursive resolution. * * If the nexthop struct needs to be resolved recursively, @@ -104,15 +135,9 @@ struct nexthop { /* Recursive parent */ struct nexthop *rparent; - /* Type of label(s), if any */ - enum lsp_types_t nh_label_type; - /* Label(s) associated with this nexthop. */ struct mpls_label_stack *nh_label; - /* Weight of the nexthop ( for unequal cost ECMP ) */ - uint8_t weight; - /* Count and index of corresponding backup nexthop(s) in a backup list; * only meaningful if the HAS_BACKUP flag is set. */ @@ -138,6 +163,29 @@ struct nexthop { struct nexthop_srv6 *nh_srv6; }; +/* all hashed fields (including padding, if it is necessary to add) need to + * be listed in the static_assert below + */ + +#define S(field) sizeof(((struct nexthop *)NULL)->field) + +static_assert( + offsetof(struct nexthop, _hash_end) - offsetof(struct nexthop, _hash_begin) == + S(type) + S(vrf_id) + S(ifindex) + S(nh_label_type) + S(gate) + S(src) + S(rmap_src), + "struct nexthop contains padding, this can break things. insert _pad fields at appropriate places"); + +#undef S + +/* this is here to show exactly what is meant by the comments above about + * the hashing + */ +static inline uint32_t _nexthop_hash_bytes(const struct nexthop *nh, uint32_t seed) +{ + return jhash(&nh->_hash_begin, + offsetof(struct nexthop, _hash_end) - offsetof(struct nexthop, _hash_begin), + seed); +} + /* Utility to append one nexthop to another. */ #define NEXTHOP_APPEND(to, new) \ do { \ @@ -181,27 +229,11 @@ struct nexthop *nexthop_from_blackhole(enum blackhole_type bh_type, /* * Hash a nexthop. Suitable for use with hash tables. * - * This function uses the following values when computing the hash: - * - vrf_id - * - ifindex - * - type - * - gate - * - * nexthop - * The nexthop to hash - * - * Returns: - * 32-bit hash of nexthop + * Please double check the code on what is included in the hash, there was + * documentation here but it got outdated and the only thing worse than no + * doc is incorrect doc. */ uint32_t nexthop_hash(const struct nexthop *nexthop); -/* - * Hash a nexthop only on word-sized attributes: - * - vrf_id - * - ifindex - * - type - * - (some) flags - */ -uint32_t nexthop_hash_quick(const struct nexthop *nexthop); extern bool nexthop_same(const struct nexthop *nh1, const struct nexthop *nh2); extern bool nexthop_same_no_labels(const struct nexthop *nh1, diff --git a/lib/sigevent.h b/lib/sigevent.h index 0b07f594c1..2c51ba3767 100644 --- a/lib/sigevent.h +++ b/lib/sigevent.h @@ -45,6 +45,33 @@ bool frr_sigevent_check(sigset_t *setp); /* check whether there are signals to handle, process any found */ extern int frr_sigevent_process(void); +/* Ensure we don't handle "application-type" signals on a secondary thread by + * blocking these signals when creating threads + * + * NB: SIGSEGV, SIGABRT, etc. must be allowed on all threads or we get no + * crashlogs. Since signals vary a little bit between platforms, below is a + * list of known things to go to the main thread. Any unknown signals should + * stay thread-local. + */ +static inline void frr_sigset_add_mainonly(sigset_t *blocksigs) +{ + /* signals we actively handle */ + sigaddset(blocksigs, SIGHUP); + sigaddset(blocksigs, SIGINT); + sigaddset(blocksigs, SIGTERM); + sigaddset(blocksigs, SIGUSR1); + + /* signals we don't actively use but that semantically belong */ + sigaddset(blocksigs, SIGUSR2); + sigaddset(blocksigs, SIGQUIT); + sigaddset(blocksigs, SIGCHLD); + sigaddset(blocksigs, SIGPIPE); + sigaddset(blocksigs, SIGTSTP); + sigaddset(blocksigs, SIGTTIN); + sigaddset(blocksigs, SIGTTOU); + sigaddset(blocksigs, SIGWINCH); +} + #ifdef __cplusplus } #endif diff --git a/lib/zclient.c b/lib/zclient.c index 1491da5a3e..a8bed8e0b8 100644 --- a/lib/zclient.c +++ b/lib/zclient.c @@ -2121,7 +2121,27 @@ struct nexthop *nexthop_from_zapi_nexthop(const struct zapi_nexthop *znh) n->type = znh->type; n->vrf_id = znh->vrf_id; n->ifindex = znh->ifindex; - n->gate = znh->gate; + + /* only copy values that have meaning - make sure "spare bytes" are + * left zeroed for hashing (look at _nexthop_hash_bytes) + */ + switch (znh->type) { + case NEXTHOP_TYPE_BLACKHOLE: + n->bh_type = znh->bh_type; + break; + case NEXTHOP_TYPE_IPV4: + case NEXTHOP_TYPE_IPV4_IFINDEX: + n->gate.ipv4 = znh->gate.ipv4; + break; + case NEXTHOP_TYPE_IPV6: + case NEXTHOP_TYPE_IPV6_IFINDEX: + n->gate.ipv6 = znh->gate.ipv6; + break; + case NEXTHOP_TYPE_IFINDEX: + /* nothing, ifindex is always copied */ + break; + } + n->srte_color = znh->srte_color; /* |