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 > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
