Static routes and router policies will sometimes look up a router port based on a port name given in the northbound database. The find_route_outport() function used the entire hmap of logical router ports in order to find the corresponding port. The problem with this approach is that it can find a port that belongs to a different router than where the static route or router policy is installed.
This change limits the port search to only the ports on the ovn_datapath where the static route or router policy is configured. This should prevent the potential bug of retrieving a port for a different router. Signed-off-by: Mark Michelson <[email protected]> --- v1 -> v2: * Updated comment in en-routes northd handler. * Updated test case for static routes since this commit affects both static routes and logical router policies. --- northd/en-learned-route-sync.c | 9 ++-- northd/en-northd.c | 13 ++---- northd/northd.c | 75 +++++++++++++++++++++------------ northd/northd.h | 9 ++-- tests/ovn-northd.at | 77 ++++++++++++++++++++++++++++++++++ 5 files changed, 136 insertions(+), 47 deletions(-) diff --git a/northd/en-learned-route-sync.c b/northd/en-learned-route-sync.c index cbd516b68..db309a0b7 100644 --- a/northd/en-learned-route-sync.c +++ b/northd/en-learned-route-sync.c @@ -140,7 +140,6 @@ en_learned_route_sync_run(struct engine_node *node, void *data) static struct parsed_route * parse_route_from_sbrec_route(struct hmap *parsed_routes_out, - const struct hmap *lr_ports, const struct hmap *lr_datapaths, const struct sbrec_learned_route *route) { @@ -186,7 +185,7 @@ parse_route_from_sbrec_route(struct hmap *parsed_routes_out, /* Verify that ip_prefix and nexthop are on the same network. */ const char *lrp_addr_s = NULL; struct ovn_port *out_port = NULL; - if (!find_route_outport(lr_ports, route->logical_port->logical_port, + if (!find_route_outport(od, route->logical_port->logical_port, "static route", route->ip_prefix, route->nexthop, IN6_IS_ADDR_V4MAPPED(nexthop), true, @@ -221,7 +220,7 @@ routes_table_sync( sbrec_learned_route_delete(sb_route); continue; } - parse_route_from_sbrec_route(parsed_routes_out, lr_ports, + parse_route_from_sbrec_route(parsed_routes_out, &lr_datapaths->datapaths, sb_route); @@ -257,8 +256,8 @@ learned_route_sync_sb_learned_route_change_handler(struct engine_node *node, if (sbrec_learned_route_is_new(changed_route)) { struct parsed_route *route = parse_route_from_sbrec_route( - &data->parsed_routes, &northd_data->lr_ports, - &northd_data->lr_datapaths.datapaths, changed_route); + &data->parsed_routes, &northd_data->lr_datapaths.datapaths, + changed_route); if (route) { hmapx_add(&data->trk_data.trk_created_parsed_route, route); continue; diff --git a/northd/en-northd.c b/northd/en-northd.c index 96859e4ae..173c16d14 100644 --- a/northd/en-northd.c +++ b/northd/en-northd.c @@ -290,7 +290,6 @@ route_policies_northd_change_handler(struct engine_node *node, /* This node uses the below data from the en_northd engine node. * See (lr_stateful_get_input_data()) * 1. northd_data->lr_datapaths - * 2. northd_data->lr_ports * This data gets updated when a logical router or logical router port * is created or deleted. * Northd engine node presently falls back to full recompute when @@ -319,8 +318,7 @@ en_route_policies_run(struct engine_node *node, void *data) struct ovn_datapath *od; HMAP_FOR_EACH (od, key_node, &northd_data->lr_datapaths.datapaths) { - build_route_policies(od, &northd_data->lr_ports, - &bfd_data->bfd_connections, + build_route_policies(od, &bfd_data->bfd_connections, &route_policies_data->route_policies, &route_policies_data->bfd_active_connections, &route_policies_data->chain_ids); @@ -341,7 +339,6 @@ routes_northd_change_handler(struct engine_node *node, /* This node uses the below data from the en_northd engine node. * See (lr_stateful_get_input_data()) * 1. northd_data->lr_datapaths - * 2. northd_data->lr_ports * This data gets updated when a logical router or logical router port * is created or deleted. * Northd engine node presently falls back to full recompute when @@ -415,7 +412,7 @@ routes_static_route_change_handler(struct engine_node *node, od->nbr->static_routes[i]; if (nbrec_logical_router_static_route_is_new(sr)) { - pr = parsed_routes_add_static(od, &northd_data->lr_ports, sr, + pr = parsed_routes_add_static(od, sr, &bfd_data->bfd_connections, &routes_data->parsed_routes, &routes_data->route_tables, @@ -444,8 +441,7 @@ routes_static_route_change_handler(struct engine_node *node, } hmapx_add(&routes_data->trk_data.trk_deleted_parsed_route, pr); hmap_remove(&routes_data->parsed_routes, &pr->key_node); - pr = parsed_routes_add_static(od, &northd_data->lr_ports, sr, - &bfd_data->bfd_connections, + pr = parsed_routes_add_static(od, sr, &bfd_data->bfd_connections, &routes_data->parsed_routes, &routes_data->route_tables, &routes_data->bfd_active_connections); @@ -508,8 +504,7 @@ en_routes_run(struct engine_node *node, void *data) route_table_name); } - build_parsed_routes(od, &northd_data->lr_ports, - &bfd_data->bfd_connections, + build_parsed_routes(od, &bfd_data->bfd_connections, &routes_data->parsed_routes, &routes_data->route_tables, &routes_data->bfd_active_connections); diff --git a/northd/northd.c b/northd/northd.c index 7c4cb5a62..75fbd6eb8 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -4766,6 +4766,33 @@ ovn_port_find_in_datapath(struct ovn_datapath *od, return NULL; } +/* Use caution when calling this function. If a port is deleted and re-added + * to the northbound database quickly, it is possible for od to have a deleted + * port named port_name in it that is slated for deletion. Keeping a reference + * to the ovn_port can cause crashes. + * + * In general, consider this function unsafe to call during incremental + * processing of the en_northd node. It is safe to call during a recompute of + * en_northd. It is also safe to call from any node that is downstream from + * en_northd (i.e. they take northd_data as an input). + * + * If you need to retrieve a port by name during en_northd incremental + * processing, use the ovn_port_find_in_datapath() function instead. + */ +static struct ovn_port * +ovn_port_find_in_datapath_by_name(const struct ovn_datapath *od, + const char *port_name) +{ + struct ovn_port *op; + HMAP_FOR_EACH_WITH_HASH (op, dp_node, hash_string(port_name, 0), + &od->ports) { + if (!strcmp(op->key, port_name)) { + return op; + } + } + return NULL; +} + static bool ls_port_init(struct ovn_port *op, struct ovsdb_idl_txn *ovnsb_txn, struct ovn_datapath *od, @@ -12353,7 +12380,7 @@ lrp_find_member_ip(const struct ovn_port *op, const char *ip_s) * in 'p_output_port' and a pointer to the router IP address to be used for * this policy, in 'p_lrp_addr_s'. */ static bool -find_policy_outport(struct ovn_datapath *od, const struct hmap *lr_ports, +find_policy_outport(struct ovn_datapath *od, const struct nbrec_logical_router_policy *policy, const char *nexthop, bool is_ipv4, const char **p_lrp_addr_s, struct ovn_port **p_out_port) @@ -12366,7 +12393,7 @@ find_policy_outport(struct ovn_datapath *od, const struct hmap *lr_ports, const char *lrp_addr_s = NULL; if (policy->output_port) { - if (!find_route_outport(lr_ports, policy->output_port->name, + if (!find_route_outport(od, policy->output_port->name, "policy", policy->match, nexthop, is_ipv4, true, &out_port, &lrp_addr_s)) { @@ -12460,7 +12487,7 @@ static bool check_bfd_state(const struct nbrec_logical_router_policy *rule, static void build_routing_policy_flow(struct lflow_table *lflows, struct ovn_datapath *od, - const struct hmap *lr_ports, struct route_policy *rp, + struct route_policy *rp, const struct ovsdb_idl_row *stage_hint, struct lflow_ref *lflow_ref) { @@ -12480,8 +12507,8 @@ build_routing_policy_flow(struct lflow_table *lflows, struct ovn_datapath *od, const char *lrp_addr_s = NULL; struct ovn_port *out_port = NULL; - if (!find_policy_outport(od, lr_ports, rule, nexthop, is_ipv4, - &lrp_addr_s, &out_port)) { + if (!find_policy_outport(od, rule, nexthop, is_ipv4, &lrp_addr_s, + &out_port)) { return; } @@ -12537,7 +12564,6 @@ build_routing_policy_flow(struct lflow_table *lflows, struct ovn_datapath *od, static void build_ecmp_routing_policy_flows(struct lflow_table *lflows, struct ovn_datapath *od, - const struct hmap *lr_ports, struct route_policy *rp, uint16_t ecmp_group_id, struct lflow_ref *lflow_ref) @@ -12573,8 +12599,8 @@ build_ecmp_routing_policy_flows(struct lflow_table *lflows, const char *lrp_addr_s = NULL; struct ovn_port *out_port = NULL; - if (!find_policy_outport(od, lr_ports, rule, rp->valid_nexthops[i], - is_ipv4, &lrp_addr_s, &out_port)) { + if (!find_policy_outport(od, rule, rp->valid_nexthops[i], is_ipv4, + &lrp_addr_s, &out_port)) { goto cleanup; } @@ -12705,7 +12731,6 @@ route_hash(const struct parsed_route *route) static bool find_static_route_outport(const struct ovn_datapath *od, - const struct hmap *lr_ports, const struct nbrec_logical_router_static_route *route, bool is_ipv4, const char **p_lrp_addr_s, struct ovn_port **p_out_port); @@ -12948,7 +12973,6 @@ parsed_route_add(const struct ovn_datapath *od, struct parsed_route * parsed_routes_add_static(const struct ovn_datapath *od, - const struct hmap *lr_ports, const struct nbrec_logical_router_static_route *route, const struct hmap *bfd_connections, struct hmap *routes, struct simap *route_tables, @@ -12996,7 +13020,7 @@ parsed_routes_add_static(const struct ovn_datapath *od, const char *lrp_addr_s = NULL; struct ovn_port *out_port = NULL; if (!is_discard_route && - !find_static_route_outport(od, lr_ports, route, + !find_static_route_outport(od, route, nexthop ? IN6_IS_ADDR_V4MAPPED(nexthop) : IN6_IS_ADDR_V4MAPPED(&prefix), &lrp_addr_s, &out_port)) { @@ -13114,13 +13138,13 @@ parsed_routes_add_connected(const struct ovn_datapath *od, } void -build_parsed_routes(const struct ovn_datapath *od, const struct hmap *lr_ports, +build_parsed_routes(const struct ovn_datapath *od, const struct hmap *bfd_connections, struct hmap *routes, struct simap *route_tables, struct hmap *bfd_active_connections) { for (size_t i = 0; i < od->nbr->n_static_routes; i++) { - parsed_routes_add_static(od, lr_ports, od->nbr->static_routes[i], + parsed_routes_add_static(od, od->nbr->static_routes[i], bfd_connections, routes, route_tables, bfd_active_connections); } @@ -13198,13 +13222,13 @@ calc_priority(int plen, } bool -find_route_outport(const struct hmap *lr_ports, const char *output_port, +find_route_outport(const struct ovn_datapath *od, const char *output_port, const char *route_type, const char *route_desc, const char *nexthop, bool is_ipv4, bool force_out_port, struct ovn_port **out_port, const char **lrp_addr_s) { - *out_port = ovn_port_find(lr_ports, output_port); + *out_port = ovn_port_find_in_datapath_by_name(od, output_port); if (!*out_port) { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); VLOG_WARN_RL(&rl, "Bad out port %s for %s %s", @@ -13240,15 +13264,13 @@ find_route_outport(const struct hmap *lr_ports, const char *output_port, /* Output: p_lrp_addr_s and p_out_port. */ static bool find_static_route_outport(const struct ovn_datapath *od, - const struct hmap *lr_ports, const struct nbrec_logical_router_static_route *route, bool is_ipv4, const char **p_lrp_addr_s, struct ovn_port **p_out_port) { const char *lrp_addr_s = NULL; struct ovn_port *out_port = NULL; if (route->output_port) { - /* XXX: we should be able to use &od->ports instead of lr_ports. */ - if (!find_route_outport(lr_ports, route->output_port, + if (!find_route_outport(od, route->output_port, "static route", route->ip_prefix, route->nexthop, is_ipv4, true, &out_port, &lrp_addr_s)) { @@ -16023,7 +16045,7 @@ policy_chain_add(struct simap *chain_ids, const char *chain_name) } void -build_route_policies(struct ovn_datapath *od, const struct hmap *lr_ports, +build_route_policies(struct ovn_datapath *od, const struct hmap *bfd_connections, struct hmap *route_policies, struct hmap *bfd_active_connections, @@ -16115,8 +16137,8 @@ build_route_policies(struct ovn_datapath *od, const struct hmap *lr_ports, struct ovn_port *out_port = NULL; bool is_ipv4 = strchr(nexthop, '.') ? true : false; - if (!find_policy_outport(od, lr_ports, rule, nexthop, is_ipv4, - NULL, &out_port)) { + if (!find_policy_outport(od, rule, nexthop, is_ipv4, NULL, + &out_port)) { continue; } if (!check_bfd_state(rule, out_port, nexthop, @@ -16163,7 +16185,6 @@ build_route_policies(struct ovn_datapath *od, const struct hmap *lr_ports, static void build_ingress_policy_flows_for_lrouter( struct ovn_datapath *od, struct lflow_table *lflows, - const struct hmap *lr_ports, struct hmap *route_policies, struct lflow_ref *lflow_ref) { @@ -16189,12 +16210,12 @@ build_ingress_policy_flows_for_lrouter( (!strcmp(rule->action, "reroute") && rule->n_nexthops > 1); if (is_ecmp_reroute) { - build_ecmp_routing_policy_flows(lflows, od, lr_ports, rp, - ecmp_group_id, lflow_ref); + build_ecmp_routing_policy_flows(lflows, od, rp, ecmp_group_id, + lflow_ref); ecmp_group_id++; } else { - build_routing_policy_flow(lflows, od, lr_ports, rp, - &rule->header_, lflow_ref); + build_routing_policy_flow(lflows, od, rp, &rule->header_, + lflow_ref); } } } @@ -20643,7 +20664,7 @@ build_lswitch_and_lrouter_iterate_by_lr(struct ovn_datapath *od, lsi->bfd_ports); build_mcast_lookup_flows_for_lrouter(od, lsi->lflows, &lsi->match, od->datapath_lflows); - build_ingress_policy_flows_for_lrouter(od, lsi->lflows, lsi->lr_ports, + build_ingress_policy_flows_for_lrouter(od, lsi->lflows, lsi->route_policies, od->datapath_lflows); build_arp_resolve_flows_for_lrouter(od, lsi->lflows, od->datapath_lflows); diff --git a/northd/northd.h b/northd/northd.h index 3d1fd2d31..4700407bd 100644 --- a/northd/northd.h +++ b/northd/northd.h @@ -907,7 +907,6 @@ struct parsed_route *parsed_route_add( struct parsed_route *parsed_routes_add_static( const struct ovn_datapath *od, - const struct hmap *lr_ports, const struct nbrec_logical_router_static_route *route, const struct hmap *bfd_connections, struct hmap *routes, struct simap *route_tables, @@ -920,7 +919,7 @@ struct svc_monitors_map_data { }; bool -find_route_outport(const struct hmap *lr_ports, const char *output_port, +find_route_outport(const struct ovn_datapath *od, const char *output_port, const char *route_type, const char *route_desc, const char *nexthop, bool is_ipv4, bool force_out_port, @@ -952,8 +951,7 @@ void northd_indices_create(struct northd_data *data, void route_policies_init(struct route_policies_data *); void route_policies_destroy(struct route_policies_data *); void build_parsed_routes(const struct ovn_datapath *, const struct hmap *, - const struct hmap *, struct hmap *, struct simap *, - struct hmap *); + struct hmap *, struct simap *, struct hmap *); uint32_t get_route_table_id(struct simap *, const char *); void routes_init(struct routes_data *); void routes_destroy(struct routes_data *); @@ -1018,8 +1016,7 @@ bool northd_handle_lb_data_changes(struct tracked_lb_data *, struct northd_tracked_data *); void build_route_policies(struct ovn_datapath *, const struct hmap *, - const struct hmap *, struct hmap *, struct hmap *, - struct simap *); + struct hmap *, struct hmap *, struct simap *); void bfd_table_sync(struct ovsdb_idl_txn *, const struct nbrec_bfd_table *, const struct hmap *, const struct hmap *, const struct hmap *, const struct hmap *, diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index 7a0a400c4..59da884c0 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -24920,3 +24920,80 @@ OVN_CLEANUP_NORTHD AT_CLEANUP ]) +OVN_FOR_EACH_NORTHD_NO_HV([ +AT_SETUP([Router policy misconfigured port]) +ovn_start + +# Logical router policies can specify an outport. This test ensures that +# if the outport does not correspond with a port on the logical router +# where the policy is applied, then we do not generate any sort of bogus +# router policy flows. + +check ovn-nbctl lr-add lr1 +check ovn-nbctl lrp-add lr1 lrp1 00:00:00:00:00:01 10.0.0.1/24 +check ovn-nbctl lr-add lr2 +check ovn-nbctl lrp-add lr2 lrp2 00:00:00:00:00:02 20.0.0.1/24 + +# Our logical router policy will always live on lr1. We'll mess with the +# outport port and see what logical flows we end up with. +check ovn-nbctl --output-port=lrp2 lr-policy-add lr1 100 "ip4.src == 10.0.0.100" reroute 10.0.0.1 +check ovn-nbctl --wait=sb sync + +AT_CHECK([ovn-sbctl lflow-list lr1 > lr1flows]) +AT_CAPTURE_FILE([lr1flows]) + +# Since we configured a port on the wrong logical router, we should not be able +# to find the logical router port and therefore should not have any policy flows. +AT_CHECK([grep "lr_in_policy" lr1flows | grep "priority=100"], [1], [ignore], [ignore]) + +AT_CHECK([ovn-sbctl lflow-list lr2 > lr2flows]) +AT_CAPTURE_FILE([lr2flows]) + +# Just to be safe, let's also ensure the router policy did not get installed on lr2 +AT_CHECK([grep "lr_in_policy" lr2flows | grep "priority=100"], [1], [ignore], [ignore]) + +# Double check that the reason why is due to a bad port configured. +AT_CHECK([grep -qE "Bad out port lrp2 for policy ip4.src == 10.0.0.100" northd/ovn-northd.log], [0]) + +OVN_CLEANUP_NORTHD +AT_CLEANUP +]) + +OVN_FOR_EACH_NORTHD_NO_HV([ +AT_SETUP([Static route misconfigured port]) +ovn_start + +# Static routes can specify an outport. This test ensures that +# if the outport does not correspond with a port on the logical router +# where the static route is applied, then we do not generate any sort of bogus +# route flows. + +check ovn-nbctl lr-add lr1 +check ovn-nbctl lrp-add lr1 lrp1 00:00:00:00:00:01 10.0.0.1/24 +check ovn-nbctl lr-add lr2 +check ovn-nbctl lrp-add lr2 lrp2 00:00:00:00:00:02 20.0.0.1/24 + +# Our static route will always live on lr1. We'll mess with the +# outport port and see what logical flows we end up with. +check ovn-nbctl lr-route-add lr1 100.0.0.0/24 10.0.0.50 lrp2 +check ovn-nbctl --wait=sb sync + +AT_CHECK([ovn-sbctl lflow-list lr1 > lr1flows]) +AT_CAPTURE_FILE([lr1flows]) + +# Since we configured a port on the wrong logical router, we should not be able +# to find the logical router port and therefore should not have any policy flows. +AT_CHECK([grep "lr_in_ip_routing" lr1flows | grep "100.0.0.0"], [1], [ignore], [ignore]) + +AT_CHECK([ovn-sbctl lflow-list lr2 > lr2flows]) +AT_CAPTURE_FILE([lr2flows]) + +# Just to be safe, let's also ensure the router policy did not get installed on lr2 +AT_CHECK([grep "lr_in_policy" lr2flows | grep "priority=100"], [1], [ignore], [ignore]) + +# Double check that the reason why is due to a bad port configured. +AT_CHECK([grep -qE "Bad out port lrp2 for static route 100.0.0.0/24" northd/ovn-northd.log], [0]) + +OVN_CLEANUP_NORTHD +AT_CLEANUP +]) -- 2.55.0 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
