Thanks Jacob. I've addressed both of your findings locally, but I'm going to wait until you have feedback on other patches before I upload a v2.
On Wed, Oct 7, 2026 at 4:17 PM Jacob Tanenbaum <[email protected]> wrote: > > > Thanks for this series I will be getting reviews for all the patches in the > next day or two. Here are a few comments on this one > > On Tue, Oct 6, 2026 at 1:50 PM Mark Michelson via dev > <[email protected]> wrote: >> >> 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]> >> --- >> northd/en-learned-route-sync.c | 9 ++-- >> northd/en-northd.c | 12 ++---- >> northd/northd.c | 75 ++++++++++++++++++++++------------ >> northd/northd.h | 9 ++-- >> tests/ovn-northd.at | 38 +++++++++++++++++ >> 5 files changed, 97 insertions(+), 46 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 480dc61ca..178c53313 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 same comment was not removed from routes_northd_change_handler(), I > think it should be removed, en_routes_run() and > routes_static_route_change_handler() so the dependancy > northd_data->lr_ports will be stale, unless I am mistaken. > >> >> * 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); >> @@ -415,7 +413,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 +442,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 +505,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 f37040b57..4e07942dc 100644 >> --- a/northd/northd.c >> +++ b/northd/northd.c >> @@ -4767,6 +4767,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, >> @@ -12181,7 +12208,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) >> @@ -12194,7 +12221,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)) { >> @@ -12288,7 +12315,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) >> { >> @@ -12308,8 +12335,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; >> } >> >> @@ -12365,7 +12392,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) >> @@ -12401,8 +12427,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; >> } >> >> @@ -12533,7 +12559,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); >> >> @@ -12776,7 +12801,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, >> @@ -12824,7 +12848,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)) { >> @@ -12942,13 +12966,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); >> } >> @@ -13026,13 +13050,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", >> @@ -13068,15 +13092,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)) { >> @@ -15851,7 +15873,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, >> @@ -15943,8 +15965,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, >> @@ -15991,7 +16013,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) >> { >> @@ -16017,12 +16038,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); >> } >> } >> } >> @@ -20471,7 +20492,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 4150157b0..c4bfae177 100644 >> --- a/northd/northd.h >> +++ b/northd/northd.h >> @@ -908,7 +908,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, >> @@ -921,7 +920,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, >> @@ -953,8 +952,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 *); >> @@ -1019,8 +1017,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 f8c144918..c570922ef 100644 >> --- a/tests/ovn-northd.at >> +++ b/tests/ovn-northd.at >> @@ -24619,3 +24619,41 @@ CHECK_NO_CHANGE_AFTER_RECOMPUTE >> 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 > > > The test case here only covers the router-policy case, could you add a check > for the static route case as well? I know the mechanism is the same > but it would directly validate find_static_route_outport() > >> >> +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 >> +]) >> -- >> 2.55.0 >> >> _______________________________________________ >> dev mailing list >> [email protected] >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> > > Thanks, > Jacob Tanenbaum _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
