On Mon, Oct 19, 2026 at 10:52 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]> > --- > Hi Mark, thank you for the patch I have one question down below. > 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) > { > There are essentially 3 places calling this function, I wonder if it would be less invasive if we just changed the function to accept ovn_datapath and perform additional check if the port is within the datapath. This way we could avoid the new helper function and overall the code wouldn't change much. WDYT? - *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 > > Regards, Ales _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
