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
