Fix recursion loop - recurse through cover

Change-Id: I07870122f90e41fbb216b2f426bccbfd94049cd6
Signed-off-by: Neale Ranns <nranns@cisco.com>

2 files changed, 72 insertions, 2 deletions

diff --git a/vnet/vnet/fib/fib_entry_src_rr.c b/vnet/vnet/fib/fib_entry_src_rr.c
index 9219d58e46e..6d56541dec2 100644
--- a/vnet/vnet/fib/fib_entry_src_rr.c
+++ b/vnet/vnet/fib/fib_entry_src_rr.c
@@ -17,6 +17,7 @@
 #include <vnet/ip/format.h>
 #include <vnet/ip/lookup.h>
 #include <vnet/adj/adj.h>
+#include <vnet/dpo/drop_dpo.h>
 
 #include "fib_entry_src.h"
 #include "fib_entry_cover.h"
@@ -100,7 +101,40 @@ fib_entry_src_rr_activate (fib_entry_src_t *src,
     }
     else
     {
-	src->fes_pl = cover->fe_parent;
+        /*
+         * use the path-list of the cover, unless it would form a loop.
+         * that is unless the cover is via this entry.
+         * If a loop were to form it would be a 1 level loop (i.e. X via X),
+         * and there would be 2 locks on the path-list; one since its used
+         * by the cover, and 1 from here. The first lock will go when the
+         * cover is removed, the second, and last, when the covered walk
+         * occurs during the cover's removel - this is not a place where
+         * we can handle last lock gone.
+         * In short, don't let the loop form. The usual rules of 'we must
+         * let it form so we know when it breaks' don't apply here, since
+         * the loop will break when the cover changes, and this function
+         * will be called again when that happens.
+         */
+        fib_node_index_t *entries = NULL;
+        fib_protocol_t proto;
+
+        proto = fib_entry->fe_prefix.fp_proto;
+        vec_add1(entries, fib_entry_get_index(fib_entry));
+
+        if (fib_path_list_recursive_loop_detect(cover->fe_parent,
+                                                &entries))
+        {
+            src->fes_pl = fib_path_list_create_special(
+                              proto,
+                              FIB_PATH_LIST_FLAG_DROP,
+                              drop_dpo_get(fib_proto_to_dpo(proto)));
+        }
+        else
+        {
+            src->fes_pl = cover->fe_parent;
+        }
+        vec_free(entries);
+
     }
     fib_path_list_lock(src->fes_pl);
 
diff --git a/vnet/vnet/fib/fib_test.c b/vnet/vnet/fib/fib_test.c
index 2194fcbc2bf..49fc318cbb7 100644
--- a/vnet/vnet/fib/fib_test.c
+++ b/vnet/vnet/fib/fib_test.c
@@ -2077,6 +2077,42 @@ fib_test_v4 (void)
 	     "1-level 5.5.5.6/32 loop is removed");
 
     /*
+     * A recursive route whose next-hop is covered by the prefix.
+     * This would mean the via-fib, which inherits forwarding from its
+     * cover, thus picks up forwarding from the prfix, which is via the
+     * via-fib, and we have a loop.
+     */
+    fib_prefix_t pfx_23_23_23_0_s_24 = {
+	.fp_len = 24,
+	.fp_proto = FIB_PROTOCOL_IP4,
+	.fp_addr = {
+	    .ip4.as_u32 = clib_host_to_net_u32(0x17171700),
+	},
+    };
+    fib_prefix_t pfx_23_23_23_23_s_32 = {
+	.fp_len = 32,
+	.fp_proto = FIB_PROTOCOL_IP4,
+	.fp_addr = {
+            .ip4.as_u32 = clib_host_to_net_u32(0x17171717),
+        },
+    };
+    fei = fib_table_entry_path_add(fib_index,
+				   &pfx_23_23_23_0_s_24,
+				   FIB_SOURCE_API,
+				   FIB_ENTRY_FLAG_NONE,
+				   FIB_PROTOCOL_IP4,
+				   &pfx_23_23_23_23_s_32.fp_addr,
+				   ~0, // recursive
+				   fib_index,
+				   1,
+				   MPLS_LABEL_INVALID,
+				   FIB_ROUTE_PATH_FLAG_NONE);
+    dpo = fib_entry_contribute_ip_forwarding(fei);
+    FIB_TEST(load_balance_is_drop(dpo),
+	     "23.23.23.0/24 via covered is DROP");
+    fib_table_entry_delete_index(fei, FIB_SOURCE_API);
+
+    /*
      * add-remove test. no change.
      */
     FIB_TEST((1  == fib_path_list_db_size()),   "path list DB population:%d",
@@ -2800,7 +2836,7 @@ fib_test_v4 (void)
 
     FIB_TEST((0 == adj_nbr_db_size()), "ADJ DB size is %d",
 	     adj_nbr_db_size());
-    
+
     /*
      * CLEANUP
      *   remove the interface prefixes