summaryrefslogtreecommitdiffstats
path: root/src/plugins/cnat/cnat_client.c
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/plugins/cnat/cnat_client.c
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/plugins/cnat/cnat_client.c')
-rw-r--r--src/plugins/cnat/cnat_client.c102
1 files changed, 42 insertions, 60 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);
}