VPP-1294: add missing feature arc constraint

the ip4-dhcp-client-detect feature MUST run prior to nat44-out2in, or
inbound dhcp broadcast packets will be dropped. Certain dhcp servers
answer lease renewal dhcp-request packets with broadcast dhcp-acks, leading
to unrecoverable lease loss.

In detail, this constraint:

VNET_FEATURE_INIT (ip4_snat_out2in, static) = {
  .arc_name = "ip4-unicast",
  .node_name = "nat44-out2in",
  .runs_after = VNET_FEATURES ("acl-plugin-in-ip4-fa"),
};

doesn't get the job done:

ip4-unicast:
  [17] nat44-out2in
  [23] ip4-dhcp-client-detect
  [26] ip4-not-enabled

Add a proper constraint:

VNET_FEATURE_INIT (ip4_snat_out2in, static) = {
  .arc_name = "ip4-unicast",
  .node_name = "nat44-out2in",
  .runs_after = VNET_FEATURES ("acl-plugin-in-ip4-fa",
                               "ip4-dhcp-client-detect"),
};

and the interface feature order is OK, at least in this regard:

ip4-unicast:
  [17] ip4-dhcp-client-detect
  [18] nat44-out2in
  [26] ip4-not-enabled

We need to carefully audit (especially) the ip4-unicast feature arc,
which has [gasp] 37 features on it!

Change-Id: I5e749ead7ab2a25d80839a331de6261e112977ad
Signed-off-by: Dave Barach <dave@barachs.net>

