On 7/8/20 2:20 PM, Dumitru Ceara wrote: > Logical flows that limit the ARP/NS broadcast domain on a logical switch > should only match on ARP requests/NS for IPs that can actually be > replied to on the connected router port (i.e., an IP on the same network > is configured on the router port). >
I just realized that this also indirectly fixes a memory leak that was happening due to not freeing the result of ip_parse_masked()/ipv6_parse_masked(). As we don't use the functions anymore, the leak is gone too. tests/testsuite.dir/129/valgrind.29645:==29645== 9,520 bytes in 340 blocks are definitely lost in loss record 86 of 86 tests/testsuite.dir/129/valgrind.29645-==29645== at 0x4C29E63: malloc (vg_replace_malloc.c:309) tests/testsuite.dir/129/valgrind.29645-==29645== by 0x4AF327: xmalloc (util.c:138) tests/testsuite.dir/129/valgrind.29645-==29645== by 0x4AFCC2: xvasprintf (util.c:202) tests/testsuite.dir/129/valgrind.29645-==29645== by 0x4AFD7B: xasprintf (util.c:343) tests/testsuite.dir/129/valgrind.29645-==29645== by 0x4A19ED: ip_parse_masked_len (packets.c:603) tests/testsuite.dir/129/valgrind.29645-==29645== by 0x4A1A3F: ip_parse_masked (packets.c:619) tests/testsuite.dir/129/valgrind.29645-==29645== by 0x41066F: build_lswitch_rport_arp_req_flows (ovn-northd.c:6246) tests/testsuite.dir/129/valgrind.29645-==29645== by 0x41A3B8: build_lswitch_flows.constprop.77 (ovn-northd.c:7019) tests/testsuite.dir/129/valgrind.29645-==29645== by 0x41BDA9: build_lflows (ovn-northd.c:10540) tests/testsuite.dir/129/valgrind.29645-==29645== by 0x41BDA9: ovnnb_db_run.isra.70 (ovn-northd.c:11387) tests/testsuite.dir/129/valgrind.29645-==29645== by 0x408C83: ovn_db_run (ovn-northd.c:11940) tests/testsuite.dir/129/valgrind.29645-==29645== by 0x408C83: main (ovn-northd.c:12319) Thanks, Dumitru > Reported-by: Girish Moodalbail <[email protected]> > Reported-at: > https://mail.openvswitch.org/pipermail/ovs-discuss/2020-June/050287.html > Fixes: 5341969d3b39 ("ovn-northd: Limit IPv6 ND NS/RA/RS to the local > network.") > Signed-off-by: Dumitru Ceara <[email protected]> > --- > northd/ovn-northd.c | 160 > ++++++++++++++++++++++++++++++++++++++++++++-------- > tests/ovn.at | 74 ++++++++++++++++++++++++ > 2 files changed, 210 insertions(+), 24 deletions(-) > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > index ddfcebe..e2e875a 100644 > --- a/northd/ovn-northd.c > +++ b/northd/ovn-northd.c > @@ -6091,6 +6091,44 @@ build_lrouter_groups(struct hmap *ports, struct > ovs_list *lr_list) > } > } > > +/* Returns 'true' if the IPv4 'addr' is on the same subnet with one of the > + * IPs configured on the router port. > + */ > +static bool > +lrouter_port_ipv4_reachable(const struct ovn_port *op, ovs_be32 addr) > +{ > + for (size_t i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) { > + struct ipv4_netaddr *op_addr = &op->lrp_networks.ipv4_addrs[i]; > + > + if ((op_addr->addr & op_addr->mask) == (addr & op_addr->mask)) { > + return true; > + } > + } > + return false; > +} > + > +/* Returns 'true' if the IPv6 'addr' is on the same subnet with one of the > + * IPs configured on the router port. > + */ > +static bool > +lrouter_port_ipv6_reachable(const struct ovn_port *op, > + const struct in6_addr *addr) > +{ > + for (size_t i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) { > + struct ipv6_netaddr *op_addr = &op->lrp_networks.ipv6_addrs[i]; > + > + struct in6_addr op_addr6_masked = > + ipv6_addr_bitand(&op_addr->addr, &op_addr->mask); > + struct in6_addr nat_addr6_masked = > + ipv6_addr_bitand(addr, &op_addr->mask); > + > + if (ipv6_addr_equals(&op_addr6_masked, &nat_addr6_masked)) { > + return true; > + } > + } > + return false; > +} > + > /* > * Ingress table 19: Flows that flood self originated ARP/ND packets in the > * switching domain. > @@ -6101,8 +6139,47 @@ build_lswitch_rport_arp_req_self_orig_flow(struct > ovn_port *op, > struct ovn_datapath *od, > struct hmap *lflows) > { > - struct ds match = DS_EMPTY_INITIALIZER; > + struct sset all_eth_addrs = SSET_INITIALIZER(&all_eth_addrs); > struct ds eth_src = DS_EMPTY_INITIALIZER; > + struct ds match = DS_EMPTY_INITIALIZER; > + > + sset_add(&all_eth_addrs, op->lrp_networks.ea_s); > + > + for (size_t i = 0; i < op->od->nbr->n_nat; i++) { > + struct ovn_nat *nat_entry = &op->od->nat_entries[i]; > + const struct nbrec_nat *nat = nat_entry->nb; > + > + if (!nat_entry_is_valid(nat_entry)) { > + continue; > + } > + > + if (!strcmp(nat->type, "snat")) { > + continue; > + } > + > + if (!nat->external_mac) { > + continue; > + } > + > + /* Check if there's a network configured on op on which we could > + * expect ARP requests/NS for the DNAT external_ip. > + */ > + if (nat_entry_is_v6(nat_entry)) { > + struct in6_addr *addr = &nat_entry->ext_addrs.ipv6_addrs[0].addr; > + > + if (!lrouter_port_ipv6_reachable(op, addr)) { > + continue; > + } > + } else { > + ovs_be32 addr = nat_entry->ext_addrs.ipv4_addrs[0].addr; > + > + if (!lrouter_port_ipv4_reachable(op, addr)) { > + continue; > + } > + } > + sset_add(&all_eth_addrs, nat->external_mac); > + } > + > > /* Self originated (G)ARP requests/ND need to be flooded as usual. > * Determine that packets are self originated by also matching on > @@ -6110,15 +6187,11 @@ build_lswitch_rport_arp_req_self_orig_flow(struct > ovn_port *op, > * is a VLAN-backed network. > * Priority: 80. > */ > - ds_put_format(ð_src, "{ %s, ", op->lrp_networks.ea_s); > - for (size_t i = 0; i < op->od->nbr->n_nat; i++) { > - const struct nbrec_nat *nat = op->od->nbr->nat[i]; > + const char *eth_addr; > > - if (!nat->external_mac) { > - continue; > - } > - > - ds_put_format(ð_src, "%s, ", nat->external_mac); > + ds_put_cstr(ð_src, "{"); > + SSET_FOR_EACH (eth_addr, &all_eth_addrs) { > + ds_put_format(ð_src, "%s, ", eth_addr); > } > ds_chomp(ð_src, ' '); > ds_chomp(ð_src, ','); > @@ -6130,8 +6203,9 @@ build_lswitch_rport_arp_req_self_orig_flow(struct > ovn_port *op, > ds_cstr(&match), > "outport = \""MC_FLOOD"\"; output;"); > > - ds_destroy(&match); > + sset_destroy(&all_eth_addrs); > ds_destroy(ð_src); > + ds_destroy(&match); > } > > /* > @@ -6222,36 +6296,74 @@ build_lswitch_rport_arp_req_flows(struct ovn_port *op, > struct sset all_ips_v4 = SSET_INITIALIZER(&all_ips_v4); > struct sset all_ips_v6 = SSET_INITIALIZER(&all_ips_v6); > > - for (size_t i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) { > - sset_add(&all_ips_v4, op->lrp_networks.ipv4_addrs[i].addr_s); > - } > - for (size_t i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) { > - sset_add(&all_ips_v6, op->lrp_networks.ipv6_addrs[i].addr_s); > + get_router_load_balancer_ips(op->od, &all_ips_v4, &all_ips_v6); > + > + const char *ip_addr; > + const char *ip_addr_next; > + SSET_FOR_EACH_SAFE (ip_addr, ip_addr_next, &all_ips_v4) { > + ovs_be32 ipv4_addr; > + > + /* Check if there's a network configured on op on which we could > + * expect ARP requests for the LB VIP. > + */ > + if (ip_parse(ip_addr, &ipv4_addr) && > + lrouter_port_ipv4_reachable(op, ipv4_addr)) { > + continue; > + } > + > + sset_delete(&all_ips_v4, SSET_NODE_FROM_NAME(ip_addr)); > } > + SSET_FOR_EACH_SAFE (ip_addr, ip_addr_next, &all_ips_v6) { > + struct in6_addr ipv6_addr; > > - get_router_load_balancer_ips(op->od, &all_ips_v4, &all_ips_v6); > + /* Check if there's a network configured on op on which we could > + * expect NS requests for the LB VIP. > + */ > + if (ipv6_parse(ip_addr, &ipv6_addr) && > + lrouter_port_ipv6_reachable(op, &ipv6_addr)) { > + continue; > + } > + > + sset_delete(&all_ips_v6, SSET_NODE_FROM_NAME(ip_addr)); > + } > > for (size_t i = 0; i < op->od->nbr->n_nat; i++) { > - const struct nbrec_nat *nat = op->od->nbr->nat[i]; > + struct ovn_nat *nat_entry = &op->od->nat_entries[i]; > + const struct nbrec_nat *nat = nat_entry->nb; > + > + if (!nat_entry_is_valid(nat_entry)) { > + continue; > + } > > if (!strcmp(nat->type, "snat")) { > continue; > } > > - ovs_be32 ip; > - ovs_be32 mask; > - struct in6_addr ipv6; > - struct in6_addr mask_v6; > + /* Check if there's a network configured on op on which we could > + * expect ARP requests/NS for the DNAT external_ip. > + */ > + if (nat_entry_is_v6(nat_entry)) { > + struct in6_addr *addr = &nat_entry->ext_addrs.ipv6_addrs[0].addr; > > - if (ip_parse_masked(nat->external_ip, &ip, &mask)) { > - if (!ipv6_parse_masked(nat->external_ip, &ipv6, &mask_v6)) { > + if (lrouter_port_ipv6_reachable(op, addr)) { > sset_add(&all_ips_v6, nat->external_ip); > } > } else { > - sset_add(&all_ips_v4, nat->external_ip); > + ovs_be32 addr = nat_entry->ext_addrs.ipv4_addrs[0].addr; > + > + if (lrouter_port_ipv4_reachable(op, addr)) { > + sset_add(&all_ips_v4, nat->external_ip); > + } > } > } > > + for (size_t i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) { > + sset_add(&all_ips_v4, op->lrp_networks.ipv4_addrs[i].addr_s); > + } > + for (size_t i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) { > + sset_add(&all_ips_v6, op->lrp_networks.ipv6_addrs[i].addr_s); > + } > + > if (!sset_is_empty(&all_ips_v4)) { > build_lswitch_rport_arp_req_flow_for_ip(&all_ips_v4, AF_INET, sw_op, > sw_od, 75, lflows, > diff --git a/tests/ovn.at b/tests/ovn.at > index 24d93bc..88700e8 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -19249,12 +19249,14 @@ ovn-nbctl --wait=hv sync > > sw_dp_uuid=$(ovn-sbctl --bare --columns _uuid list datapath_binding sw-agg) > sw_dp_key=$(ovn-sbctl --bare --columns tunnel_key list datapath_binding > sw-agg) > +sw1_dp_key=$(ovn-sbctl --bare --columns tunnel_key list datapath_binding sw1) > r1_dp_key=$(ovn-sbctl --bare --columns tunnel_key list datapath_binding rtr1) > > r1_tnl_key=$(ovn-sbctl --bare --columns tunnel_key list port_binding sw-rtr1) > r2_tnl_key=$(ovn-sbctl --bare --columns tunnel_key list port_binding sw-rtr2) > > match_sw_metadata="metadata=0x${sw_dp_key}" > +match_sw1_metadata="metadata=0x${sw1_dp_key}" > match_r1_metadata="metadata=0x${r1_dp_key}" > > # Inject ARP request for first router owned IP address. > @@ -19370,6 +19372,78 @@ ovn-nbctl lr-nat-add rtr1 dnat_and_snat 10.0.0.122 > 20.0.0.12 sw1-p2 00:00:00:02: > ovn-nbctl lr-nat-add rtr1 dnat_and_snat 10::122 20::12 sw1-p2 > 00:00:00:02:00:00 > ovn-nbctl --wait=hv sync > > +# Check that broadcast domain limiting flows match only on IPs that are > +# directly reachable via the router port. > +# For sw-rtr1: > +# - 10.0.0.1, 10::1, fe80::200:ff:fe00:100 - interface IPs. > +# - 10.0.0.11, 10::11 - LB VIPs. > +# - 10.0.0.111, 10.0.0.121, 10.0.0.122, 10::111, 10::121, 10::122 - DNAT IPs. > +# For sw-rtr2: > +# - 10.0.0.2, 10::2, fe80::200:ff:fe00:200 - interface IPs. > +# - 10.0.0.22, 10::22 - LB VIPs. > +# - 10.0.0.222, 10::222 - DNAT IPs. > +as hv1 > +AT_CHECK([ovs-ofctl dump-flows br-int | grep -E > "priority=75,.*${match_sw_metadata}" | grep -oE "arp_tpa=[[0-9.]]+" | sort], > [0], [dnl > +arp_tpa=10.0.0.1 > +arp_tpa=10.0.0.11 > +arp_tpa=10.0.0.111 > +arp_tpa=10.0.0.121 > +arp_tpa=10.0.0.122 > +arp_tpa=10.0.0.2 > +arp_tpa=10.0.0.22 > +arp_tpa=10.0.0.222 > +]) > +AT_CHECK([ovs-ofctl dump-flows br-int | grep -E > "priority=75,.*${match_sw_metadata}" | grep -oE "nd_target=[[0-9a-f:]]+" | > sort], [0], [dnl > +nd_target=10::1 > +nd_target=10::11 > +nd_target=10::111 > +nd_target=10::121 > +nd_target=10::122 > +nd_target=10::2 > +nd_target=10::22 > +nd_target=10::222 > +nd_target=fe80::200:ff:fe00:100 > +nd_target=fe80::200:ff:fe00:200 > +]) > + > +# For sw1-rtr1: > +# - 20.0.0.1, 20::1, fe80::200:1ff:fe00:0 - interface IPs. > +as hv1 > +AT_CHECK([ovs-ofctl dump-flows br-int | grep -E > "priority=75,.*${match_sw1_metadata}" | grep -oE "arp_tpa=[[0-9.]]+" | sort], > [0], [dnl > +arp_tpa=20.0.0.1 > +]) > +AT_CHECK([ovs-ofctl dump-flows br-int | grep -E > "priority=75,.*${match_sw1_metadata}" | grep -oE "nd_target=[[0-9a-f:]]+" | > sort], [0], [dnl > +nd_target=20::1 > +nd_target=fe80::200:1ff:fe00:0 > +]) > + > +# Self originated ARP/NS with SMACs owned by rtr1-sw and rtr2-sw should be > +# flooded: > +# - 00:00:00:00:01:00 - interface MAC (rtr1-sw). > +# - 00:00:00:00:02:00 - interface MAC (rtr2-sw). > +# - 00:00:00:01:00:00 - dnat_and_snat external MAC. > +# - 00:00:00:02:00:00 - dnat_and_snat external MAC. > +as hv1 > +AT_CHECK([ovs-ofctl dump-flows br-int | grep -E > "priority=80,.*${match_sw_metadata}.*arp_op=1" | grep -oE > "dl_src=[[0-9a-f:]]+" | sort], [0], [dnl > +dl_src=00:00:00:00:01:00 > +dl_src=00:00:00:00:02:00 > +dl_src=00:00:00:01:00:00 > +dl_src=00:00:00:02:00:00 > +]) > +AT_CHECK([ovs-ofctl dump-flows br-int | grep -E > "priority=80,.*${match_sw_metadata}.*icmp_type=135" | grep -oE > "dl_src=[[0-9a-f:]]+" | sort], [0], [dnl > +dl_src=00:00:00:00:01:00 > +dl_src=00:00:00:00:02:00 > +dl_src=00:00:00:01:00:00 > +dl_src=00:00:00:02:00:00 > +]) > + > +# Self originated ARP/NS with SMACs owned by rtr1-sw1 should be flooded: > +# - 00:00:01:00:00:00 - interface MAC. > +as hv1 > +AT_CHECK([ovs-ofctl dump-flows br-int | grep -E > "priority=80,.*${match_sw1_metadata}.*arp_op=1" | grep -oE > "dl_src=[[0-9a-f:]]+" | sort], [0], [dnl > +dl_src=00:00:01:00:00:00 > +]) > + > # Inject ARP request for first router owned NAT address. > send_arp_request 1 0 ${src_mac} $(ip_to_hex 10 0 0 254) $(ip_to_hex 10 0 0 > 111) > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
