nat: fix EI hairpinning thread safety

Avoid doing inter-thread reads without locks by doing a handoff before
destination address rewrite. Destination address is read from a session
which is possibly owned by a different thread. By splitting the work in
two parts with a handoff in the middle, we can do both in a thread safe
way.

Type: improvement
Signed-off-by: Klement Sekera <ksekera@cisco.com>
Change-Id: I1c50d188393a610f5564fa230c75771a8065f273

src/plugins/nat/nat.c

@@ -2514,6 +2514,13 @@ do                                        \
     vlib_zero_simple_counter (&c, 0);     \
   } while (0);
 
+extern vlib_node_registration_t nat44_hairpinning_node;
+extern vlib_node_registration_t snat_hairpin_dst_node;
+extern vlib_node_registration_t
+  nat44_in2out_hairpinning_finish_ip4_lookup_node;
+extern vlib_node_registration_t
+  nat44_in2out_hairpinning_finish_interface_output_node;
+
 static clib_error_t *
 nat_init (vlib_main_t * vm)
 {
@@ -2632,6 +2639,17 @@ nat_init (vlib_main_t * vm)
   nat_ha_init (vm, sm->num_workers, num_threads);
 
   test_key_calc_split ();
+
+  sm->nat44_hairpinning_fq_index =
+    vlib_frame_queue_main_init (nat44_hairpinning_node.index, 0);
+  sm->snat_hairpin_dst_fq_index =
+    vlib_frame_queue_main_init (snat_hairpin_dst_node.index, 0);
+  sm->nat44_in2out_hairpinning_finish_ip4_lookup_node_fq_index =
+    vlib_frame_queue_main_init (
+      nat44_in2out_hairpinning_finish_ip4_lookup_node.index, 0);
+  sm->nat44_in2out_hairpinning_finish_interface_output_node_fq_index =
+    vlib_frame_queue_main_init (
+      nat44_in2out_hairpinning_finish_interface_output_node.index, 0);
   return nat44_api_hookup (vm);
 }
 

src/plugins/nat/nat.h

@@ -783,6 +783,11 @@ typedef struct snat_main_s
   u8 enabled;
 
   vnet_main_t *vnet_main;
+
+  u32 nat44_in2out_hairpinning_finish_ip4_lookup_node_fq_index;
+  u32 nat44_in2out_hairpinning_finish_interface_output_node_fq_index;
+  u32 nat44_hairpinning_fq_index;
+  u32 snat_hairpin_dst_fq_index;
 } snat_main_t;
 
 typedef struct
@@ -1149,14 +1154,17 @@ u32 icmp_match_out2in_slow (snat_main_t *sm, vlib_node_runtime_t *node,
 
 /* hairpinning functions */
 u32 snat_icmp_hairpinning (snat_main_t *sm, vlib_buffer_t *b0,
-			   ip4_header_t *ip0, icmp46_header_t *icmp0);
+			   u32 thread_index, ip4_header_t *ip0,
+			   icmp46_header_t *icmp0, u32 *required_thread_index);
 
 void nat_hairpinning_sm_unknown_proto (snat_main_t * sm, vlib_buffer_t * b,
 				       ip4_header_t * ip);
+
 int snat_hairpinning (vlib_main_t *vm, vlib_node_runtime_t *node,
-		      snat_main_t *sm, vlib_buffer_t *b0, ip4_header_t *ip0,
-		      udp_header_t *udp0, tcp_header_t *tcp0, u32 proto0,
-		      int do_trace);
+		      snat_main_t *sm, u32 thread_index, vlib_buffer_t *b0,
+		      ip4_header_t *ip0, udp_header_t *udp0,
+		      tcp_header_t *tcp0, u32 proto0, int do_trace,
+		      u32 *required_thread_index);
 
 /* Call back functions for clib_bihash_add_or_overwrite_stale */
 int nat44_i2o_is_idle_session_cb (clib_bihash_kv_8_8_t * kv, void *arg);

src/plugins/nat/nat44_hairpinning.h

