ip: fix ip4 shallow reassembly output feature handoff

Use a new frame queue for output feature instead of passing frames
to standard feature.

Fixes bug where save_rewrite_length gets overwritten on reassembly
handoff.

Type: fix
Change-Id: I6c6191aec5f1c89e1ca0510a08781e390d327bbf
Signed-off-by: Klement Sekera <[email protected]>

diff --git a/src/vnet/buffer.h b/src/vnet/buffer.h
index 2f34aa4..e60b8ff 100644 (file)
--- a/src/vnet/buffer.h
+++ b/src/vnet/buffer.h
@@ -219,16 +219,12 @@ typedef struct
          struct
          {
            /* input variables */
-           struct
-           {
-             u32 next_index;   /* index of next node - used by custom apps */
-             u32 error_next_index;     /* index of next node if error - used by custom apps */
-           };
+           u32 next_index; /* index of next node - used by custom apps */
+           u32 error_next_index; /* index of next node if error - used by
+                                    custom apps */
+           u8 _save_rewrite_length;
            /* handoff variables */
-           struct
-           {
-             u16 owner_thread_index;
-           };
+           u16 owner_thread_index;
          };
          /* output variables */
          struct
@@ -422,25 +418,26 @@ typedef struct
 STATIC_ASSERT (VNET_REWRITE_TOTAL_BYTES <= VLIB_BUFFER_PRE_DATA_SIZE,
               "VNET_REWRITE_TOTAL_BYTES too big");
 
-STATIC_ASSERT (STRUCT_SIZE_OF (vnet_buffer_opaque_t, ip.save_rewrite_length)
-              == STRUCT_SIZE_OF (vnet_buffer_opaque_t,
-                                 ip.reass.save_rewrite_length)
-              && STRUCT_SIZE_OF (vnet_buffer_opaque_t,
-                                 ip.reass.save_rewrite_length) ==
-              STRUCT_SIZE_OF (vnet_buffer_opaque_t, mpls.save_rewrite_length)
-              && STRUCT_SIZE_OF (vnet_buffer_opaque_t,
-                                 mpls.save_rewrite_length) == 1
-              && VNET_REWRITE_TOTAL_BYTES < UINT8_MAX,
-              "save_rewrite_length member must be able to hold the max value of rewrite length");
-
-STATIC_ASSERT (STRUCT_OFFSET_OF (vnet_buffer_opaque_t, ip.save_rewrite_length)
-              == STRUCT_OFFSET_OF (vnet_buffer_opaque_t,
-                                   ip.reass.save_rewrite_length)
-              && STRUCT_OFFSET_OF (vnet_buffer_opaque_t,
-                                   mpls.save_rewrite_length) ==
-              STRUCT_OFFSET_OF (vnet_buffer_opaque_t,
-                                ip.reass.save_rewrite_length),
-              "save_rewrite_length must be aligned so that reass doesn't overwrite it");
+STATIC_ASSERT (
+  STRUCT_SIZE_OF (vnet_buffer_opaque_t, ip.save_rewrite_length) ==
+      STRUCT_SIZE_OF (vnet_buffer_opaque_t, ip.reass.save_rewrite_length) &&
+    STRUCT_SIZE_OF (vnet_buffer_opaque_t, ip.save_rewrite_length) ==
+      STRUCT_SIZE_OF (vnet_buffer_opaque_t, ip.reass._save_rewrite_length) &&
+    STRUCT_SIZE_OF (vnet_buffer_opaque_t, ip.reass.save_rewrite_length) ==
+      STRUCT_SIZE_OF (vnet_buffer_opaque_t, mpls.save_rewrite_length) &&
+    STRUCT_SIZE_OF (vnet_buffer_opaque_t, mpls.save_rewrite_length) == 1 &&
+    VNET_REWRITE_TOTAL_BYTES < UINT8_MAX,
+  "save_rewrite_length member must be able to hold the max value of rewrite "
+  "length");
+
+STATIC_ASSERT (
+  STRUCT_OFFSET_OF (vnet_buffer_opaque_t, ip.save_rewrite_length) ==
+      STRUCT_OFFSET_OF (vnet_buffer_opaque_t, ip.reass.save_rewrite_length) &&
+    STRUCT_OFFSET_OF (vnet_buffer_opaque_t, ip.save_rewrite_length) ==
+      STRUCT_OFFSET_OF (vnet_buffer_opaque_t, ip.reass._save_rewrite_length) &&
+    STRUCT_OFFSET_OF (vnet_buffer_opaque_t, mpls.save_rewrite_length) ==
+      STRUCT_OFFSET_OF (vnet_buffer_opaque_t, ip.reass.save_rewrite_length),
+  "save_rewrite_length must be aligned so that reass doesn't overwrite it");
 
 /*
  * The opaque field of the vlib_buffer_t is interpreted as a

diff --git a/src/vnet/ip/reass/ip4_sv_reass.c b/src/vnet/ip/reass/ip4_sv_reass.c
index 7c3c2ff..ad8f178 100644 (file)
--- a/src/vnet/ip/reass/ip4_sv_reass.c
+++ b/src/vnet/ip/reass/ip4_sv_reass.c
@@ -150,6 +150,7 @@ typedef struct
   /** Worker handoff */
   u32 fq_index;
   u32 fq_feature_index;
