wireguard: add barrier to sync data

The current implmentation of the hash table is not thread-safe.
This design leads to a segfault when VPP is handling a lot of tunnels
for Wireguard, where one thread modifies the hash table and other
threads start the lookup at the same time.

This fix adds a barrier sync to the hash table access when Wireguard
adds or deletes an element.

Type: fix
Signed-off-by: Gabriel Oginski <gabrielx.oginski@intel.com>
Change-Id: Id460dfcd46ace17c7bdcd23bd9687d26cecf0a39

diff --git a/src/plugins/wireguard/wireguard_if.c b/src/plugins/wireguard/wireguard_if.c
index a869df0..ee744e7 100644 (file)
--- a/src/plugins/wireguard/wireguard_if.c
+++ b/src/plugins/wireguard/wireguard_if.c
@@ -116,20 +116,20 @@ wg_remote_get (const uint8_t public[NOISE_PUBLIC_KEY_LEN])
 }
 
 static uint32_t
-wg_index_set (noise_remote_t * remote)
+wg_index_set (vlib_main_t *vm, noise_remote_t *remote)
 {
   wg_main_t *wmp = &wg_main;
   u32 rnd_seed = (u32) (vlib_time_now (wmp->vlib_main) * 1e6);
   u32 ret =
-    wg_index_table_add (&wmp->index_table, remote->r_peer_idx, rnd_seed);
+    wg_index_table_add (vm, &wmp->index_table, remote->r_peer_idx, rnd_seed);
   return ret;
 }
 
 static void
-wg_index_drop (uint32_t key)
+wg_index_drop (vlib_main_t *vm, uint32_t key)
 {
   wg_main_t *wmp = &wg_main;
-  wg_index_table_del (&wmp->index_table, key);
+  wg_index_table_del (vm, &wmp->index_table, key);
 }
 
 static clib_error_t *

diff --git a/src/plugins/wireguard/wireguard_index_table.c b/src/plugins/wireguard/wireguard_index_table.c
index 5f81204..da53bfd 100644 (file)
--- a/src/plugins/wireguard/wireguard_index_table.c
+++ b/src/plugins/wireguard/wireguard_index_table.c
@@ -13,13 +13,15 @@
  * limitations under the License.
  */
 
+#include <vlib/vlib.h>
 #include <vppinfra/hash.h>
 #include <vppinfra/pool.h>
 #include <vppinfra/random.h>
 #include <wireguard/wireguard_index_table.h>
 
 u32
-wg_index_table_add (wg_index_table_t * table, u32 peer_pool_idx, u32 rnd_seed)
+wg_index_table_add (vlib_main_t *vm, wg_index_table_t *table,
+                   u32 peer_pool_idx, u32 rnd_seed)
 {
   u32 key;
 
@@ -29,19 +31,25 @@ wg_index_table_add (wg_index_table_t * table, u32 peer_pool_idx, u32 rnd_seed)
       if (hash_get (table->hash, key))
        continue;
 
+      vlib_worker_thread_barrier_sync (vm);
       hash_set (table->hash, key, peer_pool_idx);
+      vlib_worker_thread_barrier_release (vm);
       break;
     }
   return key;
 }
 
 void
-wg_index_table_del (wg_index_table_t * table, u32 key)
+wg_index_table_del (vlib_main_t *vm, wg_index_table_t *table, u32 key)
 {
   uword *p;
   p = hash_get (table->hash, key);
   if (p)
-    hash_unset (table->hash, key);
+    {
+      vlib_worker_thread_barrier_sync (vm);
+      hash_unset (table->hash, key);
+      vlib_worker_thread_barrier_release (vm);
+    }
 }
 
 u32 *

diff --git a/src/plugins/wireguard/wireguard_index_table.h b/src/plugins/wireguard/wireguard_index_table.h
index 67cae1f..e9aa374 100644 (file)
--- a/src/plugins/wireguard/wireguard_index_table.h
+++ b/src/plugins/wireguard/wireguard_index_table.h
@@ -16,6 +16,7 @@
 #ifndef __included_wg_index_table_h__
 #define __included_wg_index_table_h__
 
+#include <vlib/vlib.h>
 #include <vppinfra/types.h>
 
 typedef struct
@@ -23,9 +24,9 @@ typedef struct
   uword *hash;
 } wg_index_table_t;
 
-u32 wg_index_table_add (wg_index_table_t * table, u32 peer_pool_idx,
-                       u32 rnd_seed);
-void wg_index_table_del (wg_index_table_t * table, u32 key);
+u32 wg_index_table_add (vlib_main_t *vm, wg_index_table_t *table,
+                       u32 peer_pool_idx, u32 rnd_seed);
+void wg_index_table_del (vlib_main_t *vm, wg_index_table_t *table, u32 key);
 u32 *wg_index_table_lookup (const wg_index_table_t * table, u32 key);
 
 #endif //__included_wg_index_table_h__

