From 6ceca15dd16edb4045494df72ed967d6c13397e2 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Fri, 8 Jul 2022 16:35:51 -0400 Subject: [PATCH 1/5] tests: Make bgp_snmp_mplsl3vpn be more forgiving I rarely get this failure: @classname: bgp_snmp_mplsl3vpn.test_bgp_snmp_mplsvpn @name: test_pe1_converge_evpn @time: 44.875 @message: AssertionError: BGP SNMP does not seem to be running assert False + where False = >('bgpVersion', '10') + where > = .test_oid "Wait for protocol convergence" tgen = get_topogen() r1 = tgen.gears["r1"] r1_snmp = SnmpTester(r1, "10.1.1.1", "public", "2c") assertmsg = "BGP SNMP does not seem to be running" > assert r1_snmp.test_oid("bgpVersion", "10"), assertmsg E AssertionError: BGP SNMP does not seem to be running E assert False E + where False = >('bgpVersion', '10') E + where > = .test_oid Under heavy system load a quick test before BGP can fully come up can result in a failed test. Add some extra time for snmp to come up properly. Signed-off-by: Donald Sharp --- .../bgp_snmp_mplsl3vpn/test_bgp_snmp_mplsvpn.py | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/tests/topotests/bgp_snmp_mplsl3vpn/test_bgp_snmp_mplsvpn.py b/tests/topotests/bgp_snmp_mplsl3vpn/test_bgp_snmp_mplsvpn.py index d612ad2c94..bc53dfb469 100755 --- a/tests/topotests/bgp_snmp_mplsl3vpn/test_bgp_snmp_mplsvpn.py +++ b/tests/topotests/bgp_snmp_mplsl3vpn/test_bgp_snmp_mplsvpn.py @@ -38,6 +38,7 @@ sys.path.append(os.path.join(CWD, "../")) # Import topogen and topotest helpers from lib.topogen import Topogen, TopoRouter, get_topogen from lib.snmptest import SnmpTester +from lib import topotest # Required to instantiate the topology builder class. @@ -239,10 +240,18 @@ def test_pe1_converge_evpn(): tgen = get_topogen() r1 = tgen.gears["r1"] - r1_snmp = SnmpTester(r1, "10.1.1.1", "public", "2c") + def _convergence(): + r1 = tgen.gears["r1"] + r1_snmp = SnmpTester(r1, "10.1.1.1", "public", "2c") + + return r1_snmp.test_oid("bgpVersion", "10") + + _, result = topotest.run_and_expect(_convergence, True, count=20, wait=1) assertmsg = "BGP SNMP does not seem to be running" - assert r1_snmp.test_oid("bgpVersion", "10"), assertmsg + assert result, assertmsg + + r1_snmp = SnmpTester(r1, "10.1.1.1", "public", "2c") count = 0 passed = False while count < 125: From a0d46bcd086b150dc9a193c8307b1c0e6e643852 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Fri, 8 Jul 2022 16:14:13 -0400 Subject: [PATCH 2/5] lib: thread pointer is already null at this point in agentx_events_update the timeout_thr is canceled on line 88 just above. This already sets the pointer to NULL. No need to do this again. Signed-off-by: Donald Sharp --- lib/agentx.c | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/agentx.c b/lib/agentx.c index 821c573fb2..e03e19aad5 100644 --- a/lib/agentx.c +++ b/lib/agentx.c @@ -118,7 +118,6 @@ static void agentx_events_update(void) snmp_select_info(&maxfd, &fds, &timeout, &block); if (!block) { - timeout_thr = NULL; thread_add_timer_tv(agentx_tm, agentx_timeout, NULL, &timeout, &timeout_thr); } From 3f22218b48807e3d5fa4a041536f65e5352f358b Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Fri, 8 Jul 2022 20:39:28 -0400 Subject: [PATCH 3/5] bgpd: Prevent memory leak of listener on shutdown Signed-off-by: Donald Sharp --- bgpd/bgpd.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/bgpd/bgpd.c b/bgpd/bgpd.c index 122830343c..cc15e6f688 100644 --- a/bgpd/bgpd.c +++ b/bgpd/bgpd.c @@ -8228,18 +8228,20 @@ void bgp_terminate(void) * of a large number of peers this will ensure that no peer is left with * a dangling connection */ - /* reverse bgp_master_init */ + bgp_close(); - - if (bm->listen_sockets) - list_delete(&bm->listen_sockets); - - for (ALL_LIST_ELEMENTS(bm->bgp, mnode, mnnode, bgp)) + /* reverse bgp_master_init */ + for (ALL_LIST_ELEMENTS(bm->bgp, mnode, mnnode, bgp)) { + bgp_close_vrf_socket(bgp); for (ALL_LIST_ELEMENTS(bgp->peer, node, nnode, peer)) if (peer_established(peer) || peer->status == OpenSent || peer->status == OpenConfirm) bgp_notify_send(peer, BGP_NOTIFY_CEASE, BGP_NOTIFY_CEASE_PEER_UNCONFIG); + } + + if (bm->listen_sockets) + list_delete(&bm->listen_sockets); BGP_TIMER_OFF(bm->t_rmap_update); From 04fd828f3fec7b76be6de73eafc5af081c3b4714 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Fri, 8 Jul 2022 20:53:43 -0400 Subject: [PATCH 4/5] bgpd: rfapi code does not need to assert on memory allocations cleanup memory allocations asserting that it didn't fail as well as clean up some thread shenanigans. Signed-off-by: Donald Sharp --- bgpd/rfapi/rfapi.c | 5 +---- bgpd/rfapi/rfapi_ap.c | 1 - bgpd/rfapi/rfapi_encap_tlv.c | 1 - bgpd/rfapi/rfapi_import.c | 8 -------- bgpd/rfapi/rfapi_rib.c | 6 +----- bgpd/rfapi/vnc_export_bgp.c | 1 - bgpd/rfapi/vnc_export_table.c | 1 - 7 files changed, 2 insertions(+), 21 deletions(-) diff --git a/bgpd/rfapi/rfapi.c b/bgpd/rfapi/rfapi.c index 35a76e7836..f0e68d7f3f 100644 --- a/bgpd/rfapi/rfapi.c +++ b/bgpd/rfapi/rfapi.c @@ -721,7 +721,6 @@ void add_vnc_route(struct rfapi_descriptor *rfd, /* cookie, VPN UN addr, peer */ encaptlv = XCALLOC(MTYPE_ENCAP_TLV, sizeof(struct bgp_attr_encap_subtlv) + 4); - assert(encaptlv); encaptlv->type = BGP_VNC_SUBTLV_TYPE_LIFETIME; /* prefix lifetime */ encaptlv->length = 4; @@ -766,7 +765,6 @@ void add_vnc_route(struct rfapi_descriptor *rfd, /* cookie, VPN UN addr, peer */ MTYPE_ENCAP_TLV, sizeof(struct bgp_attr_encap_subtlv) + 2 + hop->length); - assert(encaptlv); encaptlv->type = BGP_VNC_SUBTLV_TYPE_RFPOPTION; /* RFP option @@ -1041,7 +1039,7 @@ void add_vnc_route(struct rfapi_descriptor *rfd, /* cookie, VPN UN addr, peer */ SET_FLAG(new->flags, BGP_PATH_VALID); /* save backref to rfapi handle */ - assert(bgp_path_info_extra_get(new)); + bgp_path_info_extra_get(new); new->extra->vnc.export.rfapi_handle = (void *)rfd; encode_label(label_val, &new->extra->label[0]); @@ -1963,7 +1961,6 @@ int rfapi_open(void *rfp_start_val, struct rfapi_ip_addr *vn, rfd = XCALLOC(MTYPE_RFAPI_DESC, sizeof(struct rfapi_descriptor)); } - assert(rfd); rfd->bgp = bgp; if (default_options) { diff --git a/bgpd/rfapi/rfapi_ap.c b/bgpd/rfapi/rfapi_ap.c index abb18aeb2c..fcc6168cfa 100644 --- a/bgpd/rfapi/rfapi_ap.c +++ b/bgpd/rfapi/rfapi_ap.c @@ -459,7 +459,6 @@ int rfapiApAdd(struct bgp *bgp, struct rfapi_descriptor *rfd, if (rc) { /* Not found */ adb = XCALLOC(MTYPE_RFAPI_ADB, sizeof(struct rfapi_adb)); - assert(adb); adb->lifetime = lifetime; adb->u.key = rk; diff --git a/bgpd/rfapi/rfapi_encap_tlv.c b/bgpd/rfapi/rfapi_encap_tlv.c index a7bc909c58..d4e875df2a 100644 --- a/bgpd/rfapi/rfapi_encap_tlv.c +++ b/bgpd/rfapi/rfapi_encap_tlv.c @@ -170,7 +170,6 @@ struct rfapi_un_option *rfapi_encap_tlv_to_un_option(struct attr *attr) stlv = attr->encap_subtlvs; uo = XCALLOC(MTYPE_RFAPI_UN_OPTION, sizeof(struct rfapi_un_option)); - assert(uo); uo->type = RFAPI_UN_OPTION_TYPE_TUNNELTYPE; uo->v.tunnel.type = attr->encap_tunneltype; tto = &uo->v.tunnel; diff --git a/bgpd/rfapi/rfapi_import.c b/bgpd/rfapi/rfapi_import.c index 1d58c03313..91b68b13d8 100644 --- a/bgpd/rfapi/rfapi_import.c +++ b/bgpd/rfapi/rfapi_import.c @@ -1273,7 +1273,6 @@ rfapiRouteInfo2NextHopEntry(struct rfapi_ip_prefix *rprefix, #endif new = XCALLOC(MTYPE_RFAPI_NEXTHOP, sizeof(struct rfapi_next_hop_entry)); - assert(new); new->prefix = *rprefix; @@ -1286,7 +1285,6 @@ rfapiRouteInfo2NextHopEntry(struct rfapi_ip_prefix *rprefix, vo = XCALLOC(MTYPE_RFAPI_VN_OPTION, sizeof(struct rfapi_vn_option)); - assert(vo); vo->type = RFAPI_VN_OPTION_TYPE_L2ADDR; @@ -2308,7 +2306,6 @@ rfapiMonitorEncapAdd(struct rfapi_import_table *import_table, m = XCALLOC(MTYPE_RFAPI_MONITOR_ENCAP, sizeof(struct rfapi_monitor_encap)); - assert(m); m->node = vpn_rn; m->bpi = vpn_bpi; @@ -2791,7 +2788,6 @@ rfapiBiStartWithdrawTimer(struct rfapi_import_table *import_table, * service routine, which is supposed to free the wcb. */ wcb = XCALLOC(MTYPE_RFAPI_WITHDRAW, sizeof(struct rfapi_withdraw)); - assert(wcb); wcb->node = rn; wcb->info = bpi; wcb->import_table = import_table; @@ -3224,7 +3220,6 @@ static void rfapiBgpInfoFilteredImportEncap( struct agg_table *referenced_vpn_table; referenced_vpn_table = agg_table_init(); - assert(referenced_vpn_table); /* * iterate over the set of monitors at this ENCAP node. @@ -3282,7 +3277,6 @@ static void rfapiBgpInfoFilteredImportEncap( mnext = XCALLOC( MTYPE_RFAPI_MONITOR_ENCAP, sizeof(struct rfapi_monitor_encap)); - assert(mnext); mnext->node = m->node; mnext->next = referenced_vpn_prefix->info; referenced_vpn_prefix->info = mnext; @@ -4332,7 +4326,6 @@ rfapiImportTableRefAdd(struct bgp *bgp, struct ecommunity *rt_import_list, if (!it) { it = XCALLOC(MTYPE_RFAPI_IMPORTTABLE, sizeof(struct rfapi_import_table)); - assert(it); it->next = h->imports; h->imports = it; @@ -4551,7 +4544,6 @@ static void rfapiDeleteRemotePrefixesIt( MTYPE_RFAPI_NVE_ADDR, sizeof(struct rfapi_nve_addr)); - assert(nap); *nap = na; nap->info = is_active ? pAHcount diff --git a/bgpd/rfapi/rfapi_rib.c b/bgpd/rfapi/rfapi_rib.c index 44eebe961c..c4fc96f5ae 100644 --- a/bgpd/rfapi/rfapi_rib.c +++ b/bgpd/rfapi/rfapi_rib.c @@ -357,10 +357,9 @@ static void rfapiRibStartTimer(struct rfapi_descriptor *rfd, vnc_zlog_debug_verbose("%s: rfd %p pfx %pRN life %u", __func__, rfd, rn, ri->lifetime); - ri->timer = NULL; + thread_add_timer(bm->master, rfapiRibExpireTimer, tcb, ri->lifetime, &ri->timer); - assert(ri->timer); } extern void rfapi_rib_key_init(struct prefix *prefix, /* may be NULL */ @@ -1179,7 +1178,6 @@ callback: new = XCALLOC(MTYPE_RFAPI_NEXTHOP, sizeof(struct rfapi_next_hop_entry)); - assert(new); if (ri->rk.aux_prefix.family) { rfapiQprefix2Rprefix(&ri->rk.aux_prefix, @@ -1269,7 +1267,6 @@ callback: new = XCALLOC( MTYPE_RFAPI_NEXTHOP, sizeof(struct rfapi_next_hop_entry)); - assert(new); if (ri->rk.aux_prefix.family) { rfapiQprefix2Rprefix(&ri->rk.aux_prefix, @@ -1718,7 +1715,6 @@ void rfapiRibUpdatePendingNode( urq = XCALLOC(MTYPE_RFAPI_UPDATED_RESPONSE_QUEUE, sizeof(struct rfapi_updated_responses_queue)); - assert(urq); if (!rfd->updated_responses_queue) updated_responses_queue_init(rfd); diff --git a/bgpd/rfapi/vnc_export_bgp.c b/bgpd/rfapi/vnc_export_bgp.c index 7cfa2ed67d..2152a55ca3 100644 --- a/bgpd/rfapi/vnc_export_bgp.c +++ b/bgpd/rfapi/vnc_export_bgp.c @@ -564,7 +564,6 @@ static struct ecommunity *vnc_route_origin_ecom_single(struct in_addr *origin) roec.val[7] = 0; new = ecommunity_new(); - assert(new); ecommunity_add_val(new, &roec, false, false); if (!new->size) { diff --git a/bgpd/rfapi/vnc_export_table.c b/bgpd/rfapi/vnc_export_table.c index 255f868bdf..743576d265 100644 --- a/bgpd/rfapi/vnc_export_table.c +++ b/bgpd/rfapi/vnc_export_table.c @@ -119,7 +119,6 @@ struct vnc_export_info *vnc_eti_get(struct bgp *bgp, vnc_export_type_t etype, agg_unlock_node(etn); } else { eti = XCALLOC(MTYPE_RFAPI_ETI, sizeof(struct vnc_export_info)); - assert(eti); eti->node = etn; eti->peer = peer; peer_lock(peer); From e05506b8ddc2a84cb24bf377303ccecacdeac653 Mon Sep 17 00:00:00 2001 From: Donald Sharp Date: Tue, 19 Jul 2022 13:57:56 -0400 Subject: [PATCH 5/5] zebra: Add some more data to rtadv socket failures The creation of the rtadv socket can fail but there is very very little data associated with this event to let the operator know something has gone terribly wrong. Please note if this socket fails to create or fails the setsockopt's rtadv is basically just really really messed up. I am not sure what can be done here. Signed-off-by: Donald Sharp --- zebra/rtadv.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/zebra/rtadv.c b/zebra/rtadv.c index 5d4ed1e424..bf959980be 100644 --- a/zebra/rtadv.c +++ b/zebra/rtadv.c @@ -830,39 +830,51 @@ static int rtadv_make_socket(ns_id_t ns_id) int sock = -1; int ret = 0; struct icmp6_filter filter; + int error; frr_with_privs(&zserv_privs) { sock = ns_socket(AF_INET6, SOCK_RAW, IPPROTO_ICMPV6, ns_id); - + /* + * with privs might set errno too if it fails save + * to the side + */ + error = errno; } if (sock < 0) { + zlog_warn("RTADV socket for ns: %u failure to create: %s(%u)", + ns_id, safe_strerror(error), error); return -1; } ret = setsockopt_ipv6_pktinfo(sock, 1); if (ret < 0) { + zlog_warn("RTADV failure to set Packet Information"); close(sock); return ret; } ret = setsockopt_ipv6_multicast_loop(sock, 0); if (ret < 0) { + zlog_warn("RTADV failure to set multicast Loop detection"); close(sock); return ret; } ret = setsockopt_ipv6_unicast_hops(sock, 255); if (ret < 0) { + zlog_warn("RTADV failure to set maximum unicast hops"); close(sock); return ret; } ret = setsockopt_ipv6_multicast_hops(sock, 255); if (ret < 0) { + zlog_warn("RTADV failure to set maximum multicast hops"); close(sock); return ret; } ret = setsockopt_ipv6_hoplimit(sock, 1); if (ret < 0) { + zlog_warn("RTADV failure to set maximum incoming hop limit"); close(sock); return ret; }