aboutsummaryrefslogtreecommitdiffstats
path: root/src
diff options
context:
space:
mode:
authorNathan Skrzypczak <nathan.skrzypczak@gmail.com>2021-09-17 17:29:14 +0200
committerNeale Ranns <neale@graphiant.com>2021-10-11 12:04:03 +0000
commit275bd796346c3b1618170f4eda36b9a41ade9c87 (patch)
tree197df1102b0a3981c6a875100f521806a170f45c /src
parentbd23b405fbe99034d75a46b060567e5adf62c04e (diff)
ip: fix fib and mfib locks
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>
Diffstat (limited to 'src')
-rw-r--r--src/plugins/gbp/gbp_itf.c5
-rw-r--r--src/plugins/unittest/fib_test.c4
-rw-r--r--src/plugins/unittest/mfib_test.c8
-rw-r--r--src/plugins/unittest/session_test.c2
-rw-r--r--src/vnet/fib/fib_table.c41
-rw-r--r--src/vnet/interface_api.c41
-rw-r--r--src/vnet/ip/ip.h3
-rw-r--r--src/vnet/ip/ip_api.c22
-rw-r--r--src/vnet/ip/lookup.c2
-rw-r--r--src/vnet/mfib/mfib_types.h34
-rw-r--r--src/vnet/mpls/interface.c16
-rw-r--r--src/vnet/mpls/mpls.h5
-rw-r--r--src/vnet/mpls/mpls_api.c12
13 files changed, 99 insertions, 96 deletions
diff --git a/src/plugins/gbp/gbp_itf.c b/src/plugins/gbp/gbp_itf.c
index 0c8f6a45a87..738a7ac2e39 100644
--- a/src/plugins/gbp/gbp_itf.c
+++ b/src/plugins/gbp/gbp_itf.c
@@ -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);
diff --git a/src/plugins/unittest/fib_test.c b/src/plugins/unittest/fib_test.c
index f5ee4ae69e5..26e2d247666 100644
--- a/src/plugins/unittest/fib_test.c
+++ b/src/plugins/unittest/fib_test.c
@@ -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),
diff --git a/src/plugins/unittest/mfib_test.c b/src/plugins/unittest/mfib_test.c
index 73abd07e80c..0faa20c0934 100644
--- a/src/plugins/unittest/mfib_test.c
+++ b/src/plugins/unittest/mfib_test.c
@@ -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);
/*
diff --git a/src/plugins/unittest/session_test.c b/src/plugins/unittest/session_test.c
index 292f7253a67..651658e3676 100644
--- a/src/plugins/unittest/session_test.c
+++ b/src/plugins/unittest/session_test.c
@@ -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,
diff --git a/src/vnet/fib/fib_table.c b/src/vnet/fib/fib_table.c
index f222828898b..3a46d226ebd 100644
--- a/src/vnet/fib/fib_table.c
+++ b/src/vnet/fib/fib_table.c
@@ -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
diff --git a/src/vnet/interface_api.c b/src/vnet/interface_api.c
index 80a3058303c..bbb6168df9e 100644
--- a/src/vnet/interface_api.c
+++ b/src/vnet/interface_api.c
@@ -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;
diff --git a/src/vnet/ip/ip.h b/src/vnet/ip/ip.h
index cda0de2a451..87689076697 100644
--- a/src/vnet/ip/ip.h
+++ b/src/vnet/ip/ip.h
@@ -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);
diff --git a/src/vnet/ip/ip_api.c b/src/vnet/ip/ip_api.c
index b5f3e93b33e..79c9dd61e11 100644
--- a/src/vnet/ip/ip_api.c
+++ b/src/vnet/ip/ip_api.c
@@ -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 */ ,
diff --git a/src/vnet/ip/lookup.c b/src/vnet/ip/lookup.c
index 5db14bac092..192c4c7cebc 100644
--- a/src/vnet/ip/lookup.c
+++ b/src/vnet/ip/lookup.c
@@ -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)
{
diff --git a/src/vnet/mfib/mfib_types.h b/src/vnet/mfib/mfib_types.h
index edc25fe5b99..34e8f6b928d 100644
--- a/src/vnet/mfib/mfib_types.h
+++ b/src/vnet/mfib/mfib_types.h
@@ -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; \
diff --git a/src/vnet/mpls/interface.c b/src/vnet/mpls/interface.c
index e6c3dfeb801..5e80b9d0532 100644
--- a/src/vnet/mpls/interface.c
+++ b/src/vnet/mpls/interface.c
@@ -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");
diff --git a/src/vnet/mpls/mpls.h b/src/vnet/mpls/mpls.h
index 00b493f4576..b4f90a13f3c 100644
--- a/src/vnet/mpls/mpls.h
+++ b/src/vnet/mpls/mpls.h
@@ -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);
diff --git a/src/vnet/mpls/mpls_api.c b/src/vnet/mpls/mpls_api.c
index e89732f0d10..4efb61786ad 100644
--- a/src/vnet/mpls/mpls_api.c
+++ b/src/vnet/mpls/mpls_api.c
@@ -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);