Thanks for the review, Numan! I corrected the issue you pointed out and pushed the change to main and all branches back to 24.03.
On Fri, Oct 17, 2025 at 10:28 AM Mark Michelson <[email protected]> wrote: > > On Thu, Oct 16, 2025 at 4:50 PM Numan Siddique <[email protected]> wrote: > > > > On Thu, Oct 16, 2025 at 2:46 PM Mark Michelson via dev > > <[email protected]> wrote: > > > > > > By default, northd does not install any responder flows for unicast ARP. > > > These are intended to be forwarded to the destination so that the > > > destination can respond appropriately. For VIF ports, this tends to work > > > as expected in most scenarios. > > > > > > When proxy ARP is configured on a logical switch port conntected to a > > > logical router, then we install low priority flows to ensure that > > > ARPs for the configured proxy addresses is responded to by the logical > > > switch. These proxy ARP flows are hit when unicast ARP requests are sent > > > for the VIF ports on the logical switch. We therefore end up responding > > > incorrectly to unicast ARP requests with the proxy ARP MAC instead of > > > forwarding the ARP request to the proper VIF port. > > > > > > This commit fixes the issue by installing explicit "next;" actions for > > > unicast ARP requests directed towards VIFs so that the proxy ARP flows > > > will not be hit. These flows are only installed if proxy ARP is > > > configured, since they are unnecessary otherwise. > > > > > > Reported-at: https://issues.redhat.com/browse/FDP-1646 > > > Signed-off-by: Mark Michelson <[email protected]> > > > > Hi Mark, > > > > There is one small comment. With that addressed: > > > > Acked-by: Numan Siddique <[email protected]> > > > > > > > --- > > > v4: > > > * Changed the logic in northd to be similar to what was in v1. That is, > > > instead of only installing flows for known overlaps of LSP and proxy > > > ARP addresses, we install flows for all LSP addresses if proxy ARP > > > is configured at all. This makes the logic in northd simpler, at the > > > possible expense of adding more logical flows. > > > * Added a check for scapy in the ovn.at test. > > > > > > v3: > > > * Split the new build_lswitch_arp_nd_unicast_flows() function into > > > smaller pieces. This has several benefits: > > > * v2 had several checkpatch warnings about the lines exceeding 80 > > > characters. This made it easier to stay under the line length > > > limit. > > > * v2 had a subtle indexing error that caused a crash in the > > > proxy_arp test. Splitting this up helps make the indexing easier > > > to follow. > > > * This makes the logic much easier to understand, visually. > > > * Fixed some typos in the commit message. > > > > > > v2: > > > * This version also fixes the problem with IPv6 ND_NS packets. The > > > tests have been updated to test for IPv6. > > > * This version only installs unicast ARP/ND_NS flows if the LSP > > > address overlaps with the configured proxy_arp addresses. The > > > ovn-northd test has been updated to ensure that this is correct. The > > > code that does this is pretty gnarly, since it's four levels of > > > for loops. But it works! > > > * Fixed nits from Xavier (removed ofport request, removed unnecessary > > > lflow-list, etc.). > > > --- > > > northd/northd.c | 51 ++++++++++++++++++++++++----- > > > northd/northd.h | 7 ++++ > > > tests/ovn-northd.at | 79 +++++++++++++++++++++++++++++++++++++++++++++ > > > tests/ovn.at | 77 +++++++++++++++++++++++++++++++++++++++++++ > > > 4 files changed, 206 insertions(+), 8 deletions(-) > > > > > > diff --git a/northd/northd.c b/northd/northd.c > > > index 3b1d3eba7..cdcb8550b 100644 > > > --- a/northd/northd.c > > > +++ b/northd/northd.c > > > @@ -9956,15 +9956,32 @@ build_lswitch_arp_nd_responder_known_ips(struct > > > ovn_port *op, > > > for (size_t i = 0; i < op->n_lsp_addrs; i++) { > > > for (size_t j = 0; j < op->lsp_addrs[i].n_ipv4_addrs; j++) { > > > ds_clear(match); > > > - /* Do not reply on unicast ARPs, forward them to the > > > target > > > - * to have ability to monitor target liveness via unicast > > > - * ARP requests. > > > - */ > > > ds_put_format(match, > > > "arp.tpa == %s && " > > > - "arp.op == 1 && " > > > - "eth.dst == ff:ff:ff:ff:ff:ff", > > > - op->lsp_addrs[i].ipv4_addrs[j].addr_s); > > > + "arp.op == 1", > > > op->lsp_addrs[i].ipv4_addrs[j].addr_s); > > > + > > > + /* Do not reply on unicast ARPs, forward them to the > > > target > > > + * to have ability to monitor target liveness via unicast > > > + * ARP requests. If proxy arp is configured, then we need > > > + * to set up flows to forward the packets. Otherwise, we > > > + * could end up replying with the proxy ARP erroneously. > > > + * Without proxy arp configured, these flows are > > > + * unnecessary since the packets will hit the default > > > + * "next" flow at priority 0. > > > + */ > > > + if (op->od->has_arp_proxy_port) { > > > + size_t match_len = match->length; > > > + ds_put_format(match, " && eth.dst == %s", > > > + op->lsp_addrs[i].ea_s); > > > + ovn_lflow_add_with_hint(lflows, op->od, > > > + S_SWITCH_IN_ARP_ND_RSP, 50, > > > + ds_cstr(match), > > > + "next;", &op->nbsp->header_, > > > + op->lflow_ref); > > > + ds_truncate(match, match_len); > > > + } > > > + ds_put_cstr(match, " && eth.dst == ff:ff:ff:ff:ff:ff"); > > > + > > > ds_clear(actions); > > > ds_put_format(actions, > > > "eth.dst = eth.src; " > > > @@ -10013,9 +10030,27 @@ build_lswitch_arp_nd_responder_known_ips(struct > > > ovn_port *op, > > > * > > > * - Do not reply for unicast ND solicitations. Let the > > > target > > > * reply to it, so that the sender has the ability to > > > monitor > > > - * the target liveness via the unicast ND solicitations. > > > + * the target liveness via the unicast ND solicitations. > > > Like > > > + * with IPv4, if proxy ARP is configured, then we need to > > > + * install unicast nd_ns flows that allow the packet to > > > reach > > > + * its target as intended. > > > */ > > > for (size_t j = 0; j < op->lsp_addrs[i].n_ipv6_addrs; j++) { > > > + if (op->od->has_arp_proxy_port) { > > > + ds_clear(match); > > > + ds_put_format(match, > > > + "nd_ns && ip6.dst == %s && nd.target > > > == %s", > > > + op->lsp_addrs[i].ipv6_addrs[j].addr_s, > > > + op->lsp_addrs[i].ipv6_addrs[j].addr_s); > > > + ovn_lflow_add_with_hint__(lflows, op->od, > > > + S_SWITCH_IN_ARP_ND_RSP, 50, > > > + ds_cstr(match), "next;", > > > NULL, > > > + copp_meter_get(COPP_ND_NA, > > > + > > > op->od->nbs->copp, > > > + > > > meter_groups), > > > + &op->nbsp->header_, > > > + op->lflow_ref); > > > + } > > > ds_clear(match); > > > ds_put_format( > > > match, > > > diff --git a/northd/northd.h b/northd/northd.h > > > index cdc532954..dc863ec48 100644 > > > --- a/northd/northd.h > > > +++ b/northd/northd.h > > > @@ -471,6 +471,13 @@ struct ovn_datapath { > > > /* Reference to the lflows belonging to this datapath currently > > > router > > > * only lflows. */ > > > struct lflow_ref *datapath_lflows; > > > + > > > + /* This vector contains pointers to struct lport_addresses. These are > > > + * the configured "arp_proxy" addresses of all logical switch ports > > > on > > > + * this datapath. The ovn_ports own these addresses, so we should not > > > + * free them when destroying the ovn_datapath. > > > + */ > > > + struct vector proxy_arp_addrs; > > > > This vector is not used anywhere. I'm assuming it was a leftover from > > the previous version ? > > > > I think this should be removed. > > Yes, this was leftover from v2 and v3. My strategy for v4 was to > revert northd/northd.c from v3 to v1 and work from there. I did not > revert northd/northd.h though, so that got left over in this version. > Since this is Acked, I'll fix this when I merge it. > > > > > Thanks > > Numan > > > > > }; > > > > > > const struct ovn_datapath *ovn_datapath_find(const struct hmap > > > *datapaths, > > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > > > index 07fb57bd6..d6f2470c6 100644 > > > --- a/tests/ovn-northd.at > > > +++ b/tests/ovn-northd.at > > > @@ -18716,3 +18716,82 @@ AT_CHECK( > > > > > > AT_CLEANUP > > > ]) > > > + > > > +OVN_FOR_EACH_NORTHD_NO_HV([ > > > +AT_SETUP([Unicast ARP flows]) > > > +ovn_start > > > + > > > +# Typically on logical switches, the ARP responder stage installs nothing > > > +# for unicast ARPs towards VIF MACs. Instead, we rely on the default > > > priority > > > +# 1 "next;" action to move these ARPs through the pipeline, eventually > > > resulting > > > +# in the ARP reaching the destination. When proxy ARP is configured, we > > > need > > > +# to install explicit flows for the unicast ARPs at a higher priority. > > > Otherwise > > > +# the proxy ARP responder may respond with incorrect information. > > > +# > > > +# In this test, we are ensuring that the unicast flows in the ARP > > > responder stage > > > +# are only installed when proxy ARP is enabled. > > > + > > > +check ovn-nbctl ls-add ls1 > > > +check ovn-nbctl lsp-add ls1 vm1 -- \ > > > + lsp-set-addresses vm1 "00:00:00:00:00:02 192.168.0.2 fd01::2" > > > + > > > +check ovn-nbctl lr-add lr1 > > > +check ovn-nbctl lrp-add lr1 lr1-ls1 00:00:00:00:00:01 192.168.0.1 fd01::1 > > > +check ovn-nbctl lsp-add ls1 ls1-lr1 -- \ > > > + lsp-set-addresses ls1-lr1 router -- \ > > > + lsp-set-type ls1-lr1 router -- \ > > > + lsp-set-options ls1-lr1 router-port=lr1-ls1 > > > + > > > +check ovn-nbctl --wait=sb sync > > > + > > > +# If we check the ARP responder flows in ls1, we should not see any > > > unicast > > > +# flows for vm1 (00:00:00:00:00:02). We also should not see any unicast > > > +# IPv6 ND flows for vm1 (fd01::2) > > > +AT_CHECK([ovn-sbctl lflow-list ls1 | grep ls_in_arp_rsp | grep "eth.dst > > > == 00:00:00:00:00:02" | ovn_strip_lflows], [0], [dnl > > > +]) > > > +AT_CHECK([ovn-sbctl lflow-list ls1 | grep ls_in_arp_rsp | grep "ip6.dst > > > == fd01::2" | ovn_strip_lflows], [0], [dnl > > > +]) > > > + > > > +AT_CHECK([ovn-sbctl lflow-list ls1 | grep ls_in_arp_rsp | grep "eth.dst > > > == 00:00:00:00:00:01" | ovn_strip_lflows], [0], [dnl > > > +]) > > > +AT_CHECK([ovn-sbctl lflow-list ls1 | grep ls_in_arp_rsp | grep "ip6.dst > > > == fd01::1" | ovn_strip_lflows], [0], [dnl > > > +]) > > > + > > > +# Add ARP proxy configuration on the router port. > > > +check ovn-nbctl set logical_switch_port ls1-lr1 > > > options:arp_proxy="192.168.0.0/24 fd01::/64" > > > +check ovn-nbctl --wait=sb sync > > > + > > > +# Now that we have ARP proxy configured, we should see flows for > > > +# vm1's MAC. > > > +AT_CHECK([ovn-sbctl lflow-list ls1 | grep ls_in_arp_rsp | grep "eth.dst > > > == 00:00:00:00:00:02" | ovn_strip_lflows], [0], [dnl > > > + table=??(ls_in_arp_rsp ), priority=50 , match=(arp.tpa == > > > 192.168.0.2 && arp.op == 1 && eth.dst == 00:00:00:00:00:02), > > > action=(next;) > > > +]) > > > +# And we should see unicast ND flows for vm1's IP address > > > +AT_CHECK([ovn-sbctl lflow-list ls1 | grep ls_in_arp_rsp | grep "ip6.dst > > > == fd01::2" | ovn_strip_lflows], [0], [dnl > > > + table=??(ls_in_arp_rsp ), priority=50 , match=(nd_ns && ip6.dst > > > == fd01::2 && nd.target == fd01::2), action=(next;) > > > +]) > > > + > > > +# We should also see an ARP flow for ls1-lr1's MAC. > > > +AT_CHECK([ovn-sbctl lflow-list ls1 | grep ls_in_arp_rsp | grep "eth.dst > > > == 00:00:00:00:00:01" | ovn_strip_lflows], [0], [dnl > > > + table=??(ls_in_arp_rsp ), priority=50 , match=(arp.tpa == > > > 192.168.0.1 && arp.op == 1 && eth.dst == 00:00:00:00:00:01), > > > action=(next;) > > > +]) > > > +# And we should see an ND flow for ls1-lr1's IPv6 address. > > > +AT_CHECK([ovn-sbctl lflow-list ls1 | grep ls_in_arp_rsp | grep "ip6.dst > > > == fd01::1" | ovn_strip_lflows], [0], [dnl > > > + table=??(ls_in_arp_rsp ), priority=50 , match=(nd_ns && ip6.dst > > > == fd01::1 && nd.target == fd01::1), action=(next;) > > > +]) > > > + > > > +check ovn-nbctl remove logical_switch_port ls1-lr1 options arp_proxy > > > +check ovn-nbctl --wait=sb sync > > > + > > > +# We have removed ARP proxy, so we should no longer see a unicast ARP or > > > ND flow. > > > +AT_CHECK([ovn-sbctl lflow-list ls1 | grep ls_in_arp_rsp | grep "eth.dst > > > == 00:00:00:00:00:02" | ovn_strip_lflows], [0], [dnl > > > +]) > > > +AT_CHECK([ovn-sbctl lflow-list ls1 | grep ls_in_arp_rsp | grep "ip6.dst > > > == fd01::2" | ovn_strip_lflows], [0], [dnl > > > +]) > > > +AT_CHECK([ovn-sbctl lflow-list ls1 | grep ls_in_arp_rsp | grep "eth.dst > > > == 00:00:00:00:00:01" | ovn_strip_lflows], [0], [dnl > > > +]) > > > +AT_CHECK([ovn-sbctl lflow-list ls1 | grep ls_in_arp_rsp | grep "ip6.dst > > > == fd01::1" | ovn_strip_lflows], [0], [dnl > > > +]) > > > + > > > +AT_CLEANUP > > > +]) > > > diff --git a/tests/ovn.at b/tests/ovn.at > > > index 3f340e7b3..f8ad98917 100644 > > > --- a/tests/ovn.at > > > +++ b/tests/ovn.at > > > @@ -44371,3 +44371,80 @@ check ovn-nbctl --wait=hv sync > > > OVN_CLEANUP([hv1],[hv2],[hv3]) > > > AT_CLEANUP > > > ]) > > > + > > > +OVN_FOR_EACH_NORTHD([ > > > +AT_SETUP([Unicast ARP when proxy ARP is configured]) > > > +AT_SKIP_IF([test $HAVE_SCAPY = no]) > > > +ovn_start > > > + > > > +check ovn-nbctl ls-add ls1 > > > +check ovn-nbctl lsp-add ls1 vm1 -- \ > > > + lsp-set-addresses vm1 "00:00:00:00:00:02 10.0.0.2 fd01::2" > > > +check ovn-nbctl lsp-add ls1 vm2 -- \ > > > + lsp-set-addresses vm2 "00:00:00:00:00:03 10.0.0.3 fd01::3" > > > + > > > +check ovn-nbctl lr-add lr1 > > > +check ovn-nbctl lrp-add lr1 lr1-ls1 00:00:00:00:00:01 10.0.0.1 fd01::1 > > > +check ovn-nbctl lsp-add ls1 ls1-lr1 -- \ > > > + lsp-set-addresses ls1-lr1 router -- \ > > > + lsp-set-type ls1-lr1 router -- \ > > > + lsp-set-options ls1-lr1 router-port=lr1-ls1 > > > + > > > +check ovn-nbctl --wait=hv sync > > > + > > > +net_add n1 > > > +sim_add hv1 > > > +as hv1 > > > +ovs-vsctl add-br br-phys > > > +ovn_attach n1 br-phys 192.168.0.1 > > > +ovs-vsctl add-port br-int vif1 -- set Interface vif1 > > > external-ids:iface-id=vm1 \ > > > + options:tx_pcap=hv1/vif1-tx.pcap \ > > > + options:rxq_pcap=hv1/vif1-rx.pcap > > > +ovs-vsctl add-port br-int vif2 -- set Interface vif2 > > > external-ids:iface-id=vm2 \ > > > + options:tx_pcap=hv1/vif2-tx.pcap \ > > > + options:rxq_pcap=hv1/vif2-rx.pcap > > > + > > > +OVN_POPULATE_ARP > > > + > > > +wait_for_ports_up > > > + > > > +arp=$(fmt_pkt "Ether(dst='00:00:00:00:00:02', src='00:00:00:00:00:03')/ \ > > > + ARP(hwsrc='00:00:00:00:00:03', hwdst='00:00:00:00:00:02', > > > + psrc='10.0.0.3', pdst='10.0.0.2')") > > > +nd_ns=$(fmt_pkt "Ether(dst='00:00:00:00:00:02', > > > src='00:00:00:00:00:03')/ \ > > > + IPv6(src='fd01::3', dst='fd01::2')/ \ > > > + ICMPv6ND_NS(tgt='fd01::2')") > > > + > > > +# If we send a unicast ARP from vm2 towards vm1, the ARP should be > > > forwarded > > > +# to vm1 by ls1. > > > +as hv1 ovs-appctl netdev-dummy/receive vif2 $arp > > > +echo $arp > expected > > > +OVN_CHECK_PACKETS([hv1/vif1-tx.pcap], expected) > > > + > > > +# And if we send a unicast ND_NS from vm2 towards vm1, the ND_NS should > > > be > > > +# forwarded to vm1 by ls1. > > > +as hv1 ovs-appctl netdev-dummy/receive vif2 $nd_ns > > > +echo $nd_ns >> expected > > > +OVN_CHECK_PACKETS([hv1/vif1-tx.pcap], expected) > > > + > > > + > > > +# Add ARP proxy configuration on the router port. The subnet for the ARP > > > proxy > > > +# addresses overlaps with the VIF addresses for vm1 and vm2. > > > +check ovn-nbctl set logical_switch_port ls1-lr1 > > > options:arp_proxy="10.0.0.0/8 fd01::/64" > > > +check ovn-nbctl --wait=hv sync > > > + > > > +# Sending a unicast ARP from vm2 towards vm1 should still result in the > > > ARP being > > > +# forwarded to vm1 by ls1. > > > +as hv1 ovs-appctl netdev-dummy/receive vif2 $arp > > > +echo $arp >> expected > > > +OVN_CHECK_PACKETS([hv1/vif1-tx.pcap], expected) > > > + > > > +# Sending a unicast ND_NS from vm2 towards vm1 should still result in > > > the ND_NS being > > > +# forwarded to vm1 by ls1. > > > +as hv1 ovs-appctl netdev-dummy/receive vif2 $nd_ns > > > +echo $nd_ns >> expected > > > +OVN_CHECK_PACKETS([hv1/vif1-tx.pcap], expected) > > > + > > > +OVN_CLEANUP([hv1]) > > > +AT_CLEANUP > > > +]) > > > -- > > > 2.50.1 > > > > > > _______________________________________________ > > > 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
