From 19474c9c8c6b3a76c4d6b66baeff6a485fb5d5be Mon Sep 17 00:00:00 2001 From: Stephen Worley Date: Mon, 11 Nov 2019 18:22:59 -0500 Subject: [PATCH] zebra: mpls_ftn_uninstall handle nhg hash label change We were crashing due to a missed label change code path in mpls_ftn_uninstall() with the zebra_nhg hashing code. Add a static handler function for label changing everywhere in that code and use it in mpls_ftn_uninstall(). The crash was found in the ISIS-SR tests: ==23== Thread 1: ==23== Invalid read of size 4 ==23== at 0x15B20E: zebra_nhg_hash_equal (zebra_nhg.c:365) ==23== by 0x489A2FD: hash_get (hash.c:143) ==23== by 0x489A4BC: hash_lookup (hash.c:183) ==23== by 0x15B5A3: zebra_nhg_find (zebra_nhg.c:494) ==23== by 0x15C536: zebra_nhg_rib_find (zebra_nhg.c:1070) ==23== by 0x1573E8: mpls_ftn_update (zebra_mpls.c:2661) ==23== by 0x1A2554: zread_mpls_labels_replace (zapi_msg.c:1890) ==23== by 0x1A41CD: zserv_handle_commands (zapi_msg.c:2613) ==23== by 0x199B17: zserv_process_messages (zserv.c:517) ==23== by 0x48EE6B7: thread_call (thread.c:1549) ==23== by 0x48A8AD5: frr_run (libfrr.c:1064) ==23== by 0x1391B7: main (main.c:468) ==23== Address 0x5839330 is 0 bytes inside a block of size 80 free'd ==23== at 0x48369AB: free (vg_replace_malloc.c:530) ==23== by 0x48AEE6C: qfree (memory.c:129) ==23== by 0x15C5F8: zebra_nhg_free (zebra_nhg.c:1095) ==23== by 0x15BC8C: zebra_nhg_handle_uninstall (zebra_nhg.c:734) ==23== by 0x15DCFA: zebra_nhg_uninstall_kernel (zebra_nhg.c:1826) ==23== by 0x15C666: zebra_nhg_decrement_ref (zebra_nhg.c:1106) ==23== by 0x15D9D7: zebra_nhg_re_update_ref (zebra_nhg.c:1711) ==23== by 0x15D8B1: nexthop_active_update (zebra_nhg.c:1660) ==23== by 0x167072: rib_process (zebra_rib.c:1154) ==23== by 0x168D72: process_subq_route (zebra_rib.c:2039) ==23== by 0x168E92: process_subq (zebra_rib.c:2078) ==23== by 0x168F5B: meta_queue_process (zebra_rib.c:2112) ==23== Block was alloc'd at ==23== at 0x4837B65: calloc (vg_replace_malloc.c:752) ==23== by 0x48AED56: qcalloc (memory.c:110) ==23== by 0x15B07B: zebra_nhg_copy (zebra_nhg.c:307) ==23== by 0x15B13E: zebra_nhg_hash_alloc (zebra_nhg.c:329) ==23== by 0x489A339: hash_get (hash.c:148) ==23== by 0x15B6CA: zebra_nhg_find (zebra_nhg.c:532) ==23== by 0x15C536: zebra_nhg_rib_find (zebra_nhg.c:1070) ==23== by 0x15D89A: nexthop_active_update (zebra_nhg.c:1658) ==23== by 0x167072: rib_process (zebra_rib.c:1154) ==23== by 0x168D72: process_subq_route (zebra_rib.c:2039) ==23== by 0x168E92: process_subq (zebra_rib.c:2078) ==23== by 0x168F5B: meta_queue_process (zebra_rib.c:2112) Signed-off-by: Stephen Worley --- zebra/zebra_mpls.c | 40 ++++++++++++++++++++++++++-------------- 1 file changed, 26 insertions(+), 14 deletions(-) diff --git a/zebra/zebra_mpls.c b/zebra/zebra_mpls.c index ef1bd02608..6942a37989 100644 --- a/zebra/zebra_mpls.c +++ b/zebra/zebra_mpls.c @@ -2565,6 +2565,16 @@ void zebra_mpls_print_fec(struct vty *vty, struct zebra_vrf *zvrf, fec_print(rn->info, vty); } +static void mpls_zebra_nhg_update(struct route_entry *re, afi_t afi, + struct nexthop_group *new_grp) +{ + struct nhg_hash_entry *nhe; + + nhe = zebra_nhg_rib_find(0, new_grp, afi); + + zebra_nhg_re_update_ref(re, nhe); +} + static bool mpls_ftn_update_nexthop(int add, struct nexthop *nexthop, enum lsp_types_t type, mpls_label_t label) { @@ -2591,7 +2601,6 @@ int mpls_ftn_update(int add, struct zebra_vrf *zvrf, enum lsp_types_t type, struct route_entry *re; struct nexthop *nexthop; struct nexthop_group new_grp = {}; - struct nhg_hash_entry *nhe = NULL; bool found; afi_t afi = family2afi(prefix->family); @@ -2658,12 +2667,11 @@ int mpls_ftn_update(int add, struct zebra_vrf *zvrf, enum lsp_types_t type, } if (found) { - nhe = zebra_nhg_rib_find(0, &new_grp, afi); - - zebra_nhg_re_update_ref(re, nhe); - SET_FLAG(re->status, ROUTE_ENTRY_CHANGED); SET_FLAG(re->status, ROUTE_ENTRY_LABELS_CHANGED); + + mpls_zebra_nhg_update(re, afi, &new_grp); + rib_queue_add(rn); } @@ -2680,10 +2688,11 @@ int mpls_ftn_uninstall(struct zebra_vrf *zvrf, enum lsp_types_t type, struct route_node *rn; struct route_entry *re; struct nexthop *nexthop; + struct nexthop_group new_grp = {}; + afi_t afi = family2afi(prefix->family); /* Lookup table. */ - table = zebra_vrf_table(family2afi(prefix->family), SAFI_UNICAST, - zvrf_id(zvrf)); + table = zebra_vrf_table(afi, SAFI_UNICAST, zvrf_id(zvrf)); if (!table) return -1; @@ -2698,11 +2707,18 @@ int mpls_ftn_uninstall(struct zebra_vrf *zvrf, enum lsp_types_t type, if (re == NULL) return -1; - for (nexthop = re->ng->nexthop; nexthop; nexthop = nexthop->next) + nexthop_group_copy(&new_grp, re->ng); + + for (nexthop = new_grp.nexthop; nexthop; nexthop = nexthop->next) nexthop_del_labels(nexthop); SET_FLAG(re->status, ROUTE_ENTRY_CHANGED); SET_FLAG(re->status, ROUTE_ENTRY_LABELS_CHANGED); + + mpls_zebra_nhg_update(re, afi, &new_grp); + + nexthops_free(new_grp.nexthop); + rib_queue_add(rn); return 0; @@ -2904,7 +2920,6 @@ static void mpls_ftn_uninstall_all(struct zebra_vrf *zvrf, update = 0; RNODE_FOREACH_RE (rn, re) { struct nexthop_group new_grp = {}; - struct nhg_hash_entry *nhe = NULL; nexthop_group_copy(&new_grp, re->ng); @@ -2920,11 +2935,8 @@ static void mpls_ftn_uninstall_all(struct zebra_vrf *zvrf, update = 1; } - if (CHECK_FLAG(re->status, - ROUTE_ENTRY_LABELS_CHANGED)) { - nhe = zebra_nhg_rib_find(0, &new_grp, afi); - zebra_nhg_re_update_ref(re, nhe); - } + if (CHECK_FLAG(re->status, ROUTE_ENTRY_LABELS_CHANGED)) + mpls_zebra_nhg_update(re, afi, &new_grp); nexthops_free(new_grp.nexthop); } -- 2.39.5