From a2d0055aac3725eb253f3c07f33624a8a3eb15ca Mon Sep 17 00:00:00 2001 From: Chirag Shah Date: Mon, 7 May 2018 10:16:08 -0700 Subject: [PATCH] ospf6d: Fix ABR brouter calculation corruption During Intra brouter calculation, brouters will be marked for remove. if one of the brouter is removed, as part of its remove callback, ospf6_abr_examin_summary is performed where marked for brouter would be removed. Since refcount of next brouter node still higher, it will retain one node with dangled next brouter pointer. When intra brouter calculation iteration goes to next node, where accessing free node causes a crash. Ticket:CM-20807 Testing Done: Configure multilple ABR routers between area 0 and area x, y. Remove ospf6 configuration on area x, y abrs and check area 0 Intra brouter calculations. Signed-off-by: Chirag Shah --- ospf6d/ospf6_abr.c | 24 ++++++++++++++++++++---- ospf6d/ospf6_area.h | 3 +++ ospf6d/ospf6_intra.c | 33 ++++++++++++++++++++++++--------- ospf6d/ospf6_route.c | 5 +++-- 4 files changed, 50 insertions(+), 15 deletions(-) diff --git a/ospf6d/ospf6_abr.c b/ospf6d/ospf6_abr.c index ba02457b4f..bc1ce621ae 100644 --- a/ospf6d/ospf6_abr.c +++ b/ospf6d/ospf6_abr.c @@ -770,7 +770,8 @@ void ospf6_abr_examin_summary(struct ospf6_lsa *lsa, struct ospf6_area *oa) } if (OSPF6_LSA_IS_MAXAGE(lsa)) { if (is_debug) - zlog_debug("LSA is MaxAge, ignore"); + zlog_debug("%s: LSA %s is MaxAge, ignore", + __PRETTY_FUNCTION__, lsa->name); if (old) ospf6_route_remove(old, table); return; @@ -847,9 +848,24 @@ void ospf6_abr_examin_summary(struct ospf6_lsa *lsa, struct ospf6_area *oa) || CHECK_FLAG(abr_entry->flag, OSPF6_ROUTE_REMOVE) || !CHECK_FLAG(abr_entry->path.router_bits, OSPF6_ROUTER_BIT_B)) { if (is_debug) - zlog_debug("ABR router entry does not exist, ignore"); - if (old) - ospf6_route_remove(old, table); + zlog_debug("%s: ABR router entry does not exist, ignore", + __PRETTY_FUNCTION__); + if (old) { + if (old->type == OSPF6_DEST_TYPE_ROUTER && + oa->intra_brouter_calc) { + if (is_debug) + zlog_debug( + "%s: intra_brouter_calc is on, skip brouter remove: %s (%p)", + __PRETTY_FUNCTION__, buf, + (void *)old); + } else { + if (is_debug) + zlog_debug("%s: remove old entry: %s %p ", + __PRETTY_FUNCTION__, buf, + (void *)old); + ospf6_route_remove(old, table); + } + } return; } diff --git a/ospf6d/ospf6_area.h b/ospf6d/ospf6_area.h index eaf3e5c6de..ba497a168e 100644 --- a/ospf6d/ospf6_area.h +++ b/ospf6d/ospf6_area.h @@ -50,6 +50,9 @@ struct ospf6_area { /* Area type */ int no_summary; + /* Brouter traversal protection */ + int intra_brouter_calc; + /* OSPF interface list */ struct list *if_list; diff --git a/ospf6d/ospf6_intra.c b/ospf6d/ospf6_intra.c index b234b10d51..26e6deadae 100644 --- a/ospf6d/ospf6_intra.c +++ b/ospf6d/ospf6_intra.c @@ -2047,8 +2047,10 @@ void ospf6_intra_brouter_calculation(struct ospf6_area *oa) uint32_t brouter_id; char brouter_name[16]; - if (IS_OSPF6_DEBUG_BROUTER_SPECIFIC_AREA_ID(oa->area_id)) - zlog_info("border-router calculation for area %s", oa->name); + if (IS_OSPF6_DEBUG_BROUTER_SPECIFIC_AREA_ID(oa->area_id) || + IS_OSPF6_DEBUG_ROUTE(MEMORY)) + zlog_info("%s: border-router calculation for area %s", + __PRETTY_FUNCTION__, oa->name); hook_add = oa->ospf6->brouter_table->hook_add; hook_remove = oa->ospf6->brouter_table->hook_remove; @@ -2114,6 +2116,7 @@ void ospf6_intra_brouter_calculation(struct ospf6_area *oa) for (brouter = ospf6_route_head(oa->ospf6->brouter_table); brouter; brouter = nbrouter) { + /* * brouter may have been "deleted" in the last loop iteration. * If this is the case there is still 1 final refcount lock @@ -2122,6 +2125,8 @@ void ospf6_intra_brouter_calculation(struct ospf6_area *oa) * skip processing the deleted route. */ if (brouter->lock == 1) { + if (IS_OSPF6_DEBUG_ROUTE(MEMORY)) + ospf6_brouter_debug_print(brouter); nbrouter = ospf6_route_next(brouter); continue; } else { @@ -2173,8 +2178,14 @@ void ospf6_intra_brouter_calculation(struct ospf6_area *oa) brouter_id) || IS_OSPF6_DEBUG_BROUTER_SPECIFIC_AREA_ID( oa->area_id)) - zlog_info("brouter %s disappears via area %s", - brouter_name, oa->name); + zlog_info("%s: brouter %s disappears via area %s", + __PRETTY_FUNCTION__, brouter_name, + oa->name); + /* This is used to protect nbrouter from removed from + * the table. For an example, ospf6_abr_examin_summary, + * removes brouters which are marked for remove. + */ + oa->intra_brouter_calc = 1; ospf6_route_remove(brouter, oa->ospf6->brouter_table); brouter = NULL; } else if (CHECK_FLAG(brouter->flag, OSPF6_ROUTE_ADD) @@ -2184,8 +2195,9 @@ void ospf6_intra_brouter_calculation(struct ospf6_area *oa) brouter_id) || IS_OSPF6_DEBUG_BROUTER_SPECIFIC_AREA_ID( oa->area_id)) - zlog_info("brouter %s appears via area %s", - brouter_name, oa->name); + zlog_info("%s: brouter %s appears via area %s", + __PRETTY_FUNCTION__, brouter_name, + oa->name); /* newly added */ if (hook_add) @@ -2205,11 +2217,14 @@ void ospf6_intra_brouter_calculation(struct ospf6_area *oa) UNSET_FLAG(brouter->flag, OSPF6_ROUTE_ADD); UNSET_FLAG(brouter->flag, OSPF6_ROUTE_CHANGE); } + /* Reset for nbrouter */ + oa->intra_brouter_calc = 0; } - if (IS_OSPF6_DEBUG_BROUTER_SPECIFIC_AREA_ID(oa->area_id)) - zlog_info("border-router calculation for area %s: done", - oa->name); + if (IS_OSPF6_DEBUG_BROUTER_SPECIFIC_AREA_ID(oa->area_id) || + IS_OSPF6_DEBUG_ROUTE(MEMORY)) + zlog_info("%s: border-router calculation for area %s: done", + __PRETTY_FUNCTION__, oa->name); } struct ospf6_lsa_handler router_handler = {.lh_type = OSPF6_LSTYPE_ROUTER, diff --git a/ospf6d/ospf6_route.c b/ospf6d/ospf6_route.c index 39272b3701..15d8eb6cf2 100644 --- a/ospf6d/ospf6_route.c +++ b/ospf6d/ospf6_route.c @@ -925,10 +925,11 @@ struct ospf6_route *ospf6_route_next(struct ospf6_route *route) struct ospf6_route *next = route->next; if (IS_OSPF6_DEBUG_ROUTE(MEMORY)) - zlog_info("%s %p: route next: %p<-[%p]->%p", + zlog_info("%s %p: route next: %p<-[%p]->%p , route ref count %u", ospf6_route_table_name(route->table), (void *)route->table, (void *)route->prev, - (void *)route, (void *)route->next); + (void *)route, (void *)route->next, + route->lock); ospf6_route_unlock(route); if (next) -- 2.39.5