zebra: fix refcnt/rib issues in NHG replace/delete

Fix some reference counting issues seen when replacing
a NHG and deleting one.

For replacement, we should end with the same refcnt on the new
one.

For delete, its the caller's job to decrement its ref after
its done with it.

Further, update routes in the rib with the new pointer after replace.

Signed-off-by: Stephen Worley <sworley@cumulusnetworks.com>

diff --git a/zebra/rib.h b/zebra/rib.h
index be680a112f99e98cf4053d6be9cb0f29e6649ee3..9ddc35b09198623073b2ef623928e5cf33c74af5 100644 (file)
--- a/zebra/rib.h
+++ b/zebra/rib.h
@@ -333,6 +333,10 @@ extern void route_entry_copy_nexthops(struct route_entry *re,
 int route_entry_update_nhe(struct route_entry *re,
                           struct nhg_hash_entry *new_nhghe);
 
+/* NHG replace has happend, we have to update route_entry pointers to new one */
+void rib_handle_nhg_replace(struct nhg_hash_entry *old,
+                           struct nhg_hash_entry *new);
+
 #define route_entry_dump(prefix, src, re) _route_entry_dump(__func__, prefix, src, re)
 extern void _route_entry_dump(const char *func, union prefixconstptr pp,
                              union prefixconstptr src_pp,

diff --git a/zebra/zapi_msg.c b/zebra/zapi_msg.c
index 4cd733ea3575714ceaa7138f3d5d2f0949611ad3..c8220af81ecfbe25b8f5ebf0a055cd815d58ab76 100644 (file)
--- a/zebra/zapi_msg.c
+++ b/zebra/zapi_msg.c
@@ -1825,8 +1825,6 @@ static void zread_nhg_reader(ZAPI_HANDLER_ARGS)
         * Resolution is going to need some more work.
         */
        if (nhe) {
-               zebra_nhg_increment_ref(nhe);
-               zebra_nhg_install_kernel(nhe);
                nhg_notify(proto, client->instance, id, ZAPI_NHG_INSTALLED);
        } else
                nhg_notify(proto, client->instance, id, ZAPI_NHG_FAIL_INSTALL);

diff --git a/zebra/zebra_nhg.c b/zebra/zebra_nhg.c
index 407af2902d0a2ea8f5c27f83a3fbd101c0ae44d3..1c9b9700088cb38b860078585c0bf9c44d8c1015 100644 (file)
--- a/zebra/zebra_nhg.c
+++ b/zebra/zebra_nhg.c
@@ -2741,6 +2741,26 @@ struct nhg_hash_entry *zebra_nhg_proto_add(uint32_t id, int type,
        struct nhg_hash_entry lookup;
        struct nhg_hash_entry *new, *old;
        struct nhg_connected *rb_node_dep = NULL;
+       struct nexthop *newhop;
+       bool replace = false;
+
+       if (!nhg->nexthop) {
+               if (IS_ZEBRA_DEBUG_NHG)
+                       zlog_debug("%s: id %u, no nexthops passed to add",
+                                  __func__, id);
+               return NULL;
+       }
+
+
+       /* Set nexthop list as active, since they wont go through rib
+        * processing.
+        *
+        * Assuming valid/onlink for now.
+        *
+        * Once resolution is figured out, we won't need this!
+        */
+       for (ALL_NEXTHOPS_PTR(nhg, newhop))
+               SET_FLAG(newhop->flags, NEXTHOP_FLAG_ACTIVE);
 
        zebra_nhe_init(&lookup, afi, nhg->nexthop);
        lookup.nhg.nexthop = nhg->nexthop;
@@ -2754,36 +2774,51 @@ struct nhg_hash_entry *zebra_nhg_proto_add(uint32_t id, int type,
                 * This is a replace, just release NHE from ID for now, The
                 * depends/dependents may still be used in the replacement.
                 */
+               replace = true;
                hash_release(zrouter.nhgs_id, old);
        }
 
        new = zebra_nhg_rib_find_nhe(&lookup, afi);
 
+       zebra_nhg_increment_ref(new);
+
+       zebra_nhg_set_valid_if_active(new);
+
+       zebra_nhg_install_kernel(new);
+
        if (old) {
-               /* Now release depends/dependents in old one */
-               zebra_nhg_release_all_deps(old);
-       }
+               rib_handle_nhg_replace(old, new);
 
-       if (!new)
-               return NULL;
+               /* if this != 1 at this point, we have a bug */
+               assert(old->refcnt == 1);
 
-       /* TODO: Assuming valid/onlink for now */
-       SET_FLAG(new->flags, NEXTHOP_GROUP_VALID);
+               /* We have to decrement its singletons
+                * because some might not exist in NEW.
+                */
+               if (!zebra_nhg_depends_is_empty(old)) {
+                       frr_each (nhg_connected_tree, &old->nhg_depends,
+                                 rb_node_dep)
+                               zebra_nhg_decrement_ref(rb_node_dep->nhe);
+               }
+
+               /* Free all the things */
+               zebra_nhg_release_all_deps(old);
 
-       if (!zebra_nhg_depends_is_empty(new)) {
-               frr_each (nhg_connected_tree, &new->nhg_depends, rb_node_dep)
-                       SET_FLAG(rb_node_dep->nhe->flags, NEXTHOP_GROUP_VALID);
+               /* Dont call the dec API, we dont want to uninstall the ID */
+               old->refcnt = 0;
+               zebra_nhg_free(old);
+               old = NULL;
        }
 
        if (IS_ZEBRA_DEBUG_NHG_DETAIL)
                zlog_debug("%s: %s nhe %p (%u), vrf %d, type %s", __func__,
-                          (old ? "replaced" : "added"), new, new->id,
+                          (replace ? "replaced" : "added"), new, new->id,
                           new->vrf_id, zebra_route_string(new->type));
 
        return new;
 }
 
-/* Delete NHE from upper level proto */
+/* Delete NHE from upper level proto, caller must decrement ref */
 struct nhg_hash_entry *zebra_nhg_proto_del(uint32_t id)
 {
        struct nhg_hash_entry *nhe;
@@ -2797,11 +2832,12 @@ struct nhg_hash_entry *zebra_nhg_proto_del(uint32_t id)
                return NULL;
        }
 
-       if (nhe->refcnt) {
+       if (nhe->refcnt > 1) {
                /* TODO: should be warn? */
                if (IS_ZEBRA_DEBUG_NHG)
-                       zlog_debug("%s: id %u, still being used refcnt %u",
-                                  __func__, nhe->id, nhe->refcnt);
+                       zlog_debug(
+                               "%s: id %u, still being used by routes refcnt %u",
+                               __func__, nhe->id, nhe->refcnt);
                return NULL;
        }
 

diff --git a/zebra/zebra_nhg.h b/zebra/zebra_nhg.h
index 8c33a919177e6482610545dd41c402d4aee757bf..6c9f20d0a4e66a98511721678710052d205fee21 100644 (file)
--- a/zebra/zebra_nhg.h
+++ b/zebra/zebra_nhg.h
@@ -281,7 +281,7 @@ struct nhg_hash_entry *zebra_nhg_proto_add(uint32_t id, int type,
  *
  * Returns deleted NHE on success, otherwise NULL.
  *
- * Caller must free the NHE.
+ * Caller must decrement ref with zebra_nhg_decrement_ref() when done.
  */
 struct nhg_hash_entry *zebra_nhg_proto_del(uint32_t id);
 

diff --git a/zebra/zebra_rib.c b/zebra/zebra_rib.c
index 5a7967d8173d32dfa1bf4a7070c201d75ce0c45d..aac0e628fe12fe62b65ce7cdd0962f0ff2d413ff 100644 (file)
--- a/zebra/zebra_rib.c
+++ b/zebra/zebra_rib.c
@@ -235,7 +235,7 @@ int route_entry_update_nhe(struct route_entry *re,
                goto done;
        }
 
-       if ((re->nhe_id != 0) && (re->nhe_id != new_nhghe->id)) {
+       if ((re->nhe_id != 0) && re->nhe && (re->nhe != new_nhghe)) {
                old = re->nhe;
 
                route_entry_attach_ref(re, new_nhghe);
@@ -250,6 +250,29 @@ done:
        return ret;
 }
 
+void rib_handle_nhg_replace(struct nhg_hash_entry *old,
+                           struct nhg_hash_entry *new)
+{
+       struct zebra_router_table *zrt;
+       struct route_node *rn;
+       struct route_entry *re, *next;
+
+       if (IS_ZEBRA_DEBUG_RIB_DETAILED || IS_ZEBRA_DEBUG_NHG_DETAIL)
+               zlog_debug("%s: replacing routes nhe (%u) OLD %p NEW %p",
+                          __func__, new->id, new, old);
+
+       /* We have to do them ALL */
+       RB_FOREACH (zrt, zebra_router_table_head, &zrouter.tables) {
+               for (rn = route_top(zrt->table); rn;
+                    rn = srcdest_route_next(rn)) {
+                       RNODE_FOREACH_RE_SAFE (rn, re, next) {
+                               if (re->nhe && re->nhe == old)
+                                       route_entry_update_nhe(re, new);
+                       }
+               }
+       }
+}
+
 struct route_entry *rib_match(afi_t afi, safi_t safi, vrf_id_t vrf_id,
                              union g_addr *addr, struct route_node **rn_out)
 {