From a424dd1b2e345bd8ebc5088fcd42d721a2ac00d7 Mon Sep 17 00:00:00 2001 From: Nathan Skrzypczak Date: Fri, 20 Aug 2021 15:53:43 +0200 Subject: [PATCH] ip: unlock_fib on if delete 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 --- src/plugins/unittest/fib_test.c | 43 +++++------- src/vnet/interface_api.c | 133 +++++++++++++++++++++--------------- src/vnet/ip/ip.h | 2 + src/vnet/ip/ip4_forward.c | 9 +++ src/vnet/ip/ip6_forward.c | 9 +++ test/test_ip6_vrf_multi_instance.py | 15 +++- 6 files changed, 129 insertions(+), 82 deletions(-) diff --git a/src/plugins/unittest/fib_test.c b/src/plugins/unittest/fib_test.c index 26e2d247666..3166e649bfe 100644 --- a/src/plugins/unittest/fib_test.c +++ b/src/plugins/unittest/fib_test.c @@ -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 = { diff --git a/src/vnet/interface_api.c b/src/vnet/interface_api.c index bbb6168df9e..a765c0bbc3d 100644 --- a/src/vnet/interface_api.c +++ b/src/vnet/interface_api.c @@ -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); } diff --git a/src/vnet/ip/ip.h b/src/vnet/ip/ip.h index 131d6879c14..c2a38016ba5 100644 --- a/src/vnet/ip/ip.h +++ b/src/vnet/ip/ip.h @@ -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); diff --git a/src/vnet/ip/ip4_forward.c b/src/vnet/ip/ip4_forward.c index 58af706e2b2..de8e8e8538f 100644 --- a/src/vnet/ip/ip4_forward.c +++ b/src/vnet/ip/ip4_forward.c @@ -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, diff --git a/src/vnet/ip/ip6_forward.c b/src/vnet/ip/ip6_forward.c index 833ce142999..9ee3d11cef2 100644 --- a/src/vnet/ip/ip6_forward.c +++ b/src/vnet/ip/ip6_forward.c @@ -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, diff --git a/test/test_ip6_vrf_multi_instance.py b/test/test_ip6_vrf_multi_instance.py index 97cebff8dce..d95e7927f98 100644 --- a/test/test_ip6_vrf_multi_instance.py +++ b/test/test_ip6_vrf_multi_instance.py @@ -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 """ -- 2.16.6