| Message ID | 20200917125050.19729.41529.stgit@dceara.remote.csb |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Drop packets destined to own IPs and refactor SNAT processing. | expand |
On Thu, Sep 17, 2020 at 5:51 AM Dumitru Ceara <dceara@redhat.com> wrote: > > OVN was dropping IP packets destined to IPs owned by logical routers but > only if those IPs are not used for SNAT rules. However, if a packet > doesn't match an existing NAT session and its destination is still a > router owned IP, it can be safely dropped. Otherwise it will trigger an > unnecessary packet-in in stage lr_in_arp_request. > > To achieve that we add flows that drop traffic to router owned SNAT IPs in > table lr_in_arp_resolve. > > Reported-by: Tim Rozet <trozet@redhat.com> > Reported-at: https://bugzilla.redhat.com/1876174 > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > --- > northd/ovn-northd.8.xml | 24 ++++++ > northd/ovn-northd.c | 194 +++++++++++++++++++++++++++-------------------- > tests/ovn.at | 88 +++++++++++++++++++++ > 3 files changed, 225 insertions(+), 81 deletions(-) > > diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml > index bd42105..f1c7c9b 100644 > --- a/northd/ovn-northd.8.xml > +++ b/northd/ovn-northd.8.xml > @@ -3089,6 +3089,30 @@ outport = <var>P</var>; > > <li> > <p> > + Traffic with IP destination an address owned by the router should be > + dropped. Such traffic is normally dropped in ingress table > + <code>IP Input</code> except for IPs that are also shared with SNAT > + rules. However, if there was no unSNAT operation that happened > + successfully until this point in the pipeline and the destination IP > + of the packet is still a router owned IP, the packets can be safely > + dropped. > + </p> > + > + <p> > + A priority-1 logical flow with match <code>ip4.dst = {..}</code> > + matches on traffic destined to router owned IPv4 addresses which are > + also SNAT IPs. This flow has action <code>drop;</code>. > + </p> > + > + <p> > + A priority-1 logical flow with match <code>ip6.dst = {..}</code> > + matches on traffic destined to router owned IPv6 addresses which are > + also SNAT IPs. This flow has action <code>drop;</code>. > + </p> > + </li> > + > + <li> > + <p> > Dynamic MAC bindings. These flows resolve MAC-to-IP bindings > that have become known dynamically through ARP or neighbor > discovery. (The ingress table <code>ARP Request</code> will > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > index cfec6a2..d5d7631 100644 > --- a/northd/ovn-northd.c > +++ b/northd/ovn-northd.c > @@ -623,6 +623,9 @@ struct ovn_datapath { > /* NAT entries configured on the router. */ > struct ovn_nat *nat_entries; > > + /* SNAT IPs used by the router. */ > + struct sset snat_ips; > + > struct ovn_port **localnet_ports; > size_t n_localnet_ports; > > @@ -641,6 +644,10 @@ struct ovn_nat { > struct lport_addresses ext_addrs; > }; > > +static bool > +get_force_snat_ip(struct ovn_datapath *od, const char *key_type, > + struct lport_addresses *laddrs); > + > /* Returns true if a 'nat_entry' is valid, i.e.: > * - parsing was successful. > * - the string yielded exactly one IPv4 address or exactly one IPv6 address. > @@ -663,7 +670,35 @@ nat_entry_is_v6(const struct ovn_nat *nat_entry) > static void > init_nat_entries(struct ovn_datapath *od) > { > - if (!od->nbr || od->nbr->n_nat == 0) { > + struct lport_addresses snat_addrs; > + > + if (!od->nbr) { > + return; > + } > + > + sset_init(&od->snat_ips); > + if (get_force_snat_ip(od, "dnat", &snat_addrs)) { > + if (snat_addrs.n_ipv4_addrs) { > + sset_add(&od->snat_ips, snat_addrs.ipv4_addrs[0].addr_s); > + } > + if (snat_addrs.n_ipv6_addrs) { > + sset_add(&od->snat_ips, snat_addrs.ipv6_addrs[0].addr_s); > + } > + destroy_lport_addresses(&snat_addrs); > + } > + > + memset(&snat_addrs, 0, sizeof(snat_addrs)); > + if (get_force_snat_ip(od, "lb", &snat_addrs)) { > + if (snat_addrs.n_ipv4_addrs) { > + sset_add(&od->snat_ips, snat_addrs.ipv4_addrs[0].addr_s); > + } > + if (snat_addrs.n_ipv6_addrs) { > + sset_add(&od->snat_ips, snat_addrs.ipv6_addrs[0].addr_s); > + } > + destroy_lport_addresses(&snat_addrs); > + } > + > + if (!od->nbr->n_nat) { > return; > } > > @@ -682,6 +717,13 @@ init_nat_entries(struct ovn_datapath *od) > VLOG_WARN_RL(&rl, > "Bad ip address %s in nat configuration " > "for router %s", nat->external_ip, od->nbr->name); > + continue; > + } > + > + if (!nat_entry_is_v6(nat_entry)) { > + sset_add(&od->snat_ips, nat_entry->ext_addrs.ipv4_addrs[0].addr_s); > + } else { > + sset_add(&od->snat_ips, nat_entry->ext_addrs.ipv6_addrs[0].addr_s); > } > } > } > @@ -693,6 +735,7 @@ destroy_nat_entries(struct ovn_datapath *od) > return; > } > > + sset_destroy(&od->snat_ips); > for (size_t i = 0; i < od->nbr->n_nat; i++) { > destroy_lport_addresses(&od->nat_entries[i].ext_addrs); > } > @@ -8744,6 +8787,68 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports, > op, lflows, &match, &actions); > } > > + /* Drop IP traffic destined to router owned IPs. Part of it is dropped > + * in stage "lr_in_ip_input" but traffic that could have been unSNATed > + * but didn't match any existing session might still end up here. > + */ > + HMAP_FOR_EACH (op, key_node, ports) { > + if (!op->nbrp) { > + continue; > + } > + > + if (op->lrp_networks.n_ipv4_addrs) { > + ds_clear(&match); > + for (size_t i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) { > + if (!sset_find(&op->od->snat_ips, > + op->lrp_networks.ipv4_addrs[i].addr_s)) { > + continue; > + } > + ds_put_format(&match, "%s, ", > + op->lrp_networks.ipv4_addrs[i].addr_s); > + } > + > + if (ds_last(&match) != EOF) { > + ds_chomp(&match, ' '); > + ds_chomp(&match, ','); > + > + char *drop_match = xasprintf("ip4.dst == {%s}", > + ds_cstr(&match)); > + /* Drop traffic with IP.dest == router-ip. */ > + ovn_lflow_add_with_hint(lflows, op->od, > + S_ROUTER_IN_ARP_RESOLVE, 1, > + drop_match, "drop;", > + &op->nbrp->header_); > + free(drop_match); > + } > + } > + > + if (op->lrp_networks.n_ipv6_addrs) { > + ds_clear(&match); > + for (size_t i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) { > + if (!sset_find(&op->od->snat_ips, > + op->lrp_networks.ipv6_addrs[i].addr_s)) { > + continue; > + } > + ds_put_format(&match, "%s, ", > + op->lrp_networks.ipv6_addrs[i].addr_s); > + } > + > + if (ds_last(&match) != EOF) { > + ds_chomp(&match, ' '); > + ds_chomp(&match, ','); > + > + char *drop_match = xasprintf("ip6.dst == {%s}", > + ds_cstr(&match)); > + /* Drop traffic with IP.dest == router-ip. */ > + ovn_lflow_add_with_hint(lflows, op->od, > + S_ROUTER_IN_ARP_RESOLVE, 1, > + drop_match, "drop;", > + &op->nbrp->header_); > + free(drop_match); > + } > + } > + } > + > HMAP_FOR_EACH (od, key_node, datapaths) { > if (!od->nbr) { > continue; > @@ -9035,77 +9140,15 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports, > } > > /* A gateway router can have 4 SNAT IP addresses to force DNATed and > - * LBed traffic respectively to be SNATed. In addition, there can be > - * a number of SNAT rules in the NAT table. */ > - struct v46_ip *snat_ips = xmalloc(sizeof *snat_ips > - * (op->od->nbr->n_nat + 4)); > - size_t n_snat_ips = 0; > - struct lport_addresses snat_addrs; > - > - if (get_force_snat_ip(op->od, "dnat", &snat_addrs)) { > - if (snat_addrs.n_ipv4_addrs) { > - snat_ips[n_snat_ips].family = AF_INET; > - snat_ips[n_snat_ips++].ipv4 = snat_addrs.ipv4_addrs[0].addr; > - } > - if (snat_addrs.n_ipv6_addrs) { > - snat_ips[n_snat_ips].family = AF_INET6; > - snat_ips[n_snat_ips++].ipv6 = snat_addrs.ipv6_addrs[0].addr; > - } > - destroy_lport_addresses(&snat_addrs); > - } > - > - memset(&snat_addrs, 0, sizeof(snat_addrs)); > - if (get_force_snat_ip(op->od, "lb", &snat_addrs)) { > - if (snat_addrs.n_ipv4_addrs) { > - snat_ips[n_snat_ips].family = AF_INET; > - snat_ips[n_snat_ips++].ipv4 = snat_addrs.ipv4_addrs[0].addr; > - } > - if (snat_addrs.n_ipv6_addrs) { > - snat_ips[n_snat_ips].family = AF_INET6; > - snat_ips[n_snat_ips++].ipv6 = snat_addrs.ipv6_addrs[0].addr; > - } > - destroy_lport_addresses(&snat_addrs); > - } > - > - 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; > - > - /* Skip entries we failed to parse. */ > - if (!nat_entry_is_valid(nat_entry)) { > - continue; > - } > - > - if (!strcmp(nat->type, "snat")) { > - if (nat_entry_is_v6(nat_entry)) { > - struct in6_addr *ipv6 = > - &nat_entry->ext_addrs.ipv6_addrs[0].addr; > - > - snat_ips[n_snat_ips].family = AF_INET6; > - snat_ips[n_snat_ips++].ipv6 = *ipv6; > - } else { > - ovs_be32 ip = nat_entry->ext_addrs.ipv4_addrs[0].addr; > - snat_ips[n_snat_ips].family = AF_INET; > - snat_ips[n_snat_ips++].ipv4 = ip; > - } > - } > - } > - > + * LBed traffic respectively to be SNATed. In addition, there can be > + * a number of SNAT rules in the NAT table. > + * Skip all of them for drop flows. */ > ds_clear(&match); > ds_put_cstr(&match, "ip4.dst == {"); > bool has_drop_ips = false; > for (int i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) { > - bool snat_ip_is_router_ip = false; > - for (int j = 0; j < n_snat_ips; j++) { > - /* Packets to SNAT IPs should not be dropped. */ > - if (snat_ips[j].family == AF_INET > - && op->lrp_networks.ipv4_addrs[i].addr > - == snat_ips[j].ipv4) { > - snat_ip_is_router_ip = true; > - break; > - } > - } > - if (snat_ip_is_router_ip) { > + if (sset_find(&op->od->snat_ips, > + op->lrp_networks.ipv4_addrs[i].addr_s)) { > continue; > } > ds_put_format(&match, "%s, ", > @@ -9122,17 +9165,8 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports, > } > > for (int i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) { > - bool snat_ip_is_router_ip = false; > - for (int j = 0; j < n_snat_ips; j++) { > - /* Packets to SNAT IPs should not be dropped. */ > - if (snat_ips[j].family == AF_INET6 > - && !memcmp(&op->lrp_networks.ipv6_addrs[i].addr, > - &snat_ips[j].ipv6, sizeof snat_ips[j].ipv6)) { > - snat_ip_is_router_ip = true; > - break; > - } > - } > - if (snat_ip_is_router_ip) { > + if (sset_find(&op->od->snat_ips, > + op->lrp_networks.ipv6_addrs[i].addr_s)) { > continue; > } > ds_put_format(&match, "%s, ", > @@ -9151,8 +9185,6 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports, > &op->nbrp->header_); > } > > - free(snat_ips); > - > /* ARP/NS packets are taken care of per router. The only exception > * is on the l3dgw_port where we might need to use a different > * ETH address. > diff --git a/tests/ovn.at b/tests/ovn.at > index a6f1fb5..cb7e7cc 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -21659,6 +21659,94 @@ OVS_WAIT_UNTIL([test x$(as hv1 ovn-appctl -t ovn-controller debug/status) = "xru > OVN_CLEANUP([hv1]) > AT_CLEANUP > > +# Test dropping traffic destined to router owned IPs. > +AT_SETUP([ovn -- gateway router drop traffic for own IPs]) > +ovn_start > + > +ovn-nbctl lr-add r1 -- set logical_router r1 options:chassis=hv1 > +ovn-nbctl ls-add s1 > + > +# Connnect r1 to s1. > +ovn-nbctl lrp-add r1 lrp-r1-s1 00:00:00:00:01:01 10.0.1.1/24 > +ovn-nbctl lsp-add s1 lsp-s1-r1 -- set Logical_Switch_Port lsp-s1-r1 type=router \ > + options:router-port=lrp-r1-s1 addresses=router > + > +# Create logical port p1 in s1 > +ovn-nbctl lsp-add s1 p1 \ > +-- lsp-set-addresses p1 "f0:00:00:00:01:02 10.0.1.2" > + > +# Create two hypervisor and create OVS ports corresponding to logical ports. > +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 hv1-vif1 -- \ > + set interface hv1-vif1 external-ids:iface-id=p1 \ > + options:tx_pcap=hv1/vif1-tx.pcap \ > + options:rxq_pcap=hv1/vif1-rx.pcap \ > + ofport-request=1 > + > +# Pre-populate the hypervisors' ARP tables so that we don't lose any > +# packets for ARP resolution (native tunneling doesn't queue packets > +# for ARP resolution). > +OVN_POPULATE_ARP > + > +ovn-nbctl --wait=hv sync > + > +sw_key=$(ovn-sbctl --bare --columns tunnel_key list datapath_binding r1) > + > +AT_CHECK([ovn-sbctl lflow-list | grep lr_in_arp_resolve | grep 10.0.1.1], [1], []) > + > +ip_to_hex() { > + printf "%02x%02x%02x%02x" "$@" > +} > + > +# Send ip packets from p1 to lrp-r1-s1 > +src_mac="f00000000102" > +dst_mac="000000000101" > +src_ip=`ip_to_hex 10 0 1 2` > +dst_ip=`ip_to_hex 10 0 1 1` > +packet=${dst_mac}${src_mac}08004500001c0000000040110000${src_ip}${dst_ip}0035111100080000 > +as hv1 ovs-appctl netdev-dummy/receive hv1-vif1 $packet > + > +# No packet-ins should reach ovn-controller. > +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | grep "actions=controller" | grep -v n_packets=0 -c], [1], [dnl > +0 > +]) > + > +# The packet should have been dropped in the lr_in_ip_input stage. > +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | grep -E "table=11, n_packets=1,.* priority=60,ip,metadata=0x${sw_key},nw_dst=10.0.1.1 actions=drop" -c], [0], [dnl > +1 > +]) > + > +# Use the router IP as SNAT IP. > +ovn-nbctl set logical_router r1 options:lb_force_snat_ip=10.0.1.1 > +ovn-nbctl --wait=hv sync > + > +# Send ip packets from p1 to lrp-r1-s1 > +src_mac="f00000000102" > +dst_mac="000000000101" > +src_ip=`ip_to_hex 10 0 1 2` > +dst_ip=`ip_to_hex 10 0 1 1` > +packet=${dst_mac}${src_mac}08004500001c0000000040110000${src_ip}${dst_ip}0035111100080000 > +as hv1 ovs-appctl netdev-dummy/receive hv1-vif1 $packet > + > +# Even after configuring a router owned IP for SNAT, no packet-ins should > +# reach ovn-controller. > +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | grep "actions=controller" | grep -v n_packets=0 -c], [1], [dnl > +0 > +]) > + > +# The packet should've been dropped in the lr_in_arp_resolve stage. > +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | grep -E "table=21, n_packets=1,.* priority=1,ip,metadata=0x${sw_key},nw_dst=10.0.1.1 actions=drop" -c], [0], [dnl > +1 > +]) > + > +OVN_CLEANUP([hv1]) > +AT_CLEANUP > + > AT_SETUP([ovn -- nb_cfg timestamp]) > ovn_start > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev Acked-by: Han Zhou <hzhou@ovn.org>
On Mon, Sep 21, 2020 at 12:08 PM Han Zhou <hzhou@ovn.org> wrote: > On Thu, Sep 17, 2020 at 5:51 AM Dumitru Ceara <dceara@redhat.com> wrote: > > > > OVN was dropping IP packets destined to IPs owned by logical routers but > > only if those IPs are not used for SNAT rules. However, if a packet > > doesn't match an existing NAT session and its destination is still a > > router owned IP, it can be safely dropped. Otherwise it will trigger an > > unnecessary packet-in in stage lr_in_arp_request. > > > > To achieve that we add flows that drop traffic to router owned SNAT IPs > in > > table lr_in_arp_resolve. > > > > Reported-by: Tim Rozet <trozet@redhat.com> > > Reported-at: https://bugzilla.redhat.com/1876174 > > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > > --- > > northd/ovn-northd.8.xml | 24 ++++++ > > northd/ovn-northd.c | 194 > +++++++++++++++++++++++++++-------------------- > > tests/ovn.at | 88 +++++++++++++++++++++ > > 3 files changed, 225 insertions(+), 81 deletions(-) > > > > diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml > > index bd42105..f1c7c9b 100644 > > --- a/northd/ovn-northd.8.xml > > +++ b/northd/ovn-northd.8.xml > > @@ -3089,6 +3089,30 @@ outport = <var>P</var>; > > > > <li> > > <p> > > + Traffic with IP destination an address owned by the router > should be > > + dropped. Such traffic is normally dropped in ingress table > > + <code>IP Input</code> except for IPs that are also shared with > SNAT > > + rules. However, if there was no unSNAT operation that happened > > + successfully until this point in the pipeline and the > destination IP > > + of the packet is still a router owned IP, the packets can be > safely > > + dropped. > > + </p> > > + > > + <p> > > + A priority-1 logical flow with match <code>ip4.dst = > {..}</code> > > + matches on traffic destined to router owned IPv4 addresses > which are > > + also SNAT IPs. This flow has action <code>drop;</code>. > > + </p> > > + > > + <p> > > + A priority-1 logical flow with match <code>ip6.dst = > {..}</code> > > + matches on traffic destined to router owned IPv6 addresses > which are > > + also SNAT IPs. This flow has action <code>drop;</code>. > > + </p> > > + </li> > > + > > + <li> > > + <p> > > Dynamic MAC bindings. These flows resolve MAC-to-IP bindings > > that have become known dynamically through ARP or neighbor > > discovery. (The ingress table <code>ARP Request</code> will > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > > index cfec6a2..d5d7631 100644 > > --- a/northd/ovn-northd.c > > +++ b/northd/ovn-northd.c > > @@ -623,6 +623,9 @@ struct ovn_datapath { > > /* NAT entries configured on the router. */ > > struct ovn_nat *nat_entries; > > > > + /* SNAT IPs used by the router. */ > > + struct sset snat_ips; > > + > > struct ovn_port **localnet_ports; > > size_t n_localnet_ports; > > > > @@ -641,6 +644,10 @@ struct ovn_nat { > > struct lport_addresses ext_addrs; > > }; > > > > +static bool > > +get_force_snat_ip(struct ovn_datapath *od, const char *key_type, > > + struct lport_addresses *laddrs); > > + > > /* Returns true if a 'nat_entry' is valid, i.e.: > > * - parsing was successful. > > * - the string yielded exactly one IPv4 address or exactly one IPv6 > address. > > @@ -663,7 +670,35 @@ nat_entry_is_v6(const struct ovn_nat *nat_entry) > > static void > > init_nat_entries(struct ovn_datapath *od) > > { > > - if (!od->nbr || od->nbr->n_nat == 0) { > > + struct lport_addresses snat_addrs; > > + > > + if (!od->nbr) { > > + return; > > + } > > + > > + sset_init(&od->snat_ips); > > + if (get_force_snat_ip(od, "dnat", &snat_addrs)) { > > + if (snat_addrs.n_ipv4_addrs) { > > + sset_add(&od->snat_ips, snat_addrs.ipv4_addrs[0].addr_s); > > + } > > + if (snat_addrs.n_ipv6_addrs) { > > + sset_add(&od->snat_ips, snat_addrs.ipv6_addrs[0].addr_s); > > + } > > + destroy_lport_addresses(&snat_addrs); > > + } > > + > > + memset(&snat_addrs, 0, sizeof(snat_addrs)); > > + if (get_force_snat_ip(od, "lb", &snat_addrs)) { > > + if (snat_addrs.n_ipv4_addrs) { > > + sset_add(&od->snat_ips, snat_addrs.ipv4_addrs[0].addr_s); > > + } > > + if (snat_addrs.n_ipv6_addrs) { > > + sset_add(&od->snat_ips, snat_addrs.ipv6_addrs[0].addr_s); > > + } > > + destroy_lport_addresses(&snat_addrs); > > + } > > + > > + if (!od->nbr->n_nat) { > > return; > > } > > > > @@ -682,6 +717,13 @@ init_nat_entries(struct ovn_datapath *od) > > VLOG_WARN_RL(&rl, > > "Bad ip address %s in nat configuration " > > "for router %s", nat->external_ip, > od->nbr->name); > > + continue; > > + } > > + > > + if (!nat_entry_is_v6(nat_entry)) { > > + sset_add(&od->snat_ips, > nat_entry->ext_addrs.ipv4_addrs[0].addr_s); > > + } else { > > + sset_add(&od->snat_ips, > nat_entry->ext_addrs.ipv6_addrs[0].addr_s); > > } > > } > > } > > @@ -693,6 +735,7 @@ destroy_nat_entries(struct ovn_datapath *od) > > return; > > } > > > > + sset_destroy(&od->snat_ips); > > for (size_t i = 0; i < od->nbr->n_nat; i++) { > > destroy_lport_addresses(&od->nat_entries[i].ext_addrs); > > } > > @@ -8744,6 +8787,68 @@ build_lrouter_flows(struct hmap *datapaths, struct > hmap *ports, > > op, lflows, &match, &actions); > > } > > > > + /* Drop IP traffic destined to router owned IPs. Part of it is > dropped > > + * in stage "lr_in_ip_input" but traffic that could have been > unSNATed > > + * but didn't match any existing session might still end up here. > > + */ > > + HMAP_FOR_EACH (op, key_node, ports) { > > + if (!op->nbrp) { > > + continue; > > + } > > + > > + if (op->lrp_networks.n_ipv4_addrs) { > > + ds_clear(&match); > > + for (size_t i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) { > > + if (!sset_find(&op->od->snat_ips, > > + op->lrp_networks.ipv4_addrs[i].addr_s)) { > > + continue; > > + } > > + ds_put_format(&match, "%s, ", > > + op->lrp_networks.ipv4_addrs[i].addr_s); > > + } > > + > > + if (ds_last(&match) != EOF) { > > + ds_chomp(&match, ' '); > > + ds_chomp(&match, ','); > > + > > + char *drop_match = xasprintf("ip4.dst == {%s}", > > + ds_cstr(&match)); > > + /* Drop traffic with IP.dest == router-ip. */ > > + ovn_lflow_add_with_hint(lflows, op->od, > > + S_ROUTER_IN_ARP_RESOLVE, 1, > > + drop_match, "drop;", > > + &op->nbrp->header_); > > + free(drop_match); > > + } > > + } > > + > > + if (op->lrp_networks.n_ipv6_addrs) { > > + ds_clear(&match); > > + for (size_t i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) { > > + if (!sset_find(&op->od->snat_ips, > > + op->lrp_networks.ipv6_addrs[i].addr_s)) { > > + continue; > > + } > > + ds_put_format(&match, "%s, ", > > + op->lrp_networks.ipv6_addrs[i].addr_s); > > + } > > + > > + if (ds_last(&match) != EOF) { > > + ds_chomp(&match, ' '); > > + ds_chomp(&match, ','); > > + > > + char *drop_match = xasprintf("ip6.dst == {%s}", > > + ds_cstr(&match)); > > + /* Drop traffic with IP.dest == router-ip. */ > > + ovn_lflow_add_with_hint(lflows, op->od, > > + S_ROUTER_IN_ARP_RESOLVE, 1, > > + drop_match, "drop;", > > + &op->nbrp->header_); > > + free(drop_match); > > + } > > + } > > + } > > + > > HMAP_FOR_EACH (od, key_node, datapaths) { > > if (!od->nbr) { > > continue; > > @@ -9035,77 +9140,15 @@ build_lrouter_flows(struct hmap *datapaths, > struct hmap *ports, > > } > > > > /* A gateway router can have 4 SNAT IP addresses to force DNATed > and > > - * LBed traffic respectively to be SNATed. In addition, there > can be > > - * a number of SNAT rules in the NAT table. */ > > - struct v46_ip *snat_ips = xmalloc(sizeof *snat_ips > > - * (op->od->nbr->n_nat + 4)); > > - size_t n_snat_ips = 0; > > - struct lport_addresses snat_addrs; > > - > > - if (get_force_snat_ip(op->od, "dnat", &snat_addrs)) { > > - if (snat_addrs.n_ipv4_addrs) { > > - snat_ips[n_snat_ips].family = AF_INET; > > - snat_ips[n_snat_ips++].ipv4 = > snat_addrs.ipv4_addrs[0].addr; > > - } > > - if (snat_addrs.n_ipv6_addrs) { > > - snat_ips[n_snat_ips].family = AF_INET6; > > - snat_ips[n_snat_ips++].ipv6 = > snat_addrs.ipv6_addrs[0].addr; > > - } > > - destroy_lport_addresses(&snat_addrs); > > - } > > - > > - memset(&snat_addrs, 0, sizeof(snat_addrs)); > > - if (get_force_snat_ip(op->od, "lb", &snat_addrs)) { > > - if (snat_addrs.n_ipv4_addrs) { > > - snat_ips[n_snat_ips].family = AF_INET; > > - snat_ips[n_snat_ips++].ipv4 = > snat_addrs.ipv4_addrs[0].addr; > > - } > > - if (snat_addrs.n_ipv6_addrs) { > > - snat_ips[n_snat_ips].family = AF_INET6; > > - snat_ips[n_snat_ips++].ipv6 = > snat_addrs.ipv6_addrs[0].addr; > > - } > > - destroy_lport_addresses(&snat_addrs); > > - } > > - > > - 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; > > - > > - /* Skip entries we failed to parse. */ > > - if (!nat_entry_is_valid(nat_entry)) { > > - continue; > > - } > > - > > - if (!strcmp(nat->type, "snat")) { > > - if (nat_entry_is_v6(nat_entry)) { > > - struct in6_addr *ipv6 = > > - &nat_entry->ext_addrs.ipv6_addrs[0].addr; > > - > > - snat_ips[n_snat_ips].family = AF_INET6; > > - snat_ips[n_snat_ips++].ipv6 = *ipv6; > > - } else { > > - ovs_be32 ip = > nat_entry->ext_addrs.ipv4_addrs[0].addr; > > - snat_ips[n_snat_ips].family = AF_INET; > > - snat_ips[n_snat_ips++].ipv4 = ip; > > - } > > - } > > - } > > - > > + * LBed traffic respectively to be SNATed. In addition, there > can be > > + * a number of SNAT rules in the NAT table. > > + * Skip all of them for drop flows. */ > > ds_clear(&match); > > ds_put_cstr(&match, "ip4.dst == {"); > > bool has_drop_ips = false; > > for (int i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) { > > - bool snat_ip_is_router_ip = false; > > - for (int j = 0; j < n_snat_ips; j++) { > > - /* Packets to SNAT IPs should not be dropped. */ > > - if (snat_ips[j].family == AF_INET > > - && op->lrp_networks.ipv4_addrs[i].addr > > - == snat_ips[j].ipv4) { > > - snat_ip_is_router_ip = true; > > - break; > > - } > > - } > > - if (snat_ip_is_router_ip) { > > + if (sset_find(&op->od->snat_ips, > > + op->lrp_networks.ipv4_addrs[i].addr_s)) { > > continue; > > } > > ds_put_format(&match, "%s, ", > > @@ -9122,17 +9165,8 @@ build_lrouter_flows(struct hmap *datapaths, struct > hmap *ports, > > } > > > > for (int i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) { > > - bool snat_ip_is_router_ip = false; > > - for (int j = 0; j < n_snat_ips; j++) { > > - /* Packets to SNAT IPs should not be dropped. */ > > - if (snat_ips[j].family == AF_INET6 > > - && !memcmp(&op->lrp_networks.ipv6_addrs[i].addr, > > - &snat_ips[j].ipv6, sizeof > snat_ips[j].ipv6)) { > > - snat_ip_is_router_ip = true; > > - break; > > - } > > - } > > - if (snat_ip_is_router_ip) { > > + if (sset_find(&op->od->snat_ips, > > + op->lrp_networks.ipv6_addrs[i].addr_s)) { > > continue; > > } > > ds_put_format(&match, "%s, ", > > @@ -9151,8 +9185,6 @@ build_lrouter_flows(struct hmap *datapaths, struct > hmap *ports, > > &op->nbrp->header_); > > } > > > > - free(snat_ips); > > - > > /* ARP/NS packets are taken care of per router. The only > exception > > * is on the l3dgw_port where we might need to use a different > > * ETH address. > > diff --git a/tests/ovn.at b/tests/ovn.at > > index a6f1fb5..cb7e7cc 100644 > > --- a/tests/ovn.at > > +++ b/tests/ovn.at > > @@ -21659,6 +21659,94 @@ OVS_WAIT_UNTIL([test x$(as hv1 ovn-appctl -t > ovn-controller debug/status) = "xru > > OVN_CLEANUP([hv1]) > > AT_CLEANUP > > > > +# Test dropping traffic destined to router owned IPs. > > +AT_SETUP([ovn -- gateway router drop traffic for own IPs]) > > +ovn_start > > + > > +ovn-nbctl lr-add r1 -- set logical_router r1 options:chassis=hv1 > > +ovn-nbctl ls-add s1 > > + > > +# Connnect r1 to s1. > > +ovn-nbctl lrp-add r1 lrp-r1-s1 00:00:00:00:01:01 10.0.1.1/24 > > +ovn-nbctl lsp-add s1 lsp-s1-r1 -- set Logical_Switch_Port lsp-s1-r1 > type=router \ > > + options:router-port=lrp-r1-s1 addresses=router > > + > > +# Create logical port p1 in s1 > > +ovn-nbctl lsp-add s1 p1 \ > > +-- lsp-set-addresses p1 "f0:00:00:00:01:02 10.0.1.2" > > + > > +# Create two hypervisor and create OVS ports corresponding to logical > ports. > > +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 hv1-vif1 -- \ > > + set interface hv1-vif1 external-ids:iface-id=p1 \ > > + options:tx_pcap=hv1/vif1-tx.pcap \ > > + options:rxq_pcap=hv1/vif1-rx.pcap \ > > + ofport-request=1 > > + > > +# Pre-populate the hypervisors' ARP tables so that we don't lose any > > +# packets for ARP resolution (native tunneling doesn't queue packets > > +# for ARP resolution). > > +OVN_POPULATE_ARP > > + > > +ovn-nbctl --wait=hv sync > > + > > +sw_key=$(ovn-sbctl --bare --columns tunnel_key list datapath_binding r1) > > + > > +AT_CHECK([ovn-sbctl lflow-list | grep lr_in_arp_resolve | grep > 10.0.1.1], [1], []) > > + > > +ip_to_hex() { > > + printf "%02x%02x%02x%02x" "$@" > > +} > > + > > +# Send ip packets from p1 to lrp-r1-s1 > > +src_mac="f00000000102" > > +dst_mac="000000000101" > > +src_ip=`ip_to_hex 10 0 1 2` > > +dst_ip=`ip_to_hex 10 0 1 1` > > > > +packet=${dst_mac}${src_mac}08004500001c0000000040110000${src_ip}${dst_ip}0035111100080000 > > +as hv1 ovs-appctl netdev-dummy/receive hv1-vif1 $packet > > + > > +# No packet-ins should reach ovn-controller. > > +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | grep "actions=controller" > | grep -v n_packets=0 -c], [1], [dnl > > +0 > > +]) > > + > > +# The packet should have been dropped in the lr_in_ip_input stage. > > +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | grep -E "table=11, > n_packets=1,.* priority=60,ip,metadata=0x${sw_key},nw_dst=10.0.1.1 > actions=drop" -c], [0], [dnl > > +1 > > +]) > > + > > +# Use the router IP as SNAT IP. > > +ovn-nbctl set logical_router r1 options:lb_force_snat_ip=10.0.1.1 > > +ovn-nbctl --wait=hv sync > > + > > +# Send ip packets from p1 to lrp-r1-s1 > > +src_mac="f00000000102" > > +dst_mac="000000000101" > > +src_ip=`ip_to_hex 10 0 1 2` > > +dst_ip=`ip_to_hex 10 0 1 1` > > > > +packet=${dst_mac}${src_mac}08004500001c0000000040110000${src_ip}${dst_ip}0035111100080000 > > +as hv1 ovs-appctl netdev-dummy/receive hv1-vif1 $packet > > + > > +# Even after configuring a router owned IP for SNAT, no packet-ins > should > > +# reach ovn-controller. > > +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | grep "actions=controller" > | grep -v n_packets=0 -c], [1], [dnl > > +0 > > +]) > > + > > +# The packet should've been dropped in the lr_in_arp_resolve stage. > > +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | grep -E "table=21, > n_packets=1,.* priority=1,ip,metadata=0x${sw_key},nw_dst=10.0.1.1 > actions=drop" -c], [0], [dnl > > +1 > > +]) > > + > > +OVN_CLEANUP([hv1]) > > +AT_CLEANUP > > + > > AT_SETUP([ovn -- nb_cfg timestamp]) > > ovn_start > > > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Acked-by: Han Zhou <hzhou@ovn.org> > Thanks Dumitru and Han. I applied this patch to master and branch-20.09. The patch doesn't apply cleanly to branch-20.06. There were many conflicts and I didn't try resolving myself. Dumitru - please submit the backport patches to branch-20.06 (and branch-20.03 if you would like). Thanks Numan > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
On 9/22/20 8:17 AM, Numan Siddique wrote: > > > On Mon, Sep 21, 2020 at 12:08 PM Han Zhou <hzhou@ovn.org > <mailto:hzhou@ovn.org>> wrote: > > On Thu, Sep 17, 2020 at 5:51 AM Dumitru Ceara <dceara@redhat.com > <mailto:dceara@redhat.com>> wrote: > > > > OVN was dropping IP packets destined to IPs owned by logical > routers but > > only if those IPs are not used for SNAT rules. However, if a packet > > doesn't match an existing NAT session and its destination is still a > > router owned IP, it can be safely dropped. Otherwise it will > trigger an > > unnecessary packet-in in stage lr_in_arp_request. > > > > To achieve that we add flows that drop traffic to router owned > SNAT IPs in > > table lr_in_arp_resolve. > > > > Reported-by: Tim Rozet <trozet@redhat.com <mailto:trozet@redhat.com>> > > Reported-at: https://bugzilla.redhat.com/1876174 > > Signed-off-by: Dumitru Ceara <dceara@redhat.com > <mailto:dceara@redhat.com>> > > --- > > northd/ovn-northd.8.xml | 24 ++++++ > > northd/ovn-northd.c | 194 > +++++++++++++++++++++++++++-------------------- > > tests/ovn.at <http://ovn.at> | 88 +++++++++++++++++++++ > > 3 files changed, 225 insertions(+), 81 deletions(-) > > > > diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml > > index bd42105..f1c7c9b 100644 > > --- a/northd/ovn-northd.8.xml > > +++ b/northd/ovn-northd.8.xml > > @@ -3089,6 +3089,30 @@ outport = <var>P</var>; > > > > <li> > > <p> > > + Traffic with IP destination an address owned by the router > should be > > + dropped. Such traffic is normally dropped in ingress table > > + <code>IP Input</code> except for IPs that are also > shared with > SNAT > > + rules. However, if there was no unSNAT operation that > happened > > + successfully until this point in the pipeline and the > destination IP > > + of the packet is still a router owned IP, the packets > can be > safely > > + dropped. > > + </p> > > + > > + <p> > > + A priority-1 logical flow with match <code>ip4.dst = > {..}</code> > > + matches on traffic destined to router owned IPv4 addresses > which are > > + also SNAT IPs. This flow has action <code>drop;</code>. > > + </p> > > + > > + <p> > > + A priority-1 logical flow with match <code>ip6.dst = > {..}</code> > > + matches on traffic destined to router owned IPv6 addresses > which are > > + also SNAT IPs. This flow has action <code>drop;</code>. > > + </p> > > + </li> > > + > > + <li> > > + <p> > > Dynamic MAC bindings. These flows resolve MAC-to-IP > bindings > > that have become known dynamically through ARP or neighbor > > discovery. (The ingress table <code>ARP Request</code> > will > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > > index cfec6a2..d5d7631 100644 > > --- a/northd/ovn-northd.c > > +++ b/northd/ovn-northd.c > > @@ -623,6 +623,9 @@ struct ovn_datapath { > > /* NAT entries configured on the router. */ > > struct ovn_nat *nat_entries; > > > > + /* SNAT IPs used by the router. */ > > + struct sset snat_ips; > > + > > struct ovn_port **localnet_ports; > > size_t n_localnet_ports; > > > > @@ -641,6 +644,10 @@ struct ovn_nat { > > struct lport_addresses ext_addrs; > > }; > > > > +static bool > > +get_force_snat_ip(struct ovn_datapath *od, const char *key_type, > > + struct lport_addresses *laddrs); > > + > > /* Returns true if a 'nat_entry' is valid, i.e.: > > * - parsing was successful. > > * - the string yielded exactly one IPv4 address or exactly one IPv6 > address. > > @@ -663,7 +670,35 @@ nat_entry_is_v6(const struct ovn_nat *nat_entry) > > static void > > init_nat_entries(struct ovn_datapath *od) > > { > > - if (!od->nbr || od->nbr->n_nat == 0) { > > + struct lport_addresses snat_addrs; > > + > > + if (!od->nbr) { > > + return; > > + } > > + > > + sset_init(&od->snat_ips); > > + if (get_force_snat_ip(od, "dnat", &snat_addrs)) { > > + if (snat_addrs.n_ipv4_addrs) { > > + sset_add(&od->snat_ips, snat_addrs.ipv4_addrs[0].addr_s); > > + } > > + if (snat_addrs.n_ipv6_addrs) { > > + sset_add(&od->snat_ips, snat_addrs.ipv6_addrs[0].addr_s); > > + } > > + destroy_lport_addresses(&snat_addrs); > > + } > > + > > + memset(&snat_addrs, 0, sizeof(snat_addrs)); > > + if (get_force_snat_ip(od, "lb", &snat_addrs)) { > > + if (snat_addrs.n_ipv4_addrs) { > > + sset_add(&od->snat_ips, snat_addrs.ipv4_addrs[0].addr_s); > > + } > > + if (snat_addrs.n_ipv6_addrs) { > > + sset_add(&od->snat_ips, snat_addrs.ipv6_addrs[0].addr_s); > > + } > > + destroy_lport_addresses(&snat_addrs); > > + } > > + > > + if (!od->nbr->n_nat) { > > return; > > } > > > > @@ -682,6 +717,13 @@ init_nat_entries(struct ovn_datapath *od) > > VLOG_WARN_RL(&rl, > > "Bad ip address %s in nat configuration " > > "for router %s", nat->external_ip, > od->nbr->name); > > + continue; > > + } > > + > > + if (!nat_entry_is_v6(nat_entry)) { > > + sset_add(&od->snat_ips, > nat_entry->ext_addrs.ipv4_addrs[0].addr_s); > > + } else { > > + sset_add(&od->snat_ips, > nat_entry->ext_addrs.ipv6_addrs[0].addr_s); > > } > > } > > } > > @@ -693,6 +735,7 @@ destroy_nat_entries(struct ovn_datapath *od) > > return; > > } > > > > + sset_destroy(&od->snat_ips); > > for (size_t i = 0; i < od->nbr->n_nat; i++) { > > destroy_lport_addresses(&od->nat_entries[i].ext_addrs); > > } > > @@ -8744,6 +8787,68 @@ build_lrouter_flows(struct hmap *datapaths, > struct > hmap *ports, > > op, lflows, &match, &actions); > > } > > > > + /* Drop IP traffic destined to router owned IPs. Part of it is > dropped > > + * in stage "lr_in_ip_input" but traffic that could have been > unSNATed > > + * but didn't match any existing session might still end up here. > > + */ > > + HMAP_FOR_EACH (op, key_node, ports) { > > + if (!op->nbrp) { > > + continue; > > + } > > + > > + if (op->lrp_networks.n_ipv4_addrs) { > > + ds_clear(&match); > > + for (size_t i = 0; i < op->lrp_networks.n_ipv4_addrs; > i++) { > > + if (!sset_find(&op->od->snat_ips, > > + > op->lrp_networks.ipv4_addrs[i].addr_s)) { > > + continue; > > + } > > + ds_put_format(&match, "%s, ", > > + op->lrp_networks.ipv4_addrs[i].addr_s); > > + } > > + > > + if (ds_last(&match) != EOF) { > > + ds_chomp(&match, ' '); > > + ds_chomp(&match, ','); > > + > > + char *drop_match = xasprintf("ip4.dst == {%s}", > > + ds_cstr(&match)); > > + /* Drop traffic with IP.dest == router-ip. */ > > + ovn_lflow_add_with_hint(lflows, op->od, > > + S_ROUTER_IN_ARP_RESOLVE, 1, > > + drop_match, "drop;", > > + &op->nbrp->header_); > > + free(drop_match); > > + } > > + } > > + > > + if (op->lrp_networks.n_ipv6_addrs) { > > + ds_clear(&match); > > + for (size_t i = 0; i < op->lrp_networks.n_ipv6_addrs; > i++) { > > + if (!sset_find(&op->od->snat_ips, > > + > op->lrp_networks.ipv6_addrs[i].addr_s)) { > > + continue; > > + } > > + ds_put_format(&match, "%s, ", > > + op->lrp_networks.ipv6_addrs[i].addr_s); > > + } > > + > > + if (ds_last(&match) != EOF) { > > + ds_chomp(&match, ' '); > > + ds_chomp(&match, ','); > > + > > + char *drop_match = xasprintf("ip6.dst == {%s}", > > + ds_cstr(&match)); > > + /* Drop traffic with IP.dest == router-ip. */ > > + ovn_lflow_add_with_hint(lflows, op->od, > > + S_ROUTER_IN_ARP_RESOLVE, 1, > > + drop_match, "drop;", > > + &op->nbrp->header_); > > + free(drop_match); > > + } > > + } > > + } > > + > > HMAP_FOR_EACH (od, key_node, datapaths) { > > if (!od->nbr) { > > continue; > > @@ -9035,77 +9140,15 @@ build_lrouter_flows(struct hmap *datapaths, > struct hmap *ports, > > } > > > > /* A gateway router can have 4 SNAT IP addresses to force > DNATed > and > > - * LBed traffic respectively to be SNATed. In addition, > there > can be > > - * a number of SNAT rules in the NAT table. */ > > - struct v46_ip *snat_ips = xmalloc(sizeof *snat_ips > > - * (op->od->nbr->n_nat + > 4)); > > - size_t n_snat_ips = 0; > > - struct lport_addresses snat_addrs; > > - > > - if (get_force_snat_ip(op->od, "dnat", &snat_addrs)) { > > - if (snat_addrs.n_ipv4_addrs) { > > - snat_ips[n_snat_ips].family = AF_INET; > > - snat_ips[n_snat_ips++].ipv4 = > snat_addrs.ipv4_addrs[0].addr; > > - } > > - if (snat_addrs.n_ipv6_addrs) { > > - snat_ips[n_snat_ips].family = AF_INET6; > > - snat_ips[n_snat_ips++].ipv6 = > snat_addrs.ipv6_addrs[0].addr; > > - } > > - destroy_lport_addresses(&snat_addrs); > > - } > > - > > - memset(&snat_addrs, 0, sizeof(snat_addrs)); > > - if (get_force_snat_ip(op->od, "lb", &snat_addrs)) { > > - if (snat_addrs.n_ipv4_addrs) { > > - snat_ips[n_snat_ips].family = AF_INET; > > - snat_ips[n_snat_ips++].ipv4 = > snat_addrs.ipv4_addrs[0].addr; > > - } > > - if (snat_addrs.n_ipv6_addrs) { > > - snat_ips[n_snat_ips].family = AF_INET6; > > - snat_ips[n_snat_ips++].ipv6 = > snat_addrs.ipv6_addrs[0].addr; > > - } > > - destroy_lport_addresses(&snat_addrs); > > - } > > - > > - 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; > > - > > - /* Skip entries we failed to parse. */ > > - if (!nat_entry_is_valid(nat_entry)) { > > - continue; > > - } > > - > > - if (!strcmp(nat->type, "snat")) { > > - if (nat_entry_is_v6(nat_entry)) { > > - struct in6_addr *ipv6 = > > - &nat_entry->ext_addrs.ipv6_addrs[0].addr; > > - > > - snat_ips[n_snat_ips].family = AF_INET6; > > - snat_ips[n_snat_ips++].ipv6 = *ipv6; > > - } else { > > - ovs_be32 ip = > nat_entry->ext_addrs.ipv4_addrs[0].addr; > > - snat_ips[n_snat_ips].family = AF_INET; > > - snat_ips[n_snat_ips++].ipv4 = ip; > > - } > > - } > > - } > > - > > + * LBed traffic respectively to be SNATed. In addition, there > can be > > + * a number of SNAT rules in the NAT table. > > + * Skip all of them for drop flows. */ > > ds_clear(&match); > > ds_put_cstr(&match, "ip4.dst == {"); > > bool has_drop_ips = false; > > for (int i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) { > > - bool snat_ip_is_router_ip = false; > > - for (int j = 0; j < n_snat_ips; j++) { > > - /* Packets to SNAT IPs should not be dropped. */ > > - if (snat_ips[j].family == AF_INET > > - && op->lrp_networks.ipv4_addrs[i].addr > > - == snat_ips[j].ipv4) { > > - snat_ip_is_router_ip = true; > > - break; > > - } > > - } > > - if (snat_ip_is_router_ip) { > > + if (sset_find(&op->od->snat_ips, > > + op->lrp_networks.ipv4_addrs[i].addr_s)) { > > continue; > > } > > ds_put_format(&match, "%s, ", > > @@ -9122,17 +9165,8 @@ build_lrouter_flows(struct hmap *datapaths, > struct > hmap *ports, > > } > > > > for (int i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) { > > - bool snat_ip_is_router_ip = false; > > - for (int j = 0; j < n_snat_ips; j++) { > > - /* Packets to SNAT IPs should not be dropped. */ > > - if (snat_ips[j].family == AF_INET6 > > - && !memcmp(&op->lrp_networks.ipv6_addrs[i].addr, > > - &snat_ips[j].ipv6, sizeof > snat_ips[j].ipv6)) { > > - snat_ip_is_router_ip = true; > > - break; > > - } > > - } > > - if (snat_ip_is_router_ip) { > > + if (sset_find(&op->od->snat_ips, > > + op->lrp_networks.ipv6_addrs[i].addr_s)) { > > continue; > > } > > ds_put_format(&match, "%s, ", > > @@ -9151,8 +9185,6 @@ build_lrouter_flows(struct hmap *datapaths, > struct > hmap *ports, > > &op->nbrp->header_); > > } > > > > - free(snat_ips); > > - > > /* ARP/NS packets are taken care of per router. The only > exception > > * is on the l3dgw_port where we might need to use a > different > > * ETH address. > > diff --git a/tests/ovn.at <http://ovn.at> b/tests/ovn.at > <http://ovn.at> > > index a6f1fb5..cb7e7cc 100644 > > --- a/tests/ovn.at <http://ovn.at> > > +++ b/tests/ovn.at <http://ovn.at> > > @@ -21659,6 +21659,94 @@ OVS_WAIT_UNTIL([test x$(as hv1 ovn-appctl -t > ovn-controller debug/status) = "xru > > OVN_CLEANUP([hv1]) > > AT_CLEANUP > > > > +# Test dropping traffic destined to router owned IPs. > > +AT_SETUP([ovn -- gateway router drop traffic for own IPs]) > > +ovn_start > > + > > +ovn-nbctl lr-add r1 -- set logical_router r1 options:chassis=hv1 > > +ovn-nbctl ls-add s1 > > + > > +# Connnect r1 to s1. > > +ovn-nbctl lrp-add r1 lrp-r1-s1 00:00:00:00:01:01 10.0.1.1/24 > <http://10.0.1.1/24> > > +ovn-nbctl lsp-add s1 lsp-s1-r1 -- set Logical_Switch_Port lsp-s1-r1 > type=router \ > > + options:router-port=lrp-r1-s1 addresses=router > > + > > +# Create logical port p1 in s1 > > +ovn-nbctl lsp-add s1 p1 \ > > +-- lsp-set-addresses p1 "f0:00:00:00:01:02 10.0.1.2" > > + > > +# Create two hypervisor and create OVS ports corresponding to logical > ports. > > +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 hv1-vif1 -- \ > > + set interface hv1-vif1 external-ids:iface-id=p1 \ > > + options:tx_pcap=hv1/vif1-tx.pcap \ > > + options:rxq_pcap=hv1/vif1-rx.pcap \ > > + ofport-request=1 > > + > > +# Pre-populate the hypervisors' ARP tables so that we don't lose any > > +# packets for ARP resolution (native tunneling doesn't queue packets > > +# for ARP resolution). > > +OVN_POPULATE_ARP > > + > > +ovn-nbctl --wait=hv sync > > + > > +sw_key=$(ovn-sbctl --bare --columns tunnel_key list > datapath_binding r1) > > + > > +AT_CHECK([ovn-sbctl lflow-list | grep lr_in_arp_resolve | grep > 10.0.1.1], [1], []) > > + > > +ip_to_hex() { > > + printf "%02x%02x%02x%02x" "$@" > > +} > > + > > +# Send ip packets from p1 to lrp-r1-s1 > > +src_mac="f00000000102" > > +dst_mac="000000000101" > > +src_ip=`ip_to_hex 10 0 1 2` > > +dst_ip=`ip_to_hex 10 0 1 1` > > > +packet=${dst_mac}${src_mac}08004500001c0000000040110000${src_ip}${dst_ip}0035111100080000 > > +as hv1 ovs-appctl netdev-dummy/receive hv1-vif1 $packet > > + > > +# No packet-ins should reach ovn-controller. > > +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | grep > "actions=controller" > | grep -v n_packets=0 -c], [1], [dnl > > +0 > > +]) > > + > > +# The packet should have been dropped in the lr_in_ip_input stage. > > +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | grep -E "table=11, > n_packets=1,.* priority=60,ip,metadata=0x${sw_key},nw_dst=10.0.1.1 > actions=drop" -c], [0], [dnl > > +1 > > +]) > > + > > +# Use the router IP as SNAT IP. > > +ovn-nbctl set logical_router r1 options:lb_force_snat_ip=10.0.1.1 > > +ovn-nbctl --wait=hv sync > > + > > +# Send ip packets from p1 to lrp-r1-s1 > > +src_mac="f00000000102" > > +dst_mac="000000000101" > > +src_ip=`ip_to_hex 10 0 1 2` > > +dst_ip=`ip_to_hex 10 0 1 1` > > > +packet=${dst_mac}${src_mac}08004500001c0000000040110000${src_ip}${dst_ip}0035111100080000 > > +as hv1 ovs-appctl netdev-dummy/receive hv1-vif1 $packet > > + > > +# Even after configuring a router owned IP for SNAT, no > packet-ins should > > +# reach ovn-controller. > > +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | grep > "actions=controller" > | grep -v n_packets=0 -c], [1], [dnl > > +0 > > +]) > > + > > +# The packet should've been dropped in the lr_in_arp_resolve stage. > > +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | grep -E "table=21, > n_packets=1,.* priority=1,ip,metadata=0x${sw_key},nw_dst=10.0.1.1 > actions=drop" -c], [0], [dnl > > +1 > > +]) > > + > > +OVN_CLEANUP([hv1]) > > +AT_CLEANUP > > + > > AT_SETUP([ovn -- nb_cfg timestamp]) > > ovn_start > > > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org <mailto:dev@openvswitch.org> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Acked-by: Han Zhou <hzhou@ovn.org <mailto:hzhou@ovn.org>> > > > Thanks Dumitru and Han. I applied this patch to master and branch-20.09. > > The patch doesn't apply cleanly to branch-20.06. There were many > conflicts and I didn't try resolving myself. > Dumitru - please submit the backport patches to branch-20.06 (and > branch-20.03 if you would like). > > Thanks > Numan > Thanks! While this is a bug fix I don't think it's critical to have it on branch-20.06/branch-20.03. Unless someone requires it on older releases too I'd skip sending a backport patch for it. Regards, Dumitru
diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml index bd42105..f1c7c9b 100644 --- a/northd/ovn-northd.8.xml +++ b/northd/ovn-northd.8.xml @@ -3089,6 +3089,30 @@ outport = <var>P</var>; <li> <p> + Traffic with IP destination an address owned by the router should be + dropped. Such traffic is normally dropped in ingress table + <code>IP Input</code> except for IPs that are also shared with SNAT + rules. However, if there was no unSNAT operation that happened + successfully until this point in the pipeline and the destination IP + of the packet is still a router owned IP, the packets can be safely + dropped. + </p> + + <p> + A priority-1 logical flow with match <code>ip4.dst = {..}</code> + matches on traffic destined to router owned IPv4 addresses which are + also SNAT IPs. This flow has action <code>drop;</code>. + </p> + + <p> + A priority-1 logical flow with match <code>ip6.dst = {..}</code> + matches on traffic destined to router owned IPv6 addresses which are + also SNAT IPs. This flow has action <code>drop;</code>. + </p> + </li> + + <li> + <p> Dynamic MAC bindings. These flows resolve MAC-to-IP bindings that have become known dynamically through ARP or neighbor discovery. (The ingress table <code>ARP Request</code> will diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c index cfec6a2..d5d7631 100644 --- a/northd/ovn-northd.c +++ b/northd/ovn-northd.c @@ -623,6 +623,9 @@ struct ovn_datapath { /* NAT entries configured on the router. */ struct ovn_nat *nat_entries; + /* SNAT IPs used by the router. */ + struct sset snat_ips; + struct ovn_port **localnet_ports; size_t n_localnet_ports; @@ -641,6 +644,10 @@ struct ovn_nat { struct lport_addresses ext_addrs; }; +static bool +get_force_snat_ip(struct ovn_datapath *od, const char *key_type, + struct lport_addresses *laddrs); + /* Returns true if a 'nat_entry' is valid, i.e.: * - parsing was successful. * - the string yielded exactly one IPv4 address or exactly one IPv6 address. @@ -663,7 +670,35 @@ nat_entry_is_v6(const struct ovn_nat *nat_entry) static void init_nat_entries(struct ovn_datapath *od) { - if (!od->nbr || od->nbr->n_nat == 0) { + struct lport_addresses snat_addrs; + + if (!od->nbr) { + return; + } + + sset_init(&od->snat_ips); + if (get_force_snat_ip(od, "dnat", &snat_addrs)) { + if (snat_addrs.n_ipv4_addrs) { + sset_add(&od->snat_ips, snat_addrs.ipv4_addrs[0].addr_s); + } + if (snat_addrs.n_ipv6_addrs) { + sset_add(&od->snat_ips, snat_addrs.ipv6_addrs[0].addr_s); + } + destroy_lport_addresses(&snat_addrs); + } + + memset(&snat_addrs, 0, sizeof(snat_addrs)); + if (get_force_snat_ip(od, "lb", &snat_addrs)) { + if (snat_addrs.n_ipv4_addrs) { + sset_add(&od->snat_ips, snat_addrs.ipv4_addrs[0].addr_s); + } + if (snat_addrs.n_ipv6_addrs) { + sset_add(&od->snat_ips, snat_addrs.ipv6_addrs[0].addr_s); + } + destroy_lport_addresses(&snat_addrs); + } + + if (!od->nbr->n_nat) { return; } @@ -682,6 +717,13 @@ init_nat_entries(struct ovn_datapath *od) VLOG_WARN_RL(&rl, "Bad ip address %s in nat configuration " "for router %s", nat->external_ip, od->nbr->name); + continue; + } + + if (!nat_entry_is_v6(nat_entry)) { + sset_add(&od->snat_ips, nat_entry->ext_addrs.ipv4_addrs[0].addr_s); + } else { + sset_add(&od->snat_ips, nat_entry->ext_addrs.ipv6_addrs[0].addr_s); } } } @@ -693,6 +735,7 @@ destroy_nat_entries(struct ovn_datapath *od) return; } + sset_destroy(&od->snat_ips); for (size_t i = 0; i < od->nbr->n_nat; i++) { destroy_lport_addresses(&od->nat_entries[i].ext_addrs); } @@ -8744,6 +8787,68 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports, op, lflows, &match, &actions); } + /* Drop IP traffic destined to router owned IPs. Part of it is dropped + * in stage "lr_in_ip_input" but traffic that could have been unSNATed + * but didn't match any existing session might still end up here. + */ + HMAP_FOR_EACH (op, key_node, ports) { + if (!op->nbrp) { + continue; + } + + if (op->lrp_networks.n_ipv4_addrs) { + ds_clear(&match); + for (size_t i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) { + if (!sset_find(&op->od->snat_ips, + op->lrp_networks.ipv4_addrs[i].addr_s)) { + continue; + } + ds_put_format(&match, "%s, ", + op->lrp_networks.ipv4_addrs[i].addr_s); + } + + if (ds_last(&match) != EOF) { + ds_chomp(&match, ' '); + ds_chomp(&match, ','); + + char *drop_match = xasprintf("ip4.dst == {%s}", + ds_cstr(&match)); + /* Drop traffic with IP.dest == router-ip. */ + ovn_lflow_add_with_hint(lflows, op->od, + S_ROUTER_IN_ARP_RESOLVE, 1, + drop_match, "drop;", + &op->nbrp->header_); + free(drop_match); + } + } + + if (op->lrp_networks.n_ipv6_addrs) { + ds_clear(&match); + for (size_t i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) { + if (!sset_find(&op->od->snat_ips, + op->lrp_networks.ipv6_addrs[i].addr_s)) { + continue; + } + ds_put_format(&match, "%s, ", + op->lrp_networks.ipv6_addrs[i].addr_s); + } + + if (ds_last(&match) != EOF) { + ds_chomp(&match, ' '); + ds_chomp(&match, ','); + + char *drop_match = xasprintf("ip6.dst == {%s}", + ds_cstr(&match)); + /* Drop traffic with IP.dest == router-ip. */ + ovn_lflow_add_with_hint(lflows, op->od, + S_ROUTER_IN_ARP_RESOLVE, 1, + drop_match, "drop;", + &op->nbrp->header_); + free(drop_match); + } + } + } + HMAP_FOR_EACH (od, key_node, datapaths) { if (!od->nbr) { continue; @@ -9035,77 +9140,15 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports, } /* A gateway router can have 4 SNAT IP addresses to force DNATed and - * LBed traffic respectively to be SNATed. In addition, there can be - * a number of SNAT rules in the NAT table. */ - struct v46_ip *snat_ips = xmalloc(sizeof *snat_ips - * (op->od->nbr->n_nat + 4)); - size_t n_snat_ips = 0; - struct lport_addresses snat_addrs; - - if (get_force_snat_ip(op->od, "dnat", &snat_addrs)) { - if (snat_addrs.n_ipv4_addrs) { - snat_ips[n_snat_ips].family = AF_INET; - snat_ips[n_snat_ips++].ipv4 = snat_addrs.ipv4_addrs[0].addr; - } - if (snat_addrs.n_ipv6_addrs) { - snat_ips[n_snat_ips].family = AF_INET6; - snat_ips[n_snat_ips++].ipv6 = snat_addrs.ipv6_addrs[0].addr; - } - destroy_lport_addresses(&snat_addrs); - } - - memset(&snat_addrs, 0, sizeof(snat_addrs)); - if (get_force_snat_ip(op->od, "lb", &snat_addrs)) { - if (snat_addrs.n_ipv4_addrs) { - snat_ips[n_snat_ips].family = AF_INET; - snat_ips[n_snat_ips++].ipv4 = snat_addrs.ipv4_addrs[0].addr; - } - if (snat_addrs.n_ipv6_addrs) { - snat_ips[n_snat_ips].family = AF_INET6; - snat_ips[n_snat_ips++].ipv6 = snat_addrs.ipv6_addrs[0].addr; - } - destroy_lport_addresses(&snat_addrs); - } - - 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; - - /* Skip entries we failed to parse. */ - if (!nat_entry_is_valid(nat_entry)) { - continue; - } - - if (!strcmp(nat->type, "snat")) { - if (nat_entry_is_v6(nat_entry)) { - struct in6_addr *ipv6 = - &nat_entry->ext_addrs.ipv6_addrs[0].addr; - - snat_ips[n_snat_ips].family = AF_INET6; - snat_ips[n_snat_ips++].ipv6 = *ipv6; - } else { - ovs_be32 ip = nat_entry->ext_addrs.ipv4_addrs[0].addr; - snat_ips[n_snat_ips].family = AF_INET; - snat_ips[n_snat_ips++].ipv4 = ip; - } - } - } - + * LBed traffic respectively to be SNATed. In addition, there can be + * a number of SNAT rules in the NAT table. + * Skip all of them for drop flows. */ ds_clear(&match); ds_put_cstr(&match, "ip4.dst == {"); bool has_drop_ips = false; for (int i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) { - bool snat_ip_is_router_ip = false; - for (int j = 0; j < n_snat_ips; j++) { - /* Packets to SNAT IPs should not be dropped. */ - if (snat_ips[j].family == AF_INET - && op->lrp_networks.ipv4_addrs[i].addr - == snat_ips[j].ipv4) { - snat_ip_is_router_ip = true; - break; - } - } - if (snat_ip_is_router_ip) { + if (sset_find(&op->od->snat_ips, + op->lrp_networks.ipv4_addrs[i].addr_s)) { continue; } ds_put_format(&match, "%s, ", @@ -9122,17 +9165,8 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports, } for (int i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) { - bool snat_ip_is_router_ip = false; - for (int j = 0; j < n_snat_ips; j++) { - /* Packets to SNAT IPs should not be dropped. */ - if (snat_ips[j].family == AF_INET6 - && !memcmp(&op->lrp_networks.ipv6_addrs[i].addr, - &snat_ips[j].ipv6, sizeof snat_ips[j].ipv6)) { - snat_ip_is_router_ip = true; - break; - } - } - if (snat_ip_is_router_ip) { + if (sset_find(&op->od->snat_ips, + op->lrp_networks.ipv6_addrs[i].addr_s)) { continue; } ds_put_format(&match, "%s, ", @@ -9151,8 +9185,6 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports, &op->nbrp->header_); } - free(snat_ips); - /* ARP/NS packets are taken care of per router. The only exception * is on the l3dgw_port where we might need to use a different * ETH address. diff --git a/tests/ovn.at b/tests/ovn.at index a6f1fb5..cb7e7cc 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -21659,6 +21659,94 @@ OVS_WAIT_UNTIL([test x$(as hv1 ovn-appctl -t ovn-controller debug/status) = "xru OVN_CLEANUP([hv1]) AT_CLEANUP +# Test dropping traffic destined to router owned IPs. +AT_SETUP([ovn -- gateway router drop traffic for own IPs]) +ovn_start + +ovn-nbctl lr-add r1 -- set logical_router r1 options:chassis=hv1 +ovn-nbctl ls-add s1 + +# Connnect r1 to s1. +ovn-nbctl lrp-add r1 lrp-r1-s1 00:00:00:00:01:01 10.0.1.1/24 +ovn-nbctl lsp-add s1 lsp-s1-r1 -- set Logical_Switch_Port lsp-s1-r1 type=router \ + options:router-port=lrp-r1-s1 addresses=router + +# Create logical port p1 in s1 +ovn-nbctl lsp-add s1 p1 \ +-- lsp-set-addresses p1 "f0:00:00:00:01:02 10.0.1.2" + +# Create two hypervisor and create OVS ports corresponding to logical ports. +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 hv1-vif1 -- \ + set interface hv1-vif1 external-ids:iface-id=p1 \ + options:tx_pcap=hv1/vif1-tx.pcap \ + options:rxq_pcap=hv1/vif1-rx.pcap \ + ofport-request=1 + +# Pre-populate the hypervisors' ARP tables so that we don't lose any +# packets for ARP resolution (native tunneling doesn't queue packets +# for ARP resolution). +OVN_POPULATE_ARP + +ovn-nbctl --wait=hv sync + +sw_key=$(ovn-sbctl --bare --columns tunnel_key list datapath_binding r1) + +AT_CHECK([ovn-sbctl lflow-list | grep lr_in_arp_resolve | grep 10.0.1.1], [1], []) + +ip_to_hex() { + printf "%02x%02x%02x%02x" "$@" +} + +# Send ip packets from p1 to lrp-r1-s1 +src_mac="f00000000102" +dst_mac="000000000101" +src_ip=`ip_to_hex 10 0 1 2` +dst_ip=`ip_to_hex 10 0 1 1` +packet=${dst_mac}${src_mac}08004500001c0000000040110000${src_ip}${dst_ip}0035111100080000 +as hv1 ovs-appctl netdev-dummy/receive hv1-vif1 $packet + +# No packet-ins should reach ovn-controller. +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | grep "actions=controller" | grep -v n_packets=0 -c], [1], [dnl +0 +]) + +# The packet should have been dropped in the lr_in_ip_input stage. +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | grep -E "table=11, n_packets=1,.* priority=60,ip,metadata=0x${sw_key},nw_dst=10.0.1.1 actions=drop" -c], [0], [dnl +1 +]) + +# Use the router IP as SNAT IP. +ovn-nbctl set logical_router r1 options:lb_force_snat_ip=10.0.1.1 +ovn-nbctl --wait=hv sync + +# Send ip packets from p1 to lrp-r1-s1 +src_mac="f00000000102" +dst_mac="000000000101" +src_ip=`ip_to_hex 10 0 1 2` +dst_ip=`ip_to_hex 10 0 1 1` +packet=${dst_mac}${src_mac}08004500001c0000000040110000${src_ip}${dst_ip}0035111100080000 +as hv1 ovs-appctl netdev-dummy/receive hv1-vif1 $packet + +# Even after configuring a router owned IP for SNAT, no packet-ins should +# reach ovn-controller. +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | grep "actions=controller" | grep -v n_packets=0 -c], [1], [dnl +0 +]) + +# The packet should've been dropped in the lr_in_arp_resolve stage. +AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | grep -E "table=21, n_packets=1,.* priority=1,ip,metadata=0x${sw_key},nw_dst=10.0.1.1 actions=drop" -c], [0], [dnl +1 +]) + +OVN_CLEANUP([hv1]) +AT_CLEANUP + AT_SETUP([ovn -- nb_cfg timestamp]) ovn_start
OVN was dropping IP packets destined to IPs owned by logical routers but only if those IPs are not used for SNAT rules. However, if a packet doesn't match an existing NAT session and its destination is still a router owned IP, it can be safely dropped. Otherwise it will trigger an unnecessary packet-in in stage lr_in_arp_request. To achieve that we add flows that drop traffic to router owned SNAT IPs in table lr_in_arp_resolve. Reported-by: Tim Rozet <trozet@redhat.com> Reported-at: https://bugzilla.redhat.com/1876174 Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- northd/ovn-northd.8.xml | 24 ++++++ northd/ovn-northd.c | 194 +++++++++++++++++++++++++++-------------------- tests/ovn.at | 88 +++++++++++++++++++++ 3 files changed, 225 insertions(+), 81 deletions(-)