ligang/vpp
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

ip: fix fib and mfib locks

Browse Source 
This patches fixes an issue that could cause
fib locks to underflow: if an API user deletes
a fib and quickly recreates it, the fib may not
have been actually deleted. As a result, the
lock would not be incremented on the create call
leading to the fib potentially disappearing
afterwards - or to the lock to underflow when
the fib is deleted again.

In order to keep the existing API semantics,
we use the locks with API and CLI source as flags.
This means we need to use a different counter
for the interface-related locks.

This also prevents an issue where an interface being
bound to a vrf via API and released via CLI could
mess up the lock counter.

Finally, this will help with cleaning up the
interface-related locks on interface deletion
in a later patch.

Type: fix

Change-Id: I93030a7660646d6dd179ddf27fe4e708aa11b90e
Signed-off-by: Nathan Skrzypczak <nathan.skrzypczak@gmail.com>
Signed-off-by: Aloys Augustin <aloaugus@cisco.com>
This commit is contained in:
Nathan Skrzypczak
2021-09-17 17:29:14 +02:00
committed by Neale Ranns
parent bd23b405fb
commit 275bd79634
14 changed files with 126 additions and 96 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/plugins/gbp/gbp_itf.c
+2 -3
View File
@@ -232,8 +232,7 @@ gbp_itf_l3_add_and_lock_i (u32 sw_if_index, index_t gri, gbp_itf_free_fn_t ff)
ip6_sw_interface_enable_disable (gi->gi_sw_if_index, 1);
FOR_EACH_FIB_IP_PROTOCOL (fproto)
ip_table_bind (fproto, gi->gi_sw_if_index,
grd->grd_table_id[fproto], 1);
ip_table_bind (fproto, gi->gi_sw_if_index, grd->grd_table_id[fproto]);
hash_set (gbp_itf_db, gi->gi_sw_if_index, gi - gbp_itf_pool);
}
@@ -312,7 +311,7 @@ gbp_itf_unlock (gbp_itf_hdl_t * gh)
gbp_itf_l3_set_input_feature (*gh, GBP_ITF_L3_FEAT_NONE);
FOR_EACH_FIB_IP_PROTOCOL (fproto)
ip_table_bind (fproto, gi->gi_sw_if_index, 0, 0);
ip_table_bind (fproto, gi->gi_sw_if_index, 0);
ip4_sw_interface_enable_disable (gi->gi_sw_if_index, 0);
ip6_sw_interface_enable_disable (gi->gi_sw_if_index, 0);
src/plugins/unittest/fib_test.c
+2 -2
View File
@@ -8564,7 +8564,7 @@ lfib_test (void)
mpls_table_create(MPLS_FIB_DEFAULT_TABLE_ID, FIB_SOURCE_API, NULL);
mpls_sw_interface_enable_disable(&mpls_main,
tm->hw[0]->sw_if_index,
1, 1);
1);
ip46_address_t nh_10_10_10_1 = {
.ip4.as_u32 = clib_host_to_net_u32(0x0a0a0a01),
@@ -9135,7 +9135,7 @@ lfib_test (void)
*/
mpls_sw_interface_enable_disable(&mpls_main,
tm->hw[0]->sw_if_index,
0, 1);
0);
mpls_table_delete(MPLS_FIB_DEFAULT_TABLE_ID, FIB_SOURCE_API);
FIB_TEST(0 == pool_elts(mpls_disp_dpo_pool),
src/plugins/unittest/mfib_test.c
+2 -6
View File
@@ -1193,9 +1193,7 @@ mfib_test_i (fib_protocol_t PROTO,
* MPLS enable an interface so we get the MPLS table created
*/
mpls_table_create(MPLS_FIB_DEFAULT_TABLE_ID, FIB_SOURCE_API, NULL);
mpls_sw_interface_enable_disable(&mpls_main,
tm->hw[0]->sw_if_index,
1, 0);
mpls_sw_interface_enable_disable (&mpls_main, tm->hw[0]->sw_if_index, 1);
lfei = fib_table_entry_update_one_path(0, // default MPLS Table
&pfx_3500,
@@ -1291,9 +1289,7 @@ mfib_test_i (fib_protocol_t PROTO,
/*
* MPLS disable the interface
*/
mpls_sw_interface_enable_disable(&mpls_main,
tm->hw[0]->sw_if_index,
0, 0);
mpls_sw_interface_enable_disable (&mpls_main, tm->hw[0]->sw_if_index, 0);
mpls_table_delete(MPLS_FIB_DEFAULT_TABLE_ID, FIB_SOURCE_API);
/*
src/plugins/unittest/session_test.c
+1 -1
View File
@@ -136,7 +136,7 @@ session_create_lookpback (u32 table_id, u32 * sw_if_index,
if (table_id != 0)
{
ip_table_create (FIB_PROTOCOL_IP4, table_id, 0, 0);
ip_table_bind (FIB_PROTOCOL_IP4, *sw_if_index, table_id, 0);
ip_table_bind (FIB_PROTOCOL_IP4, *sw_if_index, table_id);
}
vnet_sw_interface_set_flags (vnet_get_main (), *sw_if_index,
src/vnet/fib/fib_table.c
+39 -2
View File
@@ -1338,6 +1338,36 @@ fib_table_lock_inc (fib_table_t *fib_table,
fib_table->ft_total_locks++;
}
static void
fib_table_lock_clear (fib_table_t *fib_table,
fib_source_t source)
{
vec_validate(fib_table->ft_locks, source);
ASSERT(fib_table->ft_locks[source] <= 1);
if (fib_table->ft_locks[source])
{
fib_table->ft_locks[source]--;
fib_table->ft_total_locks--;
}
}
static void
fib_table_lock_set (fib_table_t *fib_table,
fib_source_t source)
{
vec_validate(fib_table->ft_locks, source);
ASSERT(fib_table->ft_locks[source] <= 1);
ASSERT(fib_table->ft_total_locks < (0xffffffff - 1));
if (!fib_table->ft_locks[source])
{
fib_table->ft_locks[source]++;
fib_table->ft_total_locks++;
}
}
void
fib_table_unlock (u32 fib_index,
fib_protocol_t proto,
@@ -1346,7 +1376,11 @@ fib_table_unlock (u32 fib_index,
fib_table_t *fib_table;
fib_table = fib_table_get(fib_index, proto);
fib_table_lock_dec(fib_table, source);
if (source == FIB_SOURCE_API || source == FIB_SOURCE_CLI)
fib_table_lock_clear(fib_table, source);
else
fib_table_lock_dec(fib_table, source);
if (0 == fib_table->ft_total_locks)
{
@@ -1366,7 +1400,10 @@ fib_table_lock (u32 fib_index,
fib_table = fib_table_get(fib_index, proto);
fib_table_lock_inc(fib_table, source);
if (source == FIB_SOURCE_API || source == FIB_SOURCE_CLI)
fib_table_lock_set(fib_table, source);
else
fib_table_lock_inc(fib_table, source);
}
u32
src/vnet/interface_api.c
+14 -27
View File
@@ -461,9 +461,9 @@ vl_api_sw_interface_set_table_t_handler (vl_api_sw_interface_set_table_t * mp)
VALIDATE_SW_IF_INDEX (mp);
if (mp->is_ipv6)
rv = ip_table_bind (FIB_PROTOCOL_IP6, sw_if_index, table_id, 1);
rv = ip_table_bind (FIB_PROTOCOL_IP6, sw_if_index, table_id);
else
rv = ip_table_bind (FIB_PROTOCOL_IP4, sw_if_index, table_id, 1);
rv = ip_table_bind (FIB_PROTOCOL_IP4, sw_if_index, table_id);
BAD_SW_IF_INDEX_LABEL;
@@ -471,24 +471,10 @@ vl_api_sw_interface_set_table_t_handler (vl_api_sw_interface_set_table_t * mp)
}
int
ip_table_bind (fib_protocol_t fproto,
u32 sw_if_index, u32 table_id, u8 is_api)
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;
fib_source_t src;
mfib_source_t msrc;
if (is_api)
{
src = FIB_SOURCE_API;
msrc = MFIB_SOURCE_API;
}
else
{
src = FIB_SOURCE_CLI;
msrc = MFIB_SOURCE_CLI;
}
/*
* This if table does not exist = error is what we want in the end.
@@ -531,16 +517,17 @@ ip_table_bind (fib_protocol_t fproto,
/* unlock currently assigned tables */
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, src);
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, msrc);
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, src);
mfib_table_lock (mfib_index, FIB_PROTOCOL_IP6, msrc);
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;
@@ -576,19 +563,19 @@ ip_table_bind (fib_protocol_t fproto,
/* unlock currently assigned tables */
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, src);
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, msrc);
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, src);
fib_index = fib_table_find_or_create_and_lock (
FIB_PROTOCOL_IP4, table_id, FIB_SOURCE_INTERFACE);
mfib_index = mfib_table_find_or_create_and_lock (FIB_PROTOCOL_IP4,
table_id, msrc);
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;
src/vnet/ip/ip.h
+1 -2
View File
@@ -267,8 +267,7 @@ 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);
int ip_table_bind (fib_protocol_t fproto, u32 sw_if_index,
u32 table_id, u8 is_api);
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);
src/vnet/ip/ip_api.c
+8 -14
View File
@@ -944,20 +944,14 @@ ip_table_create (fib_protocol_t fproto,
fib_index = fib_table_find (fproto, table_id);
mfib_index = mfib_table_find (fproto, table_id);
if (~0 == fib_index)
{
fib_table_find_or_create_and_lock_w_name (fproto, table_id,
(is_api ?
FIB_SOURCE_API :
FIB_SOURCE_CLI), name);
}
if (~0 == mfib_index)
{
mfib_table_find_or_create_and_lock_w_name (fproto, table_id,
(is_api ?
MFIB_SOURCE_API :
MFIB_SOURCE_CLI), name);
}
/*
* Always try to re-lock in case the fib was deleted by an API call
* but was not yet freed because some other locks were held
*/
fib_table_find_or_create_and_lock_w_name (
fproto, table_id, (is_api ? FIB_SOURCE_API : FIB_SOURCE_CLI), name);
mfib_table_find_or_create_and_lock_w_name (
fproto, table_id, (is_api ? MFIB_SOURCE_API : MFIB_SOURCE_CLI), name);
if ((~0 == fib_index) || (~0 == mfib_index))
call_elf_section_ip_table_callbacks (vnm, table_id, 1 /* is_add */ ,
src/vnet/ip/lookup.c
+1 -1
View File
@@ -643,7 +643,7 @@ ip_table_bind_cmd (vlib_main_t * vm,
goto done;
}
rv = ip_table_bind (fproto, sw_if_index, table_id, 0);
rv = ip_table_bind (fproto, sw_if_index, table_id);
if (VNET_API_ERROR_ADDRESS_FOUND_FOR_INTERFACE == rv)
{
src/vnet/mfib/mfib_types.h
+19 -15
View File
@@ -160,23 +160,25 @@ typedef enum mfib_itf_flags_t_
*/
typedef enum mfib_source_t_
{
MFIB_SOURCE_SPECIAL,
MFIB_SOURCE_6RD,
MFIB_SOURCE_API,
MFIB_SOURCE_CLI,
MFIB_SOURCE_VXLAN,
MFIB_SOURCE_DHCP,
MFIB_SOURCE_SRv6,
MFIB_SOURCE_GTPU,
MFIB_SOURCE_VXLAN_GPE,
MFIB_SOURCE_GENEVE,
MFIB_SOURCE_IGMP,
MFIB_SOURCE_VXLAN_GBP,
MFIB_SOURCE_PLUGIN_LOW,
MFIB_SOURCE_RR,
MFIB_SOURCE_DEFAULT_ROUTE,
MFIB_SOURCE_SPECIAL,
MFIB_SOURCE_6RD,
MFIB_SOURCE_API,
MFIB_SOURCE_CLI,
MFIB_SOURCE_VXLAN,
MFIB_SOURCE_DHCP,
MFIB_SOURCE_SRv6,
MFIB_SOURCE_GTPU,
MFIB_SOURCE_VXLAN_GPE,
MFIB_SOURCE_GENEVE,
MFIB_SOURCE_IGMP,
MFIB_SOURCE_VXLAN_GBP,
MFIB_SOURCE_PLUGIN_LOW,
MFIB_SOURCE_RR,
MFIB_SOURCE_INTERFACE, /* used exclusively for mfib locks */
MFIB_SOURCE_DEFAULT_ROUTE,
} mfib_source_t;
/* clang-format off */
#define MFIB_SOURCE_NAMES { \
[MFIB_SOURCE_SPECIAL] = "Special", \
[MFIB_SOURCE_6RD] = "6RD", \
@@ -192,8 +194,10 @@ typedef enum mfib_source_t_
[MFIB_SOURCE_VXLAN_GBP] = "VXLAN-GBP", \
[MFIB_SOURCE_PLUGIN_LOW] = "plugin-low", \
[MFIB_SOURCE_RR] = "Recursive-resolution", \
[MFIB_SOURCE_INTERFACE] = "Interface", \
[MFIB_SOURCE_DEFAULT_ROUTE] = "Default Route", \
}
/* clang-format on */
#define FOREACH_MFIB_SOURCE(_ms) \
for (_ms = MFIB_SOURCE_SPECIAL; \
src/vnet/mpls/interface.c
+6 -10
View File
@@ -35,10 +35,8 @@ mpls_sw_interface_is_enabled (u32 sw_if_index)
}
int
mpls_sw_interface_enable_disable (mpls_main_t * mm,
u32 sw_if_index,
u8 is_enable,
u8 is_api)
mpls_sw_interface_enable_disable (mpls_main_t *mm, u32 sw_if_index,
u8 is_enable)
{
fib_node_index_t lfib_index;
vnet_main_t *vnm = vnet_get_main ();
@@ -60,8 +58,7 @@ mpls_sw_interface_enable_disable (mpls_main_t * mm,
if (1 != ++mm->mpls_enabled_by_sw_if_index[sw_if_index])
return (0);
fib_table_lock(lfib_index, FIB_PROTOCOL_MPLS,
(is_api? FIB_SOURCE_API: FIB_SOURCE_CLI));
fib_table_lock (lfib_index, FIB_PROTOCOL_MPLS, FIB_SOURCE_INTERFACE);
vec_validate(mm->fib_index_by_sw_if_index, sw_if_index);
mm->fib_index_by_sw_if_index[sw_if_index] = lfib_index;
@@ -72,9 +69,8 @@ mpls_sw_interface_enable_disable (mpls_main_t * mm,
if (0 != --mm->mpls_enabled_by_sw_if_index[sw_if_index])
return (0);
fib_table_unlock(mm->fib_index_by_sw_if_index[sw_if_index],
FIB_PROTOCOL_MPLS,
(is_api? FIB_SOURCE_API: FIB_SOURCE_CLI));
fib_table_unlock (mm->fib_index_by_sw_if_index[sw_if_index],
FIB_PROTOCOL_MPLS, FIB_SOURCE_INTERFACE);
}
vnet_feature_enable_disable ("mpls-input", "mpls-not-enabled",
@@ -118,7 +114,7 @@ mpls_interface_enable_disable (vlib_main_t * vm,
goto done;
}
rv = mpls_sw_interface_enable_disable(&mpls_main, sw_if_index, enable, 0);
rv = mpls_sw_interface_enable_disable (&mpls_main, sw_if_index, enable);
if (VNET_API_ERROR_NO_SUCH_FIB == rv)
error = clib_error_return (0, "default MPLS table must be created first");
src/vnet/mpls/mpls.h
+2 -3
View File
@@ -85,9 +85,8 @@ unformat_function_t unformat_mpls_unicast_label;
unformat_function_t unformat_mpls_header;
unformat_function_t unformat_pg_mpls_header;
int mpls_sw_interface_enable_disable (mpls_main_t * mm,
u32 sw_if_index,
u8 is_enable, u8 is_api);
int mpls_sw_interface_enable_disable (mpls_main_t *mm, u32 sw_if_index,
u8 is_enable);
u8 mpls_sw_interface_is_enabled (u32 sw_if_index);
src/vnet/mpls/mpls_api.c
+2 -10
View File
@@ -210,8 +210,6 @@ vl_api_mpls_route_add_del_t_handler (vl_api_mpls_route_add_del_t * mp)
void
mpls_table_create (u32 table_id, u8 is_api, const u8 * name)
{
u32 fib_index;
/*
* The MPLS defult table must also be explicitly created via the API.
* So in contrast to IP, it gets no special treatment here.
@@ -222,16 +220,11 @@ mpls_table_create (u32 table_id, u8 is_api, const u8 * name)
* i.e. it can be added many times via the API but needs to be
* deleted only once.
*/
fib_index = fib_table_find (FIB_PROTOCOL_MPLS, table_id);
if (~0 == fib_index)
{
fib_table_find_or_create_and_lock_w_name (FIB_PROTOCOL_MPLS,
table_id,
(is_api ?
FIB_SOURCE_API :
FIB_SOURCE_CLI), name);
}
}
static void
@@ -295,9 +288,8 @@ static void
VALIDATE_SW_IF_INDEX (mp);
rv = mpls_sw_interface_enable_disable (&mpls_main,
ntohl (mp->sw_if_index),
mp->enable, 1);
rv = mpls_sw_interface_enable_disable (&mpls_main, ntohl (mp->sw_if_index),
mp->enable);
BAD_SW_IF_INDEX_LABEL;
REPLY_MACRO (VL_API_SW_INTERFACE_SET_MPLS_ENABLE_REPLY);
test/test_ip6_vrf_multi_instance.py
+27
View File
@@ -592,6 +592,33 @@ class TestIP6VrfMultiInst(VppTestCase):
vrf_list_length, 0,
"List of configured VRFs is not empty: %s != 0" % vrf_list_length)
def test_ip6_vrf_06(self):
""" IP6 VRF Multi-instance test 6 - recreate 4 VRFs
"""
# Reconfigure all the VRFs
self.create_vrf_and_assign_interfaces(4)
# Verify
for vrf_id in self.vrf_list:
self.assert_equal(self.verify_vrf(vrf_id),
VRFState.configured, VRFState)
# Test
self.run_verify_test()
self.run_crosswise_vrf_test()
# Cleanup
for i in range(len(self.vrf_list)):
self.reset_vrf_and_remove_from_vrf_list(self.vrf_list[0])
# Verify
for vrf_id in self.vrf_reset_list:
self.assert_equal(self.verify_vrf(vrf_id),
VRFState.reset, VRFState)
vrf_list_length = len(self.vrf_list)
self.assertEqual(
vrf_list_length, 0,
"List of configured VRFs is not empty: %s != 0" % vrf_list_length)
# Test
self.run_verify_test()
self.run_crosswise_vrf_test()
if __name__ == '__main__':
unittest.main(testRunner=VppTestRunner)