diff --git a/src/plugins/wireguard/wireguard_noise.c b/src/plugins/wireguard/wireguard_noise.c
index c9d8e31..19b0ce5 100644 (file)
--- a/src/plugins/wireguard/wireguard_noise.c
+++ b/src/plugins/wireguard/wireguard_noise.c
@@ -33,8 +33,10 @@ noise_local_t *noise_local_pool;
 static noise_keypair_t *noise_remote_keypair_allocate (noise_remote_t *);
 static void noise_remote_keypair_free (vlib_main_t * vm, noise_remote_t *,
                                       noise_keypair_t **);
-static uint32_t noise_remote_handshake_index_get (noise_remote_t *);
-static void noise_remote_handshake_index_drop (noise_remote_t *);
+static uint32_t noise_remote_handshake_index_get (vlib_main_t *vm,
+                                                 noise_remote_t *);
+static void noise_remote_handshake_index_drop (vlib_main_t *vm,
+                                              noise_remote_t *);
 
 static uint64_t noise_counter_send (noise_counter_t *);
 bool noise_counter_recv (noise_counter_t *, uint64_t);
@@ -86,7 +88,7 @@ noise_local_set_private (noise_local_t * l,
 }
 
 void
-noise_remote_init (noise_remote_t * r, uint32_t peer_pool_idx,
+noise_remote_init (vlib_main_t *vm, noise_remote_t *r, uint32_t peer_pool_idx,
                   const uint8_t public[NOISE_PUBLIC_KEY_LEN],
                   u32 noise_local_idx)
 {
@@ -97,18 +99,18 @@ noise_remote_init (noise_remote_t * r, uint32_t peer_pool_idx,
   r->r_local_idx = noise_local_idx;
   r->r_handshake.hs_state = HS_ZEROED;
 
-  noise_remote_precompute (r);
+  noise_remote_precompute (vm, r);
 }
 
 void
-noise_remote_precompute (noise_remote_t * r)
+noise_remote_precompute (vlib_main_t *vm, noise_remote_t *r)
 {
   noise_local_t *l = noise_local_get (r->r_local_idx);
 
   if (!curve25519_gen_shared (r->r_ss, l->l_private, r->r_public))
     clib_memset (r->r_ss, 0, NOISE_PUBLIC_KEY_LEN);
 
-  noise_remote_handshake_index_drop (r);
+  noise_remote_handshake_index_drop (vm, r);
   wg_secure_zero_memory (&r->r_handshake, sizeof (r->r_handshake));
 }
 
@@ -154,9 +156,9 @@ noise_create_initiation (vlib_main_t * vm, noise_remote_t * r,
   /* {t} */
   noise_tai64n_now (ets);
   noise_msg_encrypt (vm, ets, ets, NOISE_TIMESTAMP_LEN, key_idx, hs->hs_hash);
-  noise_remote_handshake_index_drop (r);
+  noise_remote_handshake_index_drop (vm, r);
   hs->hs_state = CREATED_INITIATION;
-  hs->hs_local_index = noise_remote_handshake_index_get (r);
+  hs->hs_local_index = noise_remote_handshake_index_get (vm, r);
   *s_idx = hs->hs_local_index;
   ret = true;
 error:
@@ -237,7 +239,7 @@ noise_consume_initiation (vlib_main_t * vm, noise_local_t * l,
     goto error;
 
   /* Ok, we're happy to accept this initiation now */
-  noise_remote_handshake_index_drop (r);
+  noise_remote_handshake_index_drop (vm, r);
   r->r_handshake = hs;
   *rp = r;
   ret = true;
@@ -291,7 +293,7 @@ noise_create_response (vlib_main_t * vm, noise_remote_t * r, uint32_t * s_idx,
 
 
   hs->hs_state = CREATED_RESPONSE;
-  hs->hs_local_index = noise_remote_handshake_index_get (r);
+  hs->hs_local_index = noise_remote_handshake_index_get (vm, r);
   *r_idx = hs->hs_remote_index;
   *s_idx = hs->hs_local_index;
   ret = true;
@@ -451,7 +453,7 @@ noise_remote_begin_session (vlib_main_t * vm, noise_remote_t * r)
 void
 noise_remote_clear (vlib_main_t * vm, noise_remote_t * r)
 {
-  noise_remote_handshake_index_drop (r);
+  noise_remote_handshake_index_drop (vm, r);
   wg_secure_zero_memory (&r->r_handshake, sizeof (r->r_handshake));
 
   clib_rwlock_writer_lock (&r->r_keypair_lock);
@@ -555,21 +557,21 @@ noise_remote_keypair_allocate (noise_remote_t * r)
 }
 
 static uint32_t
-noise_remote_handshake_index_get (noise_remote_t * r)
+noise_remote_handshake_index_get (vlib_main_t *vm, noise_remote_t *r)
 {
   noise_local_t *local = noise_local_get (r->r_local_idx);
   struct noise_upcall *u = &local->l_upcall;
-  return u->u_index_set (r);
+  return u->u_index_set (vm, r);
 }
 
 static void
-noise_remote_handshake_index_drop (noise_remote_t * r)
+noise_remote_handshake_index_drop (vlib_main_t *vm, noise_remote_t *r)
 {
   noise_handshake_t *hs = &r->r_handshake;
   noise_local_t *local = noise_local_get (r->r_local_idx);
   struct noise_upcall *u = &local->l_upcall;
   if (hs->hs_state != HS_ZEROED)
-    u->u_index_drop (hs->hs_local_index);
+    u->u_index_drop (vm, hs->hs_local_index);
 }
 
 static void

diff --git a/src/plugins/wireguard/wireguard_noise.h b/src/plugins/wireguard/wireguard_noise.h
index e95211b..fd2c09e 100644 (file)
--- a/src/plugins/wireguard/wireguard_noise.h
+++ b/src/plugins/wireguard/wireguard_noise.h
@@ -121,8 +121,8 @@ typedef struct noise_local
   {
     void *u_arg;
     noise_remote_t *(*u_remote_get) (const uint8_t[NOISE_PUBLIC_KEY_LEN]);
-      uint32_t (*u_index_set) (noise_remote_t *);
-    void (*u_index_drop) (uint32_t);
+    uint32_t (*u_index_set) (vlib_main_t *, noise_remote_t *);
+    void (*u_index_drop) (vlib_main_t *, uint32_t);
   } l_upcall;
 } noise_local_t;
 
@@ -148,11 +148,11 @@ void noise_local_init (noise_local_t *, struct noise_upcall *);
 bool noise_local_set_private (noise_local_t *,
                              const uint8_t[NOISE_PUBLIC_KEY_LEN]);
 
-void noise_remote_init (noise_remote_t *, uint32_t,
+void noise_remote_init (vlib_main_t *, noise_remote_t *, uint32_t,
                        const uint8_t[NOISE_PUBLIC_KEY_LEN], uint32_t);
 
 /* Should be called anytime noise_local_set_private is called */
-void noise_remote_precompute (noise_remote_t *);
+void noise_remote_precompute (vlib_main_t *, noise_remote_t *);
 
 /* Cryptographic functions */
 bool noise_create_initiation (vlib_main_t * vm, noise_remote_t *,
@@ -266,7 +266,7 @@ noise_remote_keypair_free (vlib_main_t *vm, noise_remote_t *r,
   struct noise_upcall *u = &local->l_upcall;
   if (*kp)
     {
-      u->u_index_drop ((*kp)->kp_local_index);
+      u->u_index_drop (vm, (*kp)->kp_local_index);
       vnet_crypto_key_del (vm, (*kp)->kp_send_index);
       vnet_crypto_key_del (vm, (*kp)->kp_recv_index);
       clib_mem_free (*kp);

diff --git a/src/plugins/wireguard/wireguard_peer.c b/src/plugins/wireguard/wireguard_peer.c
index f7bf235..32a92da 100644 (file)
--- a/src/plugins/wireguard/wireguard_peer.c
+++ b/src/plugins/wireguard/wireguard_peer.c
@@ -242,7 +242,7 @@ wg_peer_enable (vlib_main_t *vm, wg_peer_t *peer)
   wg_if = wg_if_get (wg_if_find_by_sw_if_index (peer->wg_sw_if_index));
   clib_memcpy (public_key, peer->remote.r_public, NOISE_PUBLIC_KEY_LEN);
 
-  noise_remote_init (&peer->remote, peeri, public_key, wg_if->local_idx);
+  noise_remote_init (vm, &peer->remote, peeri, public_key, wg_if->local_idx);
 
   wg_timers_send_first_handshake (peer);
 }
@@ -484,7 +484,7 @@ wg_peer_add (u32 tun_sw_if_index, const u8 public_key[NOISE_PUBLIC_KEY_LEN],
       return (rv);
     }
 
-  noise_remote_init (&peer->remote, peer - wg_peer_pool, public_key,
+  noise_remote_init (vm, &peer->remote, peer - wg_peer_pool, public_key,
                     wg_if->local_idx);
   cookie_maker_init (&peer->cookie_maker, public_key);