From 525c9d0f8645ef9901316f042c195adc970b4546 Mon Sep 17 00:00:00 2001 From: Dave Barach Date: Sat, 26 May 2018 10:48:55 -0400 Subject: [PATCH] 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 --- src/plugins/nat/nat.c | 3 +- src/vnet/dhcp/client.c | 24 ++++++++++--- src/vnet/dhcp/client.h | 2 +- src/vnet/feature/feature.c | 46 ++++++++++++++++++++---- src/vnet/feature/feature.h | 3 +- src/vnet/interface_cli.c | 90 ++++++++++++++++++++++++++-------------------- 6 files changed, 116 insertions(+), 52 deletions(-) diff --git a/src/plugins/nat/nat.c b/src/plugins/nat/nat.c index ae34f235a3d..8ebec585d8a 100755 --- 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 dbcb2a53dbc..11e47f9ad7a 100644 --- 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 0334041652e..d3be3ebf271 100644 --- 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 714e20e8e72..e1cea21d7b6 100644 --- 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 ce9e2ca3570..81224ebf0e2 100644 --- 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 d151335aa1f..b803a31c05f 100644 --- 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] [ [ [..]]]", + .short_help = "show interface [address|addr|features|feat] [ [ [..]]] [verbose]", .function = show_sw_interfaces, }; /* *INDENT-ON* */ -- 2.16.6