ip: unlock_fib on if delete 57/33557/7
authorNathan Skrzypczak <nathan.skrzypczak@gmail.com>
Fri, 20 Aug 2021 13:53:43 +0000 (15:53 +0200)
committerNeale Ranns <neale@graphiant.com>
Tue, 23 Nov 2021 09:03:30 +0000 (09:03 +0000)
On interface delete we were not removing
the lock taken by a previous ip_table_bind()
call thus preventing the VRFs to be removed.

Type: fix

Change-Id: I11abbb51a09b45cd3390b23d5d601d029c5ea485
Signed-off-by: Nathan Skrzypczak <nathan.skrzypczak@gmail.com>
src/plugins/unittest/fib_test.c
src/vnet/interface_api.c
src/vnet/ip/ip.h
src/vnet/ip/ip4_forward.c
src/vnet/ip/ip6_forward.c
test/test_ip6_vrf_multi_instance.py

index 26e2d24..3166e64 100644 (file)
@@ -158,8 +158,6 @@ fib_test_mk_intf (u32 ninterfaces)
                                             VNET_HW_INTERFACE_FLAG_LINK_UP);
         tm->hw[i] = vnet_get_hw_interface(vnet_get_main(),
                                           tm->hw_if_indicies[i]);
-        ip4_main.fib_index_by_sw_if_index[tm->hw[i]->sw_if_index] = 0;
-        ip6_main.fib_index_by_sw_if_index[tm->hw[i]->sw_if_index] = 0;
 
         error = vnet_sw_interface_set_flags(vnet_get_main(),
                                             tm->hw[i]->sw_if_index,
@@ -822,9 +820,7 @@ fib_test_v4 (void)
                                                   FIB_SOURCE_API);
 
     for (ii = 0; ii < 4; ii++)
-    {
-        ip4_main.fib_index_by_sw_if_index[tm->hw[ii]->sw_if_index] = fib_index;
-    }
+      fib_table_bind (FIB_PROTOCOL_IP4, tm->hw[ii]->sw_if_index, fib_index);
 
     fib_prefix_t pfx_0_0_0_0_s_0 = {
         .fp_len = 0,
@@ -4393,6 +4389,9 @@ fib_test_v4 (void)
                                              FIB_SOURCE_INTERFACE)),
              "NO INterface Source'd prefixes");
 
+    for (ii = 0; ii < 4; ii++)
+      fib_table_bind (FIB_PROTOCOL_IP4, tm->hw[ii]->sw_if_index, 0);
+
     fib_table_unlock(fib_index, FIB_PROTOCOL_IP4, FIB_SOURCE_API);
 
     FIB_TEST((0  == fib_path_list_db_size()), "path list DB population:%d",
@@ -4451,9 +4450,7 @@ fib_test_v6 (void)
                                                   FIB_SOURCE_API);
 
     for (ii = 0; ii < 4; ii++)
-    {
-        ip6_main.fib_index_by_sw_if_index[tm->hw[ii]->sw_if_index] = fib_index;
-    }
+      fib_table_bind (FIB_PROTOCOL_IP6, tm->hw[ii]->sw_if_index, fib_index);
 
     fib_prefix_t pfx_0_0 = {
         .fp_len = 0,
@@ -5272,6 +5269,10 @@ fib_test_v6 (void)
     /*
      * now remove the VRF
      */
+
+    for (ii = 0; ii < 4; ii++)
+      fib_table_bind (FIB_PROTOCOL_IP6, tm->hw[ii]->sw_if_index, 0);
+
     fib_table_unlock(fib_index, FIB_PROTOCOL_IP6, FIB_SOURCE_API);
 
     FIB_TEST((0 == fib_path_list_db_size()),   "path list DB population:%d",
@@ -5312,12 +5313,10 @@ fib_test_ae (void)
     const u32 fib_index = 0;
     fib_node_index_t dfrt, fei;
     test_main_t *tm;
-    ip4_main_t *im;
     int res;
 
     res = 0;
     tm = &test_main;
-    im = &ip4_main;
 
     FIB_TEST((0 == adj_nbr_db_size()), "ADJ DB size is %d",
              adj_nbr_db_size());
@@ -5337,7 +5336,7 @@ fib_test_ae (void)
         },
     };
 
