acl-plugin: implement an optional session reclassification when ACL is (re-)applied

There were several discussions in which users would expect the sessions to be deleted
if the new policy after the change does not permit them.
There is no right or wrong answer to this question - it is a policy decision.

This patch implements an idea to approach this. It uses a per-interface-per-direction counter to designate
a "policy epoch" - a period of unchanging rulesets. The moment one removes or adds an ACL applied to
an interface, this counter increments.
The newly created connections inherit the current policy epoch in a given direction.
Likewise, this counter increments if anyone updates an ACL applied to an interface.

There is also a new (so far hidden) CLI "set acl-plugin reclassify-sessions [0|1]"
(with default being 0) which allows to enable the checking of the existing sessions
against the current policy epoch in a given direction.

The session is not verified unless there is traffic hitting that session
 *in the direction of the policy creation* - if the epoch has changed,
the session is deleted and within the same processing cycle is evaluated
against the ACL rule base and recreated - thus, it should allow traffic-driven
session state refresh without affecting the connectivity for the existing sessions.

If the packet is coming in the direction opposite to which the session was initially
created, the state adjustment is never done, because doing so generically
is not really possible without diving too deep into the special cases,
which may or may not work.

Change-Id: I9e90426492d4bd474b5e89ea8dfb75a7c9de2646
Signed-off-by: Andrew Yourtchenko <ayourtch@gmail.com>

diff --git a/src/plugins/acl/acl.c b/src/plugins/acl/acl.c
index 7c2572d..90b369e 100644 (file)
--- a/src/plugins/acl/acl.c
+++ b/src/plugins/acl/acl.c
@@ -305,6 +305,45 @@ warning_acl_print_acl (vlib_main_t * vm, acl_main_t * am, int acl_index)
   acl_print_acl_x (print_clib_warning_and_reset, vm, am, acl_index);
 }
 
