[ovs-dev] [PATCH v3 ovn 1/4] ovn-northd: Drop IP packets destined to router owned IPs (after NAT).

Dumitru Ceara dceara at redhat.com
Thu Sep 17 12:50:53 UTC 2020


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 at redhat.com>
Reported-at: https://bugzilla.redhat.com/1876174
Signed-off-by: Dumitru Ceara <dceara at 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
 



More information about the dev mailing list