-    im->fib_index_by_sw_if_index[tm->hw[0]->sw_if_index] = fib_index;
+    fib_table_bind (FIB_PROTOCOL_IP4, tm->hw[0]->sw_if_index, fib_index);
 
     dpo_drop = drop_dpo_get(DPO_PROTO_IP4);
 
@@ -5904,11 +5903,9 @@ static int
 fib_test_pref (void)
 {
     test_main_t *tm;
-    ip4_main_t *im;
     int res, i;
 
     tm = &test_main;
-    im = &ip4_main;
     res = 0;
 
     const fib_prefix_t pfx_1_1_1_1_s_32 = {
@@ -5922,7 +5919,7 @@ fib_test_pref (void)
     };
 
     for (i = 0; i <= 2; i++)
-        im->fib_index_by_sw_if_index[tm->hw[i]->sw_if_index] = 0;
+      fib_table_bind (FIB_PROTOCOL_IP4, tm->hw[i]->sw_if_index, 0);
 
     /*
      * 2 high, 2 medium and 2 low preference non-recursive paths
@@ -6371,12 +6368,10 @@ fib_test_label (void)
     const u32 fib_index = 0;
     int lb_count, ii, res;
     test_main_t *tm;
-    ip4_main_t *im;
 
     res = 0;
     lb_count = pool_elts(load_balance_pool);
     tm = &test_main;
-    im = &ip4_main;
 
     /*
      * add interface routes. We'll assume this works. It's more rigorously
@@ -6396,7 +6391,7 @@ fib_test_label (void)
     FIB_TEST((0 == adj_nbr_db_size()), "ADJ DB size is %d",
              adj_nbr_db_size());
 
-    im->fib_index_by_sw_if_index[tm->hw[0]->sw_if_index] = fib_index;
+    fib_table_bind (FIB_PROTOCOL_IP4, tm->hw[0]->sw_if_index, fib_index);
 
     fib_table_entry_update_one_path(fib_index, &local0_pfx,
                                     FIB_SOURCE_INTERFACE,
@@ -6441,7 +6436,7 @@ fib_test_label (void)
         },
     };
 
-    im->fib_index_by_sw_if_index[tm->hw[1]->sw_if_index] = fib_index;
+    fib_table_bind (FIB_PROTOCOL_IP4, tm->hw[1]->sw_if_index, fib_index);
 
     fib_table_entry_update_one_path(fib_index, &local1_pfx,
                                     FIB_SOURCE_INTERFACE,
@@ -9157,19 +9152,15 @@ fib_test_inherit (void)
     fib_node_index_t fei;
     int n_feis, res, i;
     test_main_t *tm;
-    ip4_main_t *im4;
-    ip6_main_t *im6;
 
     tm = &test_main;
-    im4 = &ip4_main;
-    im6 = &ip6_main;
     res = 0;
 
     for (i = 0; i <= 2; i++)
-    {
-        im4->fib_index_by_sw_if_index[tm->hw[i]->sw_if_index] = 0;
-        im6->fib_index_by_sw_if_index[tm->hw[i]->sw_if_index] = 0;
-    }
+      {
+       fib_table_bind (FIB_PROTOCOL_IP4, tm->hw[i]->sw_if_index, 0);
+       fib_table_bind (FIB_PROTOCOL_IP6, tm->hw[i]->sw_if_index, 0);
+      }
     n_feis = fib_entry_pool_size();
 
     const ip46_address_t nh_10_10_10_1 = {
index bbb6168..a765c0b 100644 (file)
@@ -470,40 +470,16 @@ vl_api_sw_interface_set_table_t_handler (vl_api_sw_interface_set_table_t * mp)
   REPLY_MACRO (VL_API_SW_INTERFACE_SET_TABLE_REPLY);
 }
 
-int
-ip_table_bind (fib_protocol_t fproto, u32 sw_if_index, u32 table_id)
+void
+fib_table_bind (fib_protocol_t fproto, u32 sw_if_index, u32 fib_index)
 {
-  CLIB_UNUSED (ip_interface_address_t * ia);
-  u32 fib_index, mfib_index;
+  u32 table_id;
 
-  /*
-   * This if table does not exist = error is what we want in the end.
-   */
-  fib_index = fib_table_find (fproto, table_id);
-  mfib_index = mfib_table_find (fproto, table_id);
-
-  if (~0 == fib_index || ~0 == mfib_index)
-    {
-      return (VNET_API_ERROR_NO_SUCH_FIB);
-    }
+  table_id = fib_table_get_table_id (fib_index, fproto);
+  ASSERT (table_id != ~0);
 
   if (FIB_PROTOCOL_IP6 == fproto)
     {
-      /*
-       * If the interface already has in IP address, then a change int
-       * VRF is not allowed. The IP address applied must first be removed.
-       * We do not do that automatically here, since VPP has no knowledge
-       * of whether those subnets are valid in the destination VRF.
-       */
-      /* *INDENT-OFF* */
-      foreach_ip_interface_address (&ip6_main.lookup_main,
-                                   ia, sw_if_index,
-                                   1 /* honor unnumbered */ ,
-      ({
-        return (VNET_API_ERROR_ADDRESS_FOUND_FOR_INTERFACE);
-      }));
-      /* *INDENT-ON* */
-
       /*
        * tell those that are interested that the binding is changing.
        */
@@ -518,38 +494,17 @@ ip_table_bind (fib_protocol_t fproto, u32 sw_if_index, u32 table_id)
       if (0 != ip6_main.fib_index_by_sw_if_index[sw_if_index])
        fib_table_unlock (ip6_main.fib_index_by_sw_if_index[sw_if_index],
                          FIB_PROTOCOL_IP6, FIB_SOURCE_INTERFACE);
-      if (0 != ip6_main.mfib_index_by_sw_if_index[sw_if_index])
-       mfib_table_unlock (ip6_main.mfib_index_by_sw_if_index[sw_if_index],
-                          FIB_PROTOCOL_IP6, MFIB_SOURCE_INTERFACE);
 
       if (0 != table_id)
        {
          /* we need to lock the table now it's inuse */
          fib_table_lock (fib_index, FIB_PROTOCOL_IP6, FIB_SOURCE_INTERFACE);
-         mfib_table_lock (mfib_index, FIB_PROTOCOL_IP6,
-                          MFIB_SOURCE_INTERFACE);
        }
 
       ip6_main.fib_index_by_sw_if_index[sw_if_index] = fib_index;
-      ip6_main.mfib_index_by_sw_if_index[sw_if_index] = mfib_index;
     }
   else
     {
-      /*
-       * If the interface already has in IP address, then a change int
-       * VRF is not allowed. The IP address applied must first be removed.
-       * We do not do that automatically here, since VPP has no knowledge
-       * of whether those subnets are valid in the destination VRF.
-       */
-      /* *INDENT-OFF* */
-      foreach_ip_interface_address (&ip4_main.lookup_main,
-                                   ia, sw_if_index,
-                                   1 /* honor unnumbered */ ,
-      ({
-        return (VNET_API_ERROR_ADDRESS_FOUND_FOR_INTERFACE);
-      }));
-      /* *INDENT-ON* */
-
       /*
        * tell those that are interested that the binding is changing.
        */
@@ -564,23 +519,93 @@ ip_table_bind (fib_protocol_t fproto, u32 sw_if_index, u32 table_id)
       if (0 != ip4_main.fib_index_by_sw_if_index[sw_if_index])
        fib_table_unlock (ip4_main.fib_index_by_sw_if_index[sw_if_index],
                          FIB_PROTOCOL_IP4, FIB_SOURCE_INTERFACE);
-      if (0 != ip4_main.mfib_index_by_sw_if_index[sw_if_index])
-       mfib_table_unlock (ip4_main.mfib_index_by_sw_if_index[sw_if_index],
-                          FIB_PROTOCOL_IP4, MFIB_SOURCE_INTERFACE);
 
       if (0 != table_id)
        {
          /* we need to lock the table now it's inuse */
          fib_index = fib_table_find_or_create_and_lock (
            FIB_PROTOCOL_IP4, table_id, FIB_SOURCE_INTERFACE);
+       }
+
+      ip4_main.fib_index_by_sw_if_index[sw_if_index] = fib_index;
+    }
+}
+
+void
+mfib_table_bind (fib_protocol_t fproto, u32 sw_if_index, u32 mfib_index)
+{
+  u32 table_id;
+
+  table_id = mfib_table_get_table_id (mfib_index, fproto);
+  ASSERT (table_id != ~0);
+
+  if (FIB_PROTOCOL_IP6 == fproto)
+    {
+      if (0 != ip6_main.mfib_index_by_sw_if_index[sw_if_index])
+       mfib_table_unlock (ip6_main.mfib_index_by_sw_if_index[sw_if_index],
+                          FIB_PROTOCOL_IP6, MFIB_SOURCE_INTERFACE);
 
+      if (0 != table_id)
+       {
+         /* we need to lock the table now it's inuse */
+         mfib_table_lock (mfib_index, FIB_PROTOCOL_IP6,
+                          MFIB_SOURCE_INTERFACE);
+       }
+
+      ip6_main.mfib_index_by_sw_if_index[sw_if_index] = mfib_index;
+    }
+  else
+    {
+      if (0 != ip4_main.mfib_index_by_sw_if_index[sw_if_index])
+       mfib_table_unlock (ip4_main.mfib_index_by_sw_if_index[sw_if_index],
+                          FIB_PROTOCOL_IP4, MFIB_SOURCE_INTERFACE);
+
+      if (0 != table_id)
+       {
+         /* we need to lock the table now it's inuse */
          mfib_index = mfib_table_find_or_create_and_lock (
            FIB_PROTOCOL_IP4, table_id, MFIB_SOURCE_INTERFACE);
        }
 
-      ip4_main.fib_index_by_sw_if_index[sw_if_index] = fib_index;
       ip4_main.mfib_index_by_sw_if_index[sw_if_index] = mfib_index;
     }
+}
+
+int
+ip_table_bind (fib_protocol_t fproto, u32 sw_if_index, u32 table_id)
+{
+  CLIB_UNUSED (ip_interface_address_t * ia);
+  u32 fib_index, mfib_index;
+
+  /*
+   * This if table does not exist = error is what we want in the end.
+   */
+  fib_index = fib_table_find (fproto, table_id);
+  mfib_index = mfib_table_find (fproto, table_id);
+
+  if (~0 == fib_index || ~0 == mfib_index)
+    {
+      return (VNET_API_ERROR_NO_SUCH_FIB);
+    }
+
+  /*
+   * If the interface already has in IP address, then a change int
+   * VRF is not allowed. The IP address applied must first be removed.
+   * We do not do that automatically here, since VPP has no knowledge
+   * of whether those subnets are valid in the destination VRF.
+   */
+  /* clang-format off */
+  foreach_ip_interface_address (FIB_PROTOCOL_IP6 == fproto ?
+                               &ip6_main.lookup_main : &ip4_main.lookup_main,
+                               ia, sw_if_index,
+                               1 /* honor unnumbered */ ,
+  ({
+    return (VNET_API_ERROR_ADDRESS_FOUND_FOR_INTERFACE);
+  }));
+  /* clang-format on */
+
+  fib_table_bind (fproto, sw_if_index, fib_index);
+  mfib_table_bind (fproto, sw_if_index, mfib_index);
 
   return (0);
 }
