nat: fix per-vrf session bookkeeping

Each NAT44 ED session has a per_vrf_sessions_index referencing
an element in the thread-local vector per_vrf_sessions_vec.
However this index can be possibly invalidated by vec_del1() in
per_vrf_sessions_cleanup(), before a session is registered.
Such a stale index can cause an assertion failure in function
per_vrf_sessions_is_expired() when we use it to locate the
per_vrf_sessions object.

A possible sequence to reproduce is:

1. Create two NAT44 ED sessions s1, s2 so that two per_vrf_sessions are created:
     index 0: between VRF pair 10 and 11 (expired=0, ses_count=1)
     index 1: between VRF pair 20 and 21 (expired=0, ses_count=1)
   For the sessions we have:
     s1->per_vrf_sessions_index == 0
     s2->per_vrf_sessions_index == 1

2. Delete the first session via CLI, now the two per_vrf_sessions become:
     index 0: between VRF pair 10 and 11 (expired=0, ses_count=0)
     index 1: between VRF pair 20 and 21 (expired=0, ses_count=1)
   For the sessions we have:
     s2->per_vrf_sessions_index == 1

3. Delete the VRF 11:
     index 0: between VRF pair 10 and 11 (expired=1, ses_count=0)
     index 1: between VRF pair 20 and 21 (expired=0, ses_count=1)
   For the sessions we have:
     s2->per_vrf_sessions_index == 1

4. Create a new session s3 between VRF pair 20 and 21 so that the first
   per_vrf_sessions will be deleted:
     index 0: between VRF pair 20 and 21 (expired=0, ses_count=2)
   For the sessions we have:
     s2->per_vrf_sessions_index == 1
     s3->per_vrf_sessions_index == 0
   Here, note that the actual index of per_vrf_session is changed due
   to vec_del1(). The new session is added after the cleanup so it gets
   the correct index. But the index held by the existing session is not
   updated.

5. Trigger the fast path of the session s2. To achieve this, session
   s2 could be created in step 1 by
     ping -i20 -Iiface_in_vrf_10 1.1.1.1
   and steps 2-4 should then be performed within the 20-second interval.

This patch fixes this by changing per_vrf_sessions_vec to a pool so
that indicies are kept intact.

Type: fix
Signed-off-by: Jing Peng <jing@meter.com>
Change-Id: I4c08f9bfd50134bcb5f08e50ad61af2bddbcb645

diff --git a/src/plugins/nat/nat44-ed/nat44_ed.c b/src/plugins/nat/nat44-ed/nat44_ed.c
index ed93f6c..133c39e 100644 (file)
--- a/src/plugins/nat/nat44-ed/nat44_ed.c
+++ b/src/plugins/nat/nat44-ed/nat44_ed.c
@@ -1633,19 +1633,19 @@ expire_per_vrf_sessions (u32 fib_index)
 
   vec_foreach (tsm, sm->per_thread_data)
     {
-      vec_foreach (per_vrf_sessions, tsm->per_vrf_sessions_vec)
-        {
-          if ((per_vrf_sessions->rx_fib_index == fib_index) ||
-              (per_vrf_sessions->tx_fib_index == fib_index))
-            {
-              per_vrf_sessions->expired = 1;
-            }
-        }
+      pool_foreach (per_vrf_sessions, tsm->per_vrf_sessions_pool)
+       {
+         if ((per_vrf_sessions->rx_fib_index == fib_index) ||
+             (per_vrf_sessions->tx_fib_index == fib_index))
+           {
+             per_vrf_sessions->expired = 1;
+           }
+       }
     }
 }
 
 void
