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]> --- 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 | 104 ++++++++++++++++++++++++++++++++++++++------ northd/northd.h | 7 +++ tests/ovn-northd.at | 93 +++++++++++++++++++++++++++++++++++++++ tests/ovn.at | 76 ++++++++++++++++++++++++++++++++ 4 files changed, 266 insertions(+), 14 deletions(-) diff --git a/northd/northd.c b/northd/northd.c index 3b1d3eba7..3dbd7f2b0 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -557,6 +557,7 @@ ovn_datapath_create(struct hmap *datapaths, const struct uuid *key, od->tunnel_key = sdp->sb_dp->tunnel_key; init_mcast_info_for_datapath(od); od->datapath_lflows = lflow_ref_create(); + od->proxy_arp_addrs = VECTOR_EMPTY_INITIALIZER(struct lport_addresses); return od; } @@ -586,6 +587,11 @@ ovn_datapath_destroy(struct ovn_datapath *od) destroy_ports_for_datapath(od); sset_destroy(&od->router_ips); lflow_ref_destroy(od->datapath_lflows); + /* The ovn_ports own the proxy_arp_addresses, so we do not + * need to call destroy_lport_addresses() on the components + * of the vector + */ + vector_destroy(&od->proxy_arp_addrs); free(od); } } @@ -1876,6 +1882,7 @@ join_logical_ports(const struct sbrec_port_binding_table *sbrec_pb_table, if (extract_addresses(arp_proxy, &op->proxy_arp_addrs, &ofs) || extract_ip_addresses(arp_proxy, &op->proxy_arp_addrs)) { op->od->has_arp_proxy_port = true; + vector_push(&op->od->proxy_arp_addrs, &op->proxy_arp_addrs); } else { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); @@ -9836,6 +9843,85 @@ build_lswitch_arp_nd_responder_skip_local(struct ovn_port *op, &op->nbsp->header_, op->lflow_ref); } +static void +build_arp_match(struct ds *match, const char *ipv4_addr, const char *mac) +{ + ds_put_format(match, "arp.tpa == %s && arp.op == 1 && eth.dst == %s", + ipv4_addr, mac); +} + +static void +build_nd_match(struct ds *match, const struct ipv6_netaddr *ipv6_addr, + bool is_multicast) +{ + if (is_multicast) { + ds_put_format(match, "nd_ns_mcast && ip6.dst == %s && nd.target == %s", + ipv6_addr->sn_addr_s, ipv6_addr->addr_s); + } else { + ds_put_format(match, "nd_ns && ip6.dst == %s && nd.target == %s", + ipv6_addr->addr_s, ipv6_addr->addr_s); + } +} + +static void +build_lswitch_arp_nd_unicast_flows(struct ovn_port *op, + struct lflow_table *lflows, + struct ds *match) +{ + /* Typically, we don't need to build any unicast flows for ARP or ND + * since the natural switching behavior of the logical switch will + * get the packet to its intended destination. However, if proxy ARP + * is configured, then we may need to install flows to ensure that + * we do not incorrectly respond to unicast ARPs destined to a known + * IP with the proxy ARP instead. + */ + if (!op->od->nbs || !op->od->has_arp_proxy_port) { + return; + } + + struct lport_addresses *proxy_arp_addrs; + VECTOR_FOR_EACH_PTR(&op->od->proxy_arp_addrs, proxy_arp_addrs) { + for (size_t i = 0; i < op->n_lsp_addrs; i++) { + for (size_t j = 0; j < proxy_arp_addrs->n_ipv4_addrs; j++) { + for (size_t k = 0; k < op->lsp_addrs[i].n_ipv4_addrs; k++) { + struct ipv4_netaddr *ipv4_addr = &op->lsp_addrs[i].ipv4_addrs[k]; + ovs_be32 proxy_arp_mask = proxy_arp_addrs->ipv4_addrs[j].mask; + ovs_be32 proxy_arp_network = proxy_arp_addrs->ipv4_addrs[j].network; + if ((ipv4_addr->addr & proxy_arp_mask) != proxy_arp_network) { + continue; + } + ds_clear(match); + build_arp_match(match, ipv4_addr->addr_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); + } + } + for (size_t j = 0; j < proxy_arp_addrs->n_ipv6_addrs; j++) { + for (size_t k = 0; k < op->lsp_addrs[j].n_ipv6_addrs; k++) { + struct ipv6_netaddr *ipv6_addr = &op->lsp_addrs[i].ipv6_addrs[k]; + struct in6_addr *proxy_arp_mask = &proxy_arp_addrs->ipv6_addrs[j].mask; + struct in6_addr *proxy_arp_network = &proxy_arp_addrs->ipv6_addrs[j].network; + struct in6_addr network = ipv6_addr_bitand(&ipv6_addr->addr, proxy_arp_mask); + if (!ipv6_addr_equals(proxy_arp_network, &network)) { + continue; + } + ds_clear(match); + build_nd_match(match, ipv6_addr, false); + 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); + } + } + } + } +} + /* Ingress table 24: ARP/ND responder, reply for known IPs. * (priority 50). */ static void @@ -9956,15 +10042,8 @@ 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); + build_arp_match(match, op->lsp_addrs[i].ipv4_addrs[j].addr_s, + "ff:ff:ff:ff:ff:ff"); ds_clear(actions); ds_put_format(actions, "eth.dst = eth.src; " @@ -10017,11 +10096,7 @@ build_lswitch_arp_nd_responder_known_ips(struct ovn_port *op, */ for (size_t j = 0; j < op->lsp_addrs[i].n_ipv6_addrs; j++) { ds_clear(match); - ds_put_format( - match, - "nd_ns_mcast && ip6.dst == %s && nd.target == %s", - op->lsp_addrs[i].ipv6_addrs[j].sn_addr_s, - op->lsp_addrs[i].ipv6_addrs[j].addr_s); + build_nd_match(match, &op->lsp_addrs[i].ipv6_addrs[j], true); ds_clear(actions); ds_put_format(actions, @@ -10061,6 +10136,7 @@ build_lswitch_arp_nd_responder_known_ips(struct ovn_port *op, op->lflow_ref); } } + build_lswitch_arp_nd_unicast_flows(op, lflows, match); } if (op->proxy_arp_addrs.n_ipv4_addrs || op->proxy_arp_addrs.n_ipv6_addrs) { 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; }; 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..6b978bd6a 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -18716,3 +18716,96 @@ 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 +]) + +# Now we'll set up ARP proxy flows for networks that are different from our +# LSPs. This should result in no unicast ARP/ND flows being installed. +check ovn-nbctl set logical_switch_port ls1-lr1 options:arp_proxy="193.168.0.0/24 fd11::/64" +check ovn-nbctl --wait=sb sync + +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..0783376bc 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -44371,3 +44371,79 @@ 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]) +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