index 131d687..c2a3801 100644 (file)
@@ -267,6 +267,8 @@ void ip_table_create (fib_protocol_t fproto, u32 table_id, u8 is_api,
 
 void ip_table_delete (fib_protocol_t fproto, u32 table_id, u8 is_api);
 
+void fib_table_bind (fib_protocol_t fproto, u32 sw_if_index, u32 fib_index);
+void mfib_table_bind (fib_protocol_t fproto, u32 sw_if_index, u32 mfib_index);
 int ip_table_bind (fib_protocol_t fproto, u32 sw_if_index, u32 table_id);
 
 u32 ip_table_get_unused_id (fib_protocol_t fproto);
index 58af706..de8e8e8 100644 (file)
@@ -1090,6 +1090,15 @@ ip4_sw_interface_add_del (vnet_main_t * vnm, u32 sw_if_index, u32 is_add)
       }));
       /* *INDENT-ON* */
       ip4_mfib_interface_enable_disable (sw_if_index, 0);
+
+      if (0 != im4->fib_index_by_sw_if_index[sw_if_index])
+       fib_table_bind (FIB_PROTOCOL_IP4, sw_if_index, 0);
+      if (0 != im4->mfib_index_by_sw_if_index[sw_if_index])
+       mfib_table_bind (FIB_PROTOCOL_IP4, sw_if_index, 0);
+
+      /* Erase the lookup tables just in case */
+      im4->fib_index_by_sw_if_index[sw_if_index] = ~0;
+      im4->mfib_index_by_sw_if_index[sw_if_index] = ~0;
     }
 
   vnet_feature_enable_disable ("ip4-unicast", "ip4-not-enabled", sw_if_index,
index 833ce14..9ee3d11 100644 (file)
@@ -717,6 +717,15 @@ ip6_sw_interface_add_del (vnet_main_t * vnm, u32 sw_if_index, u32 is_add)
       }));
       /* *INDENT-ON* */
       ip6_mfib_interface_enable_disable (sw_if_index, 0);