-update_per_vrf_sessions_vec (u32 fib_index, int is_del)
+update_per_vrf_sessions_pool (u32 fib_index, int is_del)
 {
   snat_main_t *sm = &snat_main;
   nat_fib_t *fib;
@@ -1788,7 +1788,7 @@ nat44_ed_add_interface (u32 sw_if_index, u8 is_inside)
   fib_index =
     fib_table_get_index_for_sw_if_index (FIB_PROTOCOL_IP4, sw_if_index);
 
-  update_per_vrf_sessions_vec (fib_index, 0 /*is_del*/);
+  update_per_vrf_sessions_pool (fib_index, 0 /*is_del*/);
 
   if (!is_inside)
     {
@@ -1903,7 +1903,7 @@ nat44_ed_del_interface (u32 sw_if_index, u8 is_inside)
   fib_index =
     fib_table_get_index_for_sw_if_index (FIB_PROTOCOL_IP4, sw_if_index);
 
-  update_per_vrf_sessions_vec (fib_index, 1 /*is_del*/);
+  update_per_vrf_sessions_pool (fib_index, 1 /*is_del*/);
 
   if (!is_inside)
     {
@@ -2002,7 +2002,7 @@ nat44_ed_add_output_interface (u32 sw_if_index)
 
   fib_index =
     fib_table_get_index_for_sw_if_index (FIB_PROTOCOL_IP4, sw_if_index);
-  update_per_vrf_sessions_vec (fib_index, 0 /*is_del*/);
+  update_per_vrf_sessions_pool (fib_index, 0 /*is_del*/);
 
   outside_fib = nat44_ed_get_outside_fib (sm->outside_fibs, fib_index);
   if (outside_fib)
@@ -2092,7 +2092,7 @@ nat44_ed_del_output_interface (u32 sw_if_index)
 
   fib_index =
     fib_table_get_index_for_sw_if_index (FIB_PROTOCOL_IP4, sw_if_index);
-  update_per_vrf_sessions_vec (fib_index, 1 /*is_del*/);
+  update_per_vrf_sessions_pool (fib_index, 1 /*is_del*/);
 
   outside_fib = nat44_ed_get_outside_fib (sm->outside_fibs, fib_index);
   if (outside_fib)
@@ -3287,7 +3287,7 @@ nat44_ed_worker_db_free (snat_main_per_thread_data_t *tsm)
 {
   pool_free (tsm->lru_pool);
   pool_free (tsm->sessions);
-  vec_free (tsm->per_vrf_sessions_vec);
+  pool_free (tsm->per_vrf_sessions_pool);
 }
 
 void

diff --git a/src/plugins/nat/nat44-ed/nat44_ed.h b/src/plugins/nat/nat44-ed/nat44_ed.h
index bc69648..7065114 100644 (file)
--- a/src/plugins/nat/nat44-ed/nat44_ed.h
+++ b/src/plugins/nat/nat44-ed/nat44_ed.h
@@ -469,7 +469,7 @@ typedef struct
   /* real thread index */
   u32 thread_index;
 
-  per_vrf_sessions_t *per_vrf_sessions_vec;
+  per_vrf_sessions_t *per_vrf_sessions_pool;
 
 } snat_main_per_thread_data_t;
 

diff --git a/src/plugins/nat/nat44-ed/nat44_ed_inlines.h b/src/plugins/nat/nat44-ed/nat44_ed_inlines.h
index b99d152..04e5236 100644 (file)
--- a/src/plugins/nat/nat44-ed/nat44_ed_inlines.h
+++ b/src/plugins/nat/nat44-ed/nat44_ed_inlines.h
@@ -414,23 +414,16 @@ per_vrf_sessions_cleanup (u32 thread_index)
   per_vrf_sessions_t *per_vrf_sessions;
   u32 *to_free = 0, *i;
 
-  vec_foreach (per_vrf_sessions, tsm->per_vrf_sessions_vec)
+  pool_foreach (per_vrf_sessions, tsm->per_vrf_sessions_pool)
     {
-      if (per_vrf_sessions->expired)
-       {
-         if (per_vrf_sessions->ses_count == 0)
-           {
-             vec_add1 (to_free, per_vrf_sessions - tsm->per_vrf_sessions_vec);
-           }
-       }
+      if (per_vrf_sessions->expired && per_vrf_sessions->ses_count == 0)
+       vec_add1 (to_free, per_vrf_sessions - tsm->per_vrf_sessions_pool);
     }
 
-  if (vec_len (to_free))
+  vec_foreach (i, to_free)
     {
-      vec_foreach (i, to_free)
-       {
-         vec_del1 (tsm->per_vrf_sessions_vec, *i);
-       }
+      per_vrf_sessions = pool_elt_at_index (tsm->per_vrf_sessions_pool, *i);
+      pool_put (tsm->per_vrf_sessions_pool, per_vrf_sessions);
     }
 
   vec_free (to_free);
@@ -449,7 +442,7 @@ per_vrf_sessions_register_session (snat_session_t *s, u32 thread_index)
 
   // s->per_vrf_sessions_index == ~0 ... reuse of old session
 
-  vec_foreach (per_vrf_sessions, tsm->per_vrf_sessions_vec)
+  pool_foreach (per_vrf_sessions, tsm->per_vrf_sessions_pool)
     {
       // ignore already expired registrations
       if (per_vrf_sessions->expired)
@@ -468,14 +461,13 @@ per_vrf_sessions_register_session (snat_session_t *s, u32 thread_index)
     }
 
   // create a new registration
-  vec_add2 (tsm->per_vrf_sessions_vec, per_vrf_sessions, 1);
+  pool_get (tsm->per_vrf_sessions_pool, per_vrf_sessions);
   clib_memset (per_vrf_sessions, 0, sizeof (*per_vrf_sessions));
-
   per_vrf_sessions->rx_fib_index = s->in2out.fib_index;
   per_vrf_sessions->tx_fib_index = s->out2in.fib_index;
 
 done:
-  s->per_vrf_sessions_index = per_vrf_sessions - tsm->per_vrf_sessions_vec;
+  s->per_vrf_sessions_index = per_vrf_sessions - tsm->per_vrf_sessions_pool;
   per_vrf_sessions->ses_count++;
 }
 
@@ -491,7 +483,7 @@ per_vrf_sessions_unregister_session (snat_session_t *s, u32 thread_index)
 
   tsm = vec_elt_at_index (sm->per_thread_data, thread_index);
   per_vrf_sessions =
-    vec_elt_at_index (tsm->per_vrf_sessions_vec, s->per_vrf_sessions_index);
+    pool_elt_at_index (tsm->per_vrf_sessions_pool, s->per_vrf_sessions_index);
 
   ASSERT (per_vrf_sessions->ses_count != 0);
 
@@ -511,7 +503,7 @@ per_vrf_sessions_is_expired (snat_session_t *s, u32 thread_index)
 
   tsm = vec_elt_at_index (sm->per_thread_data, thread_index);
   per_vrf_sessions =
-    vec_elt_at_index (tsm->per_vrf_sessions_vec, s->per_vrf_sessions_index);
+    pool_elt_at_index (tsm->per_vrf_sessions_pool, s->per_vrf_sessions_index);
   return per_vrf_sessions->expired;
 }