@@ -0,0 +1,92 @@
+#ifndef __included_nat44_hairpinning_h__
+#define __included_nat44_hairpinning_h__
+
+#include <nat/nat.h>
+
+#define foreach_nat44_hairpinning_handoff_error                               \
+  _ (CONGESTION_DROP, "congestion drop")
+
+typedef enum
+{
+#define _(sym, str) NAT44_HAIRPINNING_HANDOFF_ERROR_##sym,
+  foreach_nat44_hairpinning_handoff_error
+#undef _
+    NAT44_HAIRPINNING_HANDOFF_N_ERROR,
+} nat44_hairpinning_handoff_error_t;
+
+static char *nat44_hairpinning_handoff_error_strings[] = {
+#define _(sym, string) string,
+  foreach_nat44_hairpinning_handoff_error
+#undef _
+};
+
+typedef struct
+{
+  u32 next_worker_index;
+} nat44_hairpinning_handoff_trace_t;
+
+static u8 *
+format_nat44_hairpinning_handoff_trace (u8 *s, va_list *args)
+{
+  CLIB_UNUSED (vlib_main_t * vm) = va_arg (*args, vlib_main_t *);
+  CLIB_UNUSED (vlib_node_t * node) = va_arg (*args, vlib_node_t *);
+  nat44_hairpinning_handoff_trace_t *t =
+    va_arg (*args, nat44_hairpinning_handoff_trace_t *);
+
+  s = format (s, "nat-hairpinning-handoff: next-worker %d",
+	      t->next_worker_index);
+
+  return s;
+}
+
+always_inline uword
+nat44_hairpinning_handoff_fn_inline (vlib_main_t *vm,
+				     vlib_node_runtime_t *node,
+				     vlib_frame_t *frame, u32 fq_index)
+{
+  vlib_buffer_t *bufs[VLIB_FRAME_SIZE], **b;
+  u32 n_enq, n_left_from, *from;
+  u16 thread_indices[VLIB_FRAME_SIZE], *ti;
+
+  from = vlib_frame_vector_args (frame);
+  n_left_from = frame->n_vectors;
+  vlib_get_buffers (vm, from, bufs, n_left_from);
+
+  b = bufs;
+  ti = thread_indices;
+
+  while (n_left_from > 0)
+    {
+      ti[0] = vnet_buffer (b[0])->snat.required_thread_index;
+
+      if (PREDICT_FALSE ((node->flags & VLIB_NODE_FLAG_TRACE) &&
+			 (b[0]->flags & VLIB_BUFFER_IS_TRACED)))
+	{
+	  nat44_hairpinning_handoff_trace_t *t =
+	    vlib_add_trace (vm, node, b[0], sizeof (*t));
+	  t->next_worker_index = ti[0];
+	}
+
+      n_left_from -= 1;
+      ti += 1;
+      b += 1;
+    }
+  n_enq = vlib_buffer_enqueue_to_thread (vm, fq_index, from, thread_indices,
+					 frame->n_vectors, 1);
+
+  if (n_enq < frame->n_vectors)
+    vlib_node_increment_counter (
+      vm, node->node_index, NAT44_HAIRPINNING_HANDOFF_ERROR_CONGESTION_DROP,
+      frame->n_vectors - n_enq);
+  return frame->n_vectors;
+}
+
+#endif // __included_nat44_hairpinning_h__
+
+/*
+ * fd.io coding-style-patch-verification: ON
+ *
+ * Local Variables:
+ * eval: (c-set-style "gnu")
+ * End:
+ */

src/vnet/buffer.h

@@ -378,6 +378,7 @@ typedef struct
     struct
     {
       u32 flags;
+      u32 required_thread_index;
     } snat;
 
     u32 unused[6];

test/run_tests.py

@@ -272,6 +272,8 @@ def handle_failed_suite(logger, last_test_temp_dir, vpp_pid):
             except Exception as e:
                 logger.exception("Unexpected error running `file' utility "
                                  "on core-file")
+            logger.error("gdb %s %s" %
+                         (os.getenv('VPP_BIN', 'vpp'), core_path))
 
     if vpp_pid:
         # Copy api post mortem