aboutsummaryrefslogtreecommitdiffstats
path: root/src
diff options
context:
space:
mode:
authorNathan Skrzypczak <nathan.skrzypczak@gmail.com>2020-11-16 18:57:52 +0100
committerBeno�t Ganne <bganne@cisco.com>2021-01-28 13:34:15 +0000
commit208891c093468b753830d1e7ebdb4a69d4c192bf (patch)
tree1e184fd62916e483d2ccee673130ea5890b97ba1 /src
parent5214f3a2c89ed0858e9383dbaefd3202f354610d (diff)
cnat: Fix throttle hash & cleanup
Type: fix This fixes two issues : - We used a hash to throttle RPC for adding fib entries, but as we rely on a refcount, we cannot accept loosing an entry, which could happen in case of a collision. - On client cleanup we weren't freeing the fib entry correctly which resulted in crashes when recreating an entry. Added a test that ensures proper cleanup Change-Id: Ie6660b0b02241f75092737410ae2299f8710d6b9 Signed-off-by: Nathan Skrzypczak <nathan.skrzypczak@gmail.com>
Diffstat (limited to 'src')
-rw-r--r--src/plugins/cnat/cnat_client.c102
-rw-r--r--src/plugins/cnat/cnat_client.h13
-rw-r--r--src/plugins/cnat/cnat_node.h52
-rw-r--r--src/plugins/cnat/cnat_types.c4
-rw-r--r--src/plugins/cnat/cnat_types.h2
5 files changed, 67 insertions, 106 deletions
diff --git a/src/plugins/cnat/cnat_client.c b/src/plugins/cnat/cnat_client.c
index 534813c1564..8beaeadb4b1 100644
--- a/src/plugins/cnat/cnat_client.c
+++ b/src/plugins/cnat/cnat_client.c
@@ -44,11 +44,10 @@ static void
cnat_client_destroy (cnat_client_t * cc)
{
ASSERT (!cnat_client_is_clone (cc));
- if (!(cc->flags & CNAT_FLAG_EXCLUSIVE))
- {
- ASSERT (fib_entry_is_sourced (cc->cc_fei, cnat_fib_source));
- fib_table_entry_delete_index (cc->cc_fei, cnat_fib_source);
- }
+
+ ASSERT (fib_entry_is_sourced (cc->cc_fei, cnat_fib_source));
+ fib_table_entry_delete_index (cc->cc_fei, cnat_fib_source);
+
cnat_client_db_remove (cc);
dpo_reset (&cc->cc_parent);
pool_put (cnat_client_pool, cc);
@@ -62,8 +61,7 @@ cnat_client_free_by_ip (ip46_address_t * ip, u8 af)
cnat_client_ip4_find (&ip->ip4) : cnat_client_ip6_find (&ip->ip6));
ASSERT (NULL != cc);
- if ((0 == cnat_client_uncnt_session (cc))
- && (cc->flags & CNAT_FLAG_EXPIRES) && (0 == cc->tr_refcnt))
+ if (0 == cnat_client_uncnt_session (cc) && 0 == cc->tr_refcnt)
cnat_client_destroy (cc);
}
@@ -73,36 +71,26 @@ cnat_client_throttle_pool_process ()
/* This processes ips stored in the throttle pool
to update session refcounts
and should be called before cnat_client_free_by_ip */
- vlib_thread_main_t *tm = vlib_get_thread_main ();
cnat_client_t *cc;
- int nthreads;
- u32 *del_vec = NULL, *ai;
- ip_address_t *addr;
- nthreads = tm->n_threads + 1;
- for (int i = 0; i < nthreads; i++)
- {
- vec_reset_length (del_vec);
- clib_spinlock_lock (&cnat_client_db.throttle_pool_lock[i]);
- /* *INDENT-OFF* */
- pool_foreach (addr, cnat_client_db.throttle_pool[i]) {
- cc = (AF_IP4 == addr->version ?
- cnat_client_ip4_find (&ip_addr_v4(addr)) :
- cnat_client_ip6_find (&ip_addr_v6(addr)));
- /* Client might not already be created */
- if (NULL != cc)
- {
- cnat_client_cnt_session (cc);
- vec_add1(del_vec, addr - cnat_client_db.throttle_pool[i]);
- }
- }
- /* *INDENT-ON* */
- vec_foreach (ai, del_vec)
+ ip_address_t *addr, *del_vec = NULL;
+ u32 refcnt;
+
+ vec_reset_length (del_vec);
+ clib_spinlock_lock (&cnat_client_db.throttle_lock);
+ hash_foreach_mem (addr, refcnt, cnat_client_db.throttle_mem, {
+ cc = (AF_IP4 == addr->version ? cnat_client_ip4_find (&ip_addr_v4 (addr)) :
+ cnat_client_ip6_find (&ip_addr_v6 (addr)));
+ /* Client might not already be created */
+ if (NULL != cc)
{
- addr = pool_elt_at_index (cnat_client_db.throttle_pool[i], *ai);
- pool_put (cnat_client_db.throttle_pool[i], addr);
+ cnat_client_t *ccp = cnat_client_get (cc->parent_cci);
+ clib_atomic_add_fetch (&ccp->session_refcnt, refcnt);
+ vec_add1 (del_vec, *addr);
}
- clib_spinlock_unlock (&cnat_client_db.throttle_pool_lock[i]);
- }
+ });
+ vec_foreach (addr, del_vec)
+ hash_unset_mem_free (&cnat_client_db.throttle_mem, addr);
+ clib_spinlock_unlock (&cnat_client_db.throttle_lock);
}
void
@@ -113,7 +101,6 @@ cnat_client_translation_added (index_t cci)
return;
cc = cnat_client_get (cci);
- ASSERT (!(cc->flags & CNAT_FLAG_EXPIRES));
cc->tr_refcnt++;
}
@@ -125,7 +112,6 @@ cnat_client_translation_deleted (index_t cci)
return;
cc = cnat_client_get (cci);
- ASSERT (!(cc->flags & CNAT_FLAG_EXPIRES));
cc->tr_refcnt--;
if (0 == cc->tr_refcnt && 0 == cc->session_refcnt)
@@ -199,12 +185,12 @@ cnat_client_add (const ip_address_t * ip, u8 flags)
}
void
-cnat_client_learn (const cnat_learn_arg_t * l)
+cnat_client_learn (const ip_address_t *addr)
{
/* RPC call to add a client from the dataplane */
index_t cci;
cnat_client_t *cc;
- cci = cnat_client_add (&l->addr, CNAT_FLAG_EXPIRES);
+ cci = cnat_client_add (addr, 0 /* flags */);
cc = pool_elt_at_index (cnat_client_pool, cci);
cnat_client_cnt_session (cc);
/* Process throttled calls if any */
@@ -241,17 +227,20 @@ cnat_client_dpo_interpose (const dpo_id_t * original,
int
cnat_client_purge (void)
{
- vlib_thread_main_t *tm = vlib_get_thread_main ();
- int nthreads;
- nthreads = tm->n_threads + 1;
- ASSERT (0 == hash_elts (cnat_client_db.crd_cip6));
- ASSERT (0 == hash_elts (cnat_client_db.crd_cip4));
- ASSERT (0 == pool_elts (cnat_client_pool));
- for (int i = 0; i < nthreads; i++)
- {
- ASSERT (0 == pool_elts (cnat_client_db.throttle_pool[i]));
- }
- return (0);
+ int rv = 0, rrv = 0;
+ if ((rv = hash_elts (cnat_client_db.crd_cip6)))
+ clib_warning ("len(crd_cip6) isnt 0 but %d", rv);
+ rrv |= rv;
+ if ((rv = hash_elts (cnat_client_db.crd_cip4)))
+ clib_warning ("len(crd_cip4) isnt 0 but %d", rv);
+ rrv |= rv;
+ if ((rv = pool_elts (cnat_client_pool)))
+ clib_warning ("len(cnat_client_pool) isnt 0 but %d", rv);
+ rrv |= rv;
+ if ((rv = hash_elts (cnat_client_db.throttle_mem)))
+ clib_warning ("len(throttle_mem) isnt 0 but %d", rv);
+ rrv |= rv;
+ return (rrv);
}
u8 *
@@ -265,8 +254,6 @@ format_cnat_client (u8 * s, va_list * args)
s = format (s, "[%d] cnat-client:[%U] tr:%d sess:%d", cci,
format_ip_address, &cc->cc_ip,
cc->tr_refcnt, cc->session_refcnt);
- if (cc->flags & CNAT_FLAG_EXPIRES)
- s = format (s, " expires");
if (cc->flags & CNAT_FLAG_EXCLUSIVE)
s = format (s, " exclusive");
@@ -300,10 +287,8 @@ cnat_client_show (vlib_main_t * vm,
if (INDEX_INVALID == cci)
{
- /* *INDENT-OFF* */
pool_foreach_index (cci, cnat_client_pool)
vlib_cli_output(vm, "%U", format_cnat_client, cci, 0);
- /* *INDENT-ON* */
vlib_cli_output (vm, "%d clients", pool_elts (cnat_client_pool));
vlib_cli_output (vm, "%d timestamps", pool_elts (cnat_timestamps));
@@ -362,6 +347,7 @@ cnat_client_dpo_unlock (dpo_id_t * dpo)
if (0 == cc->cc_locks)
{
ASSERT (cnat_client_is_clone (cc));
+ dpo_reset (&cc->cc_parent);
pool_put (cnat_client_pool, cc);
}
}
@@ -387,9 +373,6 @@ const static dpo_vft_t cnat_client_dpo_vft = {
static clib_error_t *
cnat_client_init (vlib_main_t * vm)
{
- vlib_thread_main_t *tm = vlib_get_thread_main ();
- int nthreads = tm->n_threads + 1;
- int i;
cnat_client_dpo = dpo_register_new_type (&cnat_client_dpo_vft,
cnat_client_dpo_nodes);
@@ -397,10 +380,9 @@ cnat_client_init (vlib_main_t * vm)
sizeof (ip6_address_t),
sizeof (uword));
- vec_validate (cnat_client_db.throttle_pool, nthreads);
- vec_validate (cnat_client_db.throttle_pool_lock, nthreads);
- for (i = 0; i < nthreads; i++)
- clib_spinlock_init (&cnat_client_db.throttle_pool_lock[i]);
+ clib_spinlock_init (&cnat_client_db.throttle_lock);
+ cnat_client_db.throttle_mem =
+ hash_create_mem (0, sizeof (ip_address_t), sizeof (uword));
return (NULL);
}
diff --git a/src/plugins/cnat/cnat_client.h b/src/plugins/cnat/cnat_client.h
index 9bc622dcc2c..d6e3631d868 100644
--- a/src/plugins/cnat/cnat_client.h
+++ b/src/plugins/cnat/cnat_client.h
@@ -93,11 +93,6 @@ cnat_client_get (index_t i)
return (pool_elt_at_index (cnat_client_pool, i));
}
-typedef struct cnat_learn_arg_t_
-{
- ip_address_t addr;
-} cnat_learn_arg_t;
-
/**
* A translation that references this VIP was deleted
*/
@@ -111,7 +106,7 @@ extern void cnat_client_translation_added (index_t cci);
* Called in the main thread by RPC from the workers to learn a
* new client
*/
-extern void cnat_client_learn (const cnat_learn_arg_t * l);
+extern void cnat_client_learn (const ip_address_t *addr);
extern index_t cnat_client_add (const ip_address_t * ip, u8 flags);
@@ -127,8 +122,6 @@ typedef enum
{
/* IP already present in the FIB, need to interpose dpo */
CNAT_FLAG_EXCLUSIVE = (1 << 1),
- /* Prune this entry */
- CNAT_FLAG_EXPIRES = (1 << 2),
} cnat_entry_flag_t;
@@ -144,8 +137,8 @@ typedef struct cnat_client_db_t_
/* Pool of addresses that have been throttled
and need to be refcounted before calling
cnat_client_free_by_ip */
- ip_address_t **throttle_pool;
- clib_spinlock_t *throttle_pool_lock;
+ clib_spinlock_t throttle_lock;
+ uword *throttle_mem;
} cnat_client_db_t;
extern cnat_client_db_t cnat_client_db;
diff --git a/src/plugins/cnat/cnat_node.h b/src/plugins/cnat/cnat_node.h
index 64c3db1e7cc..e2a9965b2a6 100644
--- a/src/plugins/cnat/cnat_node.h
+++ b/src/plugins/cnat/cnat_node.h
@@ -738,37 +738,31 @@ cnat_session_create (cnat_session_t * session, cnat_node_ctx_t * ctx,
if (NULL == cc)
{
- u64 r0 = 17;
- if (AF_IP4 == ctx->af)
- r0 = (u64) session->value.cs_ip[VLIB_RX].ip4.as_u32;
- else
- {
- r0 = r0 * 31 + session->value.cs_ip[VLIB_RX].ip6.as_u64[0];
- r0 = r0 * 31 + session->value.cs_ip[VLIB_RX].ip6.as_u64[1];
- }
+ ip_address_t addr;
+ uword *p;
+ u32 refcnt;
+
+ addr.version = ctx->af;
+ ip46_address_copy (&addr.ip, &session->value.cs_ip[VLIB_RX]);
+
+ /* Throttle */
+ clib_spinlock_lock (&cnat_client_db.throttle_lock);
- /* Rate limit */
- if (!throttle_check (&cnat_throttle, ctx->thread_index, r0, ctx->seed))
+ p = hash_get_mem (cnat_client_db.throttle_mem, &addr);
+ if (p)
{
- cnat_learn_arg_t l;
- l.addr.version = ctx->af;
- ip46_address_copy (&l.addr.ip, &session->value.cs_ip[VLIB_RX]);
- /* fire client create to the main thread */
- vl_api_rpc_call_main_thread (cnat_client_learn,
- (u8 *) & l, sizeof (l));
+ refcnt = p[0] + 1;
+ hash_set_mem (cnat_client_db.throttle_mem, &addr, refcnt);
}
else
- {
- /* Will still need to count those for session refcnt */
- ip_address_t *addr;
- clib_spinlock_lock (&cnat_client_db.throttle_pool_lock
- [ctx->thread_index]);
- pool_get (cnat_client_db.throttle_pool[ctx->thread_index], addr);
- addr->version = ctx->af;
- ip46_address_copy (&addr->ip, &session->value.cs_ip[VLIB_RX]);
- clib_spinlock_unlock (&cnat_client_db.throttle_pool_lock
- [ctx->thread_index]);
- }
+ hash_set_mem_alloc (&cnat_client_db.throttle_mem, &addr, 0);
+
+ clib_spinlock_unlock (&cnat_client_db.throttle_lock);
+
+ /* fire client create to the main thread */
+ if (!p)
+ vl_api_rpc_call_main_thread (cnat_client_learn, (u8 *) &addr,
+ sizeof (addr));
}
else
{
@@ -823,7 +817,6 @@ cnat_node_inline (vlib_main_t * vm,
vlib_buffer_t **b = bufs;
u16 nexts[VLIB_FRAME_SIZE], *next;
f64 now;
- u64 seed;
thread_index = vm->thread_index;
from = vlib_frame_vector_args (frame);
@@ -831,13 +824,12 @@ cnat_node_inline (vlib_main_t * vm,
next = nexts;
vlib_get_buffers (vm, from, bufs, n_left);
now = vlib_time_now (vm);
- seed = throttle_seed (&cnat_throttle, thread_index, vlib_time_now (vm));
cnat_session_t *session[4];
clib_bihash_kv_40_48_t bkey[4], bvalue[4];
u64 hash[4];
int rv[4];
- cnat_node_ctx_t ctx = { now, seed, thread_index, af, do_trace };
+ cnat_node_ctx_t ctx = { now, thread_index, af, do_trace };
if (n_left >= 8)
{
diff --git a/src/plugins/cnat/cnat_types.c b/src/plugins/cnat/cnat_types.c
index c15c2f61bc9..b6c6628961c 100644
--- a/src/plugins/cnat/cnat_types.c
+++ b/src/plugins/cnat/cnat_types.c
@@ -18,7 +18,6 @@
cnat_main_t cnat_main;
fib_source_t cnat_fib_source;
cnat_timestamp_t *cnat_timestamps;
-throttle_t cnat_throttle;
char *cnat_error_strings[] = {
#define cnat_error(n,s) s,
@@ -144,15 +143,12 @@ format_cnat_endpoint (u8 * s, va_list * args)
static clib_error_t *
cnat_types_init (vlib_main_t * vm)
{
- vlib_thread_main_t *tm = &vlib_thread_main;
- u32 n_vlib_mains = tm->n_vlib_mains;
cnat_fib_source = fib_source_allocate ("cnat",
CNAT_FIB_SOURCE_PRIORITY,
FIB_SOURCE_BH_SIMPLE);
clib_rwlock_init (&cnat_main.ts_lock);
- throttle_init (&cnat_throttle, n_vlib_mains, 1e-3);
return (NULL);
}
diff --git a/src/plugins/cnat/cnat_types.h b/src/plugins/cnat/cnat_types.h
index b1b5b2d2336..d3b7295f6b4 100644
--- a/src/plugins/cnat/cnat_types.h
+++ b/src/plugins/cnat/cnat_types.h
@@ -159,7 +159,6 @@ typedef struct cnat_timestamp_t_
typedef struct cnat_node_ctx_
{
f64 now;
- u64 seed;
u32 thread_index;
ip_address_family_t af;
u8 do_trace;
@@ -173,7 +172,6 @@ extern uword unformat_cnat_ep (unformat_input_t * input, va_list * args);
extern cnat_timestamp_t *cnat_timestamps;
extern fib_source_t cnat_fib_source;
extern cnat_main_t cnat_main;
-extern throttle_t cnat_throttle;
extern char *cnat_error_strings[];