From 4558adabfbbe3696156d20767168010d6238f434 Mon Sep 17 00:00:00 2001 From: Ondrej Zajicek Date: Tue, 22 Aug 2023 01:24:21 +0200 Subject: [PATCH] BMP: Improve peer_down handling Move all bmp_peer_down() calls to one place and make it synchronous with BGP session down, ensuring that BMP receives peer_down before route withdraws from flushing. Also refactor bmp_peer_down_() message generating code. --- proto/bgp/bgp.c | 25 ++++++++--------- proto/bgp/packets.c | 8 ++++-- proto/bmp/bmp.c | 67 +++++++++++++++++++++++++++++---------------- proto/bmp/bmp.h | 7 ++--- 4 files changed, 63 insertions(+), 44 deletions(-) diff --git a/proto/bgp/bgp.c b/proto/bgp/bgp.c index ccaa5067..a6e9cf83 100644 --- a/proto/bgp/bgp.c +++ b/proto/bgp/bgp.c @@ -391,6 +391,9 @@ bgp_close_conn(struct bgp_conn *conn) conn->local_caps = NULL; mb_free(conn->remote_caps); conn->remote_caps = NULL; + + conn->notify_data = NULL; + conn->notify_size = 0; } @@ -694,7 +697,7 @@ bgp_conn_enter_established_state(struct bgp_conn *conn) } static void -bgp_conn_leave_established_state(struct bgp_proto *p) +bgp_conn_leave_established_state(struct bgp_conn *conn, struct bgp_proto *p) { BGP_TRACE(D_EVENTS, "BGP session closed"); p->last_established = current_time(); @@ -702,6 +705,10 @@ bgp_conn_leave_established_state(struct bgp_proto *p) if (p->p.proto_state == PS_UP) bgp_stop(p, 0, NULL, 0); + + bmp_peer_down(p, p->last_error_class, + conn->notify_code, conn->notify_subcode, + conn->notify_data, conn->notify_size); } void @@ -718,7 +725,7 @@ bgp_conn_enter_close_state(struct bgp_conn *conn) bgp_start_timer(conn->hold_timer, 10); if (os == BS_ESTABLISHED) - bgp_conn_leave_established_state(p); + bgp_conn_leave_established_state(conn, p); } void @@ -732,7 +739,7 @@ bgp_conn_enter_idle_state(struct bgp_conn *conn) ev_schedule(p->event); if (os == BS_ESTABLISHED) - bgp_conn_leave_established_state(p); + bgp_conn_leave_established_state(conn, p); } /** @@ -876,10 +883,7 @@ bgp_graceful_restart_timeout(timer *t) } } else - { bgp_stop(p, 0, NULL, 0); - bmp_peer_down(p, BE_NONE, NULL, 0); - } } static void @@ -1003,10 +1007,7 @@ bgp_sock_err(sock *sk, int err) if (err) BGP_TRACE(D_EVENTS, "Connection lost (%M)", err); else - { BGP_TRACE(D_EVENTS, "Connection closed"); - bmp_peer_down(p, BE_SOCKET, NULL, 0); - } if ((conn->state == BS_ESTABLISHED) && p->gr_ready) bgp_handle_graceful_restart(p); @@ -1331,7 +1332,6 @@ bgp_neigh_notify(neighbor *n) bgp_store_error(p, NULL, BE_MISC, BEM_NEIGHBOR_LOST); /* Perhaps also run bgp_update_startup_delay(p)? */ bgp_stop(p, 0, NULL, 0); - bmp_peer_down(p, BE_MISC, NULL, 0); } } else if (p->cf->check_link && !(n->iface->flags & IF_LINK_UP)) @@ -1343,7 +1343,6 @@ bgp_neigh_notify(neighbor *n) if (ps == PS_UP) bgp_update_startup_delay(p); bgp_stop(p, 0, NULL, 0); - bmp_peer_down(p, BE_MISC, NULL, 0); } } else @@ -1385,7 +1384,6 @@ bgp_bfd_notify(struct bfd_request *req) if (ps == PS_UP) bgp_update_startup_delay(p); bgp_stop(p, 0, NULL, 0); - bmp_peer_down(p, BE_MISC, NULL, 0); } } } @@ -2266,12 +2264,13 @@ bgp_error(struct bgp_conn *c, uint code, uint subcode, byte *data, int len) bgp_log_error(p, BE_BGP_TX, "Error", code, subcode, data, ABS(len)); bgp_store_error(p, c, BE_BGP_TX, (code << 16) | subcode); - bgp_conn_enter_close_state(c); c->notify_code = code; c->notify_subcode = subcode; c->notify_data = data; c->notify_size = (len > 0) ? len : 0; + + bgp_conn_enter_close_state(c); bgp_schedule_packet(c, NULL, PKT_NOTIFICATION); if (code != 6) diff --git a/proto/bgp/packets.c b/proto/bgp/packets.c index 2f1ff659..6b728b4e 100644 --- a/proto/bgp/packets.c +++ b/proto/bgp/packets.c @@ -3042,7 +3042,6 @@ bgp_fire_tx(struct bgp_conn *conn) { conn->packets_to_send = 1 << PKT_SCHEDULE_CLOSE; end = bgp_create_notification(conn, pkt); - bmp_peer_down(p, BE_BGP_TX, pkt, end - pkt); return bgp_send(conn, PKT_NOTIFICATION, end - buf); } else if (s & (1 << PKT_OPEN)) @@ -3334,6 +3333,11 @@ bgp_rx_notification(struct bgp_conn *conn, byte *pkt, uint len) bgp_log_error(p, BE_BGP_RX, "Received", code, subcode, pkt+21, len-21); bgp_store_error(p, conn, BE_BGP_RX, (code << 16) | subcode); + conn->notify_code = code; + conn->notify_subcode = subcode; + conn->notify_data = pkt+21; + conn->notify_size = len-21; + bgp_conn_enter_close_state(conn); bgp_schedule_packet(conn, NULL, PKT_SCHEDULE_CLOSE); @@ -3352,8 +3356,6 @@ bgp_rx_notification(struct bgp_conn *conn, byte *pkt, uint len) p->p.disabled = 1; } } - - bmp_peer_down(p, BE_BGP_RX, pkt + BGP_HEADER_LENGTH, len - BGP_HEADER_LENGTH); } static void diff --git a/proto/bmp/bmp.c b/proto/bmp/bmp.c index 22b9f2fb..261e9fdd 100644 --- a/proto/bmp/bmp.c +++ b/proto/bmp/bmp.c @@ -888,7 +888,7 @@ bmp_send_peer_down_notif_msg(struct bmp_proto *p, const struct bgp_proto *bgp, static void bmp_peer_down_(struct bmp_proto *p, const struct bgp_proto *bgp, - const int err_class, const byte *msg, size_t msg_length) + int err_class, int err_code, int err_subcode, const byte *data, int length) { if (!p->started) return; @@ -899,31 +899,43 @@ bmp_peer_down_(struct bmp_proto *p, const struct bgp_proto *bgp, TRACE(D_STATES, "Peer down for %s", bgp->p.name); - buffer payload = bmp_buffer_alloc(p->buffer_mpool, 1 + BGP_HEADER_LENGTH + msg_length); + uint bmp_code = 0; + uint fsm_code = 0; - if (msg) + switch (err_class) { - if (err_class == BE_BGP_TX) - bmp_put_u8(&payload, BMP_PEER_DOWN_REASON_LOCAL_BGP_NOTIFICATION); - else - bmp_put_u8(&payload, BMP_PEER_DOWN_REASON_REMOTE_BGP_NOTIFICATION); + case BE_BGP_RX: + bmp_code = BMP_PEER_DOWN_REASON_REMOTE_BGP_NOTIFICATION; + break; - bmp_put_bgp_hdr(&payload, BGP_HEADER_LENGTH + msg_length, PKT_NOTIFICATION); - bmp_put_data(&payload, msg, msg_length); + case BE_BGP_TX: + case BE_AUTO_DOWN: + case BE_MAN_DOWN: + bmp_code = BMP_PEER_DOWN_REASON_LOCAL_BGP_NOTIFICATION; + break; + + default: + bmp_code = BMP_PEER_DOWN_REASON_REMOTE_NO_NOTIFICATION; + length = 0; + break; } - else + + buffer payload = bmp_buffer_alloc(p->buffer_mpool, 1 + BGP_HEADER_LENGTH + 2 + length); + bmp_put_u8(&payload, bmp_code); + + switch (bmp_code) { - // TODO: Handle De-configured Peer Down Reason Code - if (err_class == BE_SOCKET || err_class == BE_MISC) - { - bmp_put_u8(&payload, BMP_PEER_DOWN_REASON_REMOTE_NO_NOTIFICATION); - } - else - { - bmp_put_u8(&payload, BMP_PEER_DOWN_REASON_LOCAL_NO_NOTIFICATION); - // TODO: Fill in with appropriate FSM event code - bmp_put_u16(&payload, 0x00); // no relevant Event code is defined - } + case BMP_PEER_DOWN_REASON_LOCAL_BGP_NOTIFICATION: + case BMP_PEER_DOWN_REASON_REMOTE_BGP_NOTIFICATION: + bmp_put_bgp_hdr(&payload, BGP_HEADER_LENGTH + 2 + length, PKT_NOTIFICATION); + bmp_put_u8(&payload, err_code); + bmp_put_u8(&payload, err_subcode); + bmp_put_data(&payload, data, length); + break; + + case BMP_PEER_DOWN_REASON_LOCAL_NO_NOTIFICATION: + bmp_put_u16(&payload, fsm_code); + break; } bmp_send_peer_down_notif_msg(p, bgp, bmp_buffer_data(&payload), bmp_buffer_pos(&payload)); @@ -934,12 +946,12 @@ bmp_peer_down_(struct bmp_proto *p, const struct bgp_proto *bgp, } void -bmp_peer_down(const struct bgp_proto *bgp, const int err_class, - const byte *msg, size_t msg_length) +bmp_peer_down(const struct bgp_proto *bgp, + int err_class, int code, int subcode, const byte *data, int length) { struct bmp_proto *p; node *n; WALK_LIST2(p, n, bmp_proto_list, bmp_node) - bmp_peer_down_(p, bgp, err_class, msg, msg_length); + bmp_peer_down_(p, bgp, err_class, code, subcode, data, length); } static void @@ -988,6 +1000,13 @@ bmp_rt_notify(struct proto *P, struct channel *c, struct network *net, struct bgp_proto *bgp = (void *) src->c.proto; bool policy = (c->table == src->c.table); + /* + * We assume that we receive peer_up before the first route and peer_down + * synchronously with BGP session close. So if bmp_stream exists, the related + * BGP session is up and could be accessed. That may not be true in + * multithreaded setup. + */ + struct bmp_stream *bs = bmp_find_stream(p, bgp, src->afi, policy); if (!bs) return; diff --git a/proto/bmp/bmp.h b/proto/bmp/bmp.h index e51c2ea0..2d700c25 100644 --- a/proto/bmp/bmp.h +++ b/proto/bmp/bmp.h @@ -116,14 +116,13 @@ bmp_peer_up(struct bgp_proto *bgp, * established state */ void -bmp_peer_down(const struct bgp_proto *bgp, const int err_class, const byte *pkt, - size_t pkt_size); +bmp_peer_down(const struct bgp_proto *bgp, int err_class, int code, int subcode, const byte *data, int length); #else /* BMP build disabled */ -static inline void bmp_peer_up(const struct bgp_proto *bgp UNUSED, const byte *tx_open_msg UNUSED, uint tx_open_length UNUSED, const byte *rx_open_msg UNUSED, uint rx_open_length UNUSED) { } -static inline void bmp_peer_down(const struct bgp_proto *bgp UNUSED, const int err_class UNUSED, const byte *pkt UNUSED, size_t pkt_size UNUSED) { } +static inline void bmp_peer_up(struct bgp_proto *bgp UNUSED, const byte *tx_open_msg UNUSED, uint tx_open_length UNUSED, const byte *rx_open_msg UNUSED, uint rx_open_length UNUSED) { } +static inline void bmp_peer_down(const struct bgp_proto *bgp UNUSED, const int err_class UNUSED, int code UNUSED, int subcode UNUSED, const byte *data UNUSED, int length UNUSED) { } #endif /* CONFIG_BMP */