+
+      if (0 != im6->fib_index_by_sw_if_index[sw_if_index])
+       fib_table_bind (FIB_PROTOCOL_IP6, sw_if_index, 0);
+      if (0 != im6->mfib_index_by_sw_if_index[sw_if_index])
+       mfib_table_bind (FIB_PROTOCOL_IP6, sw_if_index, 0);
+
+      /* Erase the lookup tables just in case */
+      im6->fib_index_by_sw_if_index[sw_if_index] = ~0;
+      im6->mfib_index_by_sw_if_index[sw_if_index] = ~0;
     }
 
   vnet_feature_enable_disable ("ip6-unicast", "ip6-not-enabled", sw_if_index,
index 97cebff..d95e792 100644 (file)
@@ -256,6 +256,7 @@ class TestIP6VrfMultiInst(VppTestCase):
         for j in range(self.pg_ifs_per_vrf):
             pg_if = self.pg_if_sets[if_set_id][j]
             pg_if.unconfig_ip6()
+            pg_if.set_table_ip6(0)
             if pg_if in self.pg_in_vrf:
                 self.pg_in_vrf.remove(pg_if)
             if pg_if not in self.pg_not_in_vrf:
@@ -263,6 +264,12 @@ class TestIP6VrfMultiInst(VppTestCase):
         self.logger.info("IPv6 VRF ID %d reset finished" % vrf_id)
         self.logger.debug(self.vapi.ppcli("show ip6 fib"))
         self.logger.debug(self.vapi.ppcli("show ip6 neighbors"))
+
+    def delete_vrf(self, vrf_id):
+        if vrf_id in self.vrf_list:
+            self.vrf_list.remove(vrf_id)
+        if vrf_id in self.vrf_reset_list:
+            self.vrf_reset_list.remove(vrf_id)
         self.vapi.ip_table_add_del(is_add=0,
                                    table={'table_id': vrf_id, 'is_ip6': 1})
 
@@ -551,8 +558,6 @@ class TestIP6VrfMultiInst(VppTestCase):
         self.run_verify_test()
         self.run_crosswise_vrf_test()
 
-    @unittest.skip('VPP crashes after running this test. \
-    There seems to be an issue with the way fib locks are managed')
     def test_ip6_vrf_05(self):
         """ IP6 VRF  Multi-instance test 5 - auto allocate vrf id
         """
@@ -592,6 +597,12 @@ class TestIP6VrfMultiInst(VppTestCase):
             vrf_list_length, 0,
             "List of configured VRFs is not empty: %s != 0" % vrf_list_length)
 
+        # Cleanup our extra created VRFs
+        for vrf in auto_vrf_id:
+            self.delete_vrf(vrf)
+        self.delete_vrf(5)
+        self.delete_vrf(10)
+
     def test_ip6_vrf_06(self):
         """ IP6 VRF  Multi-instance test 6 - recreate 4 VRFs
         """