On Thu, Mar 12, 2026 at 7:30 PM Mark Michelson <[email protected]> wrote:
> Thanks for the fix Ales, it looks right by me. > > Acked-by: Mark Michelson <[email protected]> > > On Tue, Mar 10, 2026 at 7:10 AM Ales Musil via dev > <[email protected]> wrote: > > > > The configuration of "discard" and ECMP is prohibited by the > > ovn-nbctl. However, nothing prevents from this configuration to > > happen directly at NB level. In case of this configuration northd > > would crash during deref of the route nexthop which is NULL in the > > case of "discard" route. To prevent that make sure we drop all > > traffic for given ECMP group, at the same time log a warning into > > northd logs as this isn't really a valid configuration. > > > > Fixes: 494e59e341b0 ("Static Routes: Add ability to specify "discard" > nexthop") > > Reported-at: https://github.com/ovn-org/ovn/issues/302 > > Signed-off-by: Ales Musil <[email protected]> > > --- > > northd/en-group-ecmp-route.c | 11 ++++- > > northd/en-group-ecmp-route.h | 1 + > > northd/northd.c | 23 ++++++++--- > > tests/ovn-northd.at | 80 ++++++++++++++++++++++++++++++++++++ > > 4 files changed, 108 insertions(+), 7 deletions(-) > > > > diff --git a/northd/en-group-ecmp-route.c b/northd/en-group-ecmp-route.c > > index cd4cdb991..c4c93fd84 100644 > > --- a/northd/en-group-ecmp-route.c > > +++ b/northd/en-group-ecmp-route.c > > @@ -206,12 +206,21 @@ static void > > ecmp_groups_add_route(struct ecmp_groups_node *group, > > const struct parsed_route *route) > > { > > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); > > if (group->route_count == UINT16_MAX) { > > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); > > VLOG_WARN_RL(&rl, "too many routes in a single ecmp group."); > > return; > > } > > > > + if (route->is_discard_route) { > > + group->has_discard_route = true; > > + > > + char *prefix = normalize_v46_prefix(&route->prefix, > route->plen); > > + VLOG_WARN_RL(&rl, "The ECMP route \"%s\" contains \"discard\" " > > + "route, the whole group will drop traffic.", > prefix); > > + free(prefix); > > + } > > + > > struct ecmp_route_list_node er = (struct ecmp_route_list_node) { > > .route = route, > > .id = ++group->route_count, > > diff --git a/northd/en-group-ecmp-route.h b/northd/en-group-ecmp-route.h > > index 784004347..d4a3248d0 100644 > > --- a/northd/en-group-ecmp-route.h > > +++ b/northd/en-group-ecmp-route.h > > @@ -34,6 +34,7 @@ struct ecmp_groups_node { > > struct in6_addr prefix; > > unsigned int plen; > > bool is_src_route; > > + bool has_discard_route; > > enum route_source source; > > uint32_t route_table_id; > > uint16_t route_count; > > diff --git a/northd/northd.c b/northd/northd.c > > index 0da15629e..7c6876dad 100644 > > --- a/northd/northd.c > > +++ b/northd/northd.c > > @@ -12622,7 +12622,21 @@ build_ecmp_route_flow(struct lflow_table > *lflows, > > struct sset visited_ports = SSET_INITIALIZER(&visited_ports); > > VECTOR_FOR_EACH_PTR (&eg->route_list, er) { > > const struct parsed_route *route = er->route; > > - bool is_ipv4_nexthop = IN6_IS_ADDR_V4MAPPED(route->nexthop); > > + > > + ds_clear(&match); > > + ds_put_format(&match, REG_ECMP_GROUP_ID" == %"PRIu16" && " > > + REG_ECMP_MEMBER_ID" == %"PRIu16, > > + eg->id, er->id); > > + ds_clear(&actions); > > + > > + if (eg->has_discard_route) { > > + ds_put_cstr(&actions, debug_drop_action()); > > + ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_ROUTING_ECMP, 100, > > + ds_cstr(&match), ds_cstr(&actions), lflow_ref, > > + WITH_HINT(route->source_hint)); > > + continue; > > + } > > + > > /* Symmetric ECMP reply is only usable on gateway routers. > > * It is NOT usable on distributed routers with a gateway port. > > */ > > @@ -12634,11 +12648,8 @@ build_ecmp_route_flow(struct lflow_table > *lflows, > > route, &route_match, > > lflow_ref); > > } > > - ds_clear(&match); > > - ds_put_format(&match, REG_ECMP_GROUP_ID" == %"PRIu16" && " > > - REG_ECMP_MEMBER_ID" == %"PRIu16, > > - eg->id, er->id); > > - ds_clear(&actions); > > + > > + bool is_ipv4_nexthop = IN6_IS_ADDR_V4MAPPED(route->nexthop); > > ds_put_format(&actions, "%s = ", > > is_ipv4_nexthop ? REG_NEXT_HOP_IPV4 : > REG_NEXT_HOP_IPV6); > > ipv6_format_mapped(route->nexthop, &actions); > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > > index fd049d096..06808c0dd 100644 > > --- a/tests/ovn-northd.at > > +++ b/tests/ovn-northd.at > > @@ -19939,3 +19939,83 @@ done > > > > OVN_CLEANUP_NORTHD > > AT_CLEANUP > > + > > +OVN_FOR_EACH_NORTHD_NO_HV([ > > +AT_SETUP([Static routes - ECMP with discard]) > > +ovn_start > > + > > +check ovn-nbctl > \ > > + -- lr-add lr0 > \ > > + -- lrp-add lr0 lr0-sw1 00:00:00:00:10:01 10.0.10.1/24 > \ > > + -- --ecmp lr-route-add lr0 192.168.10.0/24 10.0.10.10 > \ > > + -- lrp-add lr0 lr0-sw2 00:00:00:00:20:01 10.0.20.1/24 > \ > > + -- --ecmp lr-route-add lr0 192.168.10.0/24 10.0.20.10 > > + > > +check ovn-nbctl --wait=sb sync > > + > > +ovn-sbctl dump-flows lr0 > lr0flows > > +AT_CHECK([grep 'lr_in_ip_routing' lr0flows | grep 'select' | > ovn_strip_lflows], [0], [dnl > > + table=??(lr_in_ip_routing ), priority=196 , match=(reg7 == 0 && > ip4.dst == 192.168.10.0/24), action=(ip.ttl--; flags.loopback = 1; > reg8[[0..15]] = 1; reg8[[16..31]] = select(1, 2);) > > +]) > > + > > +AT_CHECK([grep 'lr_in_ip_routing_ecmp' lr0flows | sed -E 's/== > [[1-9]]+\)/== \?)/' | ovn_strip_lflows], [0], [dnl > > + table=??(lr_in_ip_routing_ecmp), priority=0 , match=(1), > action=(drop;) > > + table=??(lr_in_ip_routing_ecmp), priority=100 , match=(reg8[[0..15]] > == 1 && reg8[[16..31]] == ?), action=(reg0 = 10.0.10.10; reg5 = 10.0.10.1; > eth.src = 00:00:00:00:10:01; outport = "lr0-sw1"; reg9[[9]] = 1; next;) > > + table=??(lr_in_ip_routing_ecmp), priority=100 , match=(reg8[[0..15]] > == 1 && reg8[[16..31]] == ?), action=(reg0 = 10.0.20.10; reg5 = 10.0.20.1; > eth.src = 00:00:00:00:20:01; outport = "lr0-sw2"; reg9[[9]] = 1; next;) > > + table=??(lr_in_ip_routing_ecmp), priority=150 , match=(reg8[[0..15]] > == 0), action=(next;) > > +]) > > + > > +ecmp1=$(fetch_column nb:Logical_Router_Static_Route _uuid > nexthop="10.0.10.10") > > +ecmp2=$(fetch_column nb:Logical_Router_Static_Route _uuid > nexthop="10.0.20.10") > > + > > +# Set one of the ECMP routes as "discard". > > +check ovn-nbctl --wait=sb set Logical_Router_Static_Route $ecmp1 > nexthop="discard" > > + > > +ovn-sbctl dump-flows lr0 > lr0flows > > +AT_CHECK([grep 'lr_in_ip_routing' lr0flows | grep 'select' | > ovn_strip_lflows], [0], [dnl > > + table=??(lr_in_ip_routing ), priority=196 , match=(reg7 == 0 && > ip4.dst == 192.168.10.0/24), action=(ip.ttl--; flags.loopback = 1; > reg8[[0..15]] = 1; reg8[[16..31]] = select(1, 2);) > > +]) > > + > > +AT_CHECK([grep 'lr_in_ip_routing_ecmp' lr0flows | sed -E 's/== > [[1-9]]+\)/== \?)/' | ovn_strip_lflows], [0], [dnl > > + table=??(lr_in_ip_routing_ecmp), priority=0 , match=(1), > action=(drop;) > > + table=??(lr_in_ip_routing_ecmp), priority=100 , match=(reg8[[0..15]] > == 1 && reg8[[16..31]] == ?), action=(drop;) > > + table=??(lr_in_ip_routing_ecmp), priority=100 , match=(reg8[[0..15]] > == 1 && reg8[[16..31]] == ?), action=(drop;) > > + table=??(lr_in_ip_routing_ecmp), priority=150 , match=(reg8[[0..15]] > == 0), action=(next;) > > +]) > > + > > +# Set the second ECMP route as "discard". > > +check ovn-nbctl --wait=sb set Logical_Router_Static_Route $ecmp2 > nexthop="discard" > > + > > +ovn-sbctl dump-flows lr0 > lr0flows > > +AT_CHECK([grep 'lr_in_ip_routing' lr0flows | grep 'select' | > ovn_strip_lflows], [0], [dnl > > + table=??(lr_in_ip_routing ), priority=196 , match=(reg7 == 0 && > ip4.dst == 192.168.10.0/24), action=(ip.ttl--; flags.loopback = 1; > reg8[[0..15]] = 1; reg8[[16..31]] = select(1, 2);) > > +]) > > + > > +AT_CHECK([grep 'lr_in_ip_routing_ecmp' lr0flows | sed -E 's/== > [[1-9]]+\)/== \?)/' | ovn_strip_lflows], [0], [dnl > > + table=??(lr_in_ip_routing_ecmp), priority=0 , match=(1), > action=(drop;) > > + table=??(lr_in_ip_routing_ecmp), priority=100 , match=(reg8[[0..15]] > == 1 && reg8[[16..31]] == ?), action=(drop;) > > + table=??(lr_in_ip_routing_ecmp), priority=100 , match=(reg8[[0..15]] > == 1 && reg8[[16..31]] == ?), action=(drop;) > > + table=??(lr_in_ip_routing_ecmp), priority=150 , match=(reg8[[0..15]] > == 0), action=(next;) > > +]) > > + > > +# Change both ECMP routes back to valid IP. > > +check ovn-nbctl set Logical_Router_Static_Route $ecmp1 > nexthop="10.0.10.10" > > +check ovn-nbctl --wait=sb set Logical_Router_Static_Route $ecmp2 > nexthop="10.0.20.10" > > + > > +ovn-sbctl dump-flows lr0 > lr0flows > > +AT_CHECK([grep 'lr_in_ip_routing' lr0flows | grep 'select' | > ovn_strip_lflows], [0], [dnl > > + table=??(lr_in_ip_routing ), priority=196 , match=(reg7 == 0 && > ip4.dst == 192.168.10.0/24), action=(ip.ttl--; flags.loopback = 1; > reg8[[0..15]] = 1; reg8[[16..31]] = select(1, 2);) > > +]) > > + > > +AT_CHECK([grep 'lr_in_ip_routing_ecmp' lr0flows | sed -E 's/== > [[1-9]]+\)/== \?)/' | ovn_strip_lflows], [0], [dnl > > + table=??(lr_in_ip_routing_ecmp), priority=0 , match=(1), > action=(drop;) > > + table=??(lr_in_ip_routing_ecmp), priority=100 , match=(reg8[[0..15]] > == 1 && reg8[[16..31]] == ?), action=(reg0 = 10.0.10.10; reg5 = 10.0.10.1; > eth.src = 00:00:00:00:10:01; outport = "lr0-sw1"; reg9[[9]] = 1; next;) > > + table=??(lr_in_ip_routing_ecmp), priority=100 , match=(reg8[[0..15]] > == 1 && reg8[[16..31]] == ?), action=(reg0 = 10.0.20.10; reg5 = 10.0.20.1; > eth.src = 00:00:00:00:20:01; outport = "lr0-sw2"; reg9[[9]] = 1; next;) > > + table=??(lr_in_ip_routing_ecmp), priority=150 , match=(reg8[[0..15]] > == 0), action=(next;) > > +]) > > + > > +check grep -q 'The ECMP route "192.168.10.0/24" contains "discard" > route, the whole group will drop traffic' northd/ovn-northd.log > > + > > +OVN_CLEANUP_NORTHD > > +AT_CLEANUP > > +]) > > -- > > 2.53.0 > > > > _______________________________________________ > > dev mailing list > > [email protected] > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > Thank you Mark, applied to main and backported all the way down to 24.03. Regards, Ales _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
