From 83495362789d961914c4bfaa590e31cb17370ed0 Mon Sep 17 00:00:00 2001 From: Maria Matejka Date: Sat, 21 Dec 2024 19:02:22 +0100 Subject: [PATCH] BFD: Fix session reconfiguration locking order The sessions have to be updated asynchronously to avoid cross-locking between protocols. Testsuite: cf-ibgp-bfd-switch, cf-ibgp-multi-bfd-auth Fixes: #139 Thanks to Daniel Suchy for reporting: https://trubka.network.cz/pipermail/bird-users/2024-December/017984.html --- nest/bfd.h | 7 ++- proto/bfd/bfd.c | 144 +++++++++++++++++++++++--------------------- proto/bfd/bfd.h | 21 +------ proto/bfd/config.Y | 42 +++++-------- proto/bfd/packets.c | 4 +- 5 files changed, 98 insertions(+), 120 deletions(-) diff --git a/nest/bfd.h b/nest/bfd.h index 5dacff5d7..c046152f8 100644 --- nest/bfd.h +++ nest/bfd.h @@ -18,8 +18,11 @@ struct bfd_options { u32 min_tx_int; u32 idle_tx_int; u8 multiplier; - u8 passive; - u8 passive_set; + PACKED enum bfd_opt_passive { + BFD_OPT_PASSIVE_UNKNOWN = 0, + BFD_OPT_PASSIVE, + BFD_OPT_NOT_PASSIVE, + } passive; u8 mode; u8 auth_type; /* Authentication type (BFD_AUTH_*) */ list *passwords; /* Passwords for authentication */ diff --git a/proto/bfd/bfd.c b/proto/bfd/bfd.c index 34f992b93..4997f803a 100644 --- proto/bfd/bfd.c +++ proto/bfd/bfd.c @@ -172,17 +172,17 @@ static void bfd_free_iface(struct bfd_iface *ifa); * BFD sessions */ -static inline struct bfd_session_config -bfd_merge_options(const struct bfd_iface_config *cf, const struct bfd_options *opts) +static inline struct bfd_options +bfd_merge_options(const struct bfd_options *bottom, const struct bfd_options *top) { - return (struct bfd_session_config) { - .min_rx_int = opts->min_rx_int ?: cf->min_rx_int, - .min_tx_int = opts->min_tx_int ?: cf->min_tx_int, - .idle_tx_int = opts->idle_tx_int ?: cf->idle_tx_int, - .multiplier = opts->multiplier ?: cf->multiplier, - .passive = opts->passive_set ? opts->passive : cf->passive, - .auth_type = opts->auth_type ?: cf->auth_type, - .passwords = opts->passwords ?: cf->passwords, + return (struct bfd_options) { + .min_rx_int = top->min_rx_int ?: bottom->min_rx_int, + .min_tx_int = top->min_tx_int ?: bottom->min_tx_int, + .idle_tx_int = top->idle_tx_int ?: bottom->idle_tx_int, + .multiplier = top->multiplier ?: bottom->multiplier, + .passive = top->passive ?: bottom->passive, + .auth_type = top->auth_type ?: bottom->auth_type, + .passwords = top->passwords ?: bottom->passwords, }; } @@ -478,7 +478,7 @@ bfd_add_session(struct bfd_proto *p, ip_addr addr, ip_addr local, struct iface * HASH_INSERT(p->session_hash_id, HASH_ID, s); HASH_INSERT(p->session_hash_ip, HASH_IP, s); - s->cf = bfd_merge_options(ifa->cf, opts); + s->cf = bfd_merge_options(&ifa->cf->opts, opts); /* Initialization of state variables - see RFC 5880 6.8.1 */ s->loc_state = BFD_STATE_DOWN; @@ -561,26 +561,58 @@ bfd_remove_session(struct bfd_proto *p, struct bfd_session *s) birdloop_leave(p->p.loop); } +struct bfd_reconfigure_sessions_deferred_call { + struct deferred_call dc; + struct bfd_proto *p; + config_ref old_config; +}; + static void -bfd_reconfigure_session(struct bfd_proto *p, struct bfd_session *s) +bfd_reconfigure_sessions(struct deferred_call *dc) { - if (EMPTY_LIST(s->request_list)) - return; + SKIP_BACK_DECLARE(struct bfd_reconfigure_sessions_deferred_call, + brsdc, dc, dc); - ASSERT_DIE(birdloop_inside(p->p.loop)); + struct bfd_proto *p = brsdc->p; + birdloop_enter(p->p.loop); - SKIP_BACK_DECLARE(struct bfd_request, req, n, HEAD(s->request_list)); - s->cf = bfd_merge_options(s->ifa->cf, &req->opts); + HASH_WALK(p->session_hash_id, next_id, s) + { + if (!EMPTY_LIST(s->request_list)) + { + SKIP_BACK_DECLARE(struct bfd_request, req, n, HEAD(s->request_list)); + struct bfd_options opts = bfd_merge_options(&s->ifa->cf->opts, &req->opts); - u32 tx = (s->loc_state == BFD_STATE_UP) ? s->cf.min_tx_int : s->cf.idle_tx_int; - bfd_session_set_min_tx(s, tx); - bfd_session_set_min_rx(s, s->cf.min_rx_int); - s->detect_mult = s->cf.multiplier; - s->passive = s->cf.passive; +#define CHK(x) (opts.x != s->cf.x) || + bool reload = MACRO_FOREACH(CHK, + min_rx_int, + min_tx_int, + idle_tx_int, + multiplier, + passive) false; /* terminating the || chain */ +#undef CHK - bfd_session_control_tx_timer(s, 0); + s->cf = opts; + + if (reload) + { + u32 tx = (s->loc_state == BFD_STATE_UP) ? s->cf.min_tx_int : s->cf.idle_tx_int; + bfd_session_set_min_tx(s, tx); + bfd_session_set_min_rx(s, s->cf.min_rx_int); + s->detect_mult = s->cf.multiplier; + s->passive = s->cf.passive; + + bfd_session_control_tx_timer(s, 0); + + TRACE(D_EVENTS, "Session to %I reconfigured", s->addr); + } + } + } + HASH_WALK_END; + birdloop_leave(p->p.loop); - TRACE(D_EVENTS, "Session to %I reconfigured", s->addr); + /* Now the config is clean */ + OBSREF_CLEAR(brsdc->old_config); } @@ -589,10 +621,12 @@ bfd_reconfigure_session(struct bfd_proto *p, struct bfd_session *s) */ static struct bfd_iface_config bfd_default_iface = { - .min_rx_int = BFD_DEFAULT_MIN_RX_INT, - .min_tx_int = BFD_DEFAULT_MIN_TX_INT, - .idle_tx_int = BFD_DEFAULT_IDLE_TX_INT, - .multiplier = BFD_DEFAULT_MULTIPLIER, + .opts = { + .min_rx_int = BFD_DEFAULT_MIN_RX_INT, + .min_tx_int = BFD_DEFAULT_MIN_TX_INT, + .idle_tx_int = BFD_DEFAULT_IDLE_TX_INT, + .multiplier = BFD_DEFAULT_MULTIPLIER, + }, }; static inline struct bfd_iface_config * @@ -650,24 +684,6 @@ bfd_free_iface(struct bfd_iface *ifa) mb_free(ifa); } -static void -bfd_reconfigure_iface(struct bfd_proto *p UNUSED, struct bfd_iface *ifa, struct bfd_config *nc) -{ - struct bfd_iface_config *new = bfd_find_iface_config(nc, ifa->iface); - struct bfd_iface_config *old = ifa->cf; - - /* Check options that are handled in bfd_reconfigure_session() */ - ifa->changed = - (new->min_rx_int != old->min_rx_int) || - (new->min_tx_int != old->min_tx_int) || - (new->idle_tx_int != old->idle_tx_int) || - (new->multiplier != old->multiplier) || - (new->passive != old->passive); - - /* This should be probably changed to not access ifa->cf from the BFD thread */ - ifa->cf = new; -} - /* * BFD requests @@ -900,20 +916,7 @@ bfd_request_session(pool *p, ip_addr addr, ip_addr local, void bfd_update_request(struct bfd_request *req, const struct bfd_options *opts) { - struct bfd_session *s = req->session; - - if (!memcmp(opts, &req->opts, sizeof(const struct bfd_options))) - return; - req->opts = *opts; - - if (s) - { - struct bfd_proto *p = s->ifa->bfd; - birdloop_enter(p->p.loop); - bfd_reconfigure_session(p, s); - birdloop_leave(p->p.loop); - } } static void @@ -1193,21 +1196,22 @@ bfd_reconfigure(struct proto *P, struct proto_config *c) (new->zero_udp6_checksum_rx != old->zero_udp6_checksum_rx)) return 0; - birdloop_mask_wakeups(p->p.loop); - WALK_LIST(ifa, p->iface_list) - bfd_reconfigure_iface(p, ifa, new); - - HASH_WALK(p->session_hash_id, next_id, s) - { - if (s->ifa->changed) - bfd_reconfigure_session(p, s); - } - HASH_WALK_END; + ifa->cf = bfd_find_iface_config(new, ifa->iface); bfd_reconfigure_neighbors(p, new); - birdloop_unmask_wakeups(p->p.loop); + /* Sessions get reconfigured after all the config is applied */ + struct bfd_reconfigure_sessions_deferred_call brsdc = { + .dc.hook = bfd_reconfigure_sessions, + .p = p, + }; + SKIP_BACK_DECLARE(struct bfd_reconfigure_sessions_deferred_call, + brsdcp, dc, defer_call(&brsdc.dc, sizeof brsdc)); + + /* We need to keep the old config alive until all the sessions get + * reconfigured */ + OBSREF_SET(brsdcp->old_config, P->cf->global); return 1; } diff --git a/proto/bfd/bfd.h b/proto/bfd/bfd.h index 578ce8755..107829b72 100644 --- proto/bfd/bfd.h +++ proto/bfd/bfd.h @@ -54,24 +54,7 @@ struct bfd_config struct bfd_iface_config { struct iface_patt i; - u32 min_rx_int; - u32 min_tx_int; - u32 idle_tx_int; - u8 multiplier; - u8 passive; - u8 auth_type; /* Authentication type (BFD_AUTH_*) */ - list *passwords; /* Passwords for authentication */ -}; - -struct bfd_session_config -{ - u32 min_rx_int; - u32 min_tx_int; - u32 idle_tx_int; - u8 multiplier; - u8 passive; - u8 auth_type; /* Authentication type (BFD_AUTH_*) */ - list *passwords; /* Passwords for authentication */ + struct bfd_options opts; }; struct bfd_neighbor @@ -146,7 +129,7 @@ struct bfd_session u32 loc_id; /* Local session ID (local discriminator) */ u32 rem_id; /* Remote session ID (remote discriminator) */ - struct bfd_session_config cf; /* Static configuration parameters */ + struct bfd_options cf; /* Static configuration parameters */ u32 des_min_tx_int; /* Desired min rx interval, local option */ u32 des_min_tx_new; /* Used for des_min_tx_int change */ diff --git a/proto/bfd/config.Y b/proto/bfd/config.Y index 9e9919c4e..56d1ffac4 100644 --- proto/bfd/config.Y +++ proto/bfd/config.Y @@ -86,44 +86,37 @@ bfd_iface_start: add_tail(&BFD_CFG->patt_list, NODE this_ipatt); init_list(&this_ipatt->ipn_list); - BFD_IFACE->min_rx_int = BFD_DEFAULT_MIN_RX_INT; - BFD_IFACE->min_tx_int = BFD_DEFAULT_MIN_TX_INT; - BFD_IFACE->idle_tx_int = BFD_DEFAULT_IDLE_TX_INT; - BFD_IFACE->multiplier = BFD_DEFAULT_MULTIPLIER; + this_bfd_opts = &BFD_IFACE->opts; + + this_bfd_opts->min_rx_int = BFD_DEFAULT_MIN_RX_INT; + this_bfd_opts->min_tx_int = BFD_DEFAULT_MIN_TX_INT; + this_bfd_opts->idle_tx_int = BFD_DEFAULT_IDLE_TX_INT; + this_bfd_opts->multiplier = BFD_DEFAULT_MULTIPLIER; reset_passwords(); }; bfd_iface_finish: { - BFD_IFACE->passwords = get_passwords(); + this_bfd_opts->passwords = get_passwords(); - if (!BFD_IFACE->auth_type != !BFD_IFACE->passwords) + if (!this_bfd_opts->auth_type != !this_bfd_opts->passwords) cf_warn("Authentication and password options should be used together"); - if (BFD_IFACE->passwords) + if (this_bfd_opts->passwords) { struct password_item *pass; - WALK_LIST(pass, *BFD_IFACE->passwords) + WALK_LIST(pass, *this_bfd_opts->passwords) { if (pass->alg) cf_error("Password algorithm option not available in BFD protocol"); - pass->alg = bfd_auth_type_to_hash_alg[BFD_IFACE->auth_type]; + pass->alg = bfd_auth_type_to_hash_alg[this_bfd_opts->auth_type]; } } -}; -bfd_iface_item: - INTERVAL expr_us { BFD_IFACE->min_rx_int = BFD_IFACE->min_tx_int = $2; } - | MIN RX INTERVAL expr_us { BFD_IFACE->min_rx_int = $4; } - | MIN TX INTERVAL expr_us { BFD_IFACE->min_tx_int = $4; } - | IDLE TX INTERVAL expr_us { BFD_IFACE->idle_tx_int = $4; } - | MULTIPLIER expr { BFD_IFACE->multiplier = $2; } - | PASSIVE bool { BFD_IFACE->passive = $2; } - | AUTHENTICATION bfd_auth_type { BFD_IFACE->auth_type = $2; } - | password_list {} - ; + this_bfd_opts = NULL; +}; bfd_auth_type: NONE { $$ = BFD_AUTH_NONE; } @@ -134,14 +127,9 @@ bfd_auth_type: | METICULOUS KEYED SHA1 { $$ = BFD_AUTH_METICULOUS_KEYED_SHA1; } ; -bfd_iface_opts: - /* empty */ - | bfd_iface_opts bfd_iface_item ';' - ; - bfd_iface_opt_list: /* empty */ - | '{' bfd_iface_opts '}' + | '{' bfd_items '}' ; bfd_iface: @@ -194,7 +182,7 @@ bfd_item: | MIN TX INTERVAL expr_us { this_bfd_opts->min_tx_int = $4; } | IDLE TX INTERVAL expr_us { this_bfd_opts->idle_tx_int = $4; } | MULTIPLIER expr { this_bfd_opts->multiplier = $2; } - | PASSIVE bool { this_bfd_opts->passive = $2; this_bfd_opts->passive_set = 1; } + | PASSIVE bool { this_bfd_opts->passive = $2 ? BFD_OPT_PASSIVE : BFD_OPT_NOT_PASSIVE; } | GRACEFUL { this_bfd_opts->mode = BGP_BFD_GRACEFUL; } | AUTHENTICATION bfd_auth_type { this_bfd_opts->auth_type = $2; } | password_list {} diff --git a/proto/bfd/packets.c b/proto/bfd/packets.c index 1ceb470c1..f8bd63d73 100644 --- proto/bfd/packets.c +++ proto/bfd/packets.c @@ -109,7 +109,7 @@ const u8 bfd_auth_type_to_hash_alg[] = { static void bfd_fill_authentication(struct bfd_proto *p, struct bfd_session *s, struct bfd_ctl_packet *pkt) { - struct bfd_session_config *cf = &s->cf; + struct bfd_options *cf = &s->cf; struct password_item *pass = password_find(cf->passwords, 0); uint meticulous = 0; @@ -179,7 +179,7 @@ bfd_fill_authentication(struct bfd_proto *p, struct bfd_session *s, struct bfd_c static int bfd_check_authentication(struct bfd_proto *p, struct bfd_session *s, struct bfd_ctl_packet *pkt) { - struct bfd_session_config *cf = &s->cf; + struct bfd_options *cf = &s->cf; const char *err_dsc = NULL; uint err_val = 0; uint auth_type = 0; -- GitLab