Hi Dumitru Thanks for the patch. Looks good to me, thanks!
Acked-by: Xavier Simonart <[email protected]> Thanks Xavier On Wed, Jul 8, 2026 at 4:09 PM Dumitru Ceara via dev < [email protected]> wrote: > On 7/8/26 2:00 PM, Dumitru Ceara via dev wrote: > > Commit 4f562e9346c2 ("ovn: Fix the invalid eth.dst and ip6.dst > > set by nd_ns action for certain cases.") added priority-200 > > flows in lr_in_arp_request that pre-computed the solicited-node > > multicast address for each IPv6 static route nexthop. Those > > flows were needed because pinctrl_handle_nd_ns() was using > > ip_flow->ipv6_dst (the original packet's IPv6 destination) > > instead of the nexthop from xxreg0 to derive the solicited-node > > multicast. > > > > After d682437f99a7 ("pinctrl: Use correct IPv6 dst for nd_ns > > action for IPv4 over v6.") fixed pinctrl to read xxreg0 for the > > cross-AF case, extend pinctrl to ALWAYS read xxreg0. This is > > correct because the generic priority-100 nd_ns flow stores the > > nexthop in xxreg0 for all cases, and compose_nd_ns() already > > derives the solicited-node multicast internally when called > > with multicast=true. > > > > Remove the now-redundant priority-200 flows from northd along > > with their documentation. Simplify pinctrl_handle_nd_ns() to > > unconditionally read xxreg0, removing the dl_type branch and > > the manual in6_addr_solicited_node() call. > > > > Assisted-by: Claude Opus 4.6, OpenCode > > Signed-off-by: Dumitru Ceara <[email protected]> > > --- > > Documentation/ref/ovn-logical-flows.7.rst | 15 ------ > > controller/pinctrl.c | 37 +++++--------- > > northd/northd.c | 47 +---------------- > > tests/ovn-northd.at | 2 - > > tests/ovn.at | 62 +++++++++++++++++++++++ > > 5 files changed, 75 insertions(+), 88 deletions(-) > > > > diff --git a/Documentation/ref/ovn-logical-flows.7.rst > b/Documentation/ref/ovn-logical-flows.7.rst > > index 3d9936414f..95f7e0d7e1 100644 > > --- a/Documentation/ref/ovn-logical-flows.7.rst > > +++ b/Documentation/ref/ovn-logical-flows.7.rst > > @@ -3585,21 +3585,6 @@ or IPv6 Neighbor Solicitation request. It holds > the following flows: > > output; > > }; > > > > - Unknown MAC address. For each IPv6 static route associated with the > router > > - with the nexthop IP: *G*, a priority-200 flow for IPv6 packets with > match > > - ``eth.dst == 00:00:00:00:00:00 && xxreg0 == G`` with the following > actions is > > - added:: > > - > > - nd_ns { > > - eth.dst = E; > > - ip6.dst = I > > - nd.target = G; > > - output; > > - }; > > - > > - Where *E* is the multicast mac derived from the Gateway IP, *I* is the > > - solicited-node multicast address corresponding to the target address > *G*. > > - > > Unknown MAC address. A priority-100 flow for IPv6 packets with match > > ``eth.dst == 00:00:00:00:00:00`` has the following actions:: > > > > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > > index 1728aa7901..f898846285 100644 > > --- a/controller/pinctrl.c > > +++ b/controller/pinctrl.c > > @@ -6553,32 +6553,19 @@ pinctrl_handle_nd_ns(struct rconn *swconn, const > struct flow *ip_flow, > > > > in6_generate_lla(ip_flow->dl_src, &ipv6_src); > > > > - /* We might be here without actually currently handling an IPv6 > packet. > > - * This can happen in the case where we route IPv4 packets over an > IPv6 > > - * link (e.g. RFC 5549 / BGP unnumbered, where an IPv4 destination > is > > - * resolved via an IPv6 link-local nexthop). > > - * > > - * In that case we have no destination IPv6 address in the trigger > packet > > - * to reuse. compose_nd_ns() needs a valid destination so it can > derive > > - * the correct solicited-node multicast (ff02::1:ff{addr[13:16]}) > for > > - * eth.dst and ip6.dst -- userdata only sets nd.target on the new > packet > > - * and does not rewrite ip6.dst, so a wrong ipv6_dst here egresses > on the > > - * wire as-is. > > - * > > - * The fallback nd_ns logical flow in S_ROUTER_IN_ARP_REQUEST > stores the > > - * actual IPv6 nexthop in xxreg0 (REG_NEXT_HOP_IPV6) before > invoking the > > - * nd_ns action, so for the IPv4-over-IPv6 case read xxreg0 from the > > - * trigger packet's flow metadata. */ > > - struct in6_addr ipv6_dst = IN6ADDR_EXACT_INIT; > > - if (get_dl_type(ip_flow) == htons(ETH_TYPE_IPV6)) { > > - ipv6_dst = ip_flow->ipv6_dst; > > - } else { > > - ovs_be128 nexthop_be = > > - hton128(flow_get_xxreg(&pin->flow_metadata.flow, 0)); > > - memcpy(&ipv6_dst, &nexthop_be, sizeof ipv6_dst); > > - } > > + /* The nd_ns logical flow in S_ROUTER_IN_ARP_REQUEST stores the > actual > > + * IPv6 nexthop in xxreg0 (REG_NEXT_HOP_IPV6) before invoking the > > + * nd_ns action. Read the nexthop and pass it to compose_nd_ns() > > + * which derives the solicited-node multicast for eth.dst and > > + * ip6.dst. */ > > + struct in6_addr ipv6_nh = IN6ADDR_EXACT_INIT; > > + > > + ovs_be128 nexthop_be = > > + hton128(flow_get_xxreg(&pin->flow_metadata.flow, 0)); > > + memcpy(&ipv6_nh, &nexthop_be, sizeof ipv6_nh); > > + > > compose_nd_ns(&packet, true, ip_flow->dl_src, eth_addr_zero, > &ipv6_src, > > - &ipv6_dst); > > + &ipv6_nh); > > > > /* Reload previous packet metadata and set actions from userdata. */ > > set_actions_and_enqueue_msg(swconn, &packet, > > diff --git a/northd/northd.c b/northd/northd.c > > index 62270893ab..e4cd93969a 100644 > > --- a/northd/northd.c > > +++ b/northd/northd.c > > @@ -16458,53 +16458,10 @@ build_lr_gateway_redirect_flows_for_nats( > > static void > > build_arp_request_flows_for_lrouter( > > struct ovn_datapath *od, struct lflow_table *lflows, > > - struct ds *match, struct ds *actions, > > const struct shash *meter_groups, > > struct lflow_ref *lflow_ref) > > { > > ovs_assert(od->nbr); > > - for (int i = 0; i < od->nbr->n_static_routes; i++) { > > - const struct nbrec_logical_router_static_route *route; > > - > > - route = od->nbr->static_routes[i]; > > - struct in6_addr gw_ip6; > > - unsigned int plen; > > - char *error = ipv6_parse_cidr(route->nexthop, &gw_ip6, &plen); > > - if (error || plen != 128) { > > - free(error); > > - continue; > > - } > > - > > - ds_clear(match); > > - ds_put_format(match, "eth.dst == 00:00:00:00:00:00 && " > > - REGBIT_NEXTHOP_IS_IPV4" == 0 && " > > - REG_NEXT_HOP_IPV6 " == %s", > > - route->nexthop); > > - struct in6_addr sn_addr; > > - struct eth_addr eth_dst; > > - in6_addr_solicited_node(&sn_addr, &gw_ip6); > > - ipv6_multicast_to_ethernet(ð_dst, &sn_addr); > > - > > - char sn_addr_s[INET6_ADDRSTRLEN + 1]; > > - ipv6_string_mapped(sn_addr_s, &sn_addr); > > - > > - ds_clear(actions); > > - ds_put_format(actions, > > - "nd_ns { " > > - "eth.dst = "ETH_ADDR_FMT"; " > > - "ip6.dst = %s; " > > - "nd.target = %s; " > > - "output; " > > - "}; next;", ETH_ADDR_ARGS(eth_dst), sn_addr_s, > > - route->nexthop); > > - > > - ovn_lflow_add(lflows, od, S_ROUTER_IN_ARP_REQUEST, 200, > > - ds_cstr(match), ds_cstr(actions), lflow_ref, > > - WITH_CTRL_METER(copp_meter_get(COPP_ND_NS_RESOLVE, > > - od->nbr->copp, > > - meter_groups)), > > - WITH_HINT(&route->header_)); > > - } > > > > ovn_lflow_add(lflows, od, S_ROUTER_IN_ARP_REQUEST, 100, > > "eth.dst == 00:00:00:00:00:00 && " > > @@ -19716,9 +19673,7 @@ build_lswitch_and_lrouter_iterate_by_lr(struct > ovn_datapath *od, > > build_gateway_redirect_flows_for_lrouter(od, lsi->lflows, > &lsi->match, > > &lsi->actions, > > od->datapath_lflows); > > - build_arp_request_flows_for_lrouter(od, lsi->lflows, &lsi->match, > > - &lsi->actions, > > - lsi->meter_groups, > > + build_arp_request_flows_for_lrouter(od, lsi->lflows, > lsi->meter_groups, > > od->datapath_lflows); > > build_ecmp_stateful_egr_flows_for_lrouter(od, lsi->lflows, > > od->datapath_lflows); > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > > index f82902c61d..886dd9aba0 100644 > > --- a/tests/ovn-northd.at > > +++ b/tests/ovn-northd.at > > @@ -7717,8 +7717,6 @@ AT_CHECK([grep -e "lr_in_arp_request" lr0flows | > ovn_strip_lflows], [0], [dnl > > table=??(lr_in_arp_request ), priority=0 , match=(1), > action=(next;) > > table=??(lr_in_arp_request ), priority=100 , match=(eth.dst == > 00:00:00:00:00:00 && reg9[[9]] == 0), action=(nd_ns { nd.target = xxreg0; > output; }; next;) > > table=??(lr_in_arp_request ), priority=100 , match=(eth.dst == > 00:00:00:00:00:00 && reg9[[9]] == 1), action=(arp { eth.dst = > ff:ff:ff:ff:ff:ff; arp.spa = reg5; arp.tpa = reg0; arp.op = 1; output; }; > next;) > > - table=??(lr_in_arp_request ), priority=200 , match=(eth.dst == > 00:00:00:00:00:00 && reg9[[9]] == 0 && xxreg0 == 2001:db8::10), > action=(nd_ns { eth.dst = 33:33:ff:00:00:10; ip6.dst = ff02::1:ff00:10; > nd.target = 2001:db8::10; output; }; next;) > > - table=??(lr_in_arp_request ), priority=200 , match=(eth.dst == > 00:00:00:00:00:00 && reg9[[9]] == 0 && xxreg0 == 2001:db8::20), > action=(nd_ns { eth.dst = 33:33:ff:00:00:20; ip6.dst = ff02::1:ff00:20; > nd.target = 2001:db8::20; output; }; next;) > > ]) > > > > # ECMP symmetric reply with IPv4 prefix + IPv6 nexthop. > > diff --git a/tests/ovn.at b/tests/ovn.at > > index abd12f0ddb..31a97aead9 100644 > > --- a/tests/ovn.at > > +++ b/tests/ovn.at > > @@ -46586,3 +46586,65 @@ OVN_CLEANUP([hv]) > > AT_CLEANUP > > ]) > > > > +OVN_FOR_EACH_NORTHD([ > > +AT_SETUP([IPv6 static route ND NS resolution]) > > +CHECK_SCAPY > > +ovn_start > > + > > +net_add n1 > > +sim_add hv > > +ovs-vsctl add-br br-phys > > +ovn_attach n1 br-phys 192.168.0.1 > > +ovs-vsctl set open . external-ids:ovn-bridge-mappings=phys:br-phys > > + > > +check ovn-nbctl ls-add sw0 > > +check ovn-nbctl lsp-add sw0 sw0-port1 > > +check ovn-nbctl lsp-set-addresses sw0-port1 "50:54:00:00:00:01 > 2001:db8::3" > > + > > +check ovn-nbctl lr-add lr0 > > +check ovn-nbctl lrp-add lr0 lr0-sw0 00:00:00:00:ff:01 2001:db8::1/64 > > +check ovn-nbctl lsp-add-router-port sw0 sw0-lr0 lr0-sw0 > > + > > +check ovn-nbctl ls-add public > > +check ovn-nbctl lrp-add lr0 lr0-public 00:00:20:20:12:13 > 2001:db8:1::1/64 > > +check ovn-nbctl lsp-add-router-port public public-lr0 lr0-public > > + > > +# localnet port > > +check ovn-nbctl lsp-add-localnet-port public ln-public phys > > + > > +check ovn-nbctl lrp-set-gateway-chassis lr0-public hv 20 > > + > > +# Add an IPv6 static route with an IPv6 nexthop whose MAC is unknown. > > +check ovn-nbctl --wait=hv lr-route-add lr0 2001:db8:2::/64 > 2001:db8:1::cafe > > + > > +ovs-vsctl -- add-port br-int vif1 -- \ > > + set interface vif1 external-ids:iface-id=sw0-port1 \ > > + options:tx_pcap=hv/vif1-tx.pcap \ > > + options:rxq_pcap=hv/vif1-rx.pcap > > + > > +wait_for_ports_up > > + > > +# Send an IPv6 packet from sw0-port1 destined for an address behind the > > +# static route. The nexthop 2001:db8:1::cafe has no MAC binding, so the > > +# router must emit an ND NS to resolve it. > > +packet=$(fmt_pkt "Ether(dst='00:00:00:00:ff:01', > src='50:54:00:00:00:01')/ \ > > + IPv6(src='2001:db8::3', dst='2001:db8:2::42', > hlim=64)/ \ > > + ICMPv6EchoRequest()") > > +check as hv ovs-appctl netdev-dummy/receive vif1 $packet > > + > > +# Verify the ND NS has correct solicited-node multicast addresses > derived > > +# from the nexthop 2001:db8:1::cafe: > > +# eth.dst = 33:33:ff:00:ca:fe (multicast MAC from solicited-node) > > +# ip6.dst = ff02::1:ff00:cafe (solicited-node multicast) > > +# nd.target = 2001:db8:1::cafe (the actual nexthop) > > +nd_ns=$(fmt_pkt "Ether(dst='33:33:ff:00:ca:fe', > src='00:00:20:20:12:13')/ \ > > + IPv6(src='fe80::200:20ff:fe20:1213', \ > > + > dst='ff02::1:ff00:cafe')/ICMPv6ND_NS(tgt='2001:db8:1::cafe')/\ > > + > ICMPv6NDOptSrcLLAddr(lladdr='00:00:20:20:12:13')") > > Ugh, the indentation is off here, but I can fix that in v2 or when > merging it if this patch is acked. > > > + > > +echo $nd_ns > expected_nd_ns > > +OVN_CHECK_PACKETS_CONTAIN([hv/br-phys_n1-tx.pcap], [expected_nd_ns]) > > + > > +OVN_CLEANUP([hv]) > > +AT_CLEANUP > > +]) > > _______________________________________________ > 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
