From 53048d33d124e60ce01ae6bf149ae03561930694 Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Fri, 11 Jun 2021 18:09:05 +0300 Subject: [PATCH 01/10] bgpd: Remove useless reuselist_node assignment before while loop Seems really not necessary pointing to initial value before while loop, where it's assigned anyway. Signed-off-by: Donatas Abraitis --- bgpd/bgp_damp.c | 1 - 1 file changed, 1 deletion(-) diff --git a/bgpd/bgp_damp.c b/bgpd/bgp_damp.c index 07c70d5aae..68bd6ad874 100644 --- a/bgpd/bgp_damp.c +++ b/bgpd/bgp_damp.c @@ -245,7 +245,6 @@ static int bgp_reuse_timer(struct thread *t) * list head entry. */ assert(bdc->reuse_offset < bdc->reuse_list_size); plist = bdc->reuse_list[bdc->reuse_offset]; - node = SLIST_FIRST(&plist); SLIST_INIT(&bdc->reuse_list[bdc->reuse_offset]); /* 2. set offset = modulo reuse-list-size ( offset + 1 ), thereby From 63fc789a109cf5e086d7494cea7a9f516550839c Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Fri, 11 Jun 2021 18:14:27 +0300 Subject: [PATCH 02/10] bgpd: Do not test against bdc again since we already validated Signed-off-by: Donatas Abraitis --- bgpd/bgp_damp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bgpd/bgp_damp.c b/bgpd/bgp_damp.c index 68bd6ad874..2a372c0ba4 100644 --- a/bgpd/bgp_damp.c +++ b/bgpd/bgp_damp.c @@ -787,7 +787,7 @@ const char *bgp_damp_reuse_time_vty(struct vty *vty, struct bgp_path_info *path, /* If dampening is not enabled or there is no dampening information, return immediately. */ - if (!bdc || !bdi) + if (!bdi) return NULL; /* Calculate new penalty. */ From 734c6f0953065a2b6a0637843b56a6bbb1402e53 Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Fri, 11 Jun 2021 18:18:59 +0300 Subject: [PATCH 03/10] bgpd: Remove double check against match_packet_length_num Signed-off-by: Donatas Abraitis --- bgpd/bgp_flowspec_util.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/bgpd/bgp_flowspec_util.c b/bgpd/bgp_flowspec_util.c index b244c87258..23baa0184e 100644 --- a/bgpd/bgp_flowspec_util.c +++ b/bgpd/bgp_flowspec_util.c @@ -641,13 +641,12 @@ int bgp_flowspec_match_rules_fill(uint8_t *nlri_content, int len, __func__, type); } } - if (bpem->match_packet_length_num || bpem->match_fragment_num || - bpem->match_tcpflags_num || bpem->match_dscp_num || - bpem->match_packet_length_num || bpem->match_icmp_code_num || - bpem->match_icmp_type_num || bpem->match_port_num || - bpem->match_src_port_num || bpem->match_dst_port_num || - bpem->match_protocol_num || bpem->match_bitmask || - bpem->match_flowlabel_num) + if (bpem->match_packet_length_num || bpem->match_fragment_num + || bpem->match_tcpflags_num || bpem->match_dscp_num + || bpem->match_icmp_code_num || bpem->match_icmp_type_num + || bpem->match_port_num || bpem->match_src_port_num + || bpem->match_dst_port_num || bpem->match_protocol_num + || bpem->match_bitmask || bpem->match_flowlabel_num) bpem->type = BGP_PBR_IPSET; else if ((bpem->match_bitmask_iprule & PREFIX_SRC_PRESENT) || (bpem->match_bitmask_iprule & PREFIX_DST_PRESENT)) From eaf8028849845afe13f25189584dbb708e4175a1 Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Fri, 11 Jun 2021 18:23:43 +0300 Subject: [PATCH 04/10] bgpd: Avoid dereferencing EVPN ES if NULL Signed-off-by: Donatas Abraitis --- bgpd/bgp_evpn_mh.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/bgpd/bgp_evpn_mh.c b/bgpd/bgp_evpn_mh.c index 59bced6f93..e8d3b5e6fc 100644 --- a/bgpd/bgp_evpn_mh.c +++ b/bgpd/bgp_evpn_mh.c @@ -2070,9 +2070,8 @@ int bgp_evpn_local_es_del(struct bgp *bgp, esi_t *esi) /* Lookup ESI hash - should exist. */ es = bgp_evpn_es_find(esi); if (!es) { - flog_warn(EC_BGP_EVPN_ESI, - "%u: ES %s missing at local ES DEL", - bgp->vrf_id, es->esi_str); + flog_warn(EC_BGP_EVPN_ESI, "%u: ES missing at local ES DEL", + bgp->vrf_id); return -1; } From 707bb5a09c8dd341179fdece8eb602120d30b83c Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Fri, 11 Jun 2021 18:31:26 +0300 Subject: [PATCH 05/10] bgpd: Remove redundand check against BGP_EVPNES_EVI_LOCAL flag It's already checked earlier at bgp_evpn_local_es_evi_do_del() Signed-off-by: Donatas Abraitis --- bgpd/bgp_evpn_mh.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/bgpd/bgp_evpn_mh.c b/bgpd/bgp_evpn_mh.c index e8d3b5e6fc..b191840f63 100644 --- a/bgpd/bgp_evpn_mh.c +++ b/bgpd/bgp_evpn_mh.c @@ -3316,9 +3316,6 @@ bgp_evpn_es_evi_local_info_clear(struct bgp_evpn_es_evi *es_evi) { struct bgpevpn *vpn = es_evi->vpn; - if (!CHECK_FLAG(es_evi->flags, BGP_EVPNES_EVI_LOCAL)) - return es_evi; - UNSET_FLAG(es_evi->flags, BGP_EVPNES_EVI_LOCAL); list_delete_node(vpn->local_es_evi_list, &es_evi->l2vni_listnode); From 9c73cd41f72c1f66dcf8dcd80760c82c63fc7c5e Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Fri, 11 Jun 2021 18:37:50 +0300 Subject: [PATCH 06/10] bgpd: Do not check against aspath `seg` which is already checked before Signed-off-by: Donatas Abraitis --- bgpd/bgp_aspath.c | 129 ++++++++++++++++++++++------------------------ 1 file changed, 61 insertions(+), 68 deletions(-) diff --git a/bgpd/bgp_aspath.c b/bgpd/bgp_aspath.c index 5cf3c60fa2..25109e030b 100644 --- a/bgpd/bgp_aspath.c +++ b/bgpd/bgp_aspath.c @@ -910,77 +910,70 @@ size_t aspath_put(struct stream *s, struct aspath *as, int use32bit) if (!seg || seg->length == 0) return 0; - if (seg) { - /* - * Hey, what do we do when we have > STREAM_WRITABLE(s) here? - * At the moment, we would write out a partial aspath, and our - * peer - * will complain and drop the session :-/ - * - * The general assumption here is that many things tested will - * never happen. And, in real live, up to now, they have not. - */ - while (seg && (ASSEGMENT_LEN(seg, use32bit) - <= STREAM_WRITEABLE(s))) { - struct assegment *next = seg->next; - int written = 0; - int asns_packed = 0; - size_t lenp; + /* + * Hey, what do we do when we have > STREAM_WRITABLE(s) here? + * At the moment, we would write out a partial aspath, and our + * peer + * will complain and drop the session :-/ + * + * The general assumption here is that many things tested will + * never happen. And, in real live, up to now, they have not. + */ + while (seg && (ASSEGMENT_LEN(seg, use32bit) <= STREAM_WRITEABLE(s))) { + struct assegment *next = seg->next; + int written = 0; + int asns_packed = 0; + size_t lenp; - /* Overlength segments have to be split up */ - while ((seg->length - written) > AS_SEGMENT_MAX) { - assegment_header_put(s, seg->type, - AS_SEGMENT_MAX); - assegment_data_put(s, (seg->as + written), AS_SEGMENT_MAX, - use32bit); - written += AS_SEGMENT_MAX; - bytes += ASSEGMENT_SIZE(AS_SEGMENT_MAX, - use32bit); - } - - /* write the final segment, probably is also the first - */ - lenp = assegment_header_put(s, seg->type, - seg->length - written); + /* Overlength segments have to be split up */ + while ((seg->length - written) > AS_SEGMENT_MAX) { + assegment_header_put(s, seg->type, AS_SEGMENT_MAX); assegment_data_put(s, (seg->as + written), - seg->length - written, use32bit); - - /* Sequence-type segments can be 'packed' together - * Case of a segment which was overlength and split up - * will be missed here, but that doesn't matter. - */ - while (next && ASSEGMENTS_PACKABLE(seg, next)) { - /* NB: We should never normally get here given - * we - * normalise aspath data when parse them. - * However, better - * safe than sorry. We potentially could call - * assegment_normalise here instead, but it's - * cheaper and - * easier to do it on the fly here rather than - * go through - * the segment list twice every time we write - * out - * aspath's. - */ - - /* Next segment's data can fit in this one */ - assegment_data_put(s, next->as, next->length, - use32bit); - - /* update the length of the segment header */ - stream_putc_at(s, lenp, - seg->length - written - + next->length); - asns_packed += next->length; - - next = next->next; - } - - bytes += ASSEGMENT_SIZE( - seg->length - written + asns_packed, use32bit); - seg = next; + AS_SEGMENT_MAX, use32bit); + written += AS_SEGMENT_MAX; + bytes += ASSEGMENT_SIZE(AS_SEGMENT_MAX, use32bit); } + + /* write the final segment, probably is also the first + */ + lenp = assegment_header_put(s, seg->type, + seg->length - written); + assegment_data_put(s, (seg->as + written), + seg->length - written, use32bit); + + /* Sequence-type segments can be 'packed' together + * Case of a segment which was overlength and split up + * will be missed here, but that doesn't matter. + */ + while (next && ASSEGMENTS_PACKABLE(seg, next)) { + /* NB: We should never normally get here given + * we + * normalise aspath data when parse them. + * However, better + * safe than sorry. We potentially could call + * assegment_normalise here instead, but it's + * cheaper and + * easier to do it on the fly here rather than + * go through + * the segment list twice every time we write + * out + * aspath's. + */ + + /* Next segment's data can fit in this one */ + assegment_data_put(s, next->as, next->length, use32bit); + + /* update the length of the segment header */ + stream_putc_at(s, lenp, + seg->length - written + next->length); + asns_packed += next->length; + + next = next->next; + } + + bytes += ASSEGMENT_SIZE(seg->length - written + asns_packed, + use32bit); + seg = next; } return bytes; } From 6198b694899a7be6563f8d6de7efeb32abc5f023 Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Fri, 11 Jun 2021 18:43:14 +0300 Subject: [PATCH 07/10] bgpd: Remove double test against rfapi which is already checked Signed-off-by: Donatas Abraitis --- bgpd/rfapi/rfapi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bgpd/rfapi/rfapi.c b/bgpd/rfapi/rfapi.c index 8c455c6ea5..f89ef7b0d2 100644 --- a/bgpd/rfapi/rfapi.c +++ b/bgpd/rfapi/rfapi.c @@ -2179,7 +2179,7 @@ int rfapi_close(void *handle) vnc_zlog_debug_verbose("%s administrative close rfd=%p", __func__, rfd); - if (h && h->rfp_methods.close_cb) { + if (h->rfp_methods.close_cb) { vnc_zlog_debug_verbose( "%s calling close callback rfd=%p", __func__, rfd); From e6b457ac06ec986f4eaaf19d67703b5d912268aa Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Fri, 11 Jun 2021 18:44:01 +0300 Subject: [PATCH 08/10] bgpd: Do not test for bgp_path_info in rfapiCopyUnEncap2VPN() Already checked. Signed-off-by: Donatas Abraitis --- bgpd/rfapi/rfapi_import.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/bgpd/rfapi/rfapi_import.c b/bgpd/rfapi/rfapi_import.c index b2732a40b4..51e051d688 100644 --- a/bgpd/rfapi/rfapi_import.c +++ b/bgpd/rfapi/rfapi_import.c @@ -2592,10 +2592,8 @@ static void rfapiCopyUnEncap2VPN(struct bgp_path_info *encap_bpi, * instrumentation to debug segfault of 091127 */ vnc_zlog_debug_verbose("%s: vpn_bpi=%p", __func__, vpn_bpi); - if (vpn_bpi) { - vnc_zlog_debug_verbose("%s: vpn_bpi->extra=%p", - __func__, vpn_bpi->extra); - } + vnc_zlog_debug_verbose("%s: vpn_bpi->extra=%p", __func__, + vpn_bpi->extra); vpn_bpi->extra->vnc.import.un_family = AF_INET; vpn_bpi->extra->vnc.import.un.addr4 = From a454d9ab448fbc850e40432d0cde18ea0bfe3439 Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Fri, 11 Jun 2021 18:58:21 +0300 Subject: [PATCH 09/10] bgpd: Drop return for void bgp_evpn_show_routes_mac_ip_es() Signed-off-by: Donatas Abraitis --- bgpd/bgp_evpn_vty.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bgpd/bgp_evpn_vty.c b/bgpd/bgp_evpn_vty.c index 192ead6fd4..2a7c2ec853 100644 --- a/bgpd/bgp_evpn_vty.c +++ b/bgpd/bgp_evpn_vty.c @@ -768,13 +768,13 @@ static void bgp_evpn_show_routes_mac_ip_es(struct vty *vty, esi_t *esi, static void bgp_evpn_show_routes_mac_ip_evi_es(struct vty *vty, esi_t *esi, json_object *json, int detail) { - return bgp_evpn_show_routes_mac_ip_es(vty, esi, json, detail, false); + bgp_evpn_show_routes_mac_ip_es(vty, esi, json, detail, false); } static void bgp_evpn_show_routes_mac_ip_global_es(struct vty *vty, esi_t *esi, json_object *json, int detail) { - return bgp_evpn_show_routes_mac_ip_es(vty, esi, json, detail, true); + bgp_evpn_show_routes_mac_ip_es(vty, esi, json, detail, true); } static void show_vni_routes(struct bgp *bgp, struct bgpevpn *vpn, int type, From aa6fe908f344ce53d143316d7f9d72b7c9f95958 Mon Sep 17 00:00:00 2001 From: Donatas Abraitis Date: Sun, 13 Jun 2021 21:44:06 +0300 Subject: [PATCH 10/10] lib: Do not double-assign freed pointer to NULL It's already done by XFREE. Signed-off-by: Donatas Abraitis --- lib/link_state.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/lib/link_state.c b/lib/link_state.c index afeb89c592..e8a6b89f89 100644 --- a/lib/link_state.c +++ b/lib/link_state.c @@ -79,7 +79,6 @@ void ls_node_del(struct ls_node *node) return; XFREE(MTYPE_LS_DB, node); - node = NULL; } int ls_node_same(struct ls_node *n1, struct ls_node *n2) @@ -168,7 +167,6 @@ void ls_attributes_del(struct ls_attributes *attr) ls_attributes_srlg_del(attr); XFREE(MTYPE_LS_DB, attr); - attr = NULL; } int ls_attributes_same(struct ls_attributes *l1, struct ls_attributes *l2) @@ -221,7 +219,6 @@ void ls_prefix_del(struct ls_prefix *pref) return; XFREE(MTYPE_LS_DB, pref); - pref = NULL; } int ls_prefix_same(struct ls_prefix *p1, struct ls_prefix *p2) @@ -839,7 +836,6 @@ void ls_ted_del(struct ls_ted *ted) subnets_fini(&ted->subnets); XFREE(MTYPE_LS_DB, ted); - ted = NULL; } void ls_ted_del_all(struct ls_ted *ted)