+  u32 fq_output_feature_index;
   u32 fq_custom_context_index;
 
   // reference count for enabling/disabling feature - per interface
@@ -506,16 +507,14 @@ ip4_sv_reass_inline (vlib_main_t *vm, vlib_node_runtime_t *node,
          clib_prefetch_load (p3->data);
        }
 
-      ip4_header_t *ip0 =
-       (ip4_header_t *) u8_ptr_add (vlib_buffer_get_current (b0),
-                                    (is_output_feature ? 1 : 0) *
-                                    vnet_buffer (b0)->
-                                    ip.save_rewrite_length);
-      ip4_header_t *ip1 =
-       (ip4_header_t *) u8_ptr_add (vlib_buffer_get_current (b1),
-                                    (is_output_feature ? 1 : 0) *
-                                    vnet_buffer (b1)->
-                                    ip.save_rewrite_length);
+      ip4_header_t *ip0 = (ip4_header_t *) u8_ptr_add (
+       vlib_buffer_get_current (b0),
+       (ptrdiff_t) (is_output_feature ? 1 : 0) *
+         vnet_buffer (b0)->ip.save_rewrite_length);
+      ip4_header_t *ip1 = (ip4_header_t *) u8_ptr_add (
+       vlib_buffer_get_current (b1),
+       (ptrdiff_t) (is_output_feature ? 1 : 0) *
+         vnet_buffer (b1)->ip.save_rewrite_length);
 
       if (PREDICT_FALSE
          (ip4_get_fragment_more (ip0) || ip4_get_fragment_offset (ip0))
@@ -638,11 +637,10 @@ ip4_sv_reass_inline (vlib_main_t *vm, vlib_node_runtime_t *node,
       b0 = *b;
       b++;
 
-      ip4_header_t *ip0 =
-       (ip4_header_t *) u8_ptr_add (vlib_buffer_get_current (b0),
-                                    (is_output_feature ? 1 : 0) *
-                                    vnet_buffer (b0)->
-                                    ip.save_rewrite_length);
+      ip4_header_t *ip0 = (ip4_header_t *) u8_ptr_add (
+       vlib_buffer_get_current (b0),
+       (ptrdiff_t) (is_output_feature ? 1 : 0) *
+         vnet_buffer (b0)->ip.save_rewrite_length);
       if (PREDICT_FALSE
          (ip4_get_fragment_more (ip0) || ip4_get_fragment_offset (ip0)))
        {
@@ -736,11 +734,10 @@ slow_path:
          bi0 = from[0];
          b0 = vlib_get_buffer (vm, bi0);
 
-         ip4_header_t *ip0 =
-           (ip4_header_t *) u8_ptr_add (vlib_buffer_get_current (b0),
-                                        (is_output_feature ? 1 : 0) *
-                                        vnet_buffer (b0)->
-                                        ip.save_rewrite_length);
+         ip4_header_t *ip0 = (ip4_header_t *) u8_ptr_add (
+           vlib_buffer_get_current (b0),
+           (ptrdiff_t) (is_output_feature ? 1 : 0) *
+             vnet_buffer (b0)->ip.save_rewrite_length);
          if (!ip4_get_fragment_more (ip0) && !ip4_get_fragment_offset (ip0))
            {
              // this is a regular packet - no fragmentation
@@ -899,11 +896,10 @@ slow_path:
              {
                u32 bi0 = vec_elt (reass->cached_buffers, idx);
                vlib_buffer_t *b0 = vlib_get_buffer (vm, bi0);
-               ip0 =
-                 (ip4_header_t *) u8_ptr_add (vlib_buffer_get_current (b0),
-                                              (is_output_feature ? 1 : 0) *
-                                              vnet_buffer (b0)->
-                                              ip.save_rewrite_length);
+               ip0 = (ip4_header_t *) u8_ptr_add (
+                 vlib_buffer_get_current (b0),
+                 (ptrdiff_t) (is_output_feature ? 1 : 0) *
+                   vnet_buffer (b0)->ip.save_rewrite_length);
                u32 next0 = IP4_SV_REASSEMBLY_NEXT_INPUT;
                if (is_feature)
                  {
@@ -1054,7 +1050,6 @@ VLIB_NODE_FN (ip4_sv_reass_node_output_feature) (vlib_main_t * vm,
     false /* is_custom */, false /* with_custom_context */);
 }
 
-
 VLIB_REGISTER_NODE (ip4_sv_reass_node_output_feature) = {
     .name = "ip4-sv-reassembly-output-feature",
     .vector_size = sizeof (u32),
@@ -1066,7 +1061,7 @@ VLIB_REGISTER_NODE (ip4_sv_reass_node_output_feature) = {
         {
                 [IP4_SV_REASSEMBLY_NEXT_INPUT] = "ip4-input",
                 [IP4_SV_REASSEMBLY_NEXT_DROP] = "ip4-drop",
-                [IP4_SV_REASSEMBLY_NEXT_HANDOFF] = "ip4-sv-reass-feature-hoff",
+                [IP4_SV_REASSEMBLY_NEXT_HANDOFF] = "ip4-sv-reass-output-feature-hoff",
         },
 };
 
@@ -1200,7 +1195,7 @@ ip4_sv_reass_set (u32 timeout_ms, u32 max_reassemblies,
       ctx.failure = 0;
       ctx.new_hash = &new_hash;
       clib_bihash_init_16_8 (&new_hash, "ip4-dr", new_nbuckets,
-                            new_nbuckets * 1024);
+                            (uword) new_nbuckets * 1024);
       clib_bihash_foreach_key_value_pair_16_8 (&ip4_sv_reass_main.hash,
                                               ip4_rehash_cb, &ctx);
       if (ctx.failure)
@@ -1260,7 +1255,8 @@ ip4_sv_reass_init_function (vlib_main_t * vm)
                           IP4_SV_REASS_EXPIRE_WALK_INTERVAL_DEFAULT_MS);
 
   nbuckets = ip4_sv_reass_get_nbuckets ();
-  clib_bihash_init_16_8 (&rm->hash, "ip4-dr", nbuckets, nbuckets * 1024);
+  clib_bihash_init_16_8 (&rm->hash, "ip4-dr", nbuckets,
+                        (uword) nbuckets * 1024);
 
   node = vlib_get_node_by_name (vm, (u8 *) "ip4-drop");
   ASSERT (node);
@@ -1269,6 +1265,8 @@ ip4_sv_reass_init_function (vlib_main_t * vm)
   rm->fq_index = vlib_frame_queue_main_init (ip4_sv_reass_node.index, 0);
   rm->fq_feature_index =
     vlib_frame_queue_main_init (ip4_sv_reass_node_feature.index, 0);
+  rm->fq_output_feature_index =
+    vlib_frame_queue_main_init (ip4_sv_reass_node_output_feature.index, 0);
   rm->fq_custom_context_index =
     vlib_frame_queue_main_init (ip4_sv_reass_custom_context_node.index, 0);
 
@@ -1504,20 +1502,26 @@ format_ip4_sv_reass_handoff_trace (u8 * s, va_list * args)
   return s;
 }
 
+struct ip4_sv_reass_hoff_args
+{
+  bool is_feature;
+  bool is_output_feature;
+  bool is_custom_context;
+};
+
 always_inline uword
 ip4_sv_reass_handoff_node_inline (vlib_main_t *vm, vlib_node_runtime_t *node,
-                                 vlib_frame_t *frame, bool is_feature,
-                                 bool is_custom_context)
+                                 vlib_frame_t *frame,
+                                 struct ip4_sv_reass_hoff_args a)
 {
   ip4_sv_reass_main_t *rm = &ip4_sv_reass_main;
 
   vlib_buffer_t *bufs[VLIB_FRAME_SIZE], **b;
   u32 n_enq, n_left_from, *from, *context;
   u16 thread_indices[VLIB_FRAME_SIZE], *ti;
-  u32 fq_index;
 
   from = vlib_frame_vector_args (frame);
-  if (is_custom_context)
+  if (a.is_custom_context)
     context = vlib_frame_aux_args (frame);
 
   n_left_from = frame->n_vectors;
@@ -1526,9 +1530,10 @@ ip4_sv_reass_handoff_node_inline (vlib_main_t *vm, vlib_node_runtime_t *node,
   b = bufs;
   ti = thread_indices;
 
-  fq_index = (is_feature) ? rm->fq_feature_index :
-                                 (is_custom_context ? rm->fq_custom_context_index :
-                                                      rm->fq_index);
+  const u32 fq_index = a.is_output_feature ? rm->fq_output_feature_index :
+                      a.is_feature        ? rm->fq_feature_index :
+                      a.is_custom_context ? rm->fq_custom_context_index :
+                                                  rm->fq_index;
 
   while (n_left_from > 0)
     {
@@ -1547,7 +1552,7 @@ ip4_sv_reass_handoff_node_inline (vlib_main_t *vm, vlib_node_runtime_t *node,
       ti += 1;
       b += 1;
     }
-  if (is_custom_context)
+  if (a.is_custom_context)
     n_enq = vlib_buffer_enqueue_to_thread_with_aux (
       vm, node, fq_index, from, context, thread_indices, frame->n_vectors, 1);
   else
@@ -1566,10 +1571,12 @@ VLIB_NODE_FN (ip4_sv_reass_handoff_node) (vlib_main_t * vm,
                                          vlib_frame_t * frame)
 {
   return ip4_sv_reass_handoff_node_inline (
-    vm, node, frame, false /* is_feature */, false /* is_custom_context */);
+    vm, node, frame,
+    (struct ip4_sv_reass_hoff_args){ .is_feature = false,
+                                    .is_output_feature = false,
+                                    .is_custom_context = false });
 }
 
-
 VLIB_REGISTER_NODE (ip4_sv_reass_handoff_node) = {
   .name = "ip4-sv-reassembly-handoff",
   .vector_size = sizeof (u32),
@@ -1588,7 +1595,10 @@ VLIB_NODE_FN (ip4_sv_reass_custom_context_handoff_node)
 (vlib_main_t *vm, vlib_node_runtime_t *node, vlib_frame_t *frame)
 {
   return ip4_sv_reass_handoff_node_inline (
-    vm, node, frame, false /* is_feature */, true /* is_custom_context */);
+    vm, node, frame,
+    (struct ip4_sv_reass_hoff_args){ .is_feature = false,
+                                    .is_output_feature = false,
+                                    .is_custom_context = true });
 }
 
 VLIB_REGISTER_NODE (ip4_sv_reass_custom_context_handoff_node) = {
@@ -1612,10 +1622,12 @@ VLIB_NODE_FN (ip4_sv_reass_feature_handoff_node) (vlib_main_t * vm,
                                                    vlib_frame_t * frame)
 {
   return ip4_sv_reass_handoff_node_inline (
-    vm, node, frame, true /* is_feature */, false /* is_custom_context */);
+    vm, node, frame,
+    (struct ip4_sv_reass_hoff_args){ .is_feature = true,
+                                    .is_output_feature = false,
+                                    .is_custom_context = false });
 }
 
-
 VLIB_REGISTER_NODE (ip4_sv_reass_feature_handoff_node) = {
   .name = "ip4-sv-reass-feature-hoff",
   .vector_size = sizeof (u32),
@@ -1630,6 +1642,30 @@ VLIB_REGISTER_NODE (ip4_sv_reass_feature_handoff_node) = {
   },
 };
 
+VLIB_NODE_FN (ip4_sv_reass_output_feature_handoff_node)
+(vlib_main_t *vm, vlib_node_runtime_t *node, vlib_frame_t *frame)
+{
+  return ip4_sv_reass_handoff_node_inline (
+    vm, node, frame,
+    (struct ip4_sv_reass_hoff_args){ .is_feature = false,
+                                    .is_output_feature = true,
+                                    .is_custom_context = false });
+}
+
+VLIB_REGISTER_NODE (ip4_sv_reass_output_feature_handoff_node) = {
+  .name = "ip4-sv-reass-output-feature-hoff",
+  .vector_size = sizeof (u32),
+  .n_errors = ARRAY_LEN(ip4_sv_reass_handoff_error_strings),
+  .error_strings = ip4_sv_reass_handoff_error_strings,
+  .format_trace = format_ip4_sv_reass_handoff_trace,
+
+  .n_next_nodes = 1,
+
+  .next_nodes = {
+    [0] = "error-drop",
+  },
+};
+
 #ifndef CLIB_MARCH_VARIANT
 int
 ip4_sv_reass_enable_disable_with_refcnt (u32 sw_if_index, int is_enable)