+static void
+increment_policy_epoch (acl_main_t * am, u32 sw_if_index, int is_input)
+{
+
+  u32 **ppolicy_epoch_by_swi =
+    is_input ? &am->input_policy_epoch_by_sw_if_index :
+    &am->output_policy_epoch_by_sw_if_index;
+  vec_validate (*ppolicy_epoch_by_swi, sw_if_index);
+
+  u32 *p_epoch = vec_elt_at_index ((*ppolicy_epoch_by_swi), sw_if_index);
+  *p_epoch =
+    ((1 + *p_epoch) & FA_POLICY_EPOCH_MASK) +
+    (is_input * FA_POLICY_EPOCH_IS_INPUT);
+}
+
+static void
+try_increment_acl_policy_epoch (acl_main_t * am, u32 acl_num, int is_input)
+{
+  u32 ***p_swi_vec_by_acl = is_input ? &am->input_sw_if_index_vec_by_acl
+    : &am->output_sw_if_index_vec_by_acl;
+  if (acl_num < vec_len (*p_swi_vec_by_acl))
+    {
+      u32 *p_swi;
+      vec_foreach (p_swi, (*p_swi_vec_by_acl)[acl_num])
+      {
+       increment_policy_epoch (am, *p_swi, is_input);
+      }
+
+    }
+}
+
+static void
+policy_notify_acl_change (acl_main_t * am, u32 acl_num)
+{
+  try_increment_acl_policy_epoch (am, acl_num, 0);
+  try_increment_acl_policy_epoch (am, acl_num, 1);
+}
+
+
 
 static int
 acl_add_list (u32 count, vl_api_acl_rule_t rules[],
@@ -392,6 +431,12 @@ acl_add_list (u32 count, vl_api_acl_rule_t rules[],
   memcpy (a->tag, tag, sizeof (a->tag));
   if (am->trace_acl > 255)
     warning_acl_print_acl (am->vlib_main, am, *acl_list_index);
+  if (am->reclassify_sessions)
+    {
+      /* a change in an ACLs if they are applied may mean a new policy epoch */
+      policy_notify_acl_change (am, *acl_list_index);
+    }
+
   /* notify the lookup contexts about the ACL changes */
   acl_plugin_lookup_context_notify_acl_change (*acl_list_index);
   clib_mem_set_heap (oldheap);
@@ -1268,12 +1313,20 @@ acl_interface_set_inout_acl_list (acl_main_t * am, u32 sw_if_index,
   (*pinout_acl_vec_by_sw_if_index)[sw_if_index] =
     vec_dup (vec_acl_list_index);
 
-  /* if no commonalities between the ACL# - then we should definitely clear the sessions */
-  if (may_clear_sessions && *may_clear_sessions
-      && !clib_bitmap_is_zero (change_acl_bitmap))
+  if (am->reclassify_sessions)
+    {
+      /* re-applying ACLs means a new policy epoch */
+      increment_policy_epoch (am, sw_if_index, is_input);
+    }
+  else
     {
-      acl_clear_sessions (am, sw_if_index);
-      *may_clear_sessions = 0;
+      /* if no commonalities between the ACL# - then we should definitely clear the sessions */
+      if (may_clear_sessions && *may_clear_sessions
+         && !clib_bitmap_is_zero (change_acl_bitmap))
+       {
+         acl_clear_sessions (am, sw_if_index);
+         *may_clear_sessions = 0;
+       }
     }
 
   /*
@@ -3202,6 +3255,11 @@ acl_set_aclplugin_fn (vlib_main_t * vm,
       am->l4_match_nonfirst_fragment = (val != 0);
       goto done;
     }
+  if (unformat (input, "reclassify-sessions %u", &val))
+    {
+      am->reclassify_sessions = (val != 0);
+      goto done;
+    }
   if (unformat (input, "event-trace"))
     {
       if (!unformat (input, "%u", &val))
@@ -3571,6 +3629,15 @@ acl_plugin_show_interface (acl_main_t * am, u32 sw_if_index, int show_acl,
        continue;
 
       vlib_cli_output (vm, "sw_if_index %d:\n", swi);
+      if (swi < vec_len (am->input_policy_epoch_by_sw_if_index))
+       vlib_cli_output (vm, "   input policy epoch: %x\n",
+                        vec_elt (am->input_policy_epoch_by_sw_if_index,
+                                 swi));
+      if (swi < vec_len (am->output_policy_epoch_by_sw_if_index))
+       vlib_cli_output (vm, "   output policy epoch: %x\n",
+                        vec_elt (am->output_policy_epoch_by_sw_if_index,
+                                 swi));
+
 
       if (intf_has_etype_whitelist (am, swi, 1))
        {
@@ -3761,13 +3828,21 @@ acl_plugin_show_sessions (acl_main_t * am,
                                               ?
                                               pw->fa_session_dels_by_sw_if_index
                                               [sw_if_index] : 0;
+                                              u64 n_epoch_changes =
+                                              sw_if_index <
+                                              vec_len
+                                              (pw->fa_session_epoch_change_by_sw_if_index)
+                                              ?
+                                              pw->fa_session_epoch_change_by_sw_if_index
+                                              [sw_if_index] : 0;
                                               vlib_cli_output (vm,
-                                                               "    sw_if_index %d: add %lu - del %lu = %lu",
+                                                               "    sw_if_index %d: add %lu - del %lu = %lu; epoch chg: %lu",
                                                                sw_if_index,
                                                                n_adds,
                                                                n_dels,
                                                                n_adds -
-                                                               n_dels);
+                                                               n_dels,
+                                                               n_epoch_changes);
                                               }
                    ));
 
@@ -3829,6 +3904,7 @@ acl_plugin_show_sessions (acl_main_t * am,
                   am->fa_cleaner_wait_time_increment * 1000.0,
                   ((f64) am->fa_current_cleaner_timer_wait_interval) *
                   1000.0 / (f64) vm->clib_time.clocks_per_second);
+  vlib_cli_output (vm, "Reclassify sessions: %d", am->reclassify_sessions);
 }
 
 static clib_error_t *
@@ -4007,6 +4083,7 @@ acl_plugin_config (vlib_main_t * vm, unformat_input_t * input)
   uword hash_heap_size;
   u32 hash_lookup_hash_buckets;
   u32 hash_lookup_hash_memory;
+  u32 reclassify_sessions;
 
   while (unformat_check_input (input) != UNFORMAT_END_OF_INPUT)
     {
@@ -4035,6 +4112,10 @@ acl_plugin_config (vlib_main_t * vm, unformat_input_t * input)
       else if (unformat (input, "hash lookup hash memory %d",
                         &hash_lookup_hash_memory))
        am->hash_lookup_hash_memory = hash_lookup_hash_memory;
+      else if (unformat (input, "reclassify sessions %d",
+                        &reclassify_sessions))
+       am->reclassify_sessions = reclassify_sessions;
+
       else
        return clib_error_return (0, "unknown input '%U'",
                                  format_unformat_error, input);
@@ -4086,6 +4167,7 @@ acl_init (vlib_main_t * vm)
   am->fa_conn_table_hash_memory_size =
     ACL_FA_CONN_TABLE_DEFAULT_HASH_MEMORY_SIZE;
   am->fa_conn_table_max_entries = ACL_FA_CONN_TABLE_DEFAULT_MAX_ENTRIES;
+  am->reclassify_sessions = 0;
   vlib_thread_main_t *tm = vlib_get_thread_main ();
   vec_validate (am->per_worker_data, tm->n_vlib_mains - 1);
   {

diff --git a/src/plugins/acl/acl.h b/src/plugins/acl/acl.h
index 2d4dc55..7af5b20 100644 (file)
--- a/src/plugins/acl/acl.h
+++ b/src/plugins/acl/acl.h
@@ -186,6 +186,15 @@ typedef struct {
   /* lookup contexts where a given ACL is used */
   u32 **lc_index_vec_by_acl;
 
+  /* input and output policy epochs by interface */
+  u32 *input_policy_epoch_by_sw_if_index;
+  u32 *output_policy_epoch_by_sw_if_index;
+
+  /* whether we need to take the epoch of the session into account */
+  int reclassify_sessions;
+
+
+
   /* Total count of interface+direction pairs enabled */
   u32 fa_total_enabled_count;
 

diff --git a/src/plugins/acl/fa_node.c b/src/plugins/acl/fa_node.c
index d29576a..833b0fa 100644 (file)
--- a/src/plugins/acl/fa_node.c
+++ b/src/plugins/acl/fa_node.c
@@ -589,7 +589,7 @@ acl_fa_try_recycle_session (acl_main_t * am, int is_input, u16 thread_index, u32
 
 static fa_session_t *
 acl_fa_add_session (acl_main_t * am, int is_input, u32 sw_if_index, u64 now,
-                   fa_5tuple_t * p5tuple)
+                   fa_5tuple_t * p5tuple, u16 current_policy_epoch)
 {
   clib_bihash_kv_40_8_t *pkv = &p5tuple->kv;
   clib_bihash_kv_40_8_t kv;
@@ -603,6 +603,7 @@ acl_fa_add_session (acl_main_t * am, int is_input, u32 sw_if_index, u64 now,
 
   pool_get_aligned (pw->fa_sessions_pool, sess, CLIB_CACHE_LINE_BYTES);
   f_sess_id.session_index = sess - pw->fa_sessions_pool;
+  f_sess_id.intf_policy_epoch = current_policy_epoch;
 
   kv.key[0] = pkv->key[0];
   kv.key[1] = pkv->key[1];
@@ -662,6 +663,7 @@ acl_fa_node_fn (vlib_main_t * vm,
   vlib_node_runtime_t *error_node;
   u64 now = clib_cpu_time_now ();
   uword thread_index = os_get_thread_index ();
+  acl_fa_per_worker_data_t *pw = &am->per_worker_data[thread_index];
 
   from = vlib_frame_vector_args (frame);
   n_left_from = frame->n_vectors;
@@ -709,6 +711,10 @@ acl_fa_node_fn (vlib_main_t * vm,
            lc_index0 = am->input_lc_index_by_sw_if_index[sw_if_index0];
          else
            lc_index0 = am->output_lc_index_by_sw_if_index[sw_if_index0];
+
+          u32 **p_epoch_vec = is_input ? &am->input_policy_epoch_by_sw_if_index
+                                       :  &am->output_policy_epoch_by_sw_if_index;
+          u16 current_policy_epoch = sw_if_index0 < vec_len(*p_epoch_vec) ? vec_elt(*p_epoch_vec, sw_if_index0) : (is_input * FA_POLICY_EPOCH_IS_INPUT);
          /*
           * Extract the L3/L4 matching info into a 5-tuple structure,
           * then create a session key whose layout is independent on forward or reverse
@@ -782,6 +788,21 @@ acl_fa_node_fn (vlib_main_t * vm,
                     acl_check_needed = 0;
                     action = 0;
                   }
+                  if (PREDICT_FALSE(am->reclassify_sessions)) {
+                   /* if the MSB of policy epoch matches but not the LSB means it is a stale session */
+                   if ( (0 == ((current_policy_epoch ^ f_sess_id.intf_policy_epoch) & FA_POLICY_EPOCH_IS_INPUT))
+                        && (current_policy_epoch != f_sess_id.intf_policy_epoch) ) {
+                      /* delete session and increment the counter */
+                      vec_validate (pw->fa_session_epoch_change_by_sw_if_index, sw_if_index0);
+                      vec_elt (pw->fa_session_epoch_change_by_sw_if_index, sw_if_index0)++;
+                      if(acl_fa_conn_list_delete_session(am, f_sess_id)) {
+                        /* delete the session only if we were able to unlink it */
+                        acl_fa_delete_session (am, sw_if_index0, f_sess_id);
+                      }
+                      acl_check_needed = 1;
+                      trace_bitmap |= 0x40000000;
+                    }
+                  }
                }
            }
 
@@ -804,7 +825,7 @@ acl_fa_node_fn (vlib_main_t * vm,
                       if (PREDICT_TRUE (valid_new_sess)) {
                         fa_session_t *sess = acl_fa_add_session (am, is_input,
                                                                  sw_if_index0,
-                                                                 now, &kv_sess);
+                                                                 now, &kv_sess, current_policy_epoch);
                         acl_fa_track_session (am, is_input, sw_if_index0, now,
                                               sess, &fa_5tuple);
                         pkts_new_session += 1;

diff --git a/src/plugins/acl/fa_node.h b/src/plugins/acl/fa_node.h
index dc4f87f..263cf14 100644 (file)
--- a/src/plugins/acl/fa_node.h
+++ b/src/plugins/acl/fa_node.h
@@ -76,6 +76,10 @@ typedef struct {
   u64 reserved2[5];       /* +5*8 bytes = 64 */
 } fa_session_t;
 
+#define FA_POLICY_EPOCH_MASK 0x7fff
+/* input policy epochs have the MSB set */
+#define FA_POLICY_EPOCH_IS_INPUT 0x8000
+
 
 /* This structure is used to fill in the u64 value
    in the per-sw-if-index hash table */
@@ -85,7 +89,7 @@ typedef struct {
     struct {
       u32 session_index;
       u16 thread_index;
-      u16 reserved0;
+      u16 intf_policy_epoch;
     };
   };
 } fa_full_session_id_t;
@@ -117,6 +121,8 @@ typedef struct {
   /* adds and deletes per-worker-per-interface */
   u64 *fa_session_dels_by_sw_if_index;
   u64 *fa_session_adds_by_sw_if_index;
+  /* sessions deleted due to epoch change */
+  u64 *fa_session_epoch_change_by_sw_if_index;
   /* Vector of expired connections retrieved from lists */
   u32 *expired;
   /* the earliest next expiry time */