diff --git a/src/plugins/nat/nat.c b/src/plugins/nat/nat.c
index ae34f23..8ebec58 100755 (executable)
--- a/src/plugins/nat/nat.c
+++ b/src/plugins/nat/nat.c
@@ -44,7 +44,8 @@ VNET_FEATURE_INIT (ip4_snat_in2out, static) = {
 VNET_FEATURE_INIT (ip4_snat_out2in, static) = {
   .arc_name = "ip4-unicast",
   .node_name = "nat44-out2in",
-  .runs_after = VNET_FEATURES ("acl-plugin-in-ip4-fa"),
+  .runs_after = VNET_FEATURES ("acl-plugin-in-ip4-fa", 
+                               "ip4-dhcp-client-detect"),
 };
 VNET_FEATURE_INIT (ip4_nat_classify, static) = {
   .arc_name = "ip4-unicast",

diff --git a/src/vnet/dhcp/client.c b/src/vnet/dhcp/client.c
index dbcb2a5..11e47f9 100644 (file)
--- a/src/vnet/dhcp/client.c
+++ b/src/vnet/dhcp/client.c
@@ -106,6 +106,7 @@ dhcp_client_addr_callback (dhcp_client_t * c)
   vnet_feature_enable_disable ("ip4-unicast",
                               "ip4-dhcp-client-detect",
                               c->sw_if_index, 0 /* disable */ , 0, 0);
+  c->client_detect_feature_enabled = 0;
 
   /* if renewing the lease, the address and route have already been added */
   if (c->state == DHCP_BOUND)
@@ -220,6 +221,7 @@ dhcp_client_for_us (u32 bi, vlib_buffer_t * b,
          {
            u32 lease_time_in_seconds =
              clib_host_to_net_u32 (o->data_as_u32[0]);
+           // for debug: lease_time_in_seconds = 20; /*$$$$*/
            c->lease_expires = now + (f64) lease_time_in_seconds;
            c->lease_lifetime = lease_time_in_seconds;
            /* Set a sensible default, in case we don't get opt 58 */
@@ -307,6 +309,7 @@ dhcp_client_for_us (u32 bi, vlib_buffer_t * b,
                                           "ip4-dhcp-client-detect",
                                           c->sw_if_index, 1 /* enable */ ,
                                           0, 0);
+             c->client_detect_feature_enabled = 1;
            }
          /* Wipe out any memory of the address we had... */
          c->state = DHCP_DISCOVER;
@@ -577,6 +580,15 @@ dhcp_discover_state (dhcp_client_main_t * dcm, dhcp_client_t * c, f64 now)
    * State machine "DISCOVER" state. Send a dhcp discover packet,
    * eventually back off the retry rate.
    */
+
+  if (c->client_detect_feature_enabled == 0)
+    {
+      vnet_feature_enable_disable ("ip4-unicast",
+                                  "ip4-dhcp-client-detect",
+                                  c->sw_if_index, 1 /* enable */ , 0, 0);
+      c->client_detect_feature_enabled = 1;
+    }
+
   send_dhcp_pkt (dcm, c, DHCP_PACKET_DISCOVER, 1 /* is_broadcast */ );
 
   c->retry_count++;
@@ -623,10 +635,13 @@ dhcp_bound_state (dhcp_client_main_t * dcm, dhcp_client_t * c, f64 now)
    * DHCP address. Turn it back on again on first renew attempt.
    * Otherwise, if the DHCP server replies we'll never see it.
    */
-  if (!c->retry_count)
-    vnet_feature_enable_disable ("ip4-unicast",
-                                "ip4-dhcp-client-detect",
-                                c->sw_if_index, 1 /* enable */ , 0, 0);
+  if (c->client_detect_feature_enabled == 0)
+    {
+      vnet_feature_enable_disable ("ip4-unicast",
+                                  "ip4-dhcp-client-detect",
+                                  c->sw_if_index, 1 /* enable */ , 0, 0);
+      c->client_detect_feature_enabled = 1;
+    }
 
   send_dhcp_pkt (dcm, c, DHCP_PACKET_REQUEST, 0 /* is_broadcast */ );
 
@@ -928,6 +943,7 @@ dhcp_client_add_del (dhcp_client_add_del_args_t * a)
       vnet_feature_enable_disable ("ip4-unicast",
                                   "ip4-dhcp-client-detect",
                                   c->sw_if_index, 1 /* enable */ , 0, 0);
+      c->client_detect_feature_enabled = 1;
 
       vlib_process_signal_event (vm, dhcp_client_process_node.index,
                                 EVENT_DHCP_CLIENT_WAKEUP, c - dcm->clients);

diff --git a/src/vnet/dhcp/client.h b/src/vnet/dhcp/client.h
index 0334041..d3be3eb 100644 (file)
--- a/src/vnet/dhcp/client.h
+++ b/src/vnet/dhcp/client.h
@@ -76,7 +76,7 @@ typedef struct
   u8 set_broadcast_flag;
   /* Interface MAC address, so we can do an rx-packet-for-us check */
   u8 client_hardware_address[6];
-  u8 pad1;
+  u8 client_detect_feature_enabled;
 
   void *event_callback;
 } dhcp_client_t;

diff --git a/src/vnet/feature/feature.c b/src/vnet/feature/feature.c
index 714e20e..e1cea21 100644 (file)
--- a/src/vnet/feature/feature.c
+++ b/src/vnet/feature/feature.c
@@ -250,6 +250,14 @@ vnet_feature_enable_disable (const char *arc_name, const char *node_name,
                                                 n_feature_config_bytes);
 }
 
+static int
+feature_cmp (void *a1, void *a2)
+{
+  vnet_feature_registration_t *reg1 = a1;
+  vnet_feature_registration_t *reg2 = a2;
+
+  return (int) reg1->feature_index - reg2->feature_index;
+}
 
 /** Display the set of available driver features.
     Useful for verifying that expected features are present
@@ -262,24 +270,45 @@ show_features_command_fn (vlib_main_t * vm,
   vnet_feature_main_t *fm = &feature_main;
   vnet_feature_arc_registration_t *areg;
   vnet_feature_registration_t *freg;
+  vnet_feature_registration_t *feature_regs = 0;
+  int verbose = 0;
+
+  if (unformat (input, "verbose"))
+    verbose = 1;
 
   vlib_cli_output (vm, "Available feature paths");
 
   areg = fm->next_arc;
   while (areg)
     {
-      vlib_cli_output (vm, "%s:", areg->arc_name);
+      if (verbose)
+       vlib_cli_output (vm, "[%2d] %s:", areg->feature_arc_index,
+                        areg->arc_name);
+      else
+       vlib_cli_output (vm, "%s:", areg->arc_name);
+
       freg = fm->next_feature_by_arc[areg->feature_arc_index];
       while (freg)
        {
-         vlib_cli_output (vm, "  %s\n", freg->node_name);
+         vec_add1 (feature_regs, freg[0]);
          freg = freg->next_in_arc;
        }
 
+      vec_sort_with_function (feature_regs, feature_cmp);
 
+      vec_foreach (freg, feature_regs)
+      {
+       if (verbose)
+         vlib_cli_output (vm, "  [%2d]: %s\n", freg->feature_index,
+                          freg->node_name);
+       else
+         vlib_cli_output (vm, "  %s\n", freg->node_name);
+      }
+      vec_reset_length (feature_regs);
       /* next */
       areg = areg->next;
     }
