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>

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);
 }

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;

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;
 
-      /* Rate limit */
-      if (!throttle_check (&cnat_throttle, ctx->thread_index, r0, ctx->seed))
+      addr.version = ctx->af;
+      ip46_address_copy (&addr.ip, &session->value.cs_ip[VLIB_RX]);
+
+      /* Throttle */
+      clib_spinlock_lock (&cnat_client_db.throttle_lock);
+
+      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)
     {

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);
 }

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[];