From 1fa6385040df57f2d175cd628d7775af2f6c7561 Mon Sep 17 00:00:00 2001 From: Olivier Dugeon Date: Fri, 5 Aug 2022 16:00:48 +0200 Subject: [PATCH] isisd: Correct Valgrind errors Runing most of isisd tests with --valgrind-memleaks give many memory errors. This is due to the way isisd is stopped: performing a "no router isis XXX" through CLI solves most of them. Indeed, isis_finish() doesn't call isis_area_destroy() leaving many allocated memory unfreed. This patch adds call to appropriate delete function or XFREE() when necessary to properly free all alocated memory before terminating isisd. Signed-off-by: Olivier Dugeon --- isisd/isis_circuit.c | 5 ++ isisd/isis_nb_config.c | 69 +++++---------------------- isisd/isis_sr.c | 3 ++ isisd/isis_te.c | 105 +++++++++++++++++++++++++++++++++++++++++ isisd/isis_te.h | 3 ++ isisd/isis_tlvs.c | 37 ++++++++++----- isisd/isis_tlvs.h | 1 + isisd/isisd.c | 35 ++++++++------ 8 files changed, 174 insertions(+), 84 deletions(-) diff --git a/isisd/isis_circuit.c b/isisd/isis_circuit.c index 178fbe7903..dcc4ed6e42 100644 --- a/isisd/isis_circuit.c +++ b/isisd/isis_circuit.c @@ -217,6 +217,11 @@ void isis_circuit_del(struct isis_circuit *circuit) list_delete(&circuit->ipv6_link); list_delete(&circuit->ipv6_non_link); + if (circuit->ext) { + isis_del_ext_subtlvs(circuit->ext); + circuit->ext = NULL; + } + XFREE(MTYPE_TMP, circuit->bfd_config.profile); XFREE(MTYPE_ISIS_CIRCUIT, circuit->tag); diff --git a/isisd/isis_nb_config.c b/isisd/isis_nb_config.c index 79b167718b..dbe4a017bc 100644 --- a/isisd/isis_nb_config.c +++ b/isisd/isis_nb_config.c @@ -54,7 +54,6 @@ #include "isisd/isis_dr.h" #include "isisd/isis_zebra.h" -DEFINE_MTYPE_STATIC(ISISD, ISIS_MPLS_TE, "ISIS MPLS_TE parameters"); DEFINE_MTYPE_STATIC(ISISD, ISIS_PLIST_NAME, "ISIS prefix-list name"); /* @@ -86,12 +85,17 @@ int isis_instance_create(struct nb_cb_create_args *args) int isis_instance_destroy(struct nb_cb_destroy_args *args) { struct isis_area *area; + struct isis *isis; if (args->event != NB_EV_APPLY) return NB_OK; area = nb_running_unset_entry(args->dnode); - + isis = area->isis; isis_area_destroy(area); + + if (listcount(isis->area_list) == 0) + isis_finish(isis); + return NB_OK; } @@ -1787,45 +1791,13 @@ int isis_instance_log_adjacency_changes_modify(struct nb_cb_modify_args *args) */ int isis_instance_mpls_te_create(struct nb_cb_create_args *args) { - struct listnode *node; struct isis_area *area; - struct isis_circuit *circuit; if (args->event != NB_EV_APPLY) return NB_OK; area = nb_running_get_entry(args->dnode, NULL, true); - if (area->mta == NULL) { - - struct mpls_te_area *new; - - zlog_debug("ISIS-TE(%s): Initialize MPLS Traffic Engineering", - area->area_tag); - - new = XCALLOC(MTYPE_ISIS_MPLS_TE, sizeof(struct mpls_te_area)); - - /* Initialize MPLS_TE structure */ - new->status = enable; - new->level = 0; - new->inter_as = off; - new->interas_areaid.s_addr = 0; - new->router_id.s_addr = 0; - new->ted = ls_ted_new(1, "ISIS", 0); - if (!new->ted) - zlog_warn("Unable to create Link State Data Base"); - - area->mta = new; - } else { - area->mta->status = enable; - } - - /* Initialize Link State Database */ - if (area->mta->ted) - isis_te_init_ted(area); - - /* Update Extended TLVs according to Interface link parameters */ - for (ALL_LIST_ELEMENTS_RO(area->circuit_list, node, circuit)) - isis_link_params_update(circuit, circuit->interface); + isis_mpls_te_create(area); /* Reoriginate STD_TE & GMPLS circuits */ lsp_regenerate_schedule(area, area->is_type, 0); @@ -1835,35 +1807,16 @@ int isis_instance_mpls_te_create(struct nb_cb_create_args *args) int isis_instance_mpls_te_destroy(struct nb_cb_destroy_args *args) { - struct listnode *node; struct isis_area *area; - struct isis_circuit *circuit; if (args->event != NB_EV_APPLY) return NB_OK; area = nb_running_get_entry(args->dnode, NULL, true); - if (IS_MPLS_TE(area->mta)) - area->mta->status = disable; - else - return NB_OK; - - /* Remove Link State Database */ - ls_ted_del_all(&area->mta->ted); - - /* Flush LSP if circuit engage */ - for (ALL_LIST_ELEMENTS_RO(area->circuit_list, node, circuit)) { - if (!IS_EXT_TE(circuit->ext)) - continue; - - /* disable MPLS_TE Circuit keeping SR one's */ - if (IS_SUBTLV(circuit->ext, EXT_ADJ_SID)) - circuit->ext->status = EXT_ADJ_SID; - else if (IS_SUBTLV(circuit->ext, EXT_LAN_ADJ_SID)) - circuit->ext->status = EXT_LAN_ADJ_SID; - else - circuit->ext->status = 0; - } + if (!IS_MPLS_TE(area->mta)) + return NB_OK; + + isis_mpls_te_disable(area); /* Reoriginate STD_TE & GMPLS circuits */ lsp_regenerate_schedule(area, area->is_type, 0); diff --git a/isisd/isis_sr.c b/isisd/isis_sr.c index 259047ff66..f70840a637 100644 --- a/isisd/isis_sr.c +++ b/isisd/isis_sr.c @@ -1253,6 +1253,9 @@ void isis_sr_area_term(struct isis_area *area) if (area->srdb.enabled) isis_sr_stop(area); + /* Free Adjacency SID list */ + list_delete(&srdb->adj_sids); + /* Clear Prefix-SID configuration. */ while (srdb_prefix_cfg_count(&srdb->config.prefix_sids) > 0) { struct sr_prefix_cfg *pcfg; diff --git a/isisd/isis_te.c b/isisd/isis_te.c index 3faff1cc4d..0093279cde 100644 --- a/isisd/isis_te.c +++ b/isisd/isis_te.c @@ -64,10 +64,115 @@ #include "isisd/isis_te.h" #include "isisd/isis_zebra.h" +DEFINE_MTYPE_STATIC(ISISD, ISIS_MPLS_TE, "ISIS MPLS_TE parameters"); + /*------------------------------------------------------------------------* * Following are control functions for MPLS-TE parameters management. *------------------------------------------------------------------------*/ +/** + * Create MPLS Traffic Engineering structure which belongs to given area. + * + * @param area IS-IS Area + */ +void isis_mpls_te_create(struct isis_area *area) +{ + struct listnode *node; + struct isis_circuit *circuit; + + if (!area) + return; + + if (area->mta == NULL) { + + struct mpls_te_area *new; + + zlog_debug("ISIS-TE(%s): Initialize MPLS Traffic Engineering", + area->area_tag); + + new = XCALLOC(MTYPE_ISIS_MPLS_TE, sizeof(struct mpls_te_area)); + + /* Initialize MPLS_TE structure */ + new->status = enable; + new->level = 0; + new->inter_as = off; + new->interas_areaid.s_addr = 0; + new->router_id.s_addr = 0; + new->ted = ls_ted_new(1, "ISIS", 0); + if (!new->ted) + zlog_warn("Unable to create Link State Data Base"); + + area->mta = new; + } else { + area->mta->status = enable; + } + + /* Initialize Link State Database */ + if (area->mta->ted) + isis_te_init_ted(area); + + /* Update Extended TLVs according to Interface link parameters */ + for (ALL_LIST_ELEMENTS_RO(area->circuit_list, node, circuit)) + isis_link_params_update(circuit, circuit->interface); +} + +/** + * Disable MPLS Traffic Engineering structure which belongs to given area. + * + * @param area IS-IS Area + */ +void isis_mpls_te_disable(struct isis_area *area) +{ + struct listnode *node; + struct isis_circuit *circuit; + + if (!area->mta) + return; + + area->mta->status = disable; + + /* Remove Link State Database */ + ls_ted_del_all(&area->mta->ted); + + /* Disable Extended SubTLVs on all circuit */ + for (ALL_LIST_ELEMENTS_RO(area->circuit_list, node, circuit)) { + if (!IS_EXT_TE(circuit->ext)) + continue; + + /* disable MPLS_TE Circuit keeping SR one's */ + if (IS_SUBTLV(circuit->ext, EXT_ADJ_SID)) + circuit->ext->status = EXT_ADJ_SID; + else if (IS_SUBTLV(circuit->ext, EXT_LAN_ADJ_SID)) + circuit->ext->status = EXT_LAN_ADJ_SID; + else + circuit->ext->status = 0; + } +} + +void isis_mpls_te_term(struct isis_area *area) +{ + struct listnode *node; + struct isis_circuit *circuit; + + if (!area->mta) + return; + + zlog_info("TE(%s): Terminate MPLS TE", __func__); + /* Remove Link State Database */ + ls_ted_del_all(&area->mta->ted); + + /* Remove Extended SubTLVs */ + zlog_info(" |- Remove Extended SubTLVS for all circuit"); + for (ALL_LIST_ELEMENTS_RO(area->circuit_list, node, circuit)) { + zlog_info(" |- Call isis_del_ext_subtlvs()"); + isis_del_ext_subtlvs(circuit->ext); + circuit->ext = NULL; + } + + zlog_info(" |- Free MTA structure at %p", area->mta); + XFREE(MTYPE_ISIS_MPLS_TE, area->mta); +} + /* Main initialization / update function of the MPLS TE Circuit context */ /* Call when interface TE Link parameters are modified */ void isis_link_params_update(struct isis_circuit *circuit, diff --git a/isisd/isis_te.h b/isisd/isis_te.h index 56954073dd..03525962f5 100644 --- a/isisd/isis_te.h +++ b/isisd/isis_te.h @@ -123,6 +123,9 @@ enum lsp_event { LSP_UNKNOWN, LSP_ADD, LSP_UPD, LSP_DEL, LSP_INC, LSP_TICK }; /* Prototypes. */ void isis_mpls_te_init(void); +void isis_mpls_te_create(struct isis_area *area); +void isis_mpls_te_disable(struct isis_area *area); +void isis_mpls_te_term(struct isis_area *area); void isis_link_params_update(struct isis_circuit *, struct interface *); int isis_mpls_te_update(struct interface *); void isis_te_lsp_event(struct isis_lsp *lsp, enum lsp_event event); diff --git a/isisd/isis_tlvs.c b/isisd/isis_tlvs.c index b3c3fd4b0b..8907fa256b 100644 --- a/isisd/isis_tlvs.c +++ b/isisd/isis_tlvs.c @@ -139,6 +139,25 @@ struct isis_ext_subtlvs *isis_alloc_ext_subtlvs(void) return ext; } +void isis_del_ext_subtlvs(struct isis_ext_subtlvs *ext) +{ + struct isis_item *item, *next_item; + + if (!ext) + return; + + /* First, free Adj SID and LAN Adj SID list if needed */ + for (item = ext->adj_sid.head; item; item = next_item) { + next_item = item->next; + XFREE(MTYPE_ISIS_SUBTLV, item); + } + for (item = ext->lan_sid.head; item; item = next_item) { + next_item = item->next; + XFREE(MTYPE_ISIS_SUBTLV, item); + } + XFREE(MTYPE_ISIS_SUBTLV, ext); +} + /* * mtid parameter is used to determine if Adjacency is related to IPv4 or IPv6 * Multi-Topology. Special 4096 value i.e. first R flag set is used to indicate @@ -648,18 +667,7 @@ static void format_item_ext_subtlvs(struct isis_ext_subtlvs *exts, static void free_item_ext_subtlvs(struct isis_ext_subtlvs *exts) { - struct isis_item *item, *next_item; - - /* First, free Adj SID and LAN Adj SID list if needed */ - for (item = exts->adj_sid.head; item; item = next_item) { - next_item = item->next; - XFREE(MTYPE_ISIS_SUBTLV, item); - } - for (item = exts->lan_sid.head; item; item = next_item) { - next_item = item->next; - XFREE(MTYPE_ISIS_SUBTLV, item); - } - XFREE(MTYPE_ISIS_SUBTLV, exts); + isis_del_ext_subtlvs(exts); } static int pack_item_ext_subtlvs(struct isis_ext_subtlvs *exts, @@ -1059,6 +1067,7 @@ static int unpack_item_ext_subtlvs(uint16_t mtid, uint8_t len, struct stream *s, log, indent, "TLV size does not match expected size for Adjacency SID!\n"); stream_forward_getp(s, subtlv_len - 2); + XFREE(MTYPE_ISIS_SUBTLV, adj); break; } @@ -1070,6 +1079,7 @@ static int unpack_item_ext_subtlvs(uint16_t mtid, uint8_t len, struct stream *s, log, indent, "TLV size does not match expected size for Adjacency SID!\n"); stream_forward_getp(s, subtlv_len - 2); + XFREE(MTYPE_ISIS_SUBTLV, adj); break; } @@ -1114,6 +1124,7 @@ static int unpack_item_ext_subtlvs(uint16_t mtid, uint8_t len, struct stream *s, stream_forward_getp( s, subtlv_len - 2 - ISIS_SYS_ID_LEN); + XFREE(MTYPE_ISIS_SUBTLV, lan); break; } @@ -1127,6 +1138,7 @@ static int unpack_item_ext_subtlvs(uint16_t mtid, uint8_t len, struct stream *s, stream_forward_getp( s, subtlv_len - 2 - ISIS_SYS_ID_LEN); + XFREE(MTYPE_ISIS_SUBTLV, lan); break; } @@ -1892,6 +1904,7 @@ static void format_item_extended_reach(uint16_t mtid, struct isis_item *i, static void free_item_extended_reach(struct isis_item *i) { struct isis_extended_reach *item = (struct isis_extended_reach *)i; + if (item->subtlvs != NULL) free_item_ext_subtlvs(item->subtlvs); XFREE(MTYPE_ISIS_TLV, item); diff --git a/isisd/isis_tlvs.h b/isisd/isis_tlvs.h index 157450dc6c..905032bda1 100644 --- a/isisd/isis_tlvs.h +++ b/isisd/isis_tlvs.h @@ -597,6 +597,7 @@ void isis_tlvs_add_ipv6_dstsrc_reach(struct isis_tlvs *tlvs, uint16_t mtid, struct prefix_ipv6 *src, uint32_t metric); struct isis_ext_subtlvs *isis_alloc_ext_subtlvs(void); +void isis_del_ext_subtlvs(struct isis_ext_subtlvs *ext); void isis_tlvs_add_adj_sid(struct isis_ext_subtlvs *exts, struct isis_adj_sid *adj); void isis_tlvs_del_adj_sid(struct isis_ext_subtlvs *exts, diff --git a/isisd/isisd.c b/isisd/isisd.c index 3fd2476ad1..bce3dbb77b 100644 --- a/isisd/isisd.c +++ b/isisd/isisd.c @@ -224,6 +224,12 @@ struct isis *isis_new(const char *vrf_name) void isis_finish(struct isis *isis) { + struct isis_area *area; + struct listnode *node, *nnode; + + for (ALL_LIST_ELEMENTS(isis->area_list, node, nnode, area)) + isis_area_destroy(area); + struct vrf *vrf = NULL; listnode_delete(im->isis, isis); @@ -273,6 +279,13 @@ void isis_area_del_circuit(struct isis_area *area, struct isis_circuit *circuit) isis_csm_state_change(ISIS_DISABLE, circuit, area); } +static void delete_area_addr(void *arg) +{ + struct area_addr *addr = (struct area_addr *)arg; + + XFREE(MTYPE_ISIS_AREA_ADDR, addr); +} + struct isis_area *isis_area_create(const char *area_tag, const char *vrf_name) { struct isis_area *area; @@ -318,6 +331,8 @@ struct isis_area *isis_area_create(const char *area_tag, const char *vrf_name) area->circuit_list = list_new(); area->adjacency_list = list_new(); area->area_addrs = list_new(); + area->area_addrs->del = delete_area_addr; + if (!CHECK_FLAG(im->options, F_ISIS_UNIT_TEST)) thread_add_timer(master, lsp_tick, area, 1, &area->t_tick); flags_initialize(&area->flags); @@ -481,17 +496,12 @@ void isis_area_destroy(struct isis_area *area) { struct listnode *node, *nnode; struct isis_circuit *circuit; - struct area_addr *addr; QOBJ_UNREG(area); if (fabricd) fabricd_finish(area->fabricd); - /* Disable MPLS if necessary before flooding LSP */ - if (IS_MPLS_TE(area->mta)) - area->mta->status = disable; - if (area->circuit_list) { for (ALL_LIST_ELEMENTS(area->circuit_list, node, nnode, circuit)) @@ -499,6 +509,9 @@ void isis_area_destroy(struct isis_area *area) list_delete(&area->circuit_list); } + if (area->flags.free_idcs) + list_delete(&area->flags.free_idcs); + list_delete(&area->adjacency_list); lsp_db_fini(&area->lspdb[0]); @@ -510,6 +523,8 @@ void isis_area_destroy(struct isis_area *area) isis_sr_area_term(area); + isis_mpls_te_term(area); + spftree_area_del(area); if (area->spf_timer[0]) @@ -525,11 +540,7 @@ void isis_area_destroy(struct isis_area *area) if (!CHECK_FLAG(im->options, F_ISIS_UNIT_TEST)) isis_redist_area_finish(area); - for (ALL_LIST_ELEMENTS(area->area_addrs, node, nnode, addr)) { - list_delete_node(area->area_addrs, node); - XFREE(MTYPE_ISIS_AREA_ADDR, addr); - } - area->area_addrs = NULL; + list_delete(&area->area_addrs); for (int i = SPF_PREFIX_PRIO_CRITICAL; i <= SPF_PREFIX_PRIO_MEDIUM; i++) { @@ -554,10 +565,6 @@ void isis_area_destroy(struct isis_area *area) area_mt_finish(area); - if (listcount(area->isis->area_list) == 0) { - isis_finish(area->isis); - } - XFREE(MTYPE_ISIS_AREA, area); } -- 2.39.5