+  vec_free (feature_regs);
 
   return 0;
 }
@@ -289,7 +318,7 @@ show_features_command_fn (vlib_main_t * vm,
  *
  * @cliexpar
  * Example:
- * @cliexcmd{show ip features}
+ * @cliexcmd{show features [verbose]}
  * @cliexend
  * @endparblock
 ?*/
@@ -306,7 +335,7 @@ VLIB_CLI_COMMAND (show_features_command, static) = {
  */
 
 void
-vnet_interface_features_show (vlib_main_t * vm, u32 sw_if_index)
+vnet_interface_features_show (vlib_main_t * vm, u32 sw_if_index, int verbose)
 {
   vnet_feature_main_t *fm = &feature_main;
   u32 node_index, current_config_index;
@@ -320,7 +349,7 @@ vnet_interface_features_show (vlib_main_t * vm, u32 sw_if_index)
   vlib_node_t *n;
   int i;
 
-  vlib_cli_output (vm, "Driver feature paths configured on %U...",
+  vlib_cli_output (vm, "Feature paths configured on %U...",
                   format_vnet_sw_if_index_name,
                   vnet_get_main (), sw_if_index);
 
@@ -361,7 +390,10 @@ vnet_interface_features_show (vlib_main_t * vm, u32 sw_if_index)
          feat = cfg->features + i;
          node_index = feat->node_index;
          n = vlib_get_node (vm, node_index);
-         vlib_cli_output (vm, "  %v", n->name);
+         if (verbose)
+           vlib_cli_output (vm, "  [%2d] %v", feat->feature_index, n->name);
+         else
+           vlib_cli_output (vm, "  %v", n->name);
        }
     }
 }
@@ -396,6 +428,8 @@ set_interface_features_command_fn (vlib_main_t * vm,
        enable = 0;
       else
        {
+         if (feature_name && arc_name)
+           break;
          error = unformat_parse_error (line_input);
          goto done;
        }

diff --git a/src/vnet/feature/feature.h b/src/vnet/feature/feature.h
index ce9e2ca..81224eb 100644 (file)
--- a/src/vnet/feature/feature.h
+++ b/src/vnet/feature/feature.h
@@ -397,7 +397,8 @@ clib_error_t *vnet_feature_arc_init (vlib_main_t * vm,
                                     vnet_feature_registration_t *
                                     first_reg, char ***feature_nodes);
 
-void vnet_interface_features_show (vlib_main_t * vm, u32 sw_if_index);
+void vnet_interface_features_show (vlib_main_t * vm, u32 sw_if_index,
+                                  int verbose);
 
 #endif /* included_feature_h */
 

diff --git a/src/vnet/interface_cli.c b/src/vnet/interface_cli.c
index d151335..b803a31 100644 (file)
--- a/src/vnet/interface_cli.c
+++ b/src/vnet/interface_cli.c
@@ -270,36 +270,47 @@ show_sw_interfaces (vlib_main_t * vm,
 {
   clib_error_t *error = 0;
   vnet_main_t *vnm = vnet_get_main ();
+  unformat_input_t _linput, *linput = &_linput;
   vnet_interface_main_t *im = &vnm->interface_main;
   vnet_sw_interface_t *si, *sorted_sis = 0;
   u32 sw_if_index = ~(u32) 0;
   u8 show_addresses = 0;
   u8 show_features = 0;
   u8 show_tag = 0;
+  int verbose = 0;
 
-  while (unformat_check_input (input) != UNFORMAT_END_OF_INPUT)
+  /*
+   * Get a line of input. Won't work if the user typed
+   * "show interface" and nothing more.
+   */
+  if (unformat_user (input, unformat_line_input, linput))
     {
-      /* See if user wants to show specific interface */
-      if (unformat
-         (input, "%U", unformat_vnet_sw_interface, vnm, &sw_if_index))
+      while (unformat_check_input (linput) != UNFORMAT_END_OF_INPUT)
        {
-         si = pool_elt_at_index (im->sw_interfaces, sw_if_index);
-         vec_add1 (sorted_sis, si[0]);
-       }
-      else if (unformat (input, "address") || unformat (input, "addr"))
-       show_addresses = 1;
-      else if (unformat (input, "features") || unformat (input, "feat"))
-       show_features = 1;
-      else if (unformat (input, "tag"))
-       show_tag = 1;
-      else
-       {
-         error = clib_error_return (0, "unknown input `%U'",
-                                    format_unformat_error, input);
-         goto done;
+         /* See if user wants to show specific interface */
+         if (unformat
+             (linput, "%U", unformat_vnet_sw_interface, vnm, &sw_if_index))
+           {
+             si = pool_elt_at_index (im->sw_interfaces, sw_if_index);
+             vec_add1 (sorted_sis, si[0]);
+           }
+         else if (unformat (linput, "address") || unformat (linput, "addr"))
+           show_addresses = 1;
+         else if (unformat (linput, "features") || unformat (linput, "feat"))
+           show_features = 1;
+         else if (unformat (linput, "tag"))
+           show_tag = 1;
+         else if (unformat (linput, "verbose"))
+           verbose = 1;
+         else
+           {
+             error = clib_error_return (0, "unknown input `%U'",
+                                        format_unformat_error, linput);
+             goto done;
+           }
        }
+      unformat_free (linput);
     }
-
   if (show_features || show_tag)
     {
       if (sw_if_index == ~(u32) 0)
@@ -308,7 +319,7 @@ show_sw_interfaces (vlib_main_t * vm,
 
   if (show_features)
     {
-      vnet_interface_features_show (vm, sw_if_index);
+      vnet_interface_features_show (vm, sw_if_index, verbose);
 
       l2_input_config_t *l2_input = l2input_intf_config (sw_if_index);
       u32 fb = l2_input->feature_bitmap;
@@ -344,14 +355,14 @@ show_sw_interfaces (vlib_main_t * vm,
       sorted_sis =
        vec_new (vnet_sw_interface_t, pool_elts (im->sw_interfaces));
       _vec_len (sorted_sis) = 0;
-      pool_foreach (si, im->sw_interfaces, (
-                                            {
-                                            int visible =
-                                            vnet_swif_is_api_visible (si);
-                                            if (visible)
-                                            vec_add1 (sorted_sis, si[0]);}
-                   ));
-
+      /* *INDENT-OFF* */
+      pool_foreach (si, im->sw_interfaces,
+      ({
+        int visible = vnet_swif_is_api_visible (si);
+        if (visible)
+          vec_add1 (sorted_sis, si[0]);}
+        ));
+      /* *INDENT-OFF* */
       /* Sort by name. */
       vec_sort_with_function (sorted_sis, sw_interface_name_compare);
     }
@@ -438,25 +449,26 @@ show_sw_interfaces (vlib_main_t * vm,
                             format_ip6_address, r6, ia->address_length);
         }));
        /* *INDENT-ON* */
-      }
-    }
-  else
-    {
-      vec_foreach (si, sorted_sis)
-      {
-       vlib_cli_output (vm, "%U\n", format_vnet_sw_interface, vnm, si);
-      }
     }
+}
+
+else
+{
+  vec_foreach (si, sorted_sis)
+  {
+    vlib_cli_output (vm, "%U\n", format_vnet_sw_interface, vnm, si);
+  }
+}
 
 done:
-  vec_free (sorted_sis);
-  return error;
+vec_free (sorted_sis);
+return error;
 }
 
 /* *INDENT-OFF* */
 VLIB_CLI_COMMAND (show_sw_interfaces_command, static) = {
   .path = "show interface",
-  .short_help = "show interface [address|addr|features|feat] [<interface> [<interface> [..]]]",
+  .short_help = "show interface [address|addr|features|feat] [<interface> [<interface> [..]]] [verbose]",
   .function = show_sw_interfaces,
 };
 /* *INDENT-ON* */