From 34953f233997bd42faa1db23de8013135d8f8b55 Mon Sep 17 00:00:00 2001 From: Philippe Guibert Date: Tue, 29 Oct 2024 22:24:47 +0100 Subject: [PATCH 1/3] bgpd: bmp, fix address sanitizer issue The following ASAN error can be seen. > ERROR: AddressSanitizer: attempting to call malloc_usable_size() for pointer which is not owned: 0x608000036c20 > #0 0x7f3d7a4b5425 in __interceptor_malloc_usable_size ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:198 > #1 0x7f3d7a426a16 in __sanitizer::BufferedStackTrace::Unwind(unsigned long, unsigned long, void*, bool, unsigned int) ../../../../src/libsanitizer/sanitizer_common > /sanitizer_stacktrace.h:122 > #2 0x7f3d7a426a16 in __asan::asan_malloc_usable_size(void const*, unsigned long, unsigned long) ../../../../src/libsanitizer/asan/asan_allocator.cpp:1074 > #3 0x7f3d7a03f330 in mt_count_free lib/memory.c:78 > #4 0x7f3d7a03f330 in qfree lib/memory.c:130 > #5 0x7f3d76ccf89b in bmp_peer_status_changed bgpd/bgp_bmp.c:982 > #6 0x560ae2aa6a94 in hook_call_peer_status_changed bgpd/bgp_fsm.c:47 > #7 0x560ae2aa6a94 in bgp_fsm_change_status bgpd/bgp_fsm.c:1287 > #8 0x560ae2c4f2e5 in peer_delete bgpd/bgpd.c:2777 > #9 0x560ae2c58d24 in bgp_delete bgpd/bgpd.c:4140 > #10 0x560ae2bbb47e in no_router_bgp bgpd/bgp_vty.c:1764 > #11 0x7f3d79fb74ed in cmd_execute_command_real lib/command.c:1003 > #12 0x7f3d79fb78a3 in cmd_execute_command lib/command.c:1062 > #13 0x7f3d79fb7e03 in cmd_execute lib/command.c:1228 > #14 0x7f3d7a107b53 in vty_command lib/vty.c:625 > #15 0x7f3d7a109902 in vty_execute lib/vty.c:1388 > #16 0x7f3d7a10cc32 in vtysh_read lib/vty.c:2400 > #17 0x7f3d7a0f848b in event_call lib/event.c:2019 > #18 0x7f3d7a01e627 in frr_run lib/libfrr.c:1232 > #19 0x560ae29e0037 in main bgpd/bgp_main.c:555 > #20 0x7f3d79a29d8f in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58 > #21 0x7f3d79a29e3f in __libc_start_main_impl ../csu/libc-start.c:392 > #22 0x560ae29e4ef4 in _start (/usr/lib/frr/bgpd+0x2eeef4) > > 0x608000036c20 is located 0 bytes inside of 81-byte region [0x608000036c20,0x608000036c71) > freed by thread T0 here: > #0 0x7f3d7a4b4537 in __interceptor_free ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:127 > #1 0x7f3d76ccf85f in bmp_peer_status_changed bgpd/bgp_bmp.c:981 > #2 0x560ae2aa6a94 in hook_call_peer_status_changed bgpd/bgp_fsm.c:47 > #3 0x560ae2aa6a94 in bgp_fsm_change_status bgpd/bgp_fsm.c:1287 > #4 0x560ae2c4f2e5 in peer_delete bgpd/bgpd.c:2777 > #5 0x560ae2c58d24 in bgp_delete bgpd/bgpd.c:4140 > #6 0x560ae2bbb47e in no_router_bgp bgpd/bgp_vty.c:1764 > #7 0x7f3d79fb74ed in cmd_execute_command_real lib/command.c:1003 > #8 0x7f3d79fb78a3 in cmd_execute_command lib/command.c:1062 > #9 0x7f3d79fb7e03 in cmd_execute lib/command.c:1228 > #10 0x7f3d7a107b53 in vty_command lib/vty.c:625 > #11 0x7f3d7a109902 in vty_execute lib/vty.c:1388 > #12 0x7f3d7a10cc32 in vtysh_read lib/vty.c:2400 > #13 0x7f3d7a0f848b in event_call lib/event.c:2019 > #14 0x7f3d7a01e627 in frr_run lib/libfrr.c:1232 > #15 0x560ae29e0037 in main bgpd/bgp_main.c:555 > #16 0x7f3d79a29d8f in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58 > > previously allocated by thread T0 here: > #0 0x7f3d7a4b4887 in __interceptor_malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:145 > #1 0x7f3d7a03f0e9 in qmalloc lib/memory.c:101 > #2 0x7f3d76cd0166 in bmp_bgp_peer_vrf bgpd/bgp_bmp.c:2194 > #3 0x7f3d76cd0166 in bmp_bgp_update_vrf_status bgpd/bgp_bmp.c:2236 > #4 0x7f3d76cd29b8 in bmp_vrf_state_changed bgpd/bgp_bmp.c:3479 > #5 0x560ae2c45b34 in hook_call_bgp_instance_state bgpd/bgpd.c:88 > #6 0x560ae2c4d158 in bgp_instance_up bgpd/bgpd.c:3936 > #7 0x560ae29e5ed1 in bgp_vrf_enable bgpd/bgp_main.c:299 > #8 0x7f3d7a0ff8b1 in vrf_enable lib/vrf.c:286 > #9 0x7f3d7a0ff8b1 in vrf_enable lib/vrf.c:275 > #10 0x7f3d7a12ab66 in zclient_vrf_add lib/zclient.c:2561 > #11 0x7f3d7a12eb43 in zclient_read lib/zclient.c:4624 > #12 0x7f3d7a0f848b in event_call lib/event.c:2019 > #13 0x7f3d7a01e627 in frr_run lib/libfrr.c:1232 > #14 0x560ae29e0037 in main bgpd/bgp_main.c:555 > #15 0x7f3d79a29d8f in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58 Signed-off-by: Philippe Guibert --- bgpd/bgp_bmp.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/bgpd/bgp_bmp.c b/bgpd/bgp_bmp.c index 2e3a0388d0ed..a3acd35195fc 100644 --- a/bgpd/bgp_bmp.c +++ b/bgpd/bgp_bmp.c @@ -1981,7 +1981,8 @@ static void bmp_bgp_peer_vrf(struct bmp_bgp_peer *bbpeer, struct bgp *bgp) memcpy(bbpeer->open_rx, s->data, open_len); bbpeer->open_tx_len = open_len; - bbpeer->open_tx = bbpeer->open_rx; + bbpeer->open_tx = XMALLOC(MTYPE_BMP_OPEN, open_len); + memcpy(bbpeer->open_tx, s->data, open_len); stream_free(s); } From 00419d25a0844ff282f5d1926d31d22ca5829930 Mon Sep 17 00:00:00 2001 From: Philippe Guibert Date: Fri, 8 Nov 2024 09:48:11 +0100 Subject: [PATCH 2/3] bgpd: fix warning of compilation when using bgp_trace MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The following warning can be seen: > In file included from ./bgpd/bgp_trace.h:21, > from bgpd/bgp_io.c:27: > bgpd/bgp_io.c: In function ‘read_ibuf_work’: > bgpd/bgp_io.c:202:53: warning: passing argument 1 of ‘lttng_ust_tracepoint_cb_frr_bgp___packet_read’ from incompatible pointer type [-Wincompatible-pointer-types] > 202 | frrtrace(2, frr_bgp, packet_read, connection->peer, pkt); > | ~~~~~~~~~~^~~~~~ > | | > | struct peer * > bgpd/bgp_io.c:202:9: note: in expansion of macro ‘frrtrace’ > 202 | frrtrace(2, frr_bgp, packet_read, connection->peer, pkt); > | ^~~~~~~~ > In file included from ./bgpd/bgp_trace.h:21, > from bgpd/bgp_io.c:27: > ./bgpd/bgp_trace.h:57:43: note: expected ‘struct peer_connection *’ but argument is of type ‘struct peer *’ > 57 | TP_ARGS(struct peer_connection *, connection, struct stream *, pkt), > | ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~ Use the appropriate connection parameter when calling the trace. Signed-off-by: Philippe Guibert --- bgpd/bgp_io.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bgpd/bgp_io.c b/bgpd/bgp_io.c index 9e9251c85459..5d0f14cc5c96 100644 --- a/bgpd/bgp_io.c +++ b/bgpd/bgp_io.c @@ -199,7 +199,7 @@ static int read_ibuf_work(struct peer_connection *connection) assert(ringbuf_get(ibw, pkt->data, pktsize) == pktsize); stream_set_endp(pkt, pktsize); - frrtrace(2, frr_bgp, packet_read, connection->peer, pkt); + frrtrace(2, frr_bgp, packet_read, connection, pkt); frr_with_mutex (&connection->io_mtx) { stream_fifo_push(connection->ibuf, pkt); } From ec11074b963a3e78e75d6c1779756778fec35aa5 Mon Sep 17 00:00:00 2001 From: Philippe Guibert Date: Tue, 29 Oct 2024 16:20:18 +0100 Subject: [PATCH 3/3] bgpd: fix do re-send post-policy bgp update when not valid When a BGP listener configured with BMP receives the first BGP IPv6 update from a connected BGP IPv6 peer, the BMP collector receives a withdraw post-policy message. > {"peer_type": "route distinguisher instance", "policy": "post-policy", > "ipv6": true, "peer_ip": "192:167::3", "peer_distinguisher": "444:1", > "peer_asn": 65501, "peer_bgp_id": "192.168.1.3", "timestamp": > "2024-10-29 11:44:47.111962", "bmp_log_type": "withdraw", "afi": 2, > "safi": 1, "ip_prefix": "2001::1125/128", "seq": 22} > {"peer_type": "route distinguisher instance", "policy": "pre-policy", > "ipv6": true, "peer_ip": "192:167::3", "peer_distinguisher": "444:1", > "peer_asn": 65501, "peer_bgp_id": "192.168.1.3", "timestamp": > "2024-10-29 11:44:47.111963", "bmp_log_type": "update", "origin": > "IGP", "as_path": "", "afi": 2, "safi": 1, "nxhp_ip": "192:167::3", > "nxhp_link-local": "fe80::7063:d8ff:fedb:9e11", "ip_prefix": "2001::1125/128", "seq": 23} Actually, the BGP update is not valid, and BMP considers it as a withdraw message. The BGP upate is not valid, because the nexthop reachability is unknown at the time of reception, and no other BMP message is sent. Fix this by re-sending a BMP post update message when nexthop tracking becomes successfull. Generalise the re-sending of messages when nexthop tracking changes. Signed-off-by: Philippe Guibert --- bgpd/bgp_bmp.c | 22 ++++++++++++++++++++++ bgpd/bgp_nht.c | 6 ++++++ bgpd/bgp_nht.h | 5 +++++ bgpd/bgp_trace.h | 18 ++++++++++++++++++ 4 files changed, 51 insertions(+) diff --git a/bgpd/bgp_bmp.c b/bgpd/bgp_bmp.c index a3acd35195fc..4249c8af5a3f 100644 --- a/bgpd/bgp_bmp.c +++ b/bgpd/bgp_bmp.c @@ -28,6 +28,7 @@ #include "bgpd/bgp_table.h" #include "bgpd/bgpd.h" #include "bgpd/bgp_route.h" +#include "bgpd/bgp_nht.h" #include "bgpd/bgp_attr.h" #include "bgpd/bgp_dump.h" #include "bgpd/bgp_errors.h" @@ -1679,6 +1680,26 @@ static int bmp_process(struct bgp *bgp, afi_t afi, safi_t safi, return 0; } +static int bmp_nht_path_valid(struct bgp *bgp, struct bgp_path_info *path, bool valid) +{ + struct bgp_dest *dest = path->net; + struct bgp_table *table; + + if (frrtrace_enabled(frr_bgp, bmp_nht_path_valid)) { + char pfxprint[PREFIX2STR_BUFFER]; + + prefix2str(&dest->rn->p, pfxprint, sizeof(pfxprint)); + frrtrace(4, frr_bgp, bmp_nht_path_valid, bgp, pfxprint, path, valid); + } + if (bgp->peer_self == path->peer) + /* self declared networks or redistributed networks are not relevant for bmp */ + return 0; + + table = bgp_dest_table(dest); + + return bmp_process(bgp, table->afi, table->safi, dest, path->peer, !valid); +} + static void bmp_stat_put_u32(struct stream *s, size_t *cnt, uint16_t type, uint32_t value) { @@ -3156,6 +3177,7 @@ static int bgp_bmp_module_init(void) hook_register(peer_status_changed, bmp_peer_status_changed); hook_register(peer_backward_transition, bmp_peer_backward); hook_register(bgp_process, bmp_process); + hook_register(bgp_nht_path_update, bmp_nht_path_valid); hook_register(bgp_inst_config_write, bmp_config_write); hook_register(bgp_inst_delete, bmp_bgp_del); hook_register(frr_late_init, bgp_bmp_init); diff --git a/bgpd/bgp_nht.c b/bgpd/bgp_nht.c index 9b633b7139b2..f4d83bd647ac 100644 --- a/bgpd/bgp_nht.c +++ b/bgpd/bgp_nht.c @@ -41,6 +41,9 @@ static void unregister_zebra_rnh(struct bgp_nexthop_cache *bnc); static int make_prefix(int afi, struct bgp_path_info *pi, struct prefix *p); static void bgp_nht_ifp_initial(struct event *thread); +DEFINE_HOOK(bgp_nht_path_update, (struct bgp *bgp, struct bgp_path_info *pi, bool valid), + (bgp, pi, valid)); + static int bgp_isvalid_nexthop(struct bgp_nexthop_cache *bnc) { return (bgp_zebra_num_connects() == 0 @@ -1442,6 +1445,9 @@ void evaluate_paths(struct bgp_nexthop_cache *bnc) } } + if (path_valid != bnc_is_valid_nexthop) + hook_call(bgp_nht_path_update, bgp_path, path, bnc_is_valid_nexthop); + bgp_process(bgp_path, dest, path, afi, safi); } diff --git a/bgpd/bgp_nht.h b/bgpd/bgp_nht.h index e7c6fdc281b6..345089ac5acf 100644 --- a/bgpd/bgp_nht.h +++ b/bgpd/bgp_nht.h @@ -83,4 +83,9 @@ extern void bgp_nht_ifp_up(struct interface *ifp); extern void bgp_nht_ifp_down(struct interface *ifp); extern void bgp_nht_interface_events(struct peer *peer); + +/* called when a path becomes valid or invalid, because of nexthop tracking */ +DECLARE_HOOK(bgp_nht_path_update, (struct bgp *bgp, struct bgp_path_info *pi, bool valid), + (bgp, pi, valid)); + #endif /* _BGP_NHT_H */ diff --git a/bgpd/bgp_trace.h b/bgpd/bgp_trace.h index a77a25e435e1..43bc7a5a1738 100644 --- a/bgpd/bgp_trace.h +++ b/bgpd/bgp_trace.h @@ -210,6 +210,24 @@ TRACEPOINT_EVENT( TRACEPOINT_LOGLEVEL(frr_bgp, bmp_process, TRACE_DEBUG) +/* + * BMP is hooked for a nexthop tracking event + */ +TRACEPOINT_EVENT( + frr_bgp, + bmp_nht_path_valid, + TP_ARGS(struct bgp *, bgp, char *, pfx, struct bgp_path_info *, + path, bool, valid), + TP_FIELDS( + ctf_string(bgp, bgp->name_pretty) + ctf_string(prefix, pfx) + ctf_string(path, PEER_HOSTNAME(path->peer)) + ctf_integer(bool, valid, valid) + ) +) + +TRACEPOINT_LOGLEVEL(frr_bgp, bmp_nht_path_valid, TRACE_DEBUG) + /* * bgp_dest_lock/bgp